You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Java: Match mixed-case weak cryptographic algorithm strings - #23027
algorithmRegex in Encryption.qll previously had two branches: one for exact case and one for all-lowercase. Strings that mixed case across / segments, such as "AES/CBC/PKCS5PADDING" or "AES/cbc/Pkcs5Padding", matched neither branch, so java/weak-cryptographic-algorithm and java/potentially-weak-cryptographic-algorithm missed them.
Add a case-insensitive branch to algorithmRegex when the algorithm name is bounded by start/end of string or non-alphabetic characters. This matches any casing variant (AES/CBC/PKCS5PADDING, AES/cbc/Pkcs5Padding, AES/ecb/NoPadding) while preserving the existing word-boundary protections against camelCase and English words (GetPrc2, Description, DESTROY, species, AESEncryption).
Add (?i) to the AES(?(ECB|CBC/PKCS[57]Padding)) negative lookahead in getASecureAlgorithmName() so mixed-case weak modes are not classified as secure AES.
Add (?i) to the RSA/ECB.* exclusion in BrokenCryptoAlgorithmQuery.qll so mixed-case Rsa/Ecb/... strings remain excluded.
Evidence
codeql test run java/ql/test/query-tests/security/CWE-327/semmle/tests java/ql/test/library-tests/Encryption passes all 5 tests (BrokenCryptoAlgorithm.qlref, MaybeBrokenCryptoAlgorithm.qlref, secure.ql, cryptoalgospec.ql, insecure.ql).
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The new regex treats property-based Rsa/ECB/NoPadding as secure, suppressing the potentially-weak-cryptography alert without a replacement RSA-without-OAEP alert.
The new branch makes a property value such as Rsa/ECB/NoPadding match the secure regex, so Cipher.getInstance(props.getProperty("algorithm")) loses its potentially-weak-cryptography alert. RsaWithoutOaepConfig only accepts compile-time constants, so no RSA-without-OAEP alert replaces it. Model non-OAEP RSA property values in that query, or keep them eligible for the potentially-weak query.
The reason will be displayed to describe this comment to others. Learn more.
The added commit addressed CCR's original review comment. The second review comment, about property-based RSA, remains a coverage gap. But it already existed for uppercase and lowercase spellings. This PR makes it also exist for mixed-case property values. But this pre-existing issue shouldn't be fixed in this PR.
This all looks good. I've started a DCA run to check performance. If that is fine then I will approve and merge.
This branch has not been deployed
No deployments
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
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.
Summary
algorithmRegexinEncryption.qllpreviously had two branches: one for exact case and one for all-lowercase. Strings that mixed case across/segments, such as"AES/CBC/PKCS5PADDING"or"AES/cbc/Pkcs5Padding", matched neither branch, sojava/weak-cryptographic-algorithmandjava/potentially-weak-cryptographic-algorithmmissed them.bindingset[algorithmString] private string algorithmRegex(string algorithmString) { result = "((^|.*[^A-Z])(" + algorithmString + ")([^A-Z].*|$))" + "|" + "((^|.*[A-Z]{2}|.*[^a-zA-Z])(" + algorithmString.toLowerCase() + ")([^a-z].*|$))" + + "|" + + "((^|.*[^a-zA-Z])(?i)(" + algorithmString + ")([^a-zA-Z].*|$))" }algorithmRegexwhen the algorithm name is bounded by start/end of string or non-alphabetic characters. This matches any casing variant (AES/CBC/PKCS5PADDING,AES/cbc/Pkcs5Padding,AES/ecb/NoPadding) while preserving the existing word-boundary protections against camelCase and English words (GetPrc2,Description,DESTROY,species,AESEncryption).(?i)to theAES(?(ECB|CBC/PKCS[57]Padding))negative lookahead ingetASecureAlgorithmName()so mixed-case weak modes are not classified as secureAES.(?i)to theRSA/ECB.*exclusion inBrokenCryptoAlgorithmQuery.qllso mixed-caseRsa/Ecb/...strings remain excluded.Evidence
codeql test run java/ql/test/query-tests/security/CWE-327/semmle/tests java/ql/test/library-tests/Encryptionpasses all 5 tests (BrokenCryptoAlgorithm.qlref,MaybeBrokenCryptoAlgorithm.qlref,secure.ql,cryptoalgospec.ql,insecure.ql).