-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Crypto: Add Java Cryptographic Analysis Queries #20605
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
base: main
Are you sure you want to change the base?
Conversation
…der-java-crypto-check
… used. Prefer 'type'.
…dards. Remove redundant query. Fix QL-for-QL issues.
This PR is based on #20568, made a copy to be able to do my own quick edits. |
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.
Pull Request Overview
This PR adds comprehensive Java cryptographic analysis queries to detect various security vulnerabilities and weak cryptographic practices. The changes include 11 new CodeQL analysis queries and updates to the underlying cryptographic models to improve consistency in naming conventions.
- Adds 11 new analysis queries to detect weak cryptographic implementations including weak symmetric/asymmetric ciphers, poor block modes, weak hashing algorithms, and insecure key derivation functions
- Updates cryptographic model naming from "Family" to "Type" for better consistency across elliptic curve and hash algorithm classifications
- Modifies precision levels for some existing queries to improve analysis accuracy
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
shared/quantum/codeql/quantum/experimental/Standardization.qll |
Renames elliptic curve type definitions from "Family" to "Type" |
shared/quantum/codeql/quantum/experimental/Model.qll |
Updates method names and documentation to use "Type" instead of "Family" |
java/ql/src/experimental/quantum/Analysis/*.ql |
Adds 11 new analysis queries for detecting cryptographic vulnerabilities |
java/ql/lib/experimental/quantum/JCA.qll |
Updates method implementations to use new "Type" naming convention |
cpp/ql/lib/experimental/quantum/OpenSSL/*.qll |
Updates C++ OpenSSL bindings to match new naming conventions |
keySize < 2048 and | ||
algName = op.getAlgorithmName() and | ||
// Can't be an elliptic curve | ||
not Crypto::isEllipticCurveAlgorithmName(algName) |
Copilot
AI
Oct 8, 2025
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.
The variable configSrc
is declared in the from
clause but never assigned a value in the where
clause, which will cause the query to fail or produce no results. Either remove configSrc
from the select statement or add a condition to assign it a value.
not Crypto::isEllipticCurveAlgorithmName(algName) | |
not Crypto::isEllipticCurveAlgorithmName(algName) and | |
configSrc = op.getConfigSource() |
Copilot uses AI. Check for mistakes.
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.
This comment does indeed point out an issue with the query. I would suggest a rewrite to something like the following (I have not tested this change... it's a draft revision):
/**
* @name Weak Asymmetric Key Size
* @id java/quantum/weak-asymmetric-key-size
* @description An asymmetric cipher with a short key size is in use
* @kind problem
* @problem.severity error
* @precision high
* @tags quantum
* experimental
*/
import java
import experimental.quantum.Language
from
Crypto::KeyOperationAlgorithmNode alg, Crypto::NodeBase configSrc, string configSrcDesc,
int keySize, string algName
where
algName = alg.getAlgorithmName() and
// Can't be an elliptic curve
not Crypto::isEllipticCurveAlgorithmName(algName) and
(
// Case 1: Key size from config source
exists(Crypto::GenericSourceNode src |
src = alg.getKeySize() and
keySize = src.toString().toInt() and
configSrc = src and
configSrcDesc = src.toString()
)
or
// Case 2: Fixed key size (no config source, or config source is fine)
not exists(Crypto::GenericSourceNode src |
src = alg.getKeySize() and
src.toString().toInt() < 2048
) and
keySize = alg.getKeySizeFixed() and
configSrc = alg and
configSrcDesc = "(implicit)"
) and
keySize < 2048
select alg,
"Use of weak asymmetric key size (" + keySize.toString() + " bits) for algorithm " + algName +
" at config source $@.", configSrc, configSrcDesc
@nicolaswill this was meant to be a draft PR while I fix up the last issues, sorry about that. I don't see how I can move it back to draft, so I'll leave this open but ping when it is ready for final review. |
…mpacts the insecure IV/Nonce query. Updated name of the Insecure nonce query to be InsecureIVorNonce
…al sources to include array literals.
… of JCA random source.
…o fix false positives in the unknown IV/Nonce query). Add the unknown IV/Nonce query and associated test cases. Fix unknown IV/Nonce query to focus on cases where the oepration isn't known or the operation subtype is not encrypt or wrap.
…wn for it, and if so do not alert on non-secure random if it is tied to decryption
…Added unknown hash.
I'm hesitant to merge this into the out-of-box queries, as many of these are prescriptive about allowed/disallowed algorithms or thresholds for things like iteration count. I would suggest putting these into an Examples directory. |
…des/codeql into santander-java-crypto-check
… for consistency.
…s' subdirectories.
@nicolaswill what do you think of an asymmetric key size checker more like this:
My thought is we check the creation operations, then separately I could ask for any key op that is asymmetric with a key size that is also statically known to be too small but I think for most cases that defaults to doing basically this no? The benefit here is I don't need an operation to detect a bad key. I just keep having issues comparing types I never seem to be able to do it right. |
class KeyGenerationAlgorithmValueConsumer extends CipherAlgorithmValueConsumer, | ||
KeyAgreementAlgorithmValueConsumer, EllipticCurveAlgorithmValueConsumer instanceof Expr | ||
KeyAgreementAlgorithmValueConsumer, EllipticCurveAlgorithmValueConsumer, | ||
SignatureAlgorithmValueConsumer instanceof Expr |
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.
Is instanceof Expr
necessary here?
These are some example queries that check the cryptography present in output from a java source repo. Again, these build on the existing examples both in java and in other CBOM and cryptographic issue checking codeQL queries:
InsecureNonceGeneration.ql - as before
InsecureNonceSource.ql - as before
KnownWeakKDFIterationCount.ql - as before
NonAESGCMCipher.ql - detects non-AES in GCM mode ciphers. Can be updated to be 'non AES256 in GCM mode' but this gives more alerts on inferred key lengths.
ReusedNonce.ql - as before
UnknownKDFIterationCount.ql - as before
WeakAsymmetric.ql - finds weak asymmetric RSA ciphers using key lengths < 2048
WeakBlockModes.ql - similar to NonAESGCM, this finds instances of known-bad block modes ECB, CFB, OFB, and CTR
WeakHashing.ql - finds potentially weak hashing instances using the whitelist of SHA256, SHA384, and SHA512 (though this is yet to be checked against SHA3 variants)
WeakKDFIterationCount.ql - as before
WeakKDFKeySize.ql - as before
WeakRSA.ql - an allternative method from WeakAsymmetric.ql, but functionally the same.
WeakSymmetricCiphers.ql - detects known-weak ciphers from a blocklist of DES, TripleDES, DoubleDES, RC2, RC4, IDEA, and Blowfish.