Skip to content

Commit fb6aa23

Browse files
guillaume-dequennesonartech
guillaume-dequenne
authored and
sonartech
committed
SONARPY-1682 S5547: Update TLS cipher suite detection logic (#201)
GitOrigin-RevId: 88ef0572d803cd1780da2daf783c22efee7f5db3
1 parent 0f8a923 commit fb6aa23

2 files changed

Lines changed: 100 additions & 49 deletions

File tree

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

Lines changed: 75 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -16,114 +16,140 @@
1616
*/
1717
package org.sonar.python.checks;
1818

19-
import java.util.Arrays;
20-
import java.util.HashSet;
19+
import java.util.LinkedHashSet;
2120
import java.util.Optional;
2221
import java.util.Set;
22+
import java.util.function.Predicate;
23+
import java.util.stream.Collectors;
2324
import java.util.stream.Stream;
2425
import javax.annotation.CheckForNull;
2526
import javax.annotation.Nullable;
2627
import org.sonar.check.Rule;
2728
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
29+
import org.sonar.plugins.python.api.SubscriptionContext;
2830
import org.sonar.plugins.python.api.symbols.Symbol;
2931
import org.sonar.plugins.python.api.tree.CallExpression;
3032
import org.sonar.plugins.python.api.tree.Expression;
3133
import org.sonar.plugins.python.api.tree.Name;
3234
import org.sonar.plugins.python.api.tree.RegularArgument;
35+
import org.sonar.plugins.python.api.tree.StringLiteral;
3336
import org.sonar.plugins.python.api.tree.Tree;
3437
import org.sonar.python.checks.utils.Expressions;
3538
import org.sonar.python.tree.StringLiteralImpl;
3639
import org.sonar.python.tree.TreeUtils;
3740

38-
import static java.util.Arrays.asList;
39-
4041
// https://jira.sonarsource.com/browse/RSPEC-5547 (general)
4142
// https://jira.sonarsource.com/browse/RSPEC-5552 (python-specific)
4243
@Rule(key = "S5547")
4344
public class RobustCipherAlgorithmCheck extends PythonSubscriptionCheck {
4445

4546
private static final String MESSAGE = "Use a strong cipher algorithm.";
46-
private static final HashSet<String> sensitiveCalleeFqns = new HashSet<>();
4747

4848
private static final Set<String> INSECURE_CIPHERS = Set.of(
4949
"NULL",
50+
"aNULL",
51+
"eNULL",
52+
"COMPLEMENTOFALL",
5053
"RC2",
5154
"RC4",
55+
"IDEA",
56+
"SEED",
5257
"DES",
5358
"3DES",
5459
"MD5",
55-
"SHA"
60+
"SHA",
61+
"SHA1",
62+
"ADH",
63+
"AECDH",
64+
"CBC",
65+
"LOW",
66+
"@SECLEVEL=0",
67+
"@SECLEVEL=1",
68+
"DEFAULT@SECLEVEL=0",
69+
"DEFAULT@SECLEVEL=1"
5670
);
5771

5872
public static final String SSL_SET_CIPHERS_FQN = "ssl.SSLContext.set_ciphers";
5973

60-
static {
61-
// `pycryptodomex`, `pycryptodome`, and `pycrypto` all share the same names of the algorithms,
62-
// moreover, `pycryptodome` is drop-in replacement for `pycrypto`, thus they share same name ("Crypto").
63-
for (String libraryName : asList("Cryptodome", "Crypto")) {
64-
for (String vulnerableMethodName : asList("DES", "DES3", "ARC2", "ARC4", "Blowfish")) {
65-
sensitiveCalleeFqns.add(String.format("%s.Cipher.%s.new", libraryName, vulnerableMethodName));
66-
}
67-
}
68-
69-
70-
// Idea is listed under "Weak Algorithms" in pyca/cryptography documentation
71-
// https://cryptography.io/en/latest/hazmat/primitives/symmetric-encryption/\
72-
// #cryptography.hazmat.primitives.ciphers.algorithms.IDEA
73-
// pyca (pyca/cryptography)
74-
for (String methodName : asList("TripleDES", "Blowfish", "ARC4", "IDEA")) {
75-
sensitiveCalleeFqns.add(String.format("cryptography.hazmat.primitives.ciphers.algorithms.%s", methodName));
76-
}
77-
78-
// pydes
79-
sensitiveCalleeFqns.add("pyDes.des");
80-
sensitiveCalleeFqns.add("pyDes.triple_des");
81-
}
74+
private static final Set<String> SENSITIVE_CALLEE_FQNS = Set.of(
75+
"Crypto.Cipher.ARC2.new",
76+
"Crypto.Cipher.ARC4.new",
77+
"Crypto.Cipher.Blowfish.new",
78+
"Crypto.Cipher.DES.new",
79+
"Crypto.Cipher.DES3.new",
80+
"Cryptodome.Cipher.ARC2.new",
81+
"Cryptodome.Cipher.ARC4.new",
82+
"Cryptodome.Cipher.Blowfish.new",
83+
"Cryptodome.Cipher.DES.new",
84+
"Cryptodome.Cipher.DES3.new",
85+
"cryptography.hazmat.primitives.ciphers.algorithms.ARC4",
86+
"cryptography.hazmat.primitives.ciphers.algorithms.Blowfish",
87+
"cryptography.hazmat.primitives.ciphers.algorithms.IDEA",
88+
"cryptography.hazmat.primitives.ciphers.algorithms.TripleDES",
89+
"pyDes.des",
90+
"pyDes.triple_des"
91+
);
8292

8393
@Override
8494
public void initialize(Context context) {
85-
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, subscriptionContext -> {
86-
CallExpression callExpr = (CallExpression) subscriptionContext.syntaxNode();
87-
Optional.ofNullable(callExpr)
88-
.map(CallExpression::calleeSymbol)
89-
.map(Symbol::fullyQualifiedName)
90-
.filter(fqn -> sensitiveCalleeFqns.contains(fqn) ||
91-
(SSL_SET_CIPHERS_FQN.equals(fqn) && hasArgumentWithSensitiveAlgorithm(callExpr)))
92-
.ifPresent(fqn -> subscriptionContext.addIssue(callExpr.callee(), MESSAGE));
93-
});
95+
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, RobustCipherAlgorithmCheck::checkCallExpression);
9496
}
9597

98+
private static void checkCallExpression(SubscriptionContext subscriptionContext) {
99+
CallExpression callExpr = (CallExpression) subscriptionContext.syntaxNode();
100+
Optional.of(callExpr)
101+
.map(CallExpression::calleeSymbol)
102+
.map(Symbol::fullyQualifiedName)
103+
.ifPresent(fullyQualifiedName -> {
104+
if (SENSITIVE_CALLEE_FQNS.contains(fullyQualifiedName)) {
105+
subscriptionContext.addIssue(callExpr.callee(), MESSAGE);
106+
} else if (SSL_SET_CIPHERS_FQN.equals(fullyQualifiedName)) {
107+
checkForInsecureCiphers(subscriptionContext, callExpr);
108+
}
109+
});
110+
}
96111

97-
private static boolean hasArgumentWithSensitiveAlgorithm(CallExpression callExpression) {
98-
return Optional.of(callExpression.arguments())
112+
private static void checkForInsecureCiphers(SubscriptionContext ctx, CallExpression callExpression) {
113+
Optional.of(callExpression.arguments())
99114
.filter(list -> list.size() == 1)
100115
.map(list -> list.get(0))
101116
.flatMap(TreeUtils.toOptionalInstanceOfMapper(RegularArgument.class))
102117
.map(RegularArgument::expression)
103118
.map(RobustCipherAlgorithmCheck::unpackArgument)
104-
.filter(RobustCipherAlgorithmCheck::containsInsecureCipher)
105-
.isPresent();
119+
.ifPresent(stringLiteral -> Optional.of(stringLiteral.trimmedQuotesValue())
120+
.map(RobustCipherAlgorithmCheck::findInsecureCiphers)
121+
.filter(Predicate.not(Set::isEmpty))
122+
.ifPresent(insecureCiphers -> {
123+
var secondaryMessage = insecureCiphers.size() > 1 ? "The following cipher strings are insecure: " :
124+
"The following cipher string is insecure: ";
125+
secondaryMessage = insecureCiphers.stream().collect(Collectors.joining("`, `", secondaryMessage + "`", "`"));
126+
127+
ctx.addIssue(callExpression.callee(), MESSAGE)
128+
.secondary(stringLiteral, secondaryMessage);
129+
}));
106130
}
107131

108132
@CheckForNull
109-
private static String unpackArgument(@Nullable Expression expression) {
133+
private static StringLiteral unpackArgument(@Nullable Expression expression) {
110134
if (expression == null) {
111135
return null;
112136
} else if (expression.is(Tree.Kind.STRING_LITERAL)) {
113-
return ((StringLiteralImpl) expression).trimmedQuotesValue();
137+
return ((StringLiteralImpl) expression);
114138
} else if (expression.is(Tree.Kind.NAME)) {
115139
return unpackArgument(Expressions.singleAssignedValue((Name) expression));
116140
} else {
117141
return null;
118142
}
119143
}
120144

121-
private static boolean containsInsecureCipher(String ciphers) {
145+
private static Set<String> findInsecureCiphers(String ciphers) {
122146
return Stream.of(ciphers)
123-
.flatMap(str -> Arrays.stream(str.split(":")))
124-
.flatMap(str -> Arrays.stream(str.split("-")))
125-
.anyMatch(INSECURE_CIPHERS::contains);
147+
.flatMap(str -> Stream.of(str.split(":")))
148+
.filter(str -> !str.startsWith("!") && !str.startsWith("-"))
149+
.flatMap(str -> Stream.of(str.split("\\+")))
150+
.flatMap(str -> Stream.of(str.split("-")))
151+
.filter(INSECURE_CIPHERS::contains)
152+
.collect(Collectors.toCollection(LinkedHashSet::new));
126153
}
127-
128-
129154
}
155+

‎python-checks/src/test/resources/checks/robustCipherAlgorithm.py‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,27 @@ def pyssl_examples():
9696
ciphers4 = 'ECDHE:RSA:AES256:SHA'
9797
ctx.set_ciphers(ciphers4) # Noncompliant
9898
ciphers5 = 'ECDHE:RSA:AES256:SHA:ECDHE-RSA-AES256-SHA'
99+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^> {{The following cipher string is insecure: `SHA`}}
99100
ctx.set_ciphers(ciphers5) # Noncompliant
101+
# ^^^^^^^^^^^^^^^
102+
103+
ctx = ssl.create_default_context()
104+
ctx.set_ciphers("NULL+SHA") # Noncompliant
105+
106+
ctx4 = ssl.create_default_context()
107+
ctx4.set_ciphers("DEFAULT:-RSA+LOW:!SHA:LOW") # Noncompliant
108+
# ^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^< {{The following cipher string is insecure: `LOW`}}
109+
110+
ctx5 = ssl.create_default_context()
111+
ctx5.set_ciphers("@SECLEVEL=1") # Noncompliant
112+
113+
ctx6 = ssl.create_default_context()
114+
ctx6.set_ciphers("@SECLEVEL=0") # Noncompliant
115+
116+
ciphers6 = 'ECDHE:RSA:AES256:LOW:ECDHE-RSA-AES256-SHA'
117+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^> {{The following cipher strings are insecure: `LOW`, `SHA`}}
118+
ctx.set_ciphers(ciphers6) # Noncompliant
119+
# ^^^^^^^^^^^^^^^
100120

101121
def pycryptodome_compliant():
102122
from Crypto.Cipher import AES
@@ -130,4 +150,9 @@ def pyssl_compliant(unknown_cipher):
130150
ctx.set_ciphers(unknown_cipher)
131151
ctx.set_ciphers(ciphers, ciphers2)
132152

153+
ctx2 = ssl.create_default_context()
154+
ctx2.set_ciphers("ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128") # Compliant
155+
156+
ctx3 = ssl.create_default_context()
157+
ctx3.set_ciphers("DEFAULT:!eNULL:!aNULL:!MD5") # Compliant
133158

0 commit comments

Comments
 (0)