Skip to content

Commit 1805c54

Browse files
joke1196sonartech
joke1196
authored and
sonartech
committed
SONARPY-2783: [S2053] Raise for salt = password for pyca/cryptography (#203)
GitOrigin-RevId: 8b2c434365166affb5ab6b28780ba31e019cb84d
1 parent 5653ef1 commit 1805c54

2 files changed

Lines changed: 120 additions & 23 deletions

File tree

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

Lines changed: 86 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -18,27 +18,34 @@
1818

1919
import java.util.ArrayList;
2020
import java.util.Map;
21+
import java.util.List;
2122
import java.util.Optional;
2223
import javax.annotation.Nullable;
2324
import org.sonar.check.Rule;
2425
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
2526
import org.sonar.plugins.python.api.SubscriptionContext;
27+
import org.sonar.plugins.python.api.tree.ArgList;
2628
import org.sonar.plugins.python.api.tree.CallExpression;
2729
import org.sonar.plugins.python.api.tree.Expression;
2830
import org.sonar.plugins.python.api.tree.Name;
31+
import org.sonar.plugins.python.api.tree.QualifiedExpression;
2932
import org.sonar.plugins.python.api.tree.RegularArgument;
3033
import org.sonar.plugins.python.api.tree.StringLiteral;
3134
import org.sonar.plugins.python.api.tree.Tree;
3235
import org.sonar.plugins.python.api.tree.UnpackingExpression;
3336
import org.sonar.python.checks.utils.Expressions;
3437
import org.sonar.python.tree.TreeUtils;
38+
import org.sonar.plugins.python.api.types.v2.TriBool;
39+
import org.sonar.python.types.v2.TypeCheckBuilder;
3540
import org.sonar.python.types.v2.TypeCheckMap;
3641

3742
@Rule(key = "S2053")
3843
public class PredictableSaltCheck extends PythonSubscriptionCheck {
3944

4045
private static final String MISSING_SALT_MESSAGE = "Add an unpredictable salt value to this hash.";
4146
private static final String PREDICTABLE_SALT_MESSAGE = "Make this salt unpredictable.";
47+
private static final String DIFFERENT_SALT_THAN_KEY_MATERIAL_MESSAGE = "Make this salt different than the derived key material.";
48+
private static final String SALT_IS_USED_HERE_MESSAGE = "The salt is used in the derive method here.";
4249
private static final String SALT_ARGUMENT_NAME = "salt";
4350
private static final String PASSWORD_ARGUMENT_NAME = "password";
4451

@@ -53,8 +60,11 @@ public class PredictableSaltCheck extends PythonSubscriptionCheck {
5360
Map.entry("Cryptodome.Protocol.KDF.bcrypt", new ArgumentInfo(2, SALT_ARGUMENT_NAME, new ArgumentInfo(0, PASSWORD_ARGUMENT_NAME))),
5461
Map.entry("Crypto.Protocol.KDF.PBKDF2", new ArgumentInfo(1, SALT_ARGUMENT_NAME, new ArgumentInfo(0, PASSWORD_ARGUMENT_NAME))),
5562
Map.entry("Crypto.Protocol.KDF.scrypt", new ArgumentInfo(1, SALT_ARGUMENT_NAME, new ArgumentInfo(0, PASSWORD_ARGUMENT_NAME))),
56-
Map.entry("Crypto.Protocol.KDF.bcrypt", new ArgumentInfo(2, SALT_ARGUMENT_NAME, false, new ArgumentInfo(0, PASSWORD_ARGUMENT_NAME)))
57-
);
63+
Map.entry("Crypto.Protocol.KDF.bcrypt", new ArgumentInfo(2, SALT_ARGUMENT_NAME, false, new ArgumentInfo(0, PASSWORD_ARGUMENT_NAME))));
64+
65+
private static final List<String> SENSITIVE_DERIVE_FUNCTIONS_FQN = List.of(
66+
"cryptography.hazmat.primitives.kdf.pbkdf2.PBKDF2HMAC.derive",
67+
"cryptography.hazmat.primitives.kdf.scrypt.Scrypt.derive");
5868

5969
private static final Map<String, ArgumentInfo> SALT_FUNCTION_ARGUMENTS_TO_CHECK = Map.of(
6070
"bytes.fromhex", new ArgumentInfo(0, "__string"),
@@ -64,25 +74,25 @@ public class PredictableSaltCheck extends PythonSubscriptionCheck {
6474
"base64.b32encode", new ArgumentInfo(0, "s"),
6575
"base64.b32decode", new ArgumentInfo(0, "s"),
6676
"base64.b16encode", new ArgumentInfo(0, "s"),
67-
"base64.b16decode", new ArgumentInfo(0, "s")
68-
);
77+
"base64.b16decode", new ArgumentInfo(0, "s"));
6978

7079
@Override
7180
public void initialize(Context context) {
7281
var sensitiveArgumentByFqnCheck = new TypeCheckMap<ArgumentInfo>();
7382
var saltFunctionArgumentsToCheck = new TypeCheckMap<ArgumentInfo>();
83+
var deriveFunctionsToCheck = new ArrayList<TypeCheckBuilder>();
7484
context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, ctx -> initializeTypeChecks(ctx, sensitiveArgumentByFqnCheck,
75-
saltFunctionArgumentsToCheck));
85+
saltFunctionArgumentsToCheck, deriveFunctionsToCheck));
7686
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, ctx -> handleCallExpression((CallExpression) ctx.syntaxNode(),
7787
ctx,
7888
sensitiveArgumentByFqnCheck,
79-
saltFunctionArgumentsToCheck
80-
));
89+
saltFunctionArgumentsToCheck,
90+
deriveFunctionsToCheck));
8191
}
8292

