Add CodeQL Quantum models and queries (Java, C++) to experimental #19469
Conversation
Concepts for elliptic curves and misc. updates.
Move language-agnostic model to shared library
…ryption operations.
…nwrap and doFinal calls. Corrected pathing for init tracing to detect what mode is being set along a path. Added support for tracing the init operation mode argument to source. Since this involved creating an Operation Mode, changes were also made to make cipher block modes (CBC) more explicit (previously just called mode, but now that term is used for various purposes).
…/codeql into quantum-experimental
|
A couple of questions/comments:
|
|
|
Thanks for your review!
Over the next month, we will be revising the existing queries and rapidly adding new ones. Already using DCA would be great if frequent changes to existing queries won't impede DCA testing.
That's the idea. At this stage, the CodeQL architecture in
Done. I've bumped the submodule pointer to this branch's HEAD. We should probably squash this PR's messy commit history before merging though. |
| /** | ||
| * An element that is flow-aware, i.e., it has an input and output node implicitly used for data flow analysis. | ||
| */ | ||
| abstract class FlowAwareElement extends LocatableElement { |
There was a problem hiding this comment.
FlowAwareElement currently has to be exposed for modeling generic data-flow between its instances at the language level. It's used in Language.qll in the current implementation:
predicate isSink(DataFlow::Node sink) {
sink = any(Crypto::FlowAwareElement other).getInputNode()
}
There was a problem hiding this comment.
Oh, sorry, I should have elaborated:
For abstract classes that are not supposed to be extended outside the module where they are declared, we typically do something like:
abstract private class FlowAwareElementImpl extends LocatableElement {
...
}
final class FlowAwareElement = FlowAwareElementImpl;
There was a problem hiding this comment.
Makes sense. I've changed FlowAwareElement to use that pattern as an example -- am also aware other similar instances exist where this pattern should be applied, but I will set that aside for later.
| } | ||
|
|
||
| // Artifacts that may be outputs or inputs | ||
| newtype TKeyArtifactType = |
There was a problem hiding this comment.
Consider making this private and use KeyArtifactType outside this module.
Introduce helper predicates isSymmetricKeyType, isAssymetricKeyType and isUnknownKeyType on KeyArtifactType.
| SM4() or | ||
| OtherSymmetricCipherType() | ||
|
|
||
| newtype TAsymmetricCipherType = |
There was a problem hiding this comment.
Some of these types can be made private.
I think it is best practice to avoid exposing IPA types if possible.
There was a problem hiding this comment.
We're currently using IPA types to enumerate various disjoint algorithms and their unions, so those attributes, explicit typing, and the minimal syntax required made them an attractive option. We would otherwise have to make a separate class for each type in order to have an interface that enables writing code like the following:
AESGCMAlgorithmNode() {
this.getAlgorithmType() = Crypto::KeyOpAlg::TSymmetricCipher(Crypto::KeyOpAlg::AES()) and
this.getModeOfOperation().getModeType() = Crypto::GCM()
}
exists(string upper | upper = name.toUpperCase() |
upper.matches("AES%") and
type = KeyOpAlg::TSymmetricCipher(KeyOpAlg::AES())
or
upper = "DES" and
type = KeyOpAlg::TSymmetricCipher(KeyOpAlg::DES())
or
...
Would this ever be acceptable or do we have to implement an alternative?
There was a problem hiding this comment.
You don't have to - these are just generic recommendations 😄
If you think it is best to keep it as it is - then definitely do so!
|
Annotating things with Thank you, though! A full-blown semantic review will have to be planned a bit or resourced and isn't what I had in mind right now before this goes on experimental. |
|
DCA run: https://github.com/github/codeql-dca-main/issues/28769 (not sure if it works - but lets see 😄 ) |
|
DCA finished - approximately 800 results on our Java suite: https://github.com/github/codeql-dca-main/blob/data/michaelnebel/quantum-experim__nightly__quantum-queries__1/reports/alert-comparison.md |
|
Thanks @michaelnebel. Lastly, should I squash the 139-commit history before merging? |
|
There's no requirements to squash merge when merging into |
##Hi, I tried it, it didn't work for me and showed a 404 error.thank you |
Yes, sorry, that is an internal performance measurement tool. |
This pull request introduces libraries and queries for inventorying and analyzing cryptography through a shared language-independent model library, language-specific implementations, and library-specific models.
There is currently modelling for the Java Cryptography Architecture (JCA) and OpenSSL (C++), with DGML-based inventory graph output for both currently supported languages. Our Java implementation further provides a set of analysis queries and inventory subset ("slice") output queries.