16 cryptography models libraries and queries migration - #17
16 cryptography models libraries and queries migration#17Ben Rodes (bdrodes) wants to merge 9 commits into
Conversation
Sorry, something went wrong.
Mathias Vorreiter Pedersen (MathiasVP)
left a comment
There was a problem hiding this comment.
A couple of comments on the design (and a couple of ad-hoc way-too-detailed comments when I spotted something).
I'm not sure what kind of review process you have on this repo, so please don't let any of this block your PR. I just wanted to highlight a subset of the things I'd like to see fixed before we get this into the official CodeQL repository 😄.
Sorry, something went wrong.
| string unknownAlgorithm() { result = "UNKNOWN" } | ||
|
|
||
| string getHashType() { result = "HASH" } | ||
| string getSymmetricEncryptionType() { result = "SYMMETRIC_ENCRYPTION" } | ||
| string getAsymmetricEncryptionType() { result = "ASYMMETRIC_ENCRYPTION" } | ||
| string getKeyDerivationType() { result = "KEY_DERIVATION" } | ||
| string getCipherBlockModeType() { result = "BLOCK_MODE" } | ||
| string getSymmetricPaddingType() { result = "SYMMETRIC_PADDING" } | ||
| string getAsymmetricPaddingType() { result = "ASYMMETRIC_PADDING" } | ||
| string getEllipticCurveType() { result = "ELLIPTIC_CURVE" } | ||
| string getSignatureType() { result = "SIGNATURE" } | ||
| string getKeyExchangeType() { result = "KEY_EXCHANGE" } |
There was a problem hiding this comment.
Is there a reason this has to be modelled using strings?
Here's a suggestion for how I'd probably structure this to not rely on hardcoded strings:
newtype TCryptoAlgorithmName =
TUnknownAlgorithm() or
THash() or
TSymmetricEncryption() or
TAsymmetricEncryption() or
TKeyDerivation() or
TCipherBlockMode() or
TSymmetricPadding() or
TAsymmetricPadding() or
TEllipticCurve() or
TSignature() or
TKeyExchange()
class CryptoAlgorithmName extends TCryptoAlgorithmName {
string toString() { none() } // overridden in subclasses
predicate isUnknown() { none() }
predicate isAsymmetric() { none() }
}
class UnknownAlgorithm extends CryptoAlgorithmName, TUnknownAlgorithm {
override predicate isUnknown() { any() }
}
class Hash extends CryptoAlgorithmName, THash { }
class SymmetricEncryption extends CryptoAlgorithmName, TSymmetricEncryption { }
class AsymmetricEncryption extends CryptoAlgorithmName, TAsymmetricEncryption {
override predicate isAsymmetric() { any() }
}
class KeyDerivation extends CryptoAlgorithmName, TKeyDerivation { }
class CipherBlockMode extends CryptoAlgorithmName, TCipherBlockMode { }
class SymmetricPadding extends CryptoAlgorithmName, TSymmetricPadding { }
class AsymmetricPadding extends CryptoAlgorithmName, TAsymmetricPadding { }
class EllipticCurve extends CryptoAlgorithmName, TEllipticCurve {
override predicate isAsymmetric() { any() }
}
class Signature extends CryptoAlgorithmName, TSignature {
override predicate isAsymmetric() { any() }
}
class KeyExchange extends CryptoAlgorithmName, TKeyExchange {
override predicate isAsymmetric() { any() }
}With potentially more predicates on the CryptoAlgorithmName class. This avoids continiously having to enumerate a bunch of cases in each of the predicates further down in this file.
Sorry, something went wrong.
| string getKeyExchangeType() { result = "KEY_EXCHANGE" } | ||
|
|
||
| string getAsymmetricType(){ | ||
| result in [getAsymmetricEncryptionType(), getSignatureType(), getKeyExchangeType(), getEllipticCurveType()] |
There was a problem hiding this comment.
For instance, this would just be:
CryptoAlgorithmName getAsymmetricType() { result.isAsymmetric() }
Sorry, something went wrong.
| * A cryptographic artifact is a DataFlow::Node associated with some | ||
| * operation, algorithm, or any other aspect of cryptography. | ||
| */ | ||
| abstract class CryptographicArtifact extends Expr {} |
There was a problem hiding this comment.
The comment specifies that an artifact is a DataFlow::Node, but this class is extending Expr?
I haven't looked in detail of all the implementations of this abstract class, but since this abstract class is so general it may be bad to couple if so tightly with either Expr or DataFlow::Nodes? If so, maybe it makes sense to apply the same design as I proposed in CryptoAlgorithmNames.qll? That is, an IPA type that defines the "universe" of possible values, and then define CryptographicArtifact as an abstract class that extends this type?
Sorry, something went wrong.
| exists(Expr cur, Expr next | | ||
| (cur = node1.asExpr() or cur = node1.asIndirectArgument()) and | ||
| (next = node2.asExpr() or next = node2.asIndirectArgument() or next = node2.asDefiningArgument()) | ||
| | | ||
| exists(Call c | knownPassthoughCall(c, cur, next))) |
There was a problem hiding this comment.
I don't see why next should ever be an indirect argument. Indirect arguments always go into a function, but it looks like next is always either an outgoing argument or a return value?
Sorry, something went wrong.
| node.asVariable().hasName("argv") | ||
| or | ||
| node.asIndirectVariable().hasName("argv") |
There was a problem hiding this comment.
Hmm... I don't think this will work since asVariable and asIndirectVariable is about global variables. You probably want to replace these with node.asExpr().(VariableAccess).getTarget().hasName("argv") (and similarly for the indirect case).
Sorry, something went wrong.
| } | ||
|
|
||
|
|
||
| abstract class AlgorithmSinkArgument extends Expr { |
There was a problem hiding this comment.
If you ever want to distinguish between a function in these two scenarios:
void runAlgorithm(unsigned algorithm_type);and
void runAlgorithm(unsigned* pointer_to_algorithm_type);then you most likely want to extend DataFlow::Node instead.
Sorry, something went wrong.
|
|
||
| /** | ||
| * A hard-coded mapping of known algorithm aliases in OpenSSL. | ||
| * This was derived by applying the same kind of logic foun din `customAliases` to the |
There was a problem hiding this comment.
| * This was derived by applying the same kind of logic foun din `customAliases` to the | |
| * This was derived by applying the same kind of logic found in `customAliases` to the |
Sorry, something went wrong.
| * `normalized` is the normalized name of the algorithm (e.g., "AES128" for "aes-128-cbc") | ||
| * `algType` is the type of algorithm (e.g., "SYMMETRIC_ENCRYPTION") | ||
| */ | ||
| predicate knownOpenSSLAlgorithm(string name, int nid, string normalized, string algType) |
There was a problem hiding this comment.
🤯
Sorry, something went wrong.
| * OpenSSL functions as defined in the OpenSSL docs | ||
| * https://www.openssl.org/docs/manmaster/man3/ | ||
| */ | ||
| predicate openSSLAPIFuncName(string name){ |
There was a problem hiding this comment.
🤯
Sorry, something went wrong.
…python models, inventory, and example alerts.
…ventory models. Added some example old models.
Changing metadata to under python namespace
|
Merged from Josh Brown (@ropwareJB)'s version of this branch that was also merged upstream. #19 |
Sorry, something went wrong.
feat(reusable-workflow-models): Reusable workflow MaD
Migration of C/C++ crypto models, inventory queries and example alerts.