8393
private static void initializeTypeChecks(SubscriptionContext ctx,
8494
TypeCheckMap<ArgumentInfo> sensitiveArgumentByFqnCheck,
85-
TypeCheckMap<ArgumentInfo> saltFunctionArgumentsToCheck) {
95+
TypeCheckMap<ArgumentInfo> saltFunctionArgumentsToCheck, List<TypeCheckBuilder> deriveFunctionsToCheck) {
8696
SENSITIVE_ARGUMENT_BY_FQN.forEach((fqn, argumentNumber) -> {
8797
var checker = ctx.typeChecker().typeCheckBuilder().isTypeWithFqn(fqn);
8898
sensitiveArgumentByFqnCheck.put(checker, argumentNumber);
@@ -91,43 +101,98 @@ private static void initializeTypeChecks(SubscriptionContext ctx,
91101
var checker = ctx.typeChecker().typeCheckBuilder().isTypeWithFqn(fqn);
92102
saltFunctionArgumentsToCheck.put(checker, argumentInfo);
93103
});
104+
SENSITIVE_DERIVE_FUNCTIONS_FQN.forEach(fqn -> {
105+
var checker = ctx.typeChecker().typeCheckBuilder().isTypeWithFqn(fqn);
106+
deriveFunctionsToCheck.add(checker);
107+
});
94108
}
95109

96110
private static void handleCallExpression(CallExpression callExpression, SubscriptionContext ctx,
97-
TypeCheckMap<ArgumentInfo> sensitiveArgumentByFqnCheck, TypeCheckMap<ArgumentInfo> saltFunctionArgumentsToCheck) {
111+
TypeCheckMap<ArgumentInfo> sensitiveArgumentByFqnCheck, TypeCheckMap<ArgumentInfo> saltFunctionArgumentsToCheck, List<TypeCheckBuilder> deriveFunctionsToCheck) {
98112
Optional.of(callExpression)
99113
.map(CallExpression::callee)
100114
.map(Expression::typeV2)
101115
.map(sensitiveArgumentByFqnCheck::getForType)
102-
.ifPresent(argumentInfo -> checkArguments(callExpression, argumentInfo, ctx, saltFunctionArgumentsToCheck));
116+
.ifPresent(argumentInfo -> checkArguments(callExpression, argumentInfo, ctx, saltFunctionArgumentsToCheck, deriveFunctionsToCheck));
103117
}
104118

105119
private static void checkArguments(CallExpression callExpression, ArgumentInfo argumentInfo, SubscriptionContext ctx,
106-
TypeCheckMap<ArgumentInfo> saltFunctionArgumentsToCheck) {
120+
TypeCheckMap<ArgumentInfo> saltFunctionArgumentsToCheck, List<TypeCheckBuilder> deriveFunctionsToCheck) {
107121
var argument = TreeUtils.nthArgumentOrKeyword(argumentInfo.position(), argumentInfo.name(), callExpression.arguments());
108122
if (argument != null) {
109-
var raised = checkSensitiveArgument(argument, ctx, saltFunctionArgumentsToCheck);
110-
if (!raised) {
111-
Optional.ofNullable(argumentInfo.shouldNotBeSameAsArgument())
112-
.map(ai -> TreeUtils.nthArgumentOrKeyword(ai.position(), ai.name(), callExpression.arguments()))
113-
.ifPresent(shouldNotBeSameAsArgument -> checkForSameArguments(argument, shouldNotBeSameAsArgument, ctx));
123+
if (hasRaisedOnSensitiveArgument(argument, ctx, saltFunctionArgumentsToCheck)) {
124+
return;
125+
}
126+
if (hasRaisedOnSameArgument(argument, argumentInfo, callExpression, ctx)) {
127+
return;
128+
}
129+
if (hasRaisedOnSameSaltAndDerivedKeyMaterial(argument, deriveFunctionsToCheck, ctx)) {
130+
return;
114131
}
132+
115133
} else if (argumentInfo.required() && callExpression.arguments().stream().noneMatch(UnpackingExpression.class::isInstance)) {
116134
ctx.addIssue(callExpression.callee(), MISSING_SALT_MESSAGE);
117135
}
118136
}
119137

120-
private static void checkForSameArguments(RegularArgument argument, RegularArgument shouldNotBeSameAsArgument, SubscriptionContext ctx) {
138+
private static boolean hasRaisedOnSameSaltAndDerivedKeyMaterial(RegularArgument argument, List<TypeCheckBuilder> deriveFunctionsToCheck, SubscriptionContext ctx) {
139+
if (!argument.expression().is(Tree.Kind.NAME)) {
140+
return false;
141+
}
142+
var symbol = ((Name) argument.expression()).symbolV2();
143+
if (symbol == null) {
144+
return false;
145+
}
146+
var usagesInDeriveCall = symbol.usages().stream()
147+
.map(usage -> usage.tree().parent())
148+
.flatMap(TreeUtils.toStreamInstanceOfMapper(RegularArgument.class))
149+
.filter(regArg -> !regArg.equals(argument))
150+
.filter(regArg -> isUsedInDeriveCall(regArg, deriveFunctionsToCheck))
151+
.map(RegularArgument::expression)
152+
.toList();
153+
154+
if (usagesInDeriveCall.isEmpty()) {
155+
return false;
156+
}
157+
var issue = ctx.addIssue(argument, DIFFERENT_SALT_THAN_KEY_MATERIAL_MESSAGE);
158+
usagesInDeriveCall.stream().forEach(arg -> issue.secondary(arg, SALT_IS_USED_HERE_MESSAGE));
159+
return true;
160+
}
161+
162+
private static boolean isUsedInDeriveCall(RegularArgument regularArgument, List<TypeCheckBuilder> deriveFunctionsToCheck) {
163+
return Optional.ofNullable(regularArgument.parent())
164+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(ArgList.class))
165+
.map(ArgList::parent)
166+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(CallExpression.class))
167+
.map(CallExpression::callee)
168+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(QualifiedExpression.class))
169+
.map(qualifiedExpr -> deriveFunctionsToCheck.stream()
170+
.anyMatch(deriveFunction -> deriveFunction.check(qualifiedExpr.name().typeV2()) == TriBool.TRUE))
171+
.orElse(false);
172+
173+
}
174+
175+
private static boolean hasRaisedOnSameArgument(RegularArgument argument, ArgumentInfo argumentInfo, CallExpression callExpression, SubscriptionContext ctx) {
176+
return Optional.ofNullable(argumentInfo.shouldNotBeSameAsArgument())
177+
.map(ai -> TreeUtils.nthArgumentOrKeyword(ai.position(), ai.name(), callExpression.arguments()))
178+
.map(shouldNotBeSameAsArgument -> raisedOnSameArgument(argument, shouldNotBeSameAsArgument, ctx))
179+
.orElseGet(() -> false);
180+
181+
}
182+
183+
private static boolean raisedOnSameArgument(RegularArgument argument, RegularArgument shouldNotBeSameAsArgument, SubscriptionContext ctx) {
121184
var exp1 = argument.expression();
122185
var exp2 = shouldNotBeSameAsArgument.expression();
123186
if (exp1 instanceof Name n1
124-
&& exp2 instanceof Name n2
125-
&& n1.symbolV2() == n2.symbolV2()) {
187+
&& exp2 instanceof Name n2
188+
&& n1.symbolV2() == n2.symbolV2()) {
126189
ctx.addIssue(argument, PREDICTABLE_SALT_MESSAGE).secondary(shouldNotBeSameAsArgument, "");
190+
return true;
127191
}
192+
return false;
128193
}
129194

