-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Crypto: Add Java cryptographic check queries #20568
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
Crypto: Add Java cryptographic check queries #20568
Conversation
* @description An AES cipher is in use without GCM | ||
* @kind problem | ||
* @problem.severity error | ||
* @security.severity low |
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.
Was there a reason for calling this security severity low? I think this might be one of those things that's too org-specific to put in the query general meta-data. This comment applies for all the queries in the PR.
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 is in line with ASVSv5. Yes, this is quite specific and of course in the future it might be a 'high' issue for reasons we know not of yet. Removing any @security.severity
might be useful.
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.
I'm inclined to remove all security.severity meta-data because of how specific it will be per org. Does that give you heartburn?
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.
Not at all. See next commit in thread. :)
I've sanitised out the |
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.
Thank you for the contribution! I've left some minor comments mainly regarding name string vs type comparison and tags.
from Crypto::KeyOperationAlgorithmNode alg, string name, string msg | ||
where | ||
name = alg.getAlgorithmName() and | ||
name in ["DES", "TripleDES", "DoubleDES", "RC2", "RC4", "IDEA", "Blowfish"] and |
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.
Rather than comparing name strings, the standardized types in Crypto::KeyOpAlg should be compared against getAlgorithmType
. Here is an example of that syntax: this.getAlgorithmType() = KeyOpAlg::TMac(KeyOpAlg::CMAC())
. To avoid redundant typing of the module namespaces, the KeyOpAlg
module could optionally also be imported as follows: import Crypto::KeyOpAlg as Alg
.
from Crypto::HashAlgorithmNode alg, string name, string msg | ||
where | ||
name = alg.getAlgorithmName() and | ||
not name in ["SHA256", "SHA384", "SHA512", "SHA-256", "SHA-384", "SHA-512"] and |
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.
Like the comment on symmetric algorithms, the hash family and hash digest size should be used rather than string comparisons. The appropriate predicates/classes to use would be HashType
(getHashFamily()
) and getDigestLength()
.
Typing this out also makes me realize we need to standardize those predicate and class names...
class NonAESGCMAlgorithmNode extends Crypto::KeyOperationAlgorithmNode { | ||
NonAESGCMAlgorithmNode() { | ||
this.getAlgorithmType() = Crypto::KeyOpAlg::TSymmetricCipher(Crypto::KeyOpAlg::AES()) and | ||
this.getModeOfOperation().getModeType() != Crypto::KeyOpAlg::GCM() |
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.getModeOfOperation().getModeType() != Crypto::KeyOpAlg::GCM() | |
not this.getModeOfOperation().getModeType() = Crypto::KeyOpAlg::GCM() |
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.
That's not actually we want we want, chatting with @nicolaswill in a side channel, so disregard.
* @kind problem | ||
* @problem.severity error | ||
* @precision high | ||
* @tags external/cwe/cwe-327 |
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 needs quantum
and experimental
tags.
* @kind problem | ||
* @problem.severity error | ||
* @precision high | ||
* @tags external/cwe/cwe-327 |
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 needs quantum
and experimental
tags.
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 security checks through CodeQL queries that identify various cryptographic vulnerabilities and weak configurations. The queries focus on detecting insecure cryptographic practices in Java codebases.
- Adds 11 new CodeQL queries for detecting cryptographic security issues
- Implements checks for weak ciphers, key sizes, hashing algorithms, and nonce handling
- Covers symmetric/asymmetric encryption, key derivation functions, and block cipher modes
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
WeakSymmetricCiphers.ql | Detects usage of weak symmetric cipher algorithms like DES, RC4, etc. |
WeakRSA.ql | Identifies RSA implementations with key lengths below 2048 bits |
WeakKDFKeySize.ql | Finds key derivation functions with output lengths below 256 bits |
WeakKDFIterationCount.ql | Detects KDF operations with iteration counts below 100,000 |
WeakHashing.ql | Identifies non-approved hash algorithms (excludes SHA-256/384/512) |
WeakBlockModes.ql | Finds AES usage with insecure block modes (ECB, CFB, OFB, CTR) |
WeakAsymmetric.ql | Detects asymmetric ciphers with key sizes below 2048 bits |
UnknownKDFIterationCount.ql | Removes severity warning metadata |
NonceReuse.ql | Identifies reuse of cryptographic nonces |
NonAESGCMCipher.ql | Detects AES usage without GCM mode |
InsecureNonceGeneration.ql | Finds nonces generated from insecure sources |
/** | ||
* @name Weak known key derivation function output length | ||
* @description Detects key derivation operations with a known weak output length | ||
* @id java/quantum/weak-kdf-iteration-count |
Copilot
AI
Oct 2, 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 @id should be 'java/quantum/weak-kdf-key-size' to match the query name and purpose, not 'weak-kdf-iteration-count'.
* @id java/quantum/weak-kdf-iteration-count | |
* @id java/quantum/weak-kdf-key-size |
Copilot uses AI. Check for mistakes.
@@ -0,0 +1,24 @@ | |||
/** | |||
* @name Weak Asymetric Key Size |
Copilot
AI
Oct 2, 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.
Corrected spelling of 'Asymetric' to 'Asymmetric'.
* @name Weak Asymetric Key Size | |
* @name Weak Asymmetric Key Size |
Copilot uses AI. Check for mistakes.
// Can't be an elliptic curve | ||
not Crypto::isEllipticCurveAlgorithmName(algName) | ||
select op, | ||
"Use of weak asymmetric key size (int bits)" + keySize.toString() + " for algorithm " + |
Copilot
AI
Oct 2, 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 error message formatting is unclear. '(int bits)' should be '(' + keySize.toString() + ' bits)' for proper readability.
"Use of weak asymmetric key size (int bits)" + keySize.toString() + " for algorithm " + | |
"Use of weak asymmetric key size (" + keySize.toString() + " bits) for algorithm " + |
Copilot uses AI. Check for mistakes.
There are also duplicate query IDs (potentially, duplicate queries, though I have not verified the contents):
|
Lastly, could you please run |
@nicolaswill can you close this PR, I created my own PR based on this PR so we can make fast edits: See #20605 |
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: