-
Notifications
You must be signed in to change notification settings - Fork 727
SONARJAVA-6440 S2245: Add security-context heuristic to reduce false positives #5667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Luqmansonar
wants to merge
9
commits into
master
Choose a base branch
from
SONARJAVA-6440-s2245-security-context-heuristic
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9049389
SONARJAVA-6440 S2245: Add security-context heuristic to reduce false …
Luqmansonar 624f475
SONARJAVA-6440 Address code review and ruling expectations
Luqmansonar d08db1a
SONARJAVA-6440 Update ruling expectations for eclipse-jetty and mall
Luqmansonar 8954b77
SONARJAVA-6440 Update S2245 autoscan FN expectation
Luqmansonar 77ebaba
SONARJAVA-6440 Refine autoscan FN expectations: S2245=17, S2440=4
Luqmansonar c1708df
SONARJAVA-6440 Address review feedback from Nils
Luqmansonar 29e9bef
SONARJAVA-6440 Drop CRYPTO_IMPORT_PREFIXES preamble; rename AES256 test
Luqmansonar 8123b0d
SONARJAVA-6440 Update autoscan baselines for S1874 and S2119
Luqmansonar 426efc0
SONARJAVA-6440 Use leaveFile to reset per-file state
Luqmansonar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "ruleKey": "S1874", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 246, | ||
| "falseNegatives": 248, | ||
| "falsePositives": 0 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "ruleKey": "S2119", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 2, | ||
| "falseNegatives": 3, | ||
| "falsePositives": 0 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "ruleKey": "S2245", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 26, | ||
| "falseNegatives": 17, | ||
| "falsePositives": 0 | ||
| } |
3 changes: 0 additions & 3 deletions
3
its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2245.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1 @@ | ||
| { | ||
| "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/es/RecoveryIndexer.java": [ | ||
| 92 | ||
| ] | ||
| } | ||
| {} |
22 changes: 22 additions & 0 deletions
22
...checks-test-sources/default/src/main/java/checks/PseudoRandomCheckCryptoImportSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } | ||
| } |
33 changes: 33 additions & 0 deletions
33
java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckFieldScopeSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
30 changes: 30 additions & 0 deletions
30
java-checks-test-sources/default/src/main/java/checks/PseudoRandomCheckNoContextSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| } | ||
| } |
89 changes: 89 additions & 0 deletions
89
...ks-test-sources/default/src/main/java/checks/PseudoRandomCheckSecurityKeywordsSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| } | ||
|
|
||
|
Luqmansonar marked this conversation as resolved.
|
||
| // --- 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 | ||
| } | ||
| } | ||
|
|
||
19 changes: 19 additions & 0 deletions
19
...checks-test-sources/default/src/main/java/checks/PseudoRandomCheckStaticImportSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| } | ||
| } |
22 changes: 22 additions & 0 deletions
22
...ecks-test-sources/default/src/main/java/checks/PseudoRandomCheckWildcardImportSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
Luqmansonar marked this conversation as resolved.
|
||
| // "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 | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: a few edge cases worth adding to the test suite
Digit-containing uppercase identifiers —
AES256has no lowercase letter soisAllUppercaseWithLetterreturnstrueand it lowercases to"aes256", which does not match the keyword"aes". A variableAES256_KEYwould therefore not trigger the rule. Please add a test to document this (Compliant or Noncompliant — either is fine, but it should be intentional).Wildcard crypto import —
import java.security.*;should trigger Part 1 (see comment onmatchesCryptoPrefix). Add a test case inPseudoRandomCheckCryptoImportSample.java.Nested inner class isolation —
findDeclarationScopereturns the innerClassTreefirst, so a PRNG call in an inner class should not pick up keywords from the outer class. Worth an explicit Compliant test to confirm this isolation is intentional.Static import of a crypto class —
import static javax.crypto.Cipher.ENCRYPT_MODE;is stillTree.Kind.IMPORT, so it should be caught byhasCryptoImport. A targeted test would confirm this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All four added in c1708df:
AES256_KEY(Compliant, with comment documenting Dart-faithful behaviour), wildcard import sample, static import sample, inner-class isolation (TokenAwareouter +NeutralInnerinner). Tokenization gap (digit-suffix +ChaCha20-style) documented on the SONARJAVA-6440 description; cross-language follow-up tracked under APPSEC-3004.