Add Security Rules for Weak Cryptographic Algorithms and Insecure SSL #115
Conversation
|
Sakshis seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis pull request introduces three new security rules for Java applications focusing on detecting and preventing the use of weak cryptographic algorithms and insecure SSL contexts. The rules target MD5 and SHA1 hashing algorithms, which are considered cryptographically weak, and deprecated SSL/TLS versions. Each rule includes detailed configuration for identifying insecure code patterns, provides warnings about potential security risks, and references relevant security standards like CWE and OWASP guidelines. Changes
Sequence DiagramsequenceDiagram
participant Code
participant SecurityRule
participant Analyzer
Code->>SecurityRule: Invoke method with algorithm
SecurityRule->>Analyzer: Check algorithm type
alt Insecure Algorithm
Analyzer-->>Code: Generate Warning
Analyzer->>Code: Suggest Secure Alternative
else Secure Algorithm
Analyzer-->>Code: No Action
end
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
rules/java/security/weak-ssl-context-java.yml (2)
4-13: Enhance security guidance in the messageWhile the message correctly identifies the issue, consider enhancing it with:
- Explicit mention of TLSv1.3 as the preferred version
- Link to NIST guidelines for TLS
- Impact of using weak encryption
message: >- 'An insecure SSL context was detected. TLS versions 1.0, 1.1, and all SSL versions are considered weak encryption and are deprecated. Use - SSLContext.getInstance("TLSv1.2") for the best security.' + SSLContext.getInstance("TLSv1.3") (preferred) or SSLContext.getInstance("TLSv1.2"). + Using weak encryption may expose sensitive data to security vulnerabilities.' note: >- [CWE-326] Inadequate Encryption Strength [REFERENCES] - https://tools.ietf.org/html/rfc7568 - https://tools.ietf.org/id/draft-ietf-tls-oldversions-deprecate-02.html + - https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-52r2.pdf
21-75: Consider additional constraints for dynamic context generationThe constraints effectively handle:
- String literals with regex validation
- Identifiers with proper type checking
- Variable declarations and field access
However, consider adding constraints for:
- Method return values
- Configuration property lookups
- Environment variable access
This would help catch cases where the SSL context version is determined dynamically at runtime.
Would you like me to provide example constraints for these scenarios?
tests/java/use-of-md5-java-test.yml (1)
1-7: Consider adding more test cases for comprehensive coverage.While the basic valid/invalid cases are good, consider adding:
- Multiple provider variants (e.g., "MD5", "SUN")
- Case sensitivity tests ("md5", "MD5")
- Different method overloads
Example additions:
valid: - | MessageDigest md5Digest = MessageDigest.getInstance("SHA-512"); + - | + MessageDigest md5Digest = MessageDigest.getInstance("SHA-256"); invalid: - | MessageDigest md5Digest = MessageDigest.getInstance("MD5"); + - | + MessageDigest md5Digest = MessageDigest.getInstance("md5", "SUN"); + - | + MessageDigest md5Digest = MessageDigest.getInstance("MD5", provider);rules/java/security/use-of-md5-java.yml (1)
13-17: Consider adding patterns for other common MD5 implementations.The current patterns focus on MessageDigest but miss other common MD5 usages.
Consider adding:
any: - pattern: java.security.MessageDigest.getInstance($ALGO) - pattern: java.security.MessageDigest.getInstance($ALGO, $$$) - pattern: MessageDigest.getInstance($ALGO) - pattern: MessageDigest.getInstance($ALGO, $$$) + - pattern: org.apache.commons.codec.digest.DigestUtils.md5($$$) + - pattern: org.apache.commons.codec.digest.DigestUtils.md5Hex($$$) + - pattern: com.google.common.hash.Hashing.md5()tests/java/use-of-sha1-java-test.yml (1)
8-18: Consider adding more comprehensive test cases.While the current invalid cases cover basic scenarios, consider adding:
invalid: - | java.security.MessageDigest md = java.security.MessageDigest.getInstance("SHA1", "SUN"); DigestUtils.getSha1Digest().digest(password.getBytes()); - | MessageDigest sha1Digest = MessageDigest.getInstance("SHA1", "SUN"); - | byte[] hashValue = DigestUtils.getSha1Digest().digest(password.getBytes()); - | MessageDigest sha1Digest = MessageDigest.getInstance("SHA1"); + - | + MessageDigest sha1Digest = MessageDigest.getInstance("sha1"); // Case variation + - | + com.google.common.hash.HashFunction hf = Hashing.sha1(); // Guava implementation + - | + org.apache.commons.codec.digest.DigestUtils.sha1Hex(password); // Commons Codecrules/java/security/use-of-sha1-java.yml (1)
4-12: Enhance security context in documentationConsider adding more specific details to strengthen the security guidance:
- Include the year SHA1 was officially broken (2017 with the SHAttered attack)
- Add NIST recommendations and references
- Specify minimum key lengths for SHA256/SHA512
message: >- Detected SHA1 hash algorithm which is considered insecure. SHA1 is not collision resistant and is therefore not suitable as a cryptographic signature. Instead, use PBKDF2 for password hashing or SHA256 or SHA512 - for other hash function applications. + for other hash function applications. SHA1 was officially broken in 2017 + through the SHAttered attack, and NIST recommends using SHA256 or stronger. note: >- [CWE-328] Use of Weak Hash. [REFERENCES] - https://owasp.org/Top10/A02_2021-Cryptographic_Failures + - https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar2.pdf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
rules/java/security/use-of-md5-java.yml(1 hunks)rules/java/security/use-of-sha1-java.yml(1 hunks)rules/java/security/weak-ssl-context-java.yml(1 hunks)tests/__snapshots__/use-of-md5-java-snapshot.yml(1 hunks)tests/__snapshots__/use-of-sha1-java-snapshot.yml(1 hunks)tests/__snapshots__/weak-ssl-context-java-snapshot.yml(1 hunks)tests/java/use-of-md5-java-test.yml(1 hunks)tests/java/use-of-sha1-java-test.yml(1 hunks)tests/java/weak-ssl-context-java-test.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/snapshots/use-of-md5-java-snapshot.yml
- tests/snapshots/use-of-sha1-java-snapshot.yml
🔇 Additional comments (5)
tests/java/weak-ssl-context-java-test.yml (2)
2-8: Consider adding validation for dynamic context retrieval
The valid patterns correctly include TLSv1.2 and TLSv1.3. However, the dynamic context retrieval via getSslContext() might need additional validation to ensure it doesn't return insecure versions at runtime.
Consider adding constraints or documentation for the getSslContext() method to ensure it only returns secure TLS versions.
9-19: LGTM! Comprehensive coverage of insecure SSL/TLS versions
The invalid patterns correctly identify all deprecated and insecure SSL/TLS versions:
- SSL (generic)
- TLS (generic)
- TLSv1
- SSLv3
- TLSv1.1
This aligns with security best practices and NIST recommendations.
tests/__snapshots__/weak-ssl-context-java-snapshot.yml (1)
1-37: LGTM! Well-structured snapshots with complete coverage
The snapshots:
- Cover all insecure SSL/TLS versions defined in the test file
- Include precise source location information for each case
- Maintain consistent styling across all cases
This provides good test coverage for the security rule.
rules/java/security/weak-ssl-context-java.yml (1)
14-20: LGTM! Effective pattern matching for SSL context detection
The rule correctly:
- Identifies calls to
SSLContext.getInstance() - Excludes secure versions (TLSv1.2 and TLSv1.3)
- Uses negative patterns to prevent false positives
rules/java/security/use-of-sha1-java.yml (1)
1-22: Verify rule coverage and false positives
Let's ensure the rule effectively catches all SHA1 usage patterns while avoiding false positives.
✅ Verification successful
Let me gather more information about the test coverage to ensure the rule's effectiveness.
Rule implementation and test coverage verified successfully
The rule effectively covers all common SHA1 usage patterns in Java:
- Direct MessageDigest.getInstance calls with "SHA1" or "SHA-1"
- Qualified MessageDigest.getInstance calls
- DigestUtils.getSha1Digest() usage
- Provider-specific instantiation
The test file comprehensively covers all patterns defined in the rule with appropriate valid and invalid test cases, ensuring the rule's reliability in detecting insecure SHA1 usage while avoiding false positives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify rule coverage and potential false positives
# Check if there are corresponding test files
echo "Checking for test files..."
fd "use-of-sha1.*test" tests/
# Check for potential SHA1 usages that might be missed
echo "Checking for potential missed SHA1 patterns..."
rg -i "sha-?1" --type java
# Check for similar security rules to ensure consistency
echo "Checking for similar security rules..."
fd "." rules/java/security/ -e yml | xargs cat | grep -i "MessageDigest"
Length of output: 960
Script:
#!/bin/bash
# Check the test file content to verify coverage of all patterns
echo "Checking test content..."
cat tests/java/use-of-sha1-java-test.yml
# Check for any other potential SHA1-related patterns in the codebase
echo "Checking for additional SHA1 patterns..."
rg -i "sha-?1|messagedigest" --type-list
rg -i "sha-?1|messagedigest" -g "*.java" || echo "No .java files found"
Length of output: 1538
| - | | ||
| MessageDigest sha1Digest = MessageDigest.getInstance("SHA1", "SUN"); | ||
| - | | ||
| byte[] hashValue = DigestUtils.getSha1Digest().digest(password.getBytes()); |
There was a problem hiding this comment.
Remove duplicate test case.
The test case for DigestUtils.getSha1Digest() appears twice (lines 14 and 18).
- |
byte[] hashValue = DigestUtils.getSha1Digest().digest(password.getBytes());
- |
MessageDigest sha1Digest = MessageDigest.getInstance("SHA1");
- - |
- byte[] hashValue = DigestUtils.getSha1Digest().digest(password.getBytes());Also applies to: 18-18
Summary by CodeRabbit
New Features
Tests
Snapshots