Skip to content

Commit a4de8f5

Browse files
rudy-regazzoni-sonarsourcerudy-regazzoni-sonarsource
authored
SONARPY-1128 Rule S4423: Weak SSL/TLS protocols should not be used (#1221)
* SONARPY-1128 Rule S4423: Weak SSL/TLS protocols should not be used * Fix after review
1 parent 941efd3 commit a4de8f5

File tree

11 files changed

+323
-45
lines changed

11 files changed

+323
-45
lines changed

‎python-checks/src/main/java/org/sonar/python/checks/WeakSSLProtocolCheck.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.sonar.plugins.python.api.tree.Name;
2828
import org.sonar.plugins.python.api.tree.Tree;
2929
import org.sonar.plugins.python.api.symbols.Symbol;
30+
import org.sonar.python.checks.cdk.WeakSSLProtocolCheckPart;
3031

3132
@Rule(key = "S4423")
3233
public class WeakSSLProtocolCheck extends PythonSubscriptionCheck {
@@ -52,6 +53,8 @@ public void initialize(Context context) {
5253
ctx.addIssue(pyNameTree, "Change this code to use a stronger protocol.");
5354
}
5455
});
56+
57+
new WeakSSLProtocolCheckPart().initialize(context);
5558
}
5659

5760
private static boolean isWeakProtocol(@Nullable Symbol symbol) {

‎python-checks/src/main/java/org/sonar/python/checks/cdk/AbstractCdkResourceCheck.java

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.sonar.plugins.python.api.tree.Expression;
3636
import org.sonar.plugins.python.api.tree.Name;
3737
import org.sonar.plugins.python.api.tree.RegularArgument;
38+
import org.sonar.plugins.python.api.tree.StringLiteral;
3839
import org.sonar.plugins.python.api.tree.Token;
3940
import org.sonar.plugins.python.api.tree.Tree;
4041
import org.sonar.python.checks.Expressions;
@@ -88,7 +89,7 @@ protected static Optional<ArgumentTrace> getArgument(SubscriptionContext ctx, Ca
8889
* For compatibility with other classes and branches.
8990
* TODO Can be removed at the end of the sprint to reduce complexity.
9091
*/
91-
static class ArgumentTrace extends ExpressionTrace {
92+
public static class ArgumentTrace extends ExpressionTrace {
9293
private ArgumentTrace(SubscriptionContext ctx, List<Expression> trace) {
9394
super(ctx, trace);
9495
}
@@ -171,4 +172,37 @@ protected static Predicate<Expression> isFqn(String fqnValue) {
171172
.isPresent();
172173
}
173174

175+
protected static Optional<String> getStringValue(Expression expression) {
176+
try {
177+
return Optional.of(((StringLiteral) expression).trimmedQuotesValue());
178+
} catch (ClassCastException e) {
179+
return Optional.empty();
180+
}
181+
}
182+
183+
/**
184+
* @return Predicate which tests if expression is a string and is equal the expected value
185+
*/
186+
protected static Predicate<Expression> isStringValue(String expectedValue) {
187+
return expression -> getStringValue(expression).filter(expectedValue::equals).isPresent();
188+
}
189+
190+
protected static Predicate<Expression> isSensitiveMethod(SubscriptionContext ctx, String methodFqn, String argName, Predicate<Expression> sensitiveValuePredicate) {
191+
return expression -> {
192+
if (!isFqn(methodFqn).test(expression)) {
193+
return false;
194+
}
195+
if (!expression.is(Tree.Kind.CALL_EXPR)) {
196+
return true;
197+
}
198+
199+
Optional<AbstractCdkResourceCheck.ArgumentTrace> argTrace = getArgument(ctx, (CallExpression) expression, argName);
200+
if (argTrace.isEmpty()) {
201+
return true;
202+
}
203+
204+
return argTrace.filter(trace -> trace.hasExpression(sensitiveValuePredicate)).isPresent();
205+
};
206+
}
207+
174208
}

‎python-checks/src/main/java/org/sonar/python/checks/cdk/ClearTextProtocolsCheckPart.java

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -201,14 +201,6 @@ private static void checkKeyValuePair(SubscriptionContext ctx, DictionaryLiteral
201201
// General expression utils
202202
// ---------------------------------------------------------------------------------------
203203

204-
private static Optional<String> getStringValue(Expression expression) {
205-
try {
206-
return Optional.of(((StringLiteral) expression).trimmedQuotesValue());
207-
} catch (ClassCastException e) {
208-
return Optional.empty();
209-
}
210-
}
211-
212204
private static Optional<Integer> getIntValue(Expression expression) {
213205
try {
214206
return Optional.of((int)((NumericLiteral) expression).valueAsLong());
@@ -229,7 +221,7 @@ private static Optional<ListLiteral> getArgumentList(SubscriptionContext ctx, Ca
229221
// ---------------------------------------------------------------------------------------
230222

231223
@CheckForNull
232-
private static KeyValuePair getKeyValuePair(DictionaryLiteralElement element) {
224+
public static KeyValuePair getKeyValuePair(DictionaryLiteralElement element) {
233225
return element.is(Tree.Kind.KEY_VALUE_PAIR) ? (KeyValuePair) element : null;
234226
}
235227

@@ -251,13 +243,6 @@ private static List<ExpressionTrace> getListElements(SubscriptionContext ctx, Li
251243
// General predicates
252244
// ---------------------------------------------------------------------------------------
253245

254-
/**
255-
* @return Predicate which tests if expression is a string and is equal the expected value
256-
*/
257-
private static Predicate<Expression> isStringValue(String expectedValue) {
258-
return expression -> getStringValue(expression).filter(expectedValue::equals).isPresent();
259-
}
260-
261246
/**
262247
* @return Predicate which tests if expression is empty list literal
263248
*/

‎python-checks/src/main/java/org/sonar/python/checks/cdk/DisabledESDomainEncryptionCheck.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ private static String unencryptedMessage(String engine) {
7474
return String.format(UNENCRYPTED_MESSAGE, engine);
7575
}
7676

77+
// TODO : refactor to replace this method with new available method 'isSensitiveOption' in AbstractCdkResourceCheck
7778
private static Predicate<Expression> isSensitiveOptionObj(SubscriptionContext ctx, String argFqnMethod) {
7879
return expr -> {
7980
if (!isFqn(argFqnMethod).test(expr)) {
@@ -88,6 +89,7 @@ private static Predicate<Expression> isSensitiveOptionObj(SubscriptionContext ct
8889
};
8990
}
9091

92+
// TODO : refactor below to use new available method 'isDictionaryWithKeyValue' in AbstractCdkResourceCheck
9193
private static Predicate<Expression> isSensitiveOptionDict(SubscriptionContext ctx) {
9294
return expression -> expression.is(Tree.Kind.DICTIONARY_LITERAL)
9395
&& asDictionaryKeyValuePairs(expression)

‎python-checks/src/main/java/org/sonar/python/checks/cdk/DisabledRDSEncryptionCheck.java

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,7 @@ protected boolean isEngineAurora(SubscriptionContext ctx, CallExpression resourc
8080
}
8181

8282
protected Predicate<Expression> startsWith(String expected) {
83-
return e -> getStringValue(e).filter(str -> str.startsWith(expected)).isPresent();
84-
}
85-
86-
protected Optional<String> getStringValue(Expression expression) {
87-
return Optional.of(expression)
88-
.filter(expr -> expr.is(Tree.Kind.STRING_LITERAL)).map(StringLiteral.class::cast)
89-
.map(str -> str.trimmedQuotesValue().toLowerCase(Locale.ROOT));
83+
return e -> getStringValue(e).filter(str -> str.toLowerCase(Locale.ROOT).startsWith(expected)).isPresent();
9084
}
9185
}
9286

‎python-checks/src/main/java/org/sonar/python/checks/cdk/WeakSSLProtocolCheckPart.java

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2022 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.python.checks.cdk;
21+
22+
import java.util.Objects;
23+
import java.util.Optional;
24+
import java.util.function.BiConsumer;
25+
import java.util.function.Predicate;
26+
import org.sonar.plugins.python.api.SubscriptionContext;
27+
import org.sonar.plugins.python.api.tree.CallExpression;
28+
import org.sonar.plugins.python.api.tree.DictionaryLiteral;
29+
import org.sonar.plugins.python.api.tree.Expression;
30+
import org.sonar.plugins.python.api.tree.Tree;
31+
32+
public class WeakSSLProtocolCheckPart extends AbstractCdkResourceCheck {
33+
private static final String ENFORCE_MESSAGE = "Change this code to enforce TLS 1.2 or above.";
34+
private static final String OMITTING_MESSAGE = "Omitting \"tls_security_policy\" enables a deprecated version of TLS. Set it to enforce TLS 1.2 or above.";
35+
36+
// api gateway
37+
private static final String APIGATEWAY_FQN = "aws_cdk.aws_apigateway.";
38+
private static final String APIGATEWAYV2_FQN = "aws_cdk.aws_apigatewayv2.";
39+
40+
// OpenSearch & ElasticSearch
41+
private static final String OPENSEARCH_FQN = "aws_cdk.aws_opensearchservice.";
42+
private static final String ELASTICSEARCH_FQN = "aws_cdk.aws_elasticsearch.";
43+
private static final String TLS_SECURITY_POLICY = "tls_security_policy";
44+
private static final String SENSITIVE_TLS_SECURITY_POLICY = "Policy-Min-TLS-1-0-2019-07";
45+
46+
@Override
47+
protected void registerFqnConsumer() {
48+
// Api gateway
49+
checkFqn(APIGATEWAY_FQN + "DomainName", checkDomainName(isFqn(APIGATEWAY_FQN + "SecurityPolicy.TLS_1_0")));
50+
checkFqn(APIGATEWAYV2_FQN + "DomainName", checkDomainName(isFqn(APIGATEWAYV2_FQN + "SecurityPolicy.TLS_1_0")));
51+
checkFqn(APIGATEWAY_FQN + "CfnDomainName", checkDomainName(isStringValue("TLS_1_0")));
52+
53+
// OpenSearch & ElasticSearch
54+
checkFqn(OPENSEARCH_FQN + "Domain", checkDomain(isFqn(OPENSEARCH_FQN + "TLSSecurityPolicy.TLS_1_0")));
55+
checkFqn(ELASTICSEARCH_FQN + "Domain", checkDomain(isFqn(ELASTICSEARCH_FQN + "TLSSecurityPolicy.TLS_1_0")));
56+
checkFqn(OPENSEARCH_FQN + "CfnDomain", checkCfnDomain(OPENSEARCH_FQN + "CfnDomain.DomainEndpointOptionsProperty"));
57+
checkFqn(ELASTICSEARCH_FQN + "CfnDomain", checkCfnDomain(ELASTICSEARCH_FQN + "CfnDomain.DomainEndpointOptionsProperty"));
58+
}
59+
60+
private static BiConsumer<SubscriptionContext, CallExpression> checkDomainName(Predicate<Expression> predicateIssue) {
61+
return (ctx, callExpression) -> getArgument(ctx, callExpression, "security_policy").ifPresent(
62+
argTrace -> argTrace.addIssueIf(predicateIssue, ENFORCE_MESSAGE)
63+
);
64+
}
65+
66+
private static BiConsumer<SubscriptionContext, CallExpression> checkDomain(Predicate<Expression> predicateIssue) {
67+
return (ctx, callExpression) -> getArgument(ctx, callExpression, TLS_SECURITY_POLICY).ifPresentOrElse(
68+
argTrace -> argTrace.addIssueIf(predicateIssue, ENFORCE_MESSAGE),
69+
() -> ctx.addIssue(callExpression.callee(), OMITTING_MESSAGE)
70+
);
71+
}
72+
73+
private static BiConsumer<SubscriptionContext, CallExpression> checkCfnDomain(String domainOptionName) {
74+
return (ctx, callExpression) -> getArgument(ctx, callExpression, "domain_endpoint_options").ifPresentOrElse(
75+
argTrace -> argTrace.addIssueIf(isSensitiveMethod(ctx, domainOptionName, TLS_SECURITY_POLICY, isStringValue(SENSITIVE_TLS_SECURITY_POLICY))
76+
.or(isSensitiveDictionaryTls(ctx)), ENFORCE_MESSAGE),
77+
() -> ctx.addIssue(callExpression.callee(), OMITTING_MESSAGE)
78+
);
79+
}
80+
81+
private static Predicate<Expression> isSensitiveDictionaryTls(SubscriptionContext ctx) {
82+
return expression -> Optional.of(expression)
83+
.filter(expr -> expr.is(Tree.Kind.DICTIONARY_LITERAL)).map(DictionaryLiteral.class::cast)
84+
.filter(hasDictionaryKeyValue(ctx, TLS_SECURITY_POLICY, isStringValue(SENSITIVE_TLS_SECURITY_POLICY)))
85+
.isPresent();
86+
}
87+
88+
private static Predicate<DictionaryLiteral> hasDictionaryKeyValue(SubscriptionContext ctx, String key, Predicate<Expression> expected) {
89+
return dict -> dict.elements().stream().map(ClearTextProtocolsCheckPart::getKeyValuePair).filter(Objects::nonNull)
90+
.map(pair -> ClearTextProtocolsCheckPart.ResolvedKeyValuePair.build(ctx, pair))
91+
.filter(pair -> pair.key.hasExpression(isStringValue(key)))
92+
.allMatch(pair -> pair.value.hasExpression(expected));
93+
}
94+
}

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S4423.html

Lines changed: 58 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,8 @@
1-
<p>Older versions of SSL/TLS protocol like "SSLv3" have been proven to be insecure.</p>
2-
<p>This rule raises an issue when an SSL/TLS context is created with an insecure protocol version, i.e. when one of the following constants is
3-
detected in the code:</p>
4-
<ul>
5-
<li> <code>OpenSSL.SSL.SSLv3_METHOD</code> (Use instead <code>OpenSSL.SSL.TLSv1_2_METHOD</code>) </li>
6-
<li> <code>ssl.PROTOCOL_SSLv3</code> (Use instead <code>ssl.PROTOCOL_TLSv1_2</code>) </li>
7-
</ul>
8-
<p>Protocol versions different from TLSv1.2 and TLSv1.3 are considered insecure.</p>
1+
<p>This rule raises an issue when an insecure TLS protocol version (i.e. a protocol different from "TLSv1.2", "TLSv1.3", "DTLSv1.2", or "DTLSv1.3") is
2+
used or allowed.</p>
3+
<p>It is recommended to enforce TLS 1.2 as the minimum protocol version and to disallow older versions like TLS 1.0. Failure to do so could open the
4+
door to downgrade attacks: a malicious actor who is able to intercept the connection could modify the requested protocol version and downgrade it to a
5+
less secure version.</p>
96
<h2>Noncompliant Code Example</h2>
107
<pre>
118
from OpenSSL import SSL
@@ -17,16 +14,65 @@ <h2>Noncompliant Code Example</h2>
1714

1815
ssl.SSLContext(ssl.PROTOCOL_SSLv3) # Noncompliant
1916
</pre>
17+
<p>For <a href="https://docs.aws.amazon.com/cdk/api/v1/python/aws_cdk.aws_apigateway/DomainName.html">aws_cdk.aws_apigateway.DomainName</a>:</p>
18+
<pre>
19+
from aws_cdk.aws_apigateway import DomainName, SecurityPolicy
20+
class ExampleStack(Stack):
21+
def __init__(self, scope: Construct, construct_id: str, **kwargs) -&gt; None:
22+
super().__init__(scope, construct_id, **kwargs)
23+
DomainName(self, "example",
24+
domain_name="example.com",
25+
certificate=certificate,
26+
security_policy=SecurityPolicy.TLS_1_0 # Noncompliant
27+
)
28+
</pre>
29+
<p>For <a
30+
href="https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_opensearchservice/CfnDomain.html">aws_cdk.aws_opensearchservice.CfnDomain</a>:</p>
31+
<pre>
32+
from aws_cdk.aws_opensearchservice import CfnDomain, EngineVersion
33+
class ExampleStack(Stack):
34+
def __init__(self, scope: Construct, construct_id: str, **kwargs) -&gt; None:
35+
super().__init__(scope, construct_id, **kwargs)
36+
CfnDomain(self, "example",
37+
version=EngineVersion.OPENSEARCH_1_3
38+
) # Noncompliant: enables TLS 1.0 which is a deprecated version of the protocol
39+
</pre>
2040
<h2>Compliant Solution</h2>
2141
<pre>
2242
from OpenSSL import SSL
2343

24-
SSL.Context(SSL.TLSv1_2_METHOD) # Compliant
44+
SSL.Context(SSL.TLSv1_2_METHOD)
2545
</pre>
2646
<pre>
2747
import ssl
2848

29-
ssl.SSLContext(ssl.PROTOCOL_TLSv1_2) # Compliant
49+
ssl.SSLContext(ssl.PROTOCOL_TLSv1_2)
50+
</pre>
51+
<p>For <a href="https://docs.aws.amazon.com/cdk/api/v1/python/aws_cdk.aws_apigateway/DomainName.html">aws_cdk.aws_apigateway.DomainName</a>:</p>
52+
<pre>
53+
from aws_cdk.aws_apigateway import DomainName, SecurityPolicy
54+
class ExampleStack(Stack):
55+
def __init__(self, scope: Construct, construct_id: str, **kwargs) -&gt; None:
56+
super().__init__(scope, construct_id, **kwargs)
57+
DomainName(self, "example",
58+
domain_name="example.com",
59+
certificate=certificate,
60+
security_policy=SecurityPolicy.TLS_1_2
61+
)
62+
</pre>
63+
<p>For <a
64+
href="https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_opensearchservice/CfnDomain.html">aws_cdk.aws_opensearchservice.CfnDomain</a>:</p>
65+
<pre>
66+
from aws_cdk.aws_opensearchservice import CfnDomain, EngineVersion
67+
class ExampleStack(Stack):
68+
def __init__(self, scope: Construct, construct_id: str, **kwargs) -&gt; None:
69+
super().__init__(scope, construct_id, **kwargs)
70+
CfnDomain(self, "example",
71+
version=EngineVersion.OPENSEARCH_1_3
72+
domain_endpoint_options=CfnDomain.DomainEndpointOptionsProperty(
73+
tls_security_policy="Policy-Min-TLS-1-2-2019-07" # Compliant
74+
)
75+
)
3076
</pre>
3177
<h2>See</h2>
3278
<ul>
@@ -46,5 +92,7 @@ <h2>See</h2>
4692
<li> <a href="https://www.sans.org/top25-software-errors/#cat3">SANS Top 25</a> - Porous Defenses </li>
4793
<li> <a href="https://github.com/ssllabs/research/wiki/SSL-and-TLS-Deployment-Best-Practices#22-use-secure-protocols">SSL and TLS Deployment Best
4894
Practices - Use secure protocols</a> </li>
95+
<li> <a href="https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-custom-domain-tls-version.html">Amazon API Gateway</a> -
96+
Choosing a minimum TLS version </li>
4997
</ul>
5098

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S4423.json

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,19 +47,9 @@
4747
"6.2.4"
4848
],
4949
"ASVS 4.0": [
50-
"1.9.2",
51-
"2.8.3",
52-
"2.9.3",
53-
"6.2.2",
54-
"6.2.3",
55-
"6.2.4",
56-
"6.2.5",
57-
"6.2.6",
58-
"6.2.7",
5950
"8.3.7",
6051
"9.1.2",
61-
"9.1.3",
62-
"9.2.1"
52+
"9.1.3"
6353
]
6454
},
6555
"quickfix": "unknown"

‎python-checks/src/test/java/org/sonar/python/checks/WeakSSLProtocolCheckTest.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,4 +33,14 @@ public void test() {
3333
public void test_fallback_import() {
3434
PythonCheckVerifier.verify("src/test/resources/checks/weakSSLProtocol_fallback_import.py", new WeakSSLProtocolCheck());
3535
}
36+
37+
@Test
38+
public void test_apigateway() {
39+
PythonCheckVerifier.verify("src/test/resources/checks/weakSSLProtocol_apigateway.py", new WeakSSLProtocolCheck());
40+
}
41+
42+
@Test
43+
public void test_elasticopensearch() {
44+
PythonCheckVerifier.verify("src/test/resources/checks/weakSSLProtocol_elastic_and_open_search.py", new WeakSSLProtocolCheck());
45+
}
3646
}

0 commit comments

Comments
 (0)