Skip to content

Commit 06240ff

Browse files
leveretkaquentin-jaquier-sonarsource
leveretka
authored and
quentin-jaquier-sonarsource
committed
SONARJAVA-2929 Rule S2053: Remove FP when salt added via PBEParameterSpec
1 parent 45755e0 commit 06240ff

File tree

4 files changed

+52
-54
lines changed

4 files changed

+52
-54
lines changed

‎java-checks-test-sources/src/main/files/non-compiling/checks/security/UnpredictableSaltCheck.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import java.util.Random;
99
import javax.crypto.SecretKeyFactory;
1010
import javax.crypto.spec.PBEKeySpec;
11+
import javax.crypto.spec.PBEParameterSpec;
1112
import org.springframework.stereotype.Controller;
1213
import org.springframework.web.bind.annotation.RequestMapping;
1314
import org.springframework.web.bind.annotation.RequestMethod;
@@ -17,17 +18,20 @@ public class UnpredictableSaltCheck {
1718
private static byte[] sSalt = new byte[];
1819

1920
public void checkPBEKeySpecWithUnknownSalt(char[] chars) throws NoSuchAlgorithmException {
20-
PBEKeySpec spec = new PBEKeySpec(chars); // Noncompliant
21+
PBEKeySpec spec = new PBEKeySpec(chars);
2122
PBEKeySpec spec1 = new PBEKeySpec(chars, salt, 1); // Compliant
2223
PBEKeySpec spec2 = new PBEKeySpec(chars, salt(), 1); // Compliant
2324
}
2425

2526
public void checkPBEKeySpec(char[] chars) throws NoSuchAlgorithmException {
2627
var salt = new byte[]{};
27-
PBEKeySpec spec = new PBEKeySpec(chars); // Noncompliant
28+
PBEKeySpec spec = new PBEKeySpec(chars);
2829
PBEKeySpec spec1 = new PBEKeySpec(chars, salt, 1); // Compliant
2930
PBEKeySpec spec2 = new PBEKeySpec(chars, salt(), 1); // Compliant
3031
PBEKeySpec spec3 = new PBEKeySpec(chars, sSalt, 1); // Compliant
32+
33+
new PBEParameterSpec("notrandom".getBytes(), 10000); // Noncompliant
34+
new PBEParameterSpec(sSalt, 10000); // Compliant
3135
}
3236

3337
}

‎java-checks-test-sources/src/main/java/checks/security/UnpredictableSaltCheck.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@
44
import java.security.MessageDigest;
55
import java.security.NoSuchAlgorithmException;
66
import javax.crypto.spec.PBEKeySpec;
7+
import javax.crypto.spec.PBEParameterSpec;
78

89
public class UnpredictableSaltCheck {
910

1011
public void testPBESpec(char[] chars, byte[] salt) throws NoSuchAlgorithmException {
11-
PBEKeySpec spec = new PBEKeySpec(chars); // Noncompliant [[sc=23;ec=44]] {{Add an unpredictable salt value to this hash.}}
12+
PBEKeySpec spec = new PBEKeySpec(chars); // Compliant
1213
PBEKeySpec spec1 = new PBEKeySpec(chars, salt, 1); // Compliant
1314
}
1415

@@ -36,6 +37,10 @@ public void testNonRandomPBESpec(char[] chars, String str) throws NoSuchAlgorith
3637

3738
PBEKeySpec spec6 = new PBEKeySpec(chars, new MyMessageDigest("").engineDigest(), 2); // Compliant
3839
PBEKeySpec spec666 = new PBEKeySpec(chars, str.getBytes(), 2); // Compliant
40+
41+
new PBEParameterSpec("notrandom".getBytes(), 10000); // Noncompliant {{Make this salt unpredictable.}}
42+
new PBEParameterSpec(salt, 10000); // Noncompliant [[sc=5;ec=38;secondary=17]]
43+
new PBEParameterSpec(finalSalt, 10000); // Noncompliant [[sc=5;ec=43;secondary=20]]
3944
}
4045

4146
private byte[] secureSalt() {

‎java-checks/src/main/java/org/sonar/java/checks/security/UnpredictableSaltCheck.java‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,21 @@
4242
@Rule(key = "S2053")
4343
public class UnpredictableSaltCheck extends IssuableSubscriptionVisitor {
4444

45-
private static final String ADD_SALT = "Add an unpredictable salt value to this hash.";
4645
private static final String UNPREDICTABLE_SALT = "Make this salt unpredictable.";
4746

47+
private static final String BYTE_ARRAY = "byte[]";
4848
private static final MethodMatchers NEW_PBE_KEY_SPEC = MethodMatchers.create()
4949
.ofSubTypes("javax.crypto.spec.PBEKeySpec")
5050
.constructor()
51-
.withAnyParameters()
51+
.addParametersMatcher("char[]", BYTE_ARRAY, "int", "int")
52+
.addParametersMatcher("char[]", BYTE_ARRAY, "int")
53+
.build();
54+
55+
private static final MethodMatchers NEW_PBE_PARAM_SPEC = MethodMatchers.create()
56+
.ofSubTypes("javax.crypto.spec.PBEParameterSpec")
57+
.constructor()
58+
.addParametersMatcher(BYTE_ARRAY, "int")
59+
.addParametersMatcher(BYTE_ARRAY, "int", "java.security.spec.AlgorithmParameterSpec")
5260
.build();
5361

5462
private static final MethodMatchers GET_BYTES = MethodMatchers.create()
@@ -65,17 +73,23 @@ public List<Tree.Kind> nodesToVisit() {
6573
@Override
6674
public void visitNode(Tree tree) {
6775
NewClassTree newClassTree = (NewClassTree) tree;
68-
if (NEW_PBE_KEY_SPEC.matches(newClassTree)) {
69-
if (newClassTree.arguments().size() <= 1) {
70-
reportIssue(newClassTree, ADD_SALT);
71-
} else {
72-
ExpressionTree saltExpression = ExpressionUtils.skipParentheses(newClassTree.arguments().get(1));
73-
List<JavaFileScannerContext.Location> locations = new ArrayList<>();
74-
if (isPredictable(saltExpression, locations)) {
75-
reportIssue(newClassTree, UNPREDICTABLE_SALT, locations, null);
76-
}
76+
saltExpression(((NewClassTree) tree))
77+
.map(ExpressionUtils::skipParentheses)
78+
.ifPresent(salt -> {
79+
List<JavaFileScannerContext.Location> locations = new ArrayList<>();
80+
if (isPredictable(salt, locations)) {
81+
reportIssue(newClassTree, UNPREDICTABLE_SALT, locations, null);
7782
}
83+
});
84+
}
85+
86+
private static Optional<ExpressionTree> saltExpression(NewClassTree tree) {
87+
if (NEW_PBE_KEY_SPEC.matches(tree)) {
88+
return Optional.of(tree.arguments().get(1));
89+
} else if (NEW_PBE_PARAM_SPEC.matches(tree)) {
90+
return Optional.of(tree.arguments().get(0));
7891
}
92+
return Optional.empty();
7993
}
8094

8195
private static boolean isPredictable(ExpressionTree saltExpression, List<JavaFileScannerContext.Location> locations) {

‎java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S2053_java.html‎

Lines changed: 15 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,56 +1,31 @@
1-
<p>In cryptography, "salt" is extra piece of data which is included in a hashing algorithm. It makes dictionary attacks more difficult. Using a
2-
cryptographic hash function without an unpredictable salt increases the likelihood that an attacker will be able to successfully guess a hashed value
3-
such as a password with a dictionary attack.</p>
4-
<p>This rule raises an issue when a hashing function which has been specifically designed for hashing sensitive data, such as PBKDF2, is used with a
5-
non-random, reused or too short salt value. It does not raise an issue on base hashing algorithms such as sha1 or md5 as these are often used for
6-
other purposes.</p>
1+
<p>In cryptography, a "salt" is an extra piece of data which is included when hashing a password. This makes <code>rainbow-table attacks</code> more
2+
difficult. Using a cryptographic hash function without an unpredictable salt increases the likelihood that an attacker could successfully find the
3+
hash value in databases of precomputed hashes (called <code>rainbow-tables</code>).</p>
4+
<p>This rule raises an issue when a hashing function which has been specifically designed for hashing passwords, such as <code>PBKDF2</code>, is used
5+
with a non-random, reused or too short salt value. It does not raise an issue on base hashing algorithms such as <code>sha1</code> or <code>md5</code>
6+
as they should not be used to hash passwords.</p>
77
<h2>Recommended Secure Coding Practices</h2>
88
<ul>
9-
<li> Use hashing functions generating their own salt or generate a long random salt of at least 32 bytes. </li>
10-
<li> The salt is at least as long as the resulting hash value. </li>
11-
<li> Provide the salt to a safe hashing function such as PBKDF2. </li>
12-
<li> Save both the salt and the hashed value in the relevant database record; during future validation operations, the salt and hash can then be
13-
retrieved from the database. The hash is recalculated with the stored salt and the value being validated, and the result compared to the stored
14-
hash. </li>
9+
<li> Use hashing functions generating their own secure salt or generate a secure random value of at least 16 bytes. </li>
10+
<li> The salt should be unique by user password. </li>
1511
</ul>
1612
<h2>Noncompliant Code Example</h2>
17-
<p>Below, the hashed password is not salted:</p>
18-
<pre>
19-
MessageDigest md = MessageDigest.getInstance("SHA-512");
20-
byte[] hashedPassword = md.digest(passwordToHash.getBytes(StandardCharsets.UTF_8)); // Noncompliant
21-
</pre>
22-
<pre>
23-
MessageDigest md = MessageDigest.getInstance("SHA-512");
24-
md.update(passwordToHash.getBytes(StandardCharsets.UTF_8));
25-
byte[] hashedPassword = md.digest(); // Noncompliant, only one "update()" call and "digest()" without parameters
26-
</pre>
27-
<pre>
28-
MessageDigest md = MessageDigest.getInstance("SHA-512");
29-
byte[] hashedPassword = md.digest(passwordToHash.getBytes(StandardCharsets.UTF_8)); // Noncompliant, no "update()" call
30-
</pre>
31-
<pre>
32-
PBEKeySpec spec = new PBEKeySpec(chars); // Noncompliant, no salt as an argument
33-
</pre>
13+
<p>Below, the hashed password use a predictable salt:</p>
3414
<pre>
3515
byte[] salt = "notrandom".getBytes();
36-
PBEKeySpec spec = new PBEKeySpec(chars, salt); // Noncompliant, predictable salt
16+
17+
PBEParameterSpec cipherSpec = new PBEParameterSpec(salt, 10000); // Noncompliant, predictable salt
18+
PBEKeySpec spec = new PBEKeySpec(chars, salt, 10000, 256); // Noncompliant, predictable salt
3719
</pre>
3820
<h2>Compliant Solution</h2>
39-
<p>Use <code>java.security.SecureRandom</code> to produce a unpredictable salt:</p>
21+
<p>Use <code>java.security.SecureRandom</code> to generate an unpredictable salt:</p>
4022
<pre>
41-
MessageDigest md = MessageDigest.getInstance("SHA-512");
42-
4323
SecureRandom random = new SecureRandom();
4424
byte[] salt = new byte[16];
4525
random.nextBytes(salt);
4626

47-
md.update(salt);
48-
49-
byte[] hashedPassword = md.digest(passwordToHash.getBytes(StandardCharsets.UTF_8)); // Compliant
50-
</pre>
51-
<pre>
52-
byte[] salt = this.secureSalt();
53-
PBEKeySpec spec = new PBEKeySpec(chars, salt); // Compliant
27+
PBEParameterSpec cipherSpec = new PBEParameterSpec(salt, 10000); // Compliant
28+
PBEKeySpec spec = new PBEKeySpec(chars, salt, 10000, 256); // Compliant
5429
</pre>
5530
<h2>See</h2>
5631
<ul>

0 commit comments

Comments
 (0)