SONARJAVA-6438: S4790 suppress weak hash algorithms on file-sourced data

[Luqmansonar]

Summary

Do not raise S4790 when MD5 or SHA-1 (including HmacMD5/HmacSHA1) digest only bytes that are proven to come from a file. File-content fingerprints (ETag, dedup, cache key) have no security decision downstream.

How it works

The exemption lives in DataHashingCheck behind one hook (isExempt) in AbstractHashAlgorithmChecker. It only applies to MD5, SHA-1 and SHA, and only when the data flow is fully provable. Anything else is still reported.

Exempted shapes:

• one-shot calls: commons-codec md5*/sha1*, Spring md5Digest*/appendMd5DigestAsHex, with a file-sourced first argument
• chained calls: MessageDigest.getInstance(..).digest(x), Hashing.md5().hashBytes(x), Mac doFinal(x)
• a local digest variable where every usage is file-fed (update/digest/doFinal, new DigestInputStream(fileStream, md)) or neutral (init, reset, no-arg digest()), and at least one usage is file-fed
• Guava Files.asByteSource(f).hash(Hashing.md5())

File sources: Files.newInputStream/readAllBytes/readString, Channels.newInputStream(FileChannel), new FileInputStream/FileReader, FileUtils.readFileToByteArray, Files.asByteSource(f).read(). Only IOUtils.toByteArray, BufferedInputStream, DigestInputStream, InputStream.readAllBytes() and single-write local variables are followed through. No other method is recursed into.

Each exemption in DataHashingCheckSample.java has a Noncompliant twin: mixed data, digest passed to another method, digest in a field, digest never fed, ByteArrayInputStream, socket streams, reassigned stream, MD2.

Known limitations

• Upload APIs (MultipartFile.getBytes/getInputStream, Part.getInputStream) are deliberately not treated as file sources, although the ticket lists them. Their content is attacker-controlled, and MD5/SHA-1 chosen-prefix collisions are practical, so a digest over uploads used as a dedup or content-addressing key can still be attacked. They are still reported.
• The heuristic proves the data source, not that the digest is used only for fingerprinting. Hashing file content and later using it in a security decision is a false negative.
• DigestOutputStream is not handled: it digests what is written into it, which is not file content.
• Ruling: the BatchIndex.java:62 expectation was removed from its/ruling/.../expected/sonar-server/java-S4790.json. No other ruling or autoscan baseline is updated; further changes, if any, depend on CI.
• A local byte[] counts as file data only when the digest call is its only usage, because writes into the array (arraycopy, data[i] ^= ...) are not tracked.

Test plan

• DataHashingCheckTest passes locally (with and without semantic)
• DataHashingCheckSample.java compiles against the test-sources classpath
• CI, then check whether other ruling and autoscan baselines change

🤖 Generated with Claude Code

[hashicorp-vault-sonar-prod]

SONARJAVA-6438

[github-actions]

❌ Ruling needs updating. A fix PR has been created: #6266

Please review and merge it into your branch.

[github-actions]

Ruling Diff Summary

Detected changes in 1 rule files: 1 issues removed, 0 issues added.

S4790 (java) on sonar-server - 1 issues removed, 0 issues added

Removed src/main/java/org/sonar/server/batch/BatchIndex.java (line 62)

(source file not found at this revision: src/main/java/org/sonar/server/batch/BatchIndex.java)

✅ Code review updated (blocking issues remain unresolved).

_{Configure merge blocking}

[asya-vorobeva]

Could be simplified to:

 if (use.parent() instanceof MemberSelectExpressionTree memberSelect
    && memberSelect.expression() == use
    && memberSelect.parent() instanceof MethodInvocationTree invocation) {
    return classifyCall(memberSelect.identifier().name(), invocation.arguments());
  }

[Luqmansonar]

Done, thanks.

[asya-vorobeva]

Honestly, I don't have a reason to have this class, as we have only one implementation of it. I guess we can get rid on it (having only DataHashingCheck). In this case this method can be turned to static.

[Luqmansonar]

Agreed. Merged into DataHashingCheck and made the method static.

[gitar-bot]

Code Review 👍 Approved with suggestions 7 closed / 8 findings

🔴 High risk · File-source heuristics suppress weak-hash alerts, risking missed security-use cases

Reworks S4790 to suppress weak hash algorithms when digesting file-sourced data, with comprehensive data-flow tracking for exempted shapes and file sources. The PR description should clarify that the sonar-server ruling baseline was updated to remove the BatchIndex.java:62 expectation, since reviewers relying on the stated "not updated" claim may miss this change and expect ruling CI to fail.

💡 Quality: PR description says ruling baselines are not updated, but one is

📄 its/ruling/src/test/resources/expected/sonar-server/java-S4790.json

Under Known limitations, the description says "S4790 ruling and autoscan baselines are expected to change and are not updated in this PR". Commit a4e78f1 does update its/ruling/src/test/resources/expected/sonar-server/java-S4790.json: it removes the BatchIndex.java:62 expectation. Reviewers who trust the description will expect ruling CI to fail and will not check that removal. Please update the description to say which baselines were updated.

✅ 7 closed

✅ Bug: Test sample expects no issue where the check still raises one

📄 java-checks-test-sources/default/src/main/java/checks/security/DataHashingCheckSample.java:171-172 📄 java-checks-test-sources/default/src/main/java/checks/security/DataHashingCheckSample.java:176-179 📄 java-checks-test-sources/default/src/main/java/checks/security/DataHashingCheckSample.java:189 📄 java-checks/src/main/java/org/sonar/java/checks/AbstractHashAlgorithmChecker.java:240-254
The new FileSourceHashing samples at lines 171, 172, 176 and 189 have no // Noncompliant marker, but the check still reports on all of them. The matched call is MessageDigest.getInstance("MD5") or Hashing.md5(), not the outer .digest(...)/.hashBytes(...). For getInstance, getDataArgument returns null because "getInstance" matches none of its branches. For Hashing.md5(), the argument list is empty, so isDataFromFileSource returns false early. DataHashingCheckTest will therefore fail on 4 unexpected issues. The line 176 sample never even passes file data to md. This also means the description's NIO and Guava claims (Files.readAllBytes, Files.newInputStream, Hashing.md5().hashBytes) are not implemented. The hashBytes branch in getDataArgument can never run, because hashBytes is not one of the method matchers. Fix: either add // Noncompliant to these lines and remove the unimplemented claims, or actually look at the enclosing .digest(...)/.hashBytes(...)/.update(...) call.

✅ Security: Recursion into any method's first argument suppresses mixed-data hashes

📄 java-checks/src/main/java/org/sonar/java/checks/AbstractHashAlgorithmChecker.java:283-288
isFileSourceExpression recurses into the first argument of any method call, whatever its owner, and ignores the receiver and all other arguments. So DigestUtils.md5Hex(concat(Files.readAllBytes(saltFile), password.getBytes())) is suppressed, and so is DigestUtils.sha1Hex(sign(new FileInputStream(key), secret)). A file read merely feeds the first parameter of an arbitrary transform there. The data is not "exclusively" file-derived, which is what the PR claims, and these can be credential hashes. That is a real loss of security signal. Fix: only recurse through known pass-through wrappers (e.g. IOUtils.toByteArray, BufferedInputStream), or require the argument to be a direct file-source expression.

✅ Bug: Stream wrapping (DigestInputStream, Buffered*) claimed but not handled

📄 java-checks/src/main/java/org/sonar/java/checks/AbstractHashAlgorithmChecker.java:289-295
The description says DigestInputStream/DigestOutputStream over file sources are recognized. The NEW_CLASS branch of isFileSourceExpression only checks whether the constructed type itself is a file constructor, and it never recurses into constructor arguments. So new DigestInputStream(new FileInputStream(f), md) and the very common DigestUtils.md5Hex(new BufferedInputStream(new FileInputStream(f))) are not suppressed. The MessageDigest that feeds a DigestInputStream also comes from getInstance, which is never suppressed (see the test-sample finding). Fix: recurse into the arguments of known FilterInputStream wrappers, or remove the claim.

✅ Bug: Non-existent java.nio.file.FileChannel and useless FileReader entry

📄 java-checks/src/main/java/org/sonar/java/checks/AbstractHashAlgorithmChecker.java:326-330
isFileConstructor matches java.nio.file.FileChannel, but that type does not exist. The real class is java.nio.channels.FileChannel, and it is abstract, so no new FileChannel(...) expression can ever appear. java.io.FileReader is a Reader, and none of the matched digest methods (commons-codec or Spring DigestUtils) accept a Reader, so that entry can never match a data argument either. Both are dead entries that suggest coverage the check does not have.

✅ Quality: Redundant/dead branches in getDataArgument

📄 java-checks/src/main/java/org/sonar/java/checks/AbstractHashAlgorithmChecker.java:256-270
The second if is unreachable. md5Digest and appendMd5DigestAsHex/md5DigestAsHex already end with "Digest" or "Hex", so the first condition catches them. The hashBytes branch is also dead, because hashBytes is never a matched invocation. The first condition's endsWith("Digest") also catches getDigest(String), so the algorithm-name string gets inspected as if it were the data. That is harmless today, but it shows the method-name heuristic does not model which argument is the data.

...and 2 more closed from earlier reviews

🤖 Prompt for agents

Code Review: Reworks S4790 to suppress weak hash algorithms when digesting file-sourced data, with comprehensive data-flow tracking for exempted shapes and file sources. The PR description should clarify that the `sonar-server` ruling baseline was updated to remove the `BatchIndex.java:62` expectation, since reviewers relying on the stated "not updated" claim may miss this change and expect ruling CI to fail.

1. 💡 Quality: PR description says ruling baselines are not updated, but one is
   Files: its/ruling/src/test/resources/expected/sonar-server/java-S4790.json

   Under Known limitations, the description says "S4790 ruling and autoscan baselines are expected to change and are not updated in this PR". Commit a4e78f1 does update `its/ruling/src/test/resources/expected/sonar-server/java-S4790.json`: it removes the `BatchIndex.java:62` expectation. Reviewers who trust the description will expect ruling CI to fail and will not check that removal. Please update the description to say which baselines were updated.

Review coverage

🧪 Functional validation 1 of 1 objectives covered

📋 Rules No rules evaluated

Cross-repo coverage 6 repositories selected

🤖 Auto-approval Not enabled · Set up

Implementation Status ✅ 1 of 1 objectives covered

✅ SONARJAVA-6438 - 1 of 1 objectives covered

This PR covers the objective to suppress S4790 alerts for weak hash algorithms and MACs when the hashed data originates from a file-like source.

✅ 1 covered here

• ✅ Do not raise S4790 on weak hash algorithms and MACs when the hashed data comes from a file-like source

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
92.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[asya-vorobeva]

💯