130-
private static boolean checkSensitiveArgument(RegularArgument regularArgument,
195+
private static boolean hasRaisedOnSensitiveArgument(RegularArgument regularArgument,
131196
SubscriptionContext ctx, TypeCheckMap<ArgumentInfo> saltFunctionArgumentsToCheck) {
132197
var secondaries = new ArrayList<Tree>();
133198
var expression = regularArgument.expression();
@@ -163,7 +228,7 @@ private static Expression getCallExpressionArgumentValueToCheck(TypeCheckMap<Arg
163228
.map(Expression::typeV2)
164229
.map(saltFunctionArgumentsToCheck::getForType)
165230
.map(argumentInfo -> Optional.ofNullable(TreeUtils.nthArgumentOrKeyword(argumentInfo.position(), argumentInfo.name(),
166-
callExpression.arguments()))
231+
callExpression.arguments()))
167232
.map(RegularArgument::expression)
168233
.orElse(expression))
169234
.orElse(null);
@@ -179,5 +244,4 @@ private ArgumentInfo(int position, String name, ArgumentInfo shouldNotBeSameAsAr
179244
}
180245
}
181246

182-
183247
}

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

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ def func():
7575
from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC
7676
from cryptography.hazmat.backends import default_backend
7777

78-
def scrypt_salt():
78+
def scrypt_salt(password):
7979
from cryptography.hazmat.primitives.kdf.scrypt import Scrypt
8080
Scrypt(
8181
salt="abc" # Noncompliant {{Make this salt unpredictable.}}
@@ -84,6 +84,18 @@ def scrypt_salt():
8484
"abc" # Noncompliant {{Make this salt unpredictable.}}
8585
)
8686

87+
hasher = Scrypt(
88+
salt=password # Noncompliant {{Make this salt different than the derived key material.}}
89+
)
90+
hasher.derive(password)
91+
# <^^^^^^^^ {{The salt is used in the derive method here.}}
92+
93+
salt_ = os.urandom(16)
94+
hasher = Scrypt(
95+
salt=salt_ # Noncompliant {{Make this salt different than the derived key material.}}
96+
).derive(key_material=salt_)
97+
# <^^^^^ {{The salt is used in the derive method here.}}
98+
8799
def derive_password(password, salt, backend):
88100
kdf = PBKDF2HMAC(
89101
algorithm=hashes.SHA256(),
@@ -114,6 +126,27 @@ def derive_password(password, salt, backend):
114126
)
115127
key = kdf.derive(password)
116128

129+
some_salt = os.urandom(16)
130+
kdf = PBKDF2HMAC(
131+
algorithm=hashes.SHA256(),
132+
length=32,
133+
salt=some_salt, # FN PBKDF2HMAC stubs are not resolved properly this should be solved with SONARPY-2053
134+
iterations=100000,
135+
backend=backend
136+
)
137+
key = kdf.derive(key_material=some_salt)
138+
139+
other_salt = os.urandom(16)
140+
PBKDF2HMAC(
141+
algorithm=hashes.SHA256(),
142+
length=32,
143+
salt=other_salt, # FN PBKDF2HMAC stubs are not resolved properly this should be solved with SONARPY-2053
144+
iterations=100000,
145+
backend=backend
146+
).derive(key_material=other_salt)
147+
148+
149+
117150
salt = os.urandom(16)
118151
backend = default_backend()
119152
derive_password(b"my great password", salt, backend)

0 commit comments

Comments
 (0)