diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json index dd83ce440c0..f304d559ef1 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json @@ -1,6 +1,6 @@ { "ruleKey": "S1874", "hasTruePositives": true, - "falseNegatives": 246, + "falseNegatives": 248, "falsePositives": 0 } diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2119.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2119.json index e7d7377f547..3f89e139fb0 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2119.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2119.json @@ -1,6 +1,6 @@ { "ruleKey": "S2119", "hasTruePositives": true, - "falseNegatives": 2, + "falseNegatives": 3, "falsePositives": 0 } diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2245.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2245.json index b208b729c10..f4f66daaafd 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2245.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2245.json @@ -1,6 +1,6 @@ { "ruleKey": "S2245", "hasTruePositives": true, - "falseNegatives": 26, + "falseNegatives": 17, "falsePositives": 0 } diff --git a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2245.json b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2245.json index d04ce975280..b7aae18f176 100644 --- a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2245.json +++ b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2245.json @@ -1,7 +1,4 @@ { -"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/ShutdownMonitor.java": [ -287 -], "org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionIdManager.java": [ 369 ] diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S2245.json b/its/ruling/src/test/resources/eclipse-jetty/java-S2245.json index d04ce975280..b7aae18f176 100644 --- a/its/ruling/src/test/resources/eclipse-jetty/java-S2245.json +++ b/its/ruling/src/test/resources/eclipse-jetty/java-S2245.json @@ -1,7 +1,4 @@ { -"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/ShutdownMonitor.java": [ -287 -], "org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionIdManager.java": [ 369 ] diff --git a/its/ruling/src/test/resources/mall/java-S2245.json b/its/ruling/src/test/resources/mall/java-S2245.json index 1511b370aaf..8815173f31d 100644 --- a/its/ruling/src/test/resources/mall/java-S2245.json +++ b/its/ruling/src/test/resources/mall/java-S2245.json @@ -1,7 +1,4 @@ { -"com.macro.mall:mall:mall-portal/src/main/java/com/macro/mall/portal/service/impl/UmsMemberCouponServiceImpl.java": [ -91 -], "com.macro.mall:mall:mall-portal/src/main/java/com/macro/mall/portal/service/impl/UmsMemberServiceImpl.java": [ 112 ] diff --git a/its/ruling/src/test/resources/sonar-server/java-S2245.json b/its/ruling/src/test/resources/sonar-server/java-S2245.json index ec56a575411..0967ef424bc 100644 --- a/its/ruling/src/test/resources/sonar-server/java-S2245.json +++ b/its/ruling/src/test/resources/sonar-server/java-S2245.json @@ -1,5 +1 @@ -{ -"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/es/RecoveryIndexer.java": [ -92 -] -} +{} diff --git a/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckCryptoImportSample.java b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckCryptoImportSample.java new file mode 100644 index 00000000000..61648f18961 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckCryptoImportSample.java @@ -0,0 +1,22 @@ +package checks; + +import java.util.Random; +import org.bouncycastle.jce.provider.BouncyCastleProvider; + +// SONARJAVA-6440 Part 1: file imports `org.bouncycastle.*` -> all PRNG calls flagged +// regardless of any per-scope keyword check. +class PseudoRandomCheckCryptoImportSample { + + // BouncyCastle reference so the import is used. + static final BouncyCastleProvider PROVIDER = new BouncyCastleProvider(); + + void noKeywordsInScope() { + int counter = 0; + Random r = new Random(); // Noncompliant {{Make sure that using this pseudorandom number generator is safe here.}} + counter += r.nextInt(); + } + + double anotherNeutralMethod() { + return Math.random(); // Noncompliant + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckFieldScopeSample.java b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckFieldScopeSample.java new file mode 100644 index 00000000000..4039f2d05fc --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckFieldScopeSample.java @@ -0,0 +1,33 @@ +package checks; + +import java.util.Random; + +// SONARJAVA-6440: no crypto import. Random call appears outside any method (field initializer +// and static initializer). Scope falls back to the enclosing class; the class identifiers +// drive the keyword check. + +class PseudoRandomCheckFieldScopeSampleSecure { + // Class name contains keyword: `TokenGenerator` tokens -> [token, generator]. `token` matches. + static class TokenGenerator { + static final Random RNG = new Random(); // Noncompliant {{Make sure that using this pseudorandom number generator is safe here.}} + } + + // No method scope, but a sibling field name contains a security keyword. + static class Holder { + String password; + static final Random R = new Random(); // Noncompliant + } + + // Static initializer: no enclosing method; class identifiers carry the keyword. + static class CipherBundle { + static Random r; + static { + r = new Random(); // Noncompliant + } + } +} + +class PseudoRandomCheckFieldScopeSampleNeutral { + // No method, no security keyword anywhere in the enclosing class -> Compliant. + static final Random R = new Random(); // Compliant +} diff --git a/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckNoContextSample.java b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckNoContextSample.java new file mode 100644 index 00000000000..8e1c5da2ad8 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckNoContextSample.java @@ -0,0 +1,30 @@ +package checks; + +import java.util.Random; +import java.util.concurrent.ThreadLocalRandom; +import org.apache.commons.lang.math.JVMRandom; +import org.apache.commons.lang.math.RandomUtils; +import org.apache.commons.lang.RandomStringUtils; + +// SONARJAVA-6440: no crypto import, no security-keyword identifiers in scope -> +// the security-context heuristic suppresses the issue. +class PseudoRandomCheckNoContextSample { + + void neutralMethod() { + Random r = new Random(); // Compliant + int v = r.nextInt(); + + JVMRandom j = new JVMRandom(); // Compliant + double d1 = j.nextDouble(); + + double d2 = Math.random(); // Compliant + int v2 = ThreadLocalRandom.current().nextInt(); // Compliant + + float f1 = RandomUtils.nextFloat(); // Compliant + String s1 = RandomStringUtils.random(1); // Compliant + } + + int doWork(int input) { + return input + 1; + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckSecurityKeywordsSample.java b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckSecurityKeywordsSample.java new file mode 100644 index 00000000000..98d24ce8006 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckSecurityKeywordsSample.java @@ -0,0 +1,89 @@ +package checks; + +import java.util.Random; +import java.util.concurrent.ThreadLocalRandom; +import org.apache.commons.lang3.RandomUtils; +import org.apache.commons.lang3.RandomStringUtils; + +// SONARJAVA-6440: no crypto import. Security keywords reached via per-scope identifier scan. +class PseudoRandomCheckSecurityKeywordsSample { + + // --- Method-scope keyword tokenized from camelCase (`userPassword` -> [user, password]). --- + void camelCaseLocal() { + String userPassword = "x"; + Random r = new Random(); // Noncompliant {{Make sure that using this pseudorandom number generator is safe here.}} + r.nextInt(); + } + + // --- Method-scope keyword tokenized from snake_case (`user_token` -> [user, token]). --- + void snakeCaseLocal() { + String user_token = "x"; + double d = Math.random(); // Noncompliant + } + + // --- Method-scope keyword from all-uppercase (`HMAC` -> [hmac]). --- + void upperCaseLocal() { + final int HMAC = 32; + int v = ThreadLocalRandom.current().nextInt(); // Noncompliant + } + + // --- Keyword in the method name itself. --- + void encryptPayload() { + float f = RandomUtils.nextFloat(); // Noncompliant + } + + // --- Keyword in a parameter name. --- + String randomFromToken(String token) { + return RandomStringUtils.random(1); // Noncompliant + } + + // --- Whole-identifier match (`password`). --- + void wholeIdentifierMatch() { + String password = "x"; + Random r = new Random(); // Noncompliant + } + + // --- No keyword in scope: must NOT be flagged (heuristic suppresses it). --- + void unrelatedScope() { + int counter = 0; + Random r = new Random(); // Compliant + counter += r.nextInt(); + } + + // --- camelCase that DOES NOT match keyword after tokenization. --- + // `randomBytes` tokenizes to [random, bytes]; neither is in the keyword set + // (the keyword `randombytes` only matches an all-lowercase or all-uppercase literal). + void splitRandomBytes() { + byte[] randomBytes = new byte[16]; + Random r = new Random(); // Compliant + r.nextBytes(randomBytes); + } + + // --- Digit-suffixed all-uppercase crypto acronym (Dart-faithful behaviour). --- + // `AES256_KEY` splits on `_` to ["AES256", "KEY"]. The "AES256" part has no lowercase + // letter, so isAllUppercaseWithLetter returns true and it is kept as the single token + // `aes256` (NOT split into `aes` + `256`). The keyword set holds `aes`, not `aes256`, + // so no token matches. `KEY` -> `[key]`, but `key` is not in the Java keyword set + // (it is in the Kotlin set; SONARKT-770 catches this case in Kotlin only). + // This is intentional: the tokenizer mirrors DART-276's _splitIntoWords exactly to + // keep cross-language behaviour aligned. Improving the tokenizer to split letters + // from trailing digits would help here AND in cases like `ChaCha20` (where the keyword + // `chacha20` is also unreachable today) but is out of scope for this faithful port. + // Tracked separately under APPSEC-3004. + void digitSuffixedAcronym() { + final int AES256_KEY = 32; + Random r = new Random(); // Compliant + r.nextInt(AES256_KEY); + } +} + +// --- Inner-class scope isolation. --- +// The outer class name `TokenAware` contains the security keyword `token`, but the +// PRNG call lives in an inner class with neutral identifiers. findDeclarationScope +// returns the inner ClassTree first, so the outer's keyword is NOT in scope. +class TokenAware { + static class NeutralInner { + static final Random R = new Random(); // Compliant + } +} + diff --git a/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckStaticImportSample.java b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckStaticImportSample.java new file mode 100644 index 00000000000..c89a240caa1 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckStaticImportSample.java @@ -0,0 +1,19 @@ +package checks; + +import static javax.crypto.Cipher.ENCRYPT_MODE; + +import java.util.Random; + +// SONARJAVA-6440 Part 1: static import of a crypto class still parses as Tree.Kind.IMPORT, +// so the concatenated name `javax.crypto.Cipher.ENCRYPT_MODE` matches the `javax.crypto.` +// prefix and the file-level crypto-import gate fires. +class PseudoRandomCheckStaticImportSample { + + static final int MODE = ENCRYPT_MODE; // retain the static import + + void noKeywordsInScope() { + int counter = 0; + Random r = new Random(); // Noncompliant {{Make sure that using this pseudorandom number generator is safe here.}} + counter += r.nextInt(); + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckWildcardImportSample.java b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckWildcardImportSample.java new file mode 100644 index 00000000000..be87893da26 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckWildcardImportSample.java @@ -0,0 +1,22 @@ +package checks; + +import java.security.*; +import java.util.Random; + +// SONARJAVA-6440 Part 1: wildcard import `java.security.*` -> all PRNG calls flagged. +// ExpressionsHelper.concatenate returns "java.security.*", which still starts with the +// "java.security." prefix, so the file-level crypto-import check fires. +class PseudoRandomCheckWildcardImportSample { + + static final KeyPair PROVIDER_KEYPAIR = null; // forces the wildcard import to be retained + + void noKeywordsInScope() { + int counter = 0; + Random r = new Random(); // Noncompliant {{Make sure that using this pseudorandom number generator is safe here.}} + counter += r.nextInt(); + } + + double anotherNeutralMethod() { + return Math.random(); // Noncompliant + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/PseudoRandomCheck.java b/java-checks/src/main/java/org/sonar/java/checks/PseudoRandomCheck.java index 6ae9b886afb..8e386da5276 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/PseudoRandomCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/PseudoRandomCheck.java @@ -16,18 +16,32 @@ */ package org.sonar.java.checks; +import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.List; +import java.util.Locale; +import java.util.Map; import java.util.Set; import org.sonar.check.Rule; -import org.sonarsource.analyzer.commons.collections.SetUtils; +import org.sonar.java.checks.helpers.ExpressionsHelper; import org.sonar.java.model.ExpressionUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.CompilationUnitTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.ImportClauseTree; +import org.sonar.plugins.java.api.tree.ImportTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.NewClassTree; import org.sonar.plugins.java.api.tree.Tree; +import org.sonarsource.analyzer.commons.collections.SetUtils; @Rule(key = "S2245") public class PseudoRandomCheck extends IssuableSubscriptionVisitor { @@ -66,27 +80,57 @@ public class PseudoRandomCheck extends IssuableSubscriptionVisitor { "org.apache.commons.lang.math.JVMRandom" ); + private static final List CRYPTO_IMPORT_PREFIXES = List.of( + "java.security.", + "javax.crypto.", + "org.springframework.security.", + "org.bouncycastle.", + "io.jsonwebtoken.", + "com.auth0.jwt.", + "at.favre.lib.crypto.", + "de.mkammerer.argon2." + ); + + private static final Set SECURITY_KEYWORDS = SetUtils.immutableSetOf( + "aes", "asymmetric", "auth", "certificate", "chacha20", "cipher", "crypto", "cryptography", + "decrypt", "ecc", "encrypt", "encryption", "hmac", "iv", "nonce", "password", "pbkdf2", + "poly1305", "randombytes", "rsa", "salt", "scrypt", "secret", "secure", "security", + "sessionid", "signature", "token", "verify" + ); + + private boolean cryptoImportPresent = false; + private final Map scopeSecurityContextCache = new IdentityHashMap<>(); + @Override public List nodesToVisit() { - return Arrays.asList(Tree.Kind.NEW_CLASS, Tree.Kind.METHOD_INVOCATION); + return Arrays.asList(Tree.Kind.COMPILATION_UNIT, Tree.Kind.NEW_CLASS, Tree.Kind.METHOD_INVOCATION); } @Override public void visitNode(Tree tree) { + if (tree.is(Tree.Kind.COMPILATION_UNIT)) { + cryptoImportPresent = hasCryptoImport((CompilationUnitTree) tree); + return; + } if (tree instanceof MethodInvocationTree mit) { - IdentifierTree reportLocation = ExpressionUtils.methodName(mit); - - if (isStaticCallToInsecureRandomMethod(mit)) { - reportIssue(reportLocation, MESSAGE); + if (isStaticCallToInsecureRandomMethod(mit) && isInSecurityContext(mit)) { + reportIssue(ExpressionUtils.methodName(mit), MESSAGE); } } else { NewClassTree newClass = (NewClassTree) tree; - if (RANDOM_CONSTRUCTOR_TYPES.contains(newClass.symbolType().fullyQualifiedName())) { + if (RANDOM_CONSTRUCTOR_TYPES.contains(newClass.symbolType().fullyQualifiedName()) + && isInSecurityContext(newClass)) { reportIssue(newClass.identifier(), MESSAGE); } } } + @Override + public void leaveFile(JavaFileScannerContext context) { + cryptoImportPresent = false; + scopeSecurityContextCache.clear(); + } + private static boolean isStaticCallToInsecureRandomMethod(MethodInvocationTree mit) { return STATIC_RANDOM_METHODS.matches(mit) && !RANDOM_STRING_UTILS_RANDOM_WITH_RANDOM_SOURCE.matches(mit) @@ -94,4 +138,111 @@ private static boolean isStaticCallToInsecureRandomMethod(MethodInvocationTree m && mit.methodSymbol().isStatic(); } + private static boolean hasCryptoImport(CompilationUnitTree cut) { + for (ImportClauseTree importClause : cut.imports()) { + if (importClause.is(Tree.Kind.IMPORT) + && ((ImportTree) importClause).qualifiedIdentifier() instanceof ExpressionTree exprTree + && matchesCryptoPrefix(ExpressionsHelper.concatenate(exprTree))) { + return true; + } + } + return false; + } + + private static boolean matchesCryptoPrefix(String importName) { + for (String prefix : CRYPTO_IMPORT_PREFIXES) { + if (importName.startsWith(prefix)) { + return true; + } + } + return false; + } + + private boolean isInSecurityContext(Tree tree) { + if (cryptoImportPresent) { + return true; + } + Tree scope = findDeclarationScope(tree); + if (scope == null) { + // Defensive: shouldn't happen in valid Java (every PRNG call has an enclosing + // method or class). Fail open and flag to avoid silent false negatives. + return true; + } + return scopeSecurityContextCache.computeIfAbsent(scope, PseudoRandomCheck::scopeHasSecurityKeyword); + } + + private static boolean scopeHasSecurityKeyword(Tree scope) { + IdentifierCollector collector = new IdentifierCollector(); + scope.accept(collector); + for (String identifier : collector.identifiers) { + for (String token : tokenizeIdentifier(identifier)) { + if (SECURITY_KEYWORDS.contains(token)) { + return true; + } + } + } + return false; + } + + // Mirrors Dart's `declarationScope`: the closest enclosing MethodTree (method/constructor) for + // local code; falls back to the enclosing ClassTree for field/initializer-block code. + private static Tree findDeclarationScope(Tree tree) { + Tree current = tree.parent(); + while (current != null) { + if (current instanceof MethodTree methodTree) { + return methodTree; + } + if (current instanceof ClassTree classTree) { + return classTree; + } + current = current.parent(); + } + return null; + } + + // Mirrors Dart's `_splitIntoWords`: split on underscores first; for each part either keep it as + // a single lowercase word when all-uppercase, or split further on capital-letter boundaries. + static List tokenizeIdentifier(String identifier) { + List words = new ArrayList<>(); + for (String part : identifier.split("_")) { + if (part.isEmpty()) { + continue; + } + if (isAllUppercaseWithLetter(part)) { + words.add(part.toLowerCase(Locale.ROOT)); + } else { + for (String sub : part.split("(?=[A-Z])")) { + if (!sub.isEmpty()) { + words.add(sub.toLowerCase(Locale.ROOT)); + } + } + } + } + return words; + } + + private static boolean isAllUppercaseWithLetter(String part) { + boolean hasLetter = false; + for (int i = 0; i < part.length(); i++) { + char c = part.charAt(i); + if (Character.isLetter(c)) { + hasLetter = true; + if (Character.isLowerCase(c)) { + return false; + } + } + } + return hasLetter; + } + + private static class IdentifierCollector extends BaseTreeVisitor { + private final Set identifiers = new HashSet<>(); + + @Override + public void visitIdentifier(IdentifierTree tree) { + identifiers.add(tree.name()); + super.visitIdentifier(tree); + } + } + } diff --git a/java-checks/src/test/java/org/sonar/java/checks/PseudoRandomCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/PseudoRandomCheckTest.java index da39e18fdb4..1a3c88e119f 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/PseudoRandomCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/PseudoRandomCheckTest.java @@ -29,4 +29,52 @@ void test() { .withCheck(new PseudoRandomCheck()) .verifyIssues(); } + + @Test + void test_no_security_context() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/PseudoRandomCheckNoContextSample.java")) + .withCheck(new PseudoRandomCheck()) + .verifyNoIssues(); + } + + @Test + void test_security_keywords() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/PseudoRandomCheckSecurityKeywordsSample.java")) + .withCheck(new PseudoRandomCheck()) + .verifyIssues(); + } + + @Test + void test_crypto_import() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/PseudoRandomCheckCryptoImportSample.java")) + .withCheck(new PseudoRandomCheck()) + .verifyIssues(); + } + + @Test + void test_field_scope() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/PseudoRandomCheckFieldScopeSample.java")) + .withCheck(new PseudoRandomCheck()) + .verifyIssues(); + } + + @Test + void test_wildcard_crypto_import() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/PseudoRandomCheckWildcardImportSample.java")) + .withCheck(new PseudoRandomCheck()) + .verifyIssues(); + } + + @Test + void test_static_crypto_import() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/PseudoRandomCheckStaticImportSample.java")) + .withCheck(new PseudoRandomCheck()) + .verifyIssues(); + } }