Conversation
…eted connecting padding to algorithm instances if through a set padding interface.
… handle the final and update mechanics, matching the JCA. Similarly need to update cipher to follow the JCA for update/final as well.
| // The operation is a direct algorithm consumer | ||
| // NOTE: the operation itself is already modeld as a value consumer, so we can | ||
| // simply return 'this', see modeled hash algorithm consuers for EVP_Q_Digest | ||
| this = result |
There was a problem hiding this comment.
In this case, why not simply define the class as follows?
class EVP_Q_Digest_Operation extends EVP_Hash_Operation, OpenSSLAlgorithmValueConsumer
I would try to reduce fragmentation across files for very closely intertwined concepts in general, but in this particular case, we can even define both concepts as one class.
| // Flow out through any return or other argument of the same type | ||
| // Assume flow in and out is asIndirectExpr or asDefinitingArgument since a pointer is assumed | ||
| // to be involved | ||
| // NOTE: not attempting to detect openssl specific copy/dup functions, but anything suspected to be copy/dup |
There was a problem hiding this comment.
Since this is supposed to be a heuristic, can you include examples?
Suggested change:
// Flow may propagate through return values or other arguments of the same type.
// Assume both incoming and outgoing flow use `asIndirectExpr` or `asDefiningArgument`,
// since pointers are likely involved.
// NOTE: This does not attempt to detect OpenSSL-specific copy/dup functions;
// it targets any function suspected to perform copying or duplication.
Also, what about type conversions, such as byte arrays to strings? Perhaps that's a different bit of logic and one handled anyway via taint-tracking, but I want to make clear what this case (vs the generic catch-all taint tracking flow steps) are for.
| this.getTarget().getName() in ["OBJ_obj2nid", "OBJ_ln2nid", "OBJ_sn2nid", "OBJ_txt2nid"] and | ||
| ( | ||
| inNode.asIndirectExpr() = this.getArgument(0) | ||
| or |
There was a problem hiding this comment.
Only asExpr or asIndirectExpr are going to be correct in any one given instance. If it depends on the function, then we need to specialize that per-function to avoid issues, particularly in the indirect case.
| alias = "ssl3-sha1" and target = "sha1" | ||
| } | ||
|
|
||
| predicate tbd(string normalized, string algType) { |
There was a problem hiding this comment.
Normalization to key size / alg family type should occur as it does in our JCA modelling and not via intermediate string types: map it to the 'fixed' internal family type as defined as Model.qll, with key size or algorithm variants defined separately (in this code, AES-128 and SHA-256 are two examples that stick out).
| * there is no input, rather it directly gets an algorithm | ||
| * and returns it. | ||
| */ | ||
| class DirectAlgorithmValueConsumer extends OpenSSLAlgorithmValueConsumer { |
There was a problem hiding this comment.
Why do we need to extend OpenSSLAlgorithmValueConsumer here?
| abstract int getFixedDigestLength(); | ||
| } | ||
|
|
||
| predicate fixedImplicitDigestLength(THashType type, int digestLength) { |
There was a problem hiding this comment.
This should be handled not in the instance but rather implicitly in the node. Documentation should exist to clarify which algorithms need getDigestLength() specified or a global predicate in Model.qll should exist as a helper to define those implicit type/digestlen mappings. I suggest a predicate on the node that takes the type from this.
| string getKeySizeFixed() { | ||
| result = instance.asAlg().getKeySizeFixed() | ||
| or | ||
| exists(int size | |
There was a problem hiding this comment.
This implementation points to inconsistent logic; either we put the responsibility of defining the key size on the modeller in all instances or we accept that we need to "fill in the blank with an implicit default" in other cases.
This needs to be defined, with one clear path and instructions added as QLDoc comments on the instance level for modellers. If that's a global predicate in Model.qll that provides defaults, so be it. Otherwise, we need to define which types need to have a size provided and which don't.
There was a problem hiding this comment.
Also, we should unify getDigestLength and getKeySizeFixed as well as the Length/Size/Fixed/ terminology.
| override string getAlgorithmName() { this.hashTypeToNameMapping(this.getHashFamily(), result) } | ||
|
|
||
| int getDigestLength() { result = instance.asAlg().getDigestLength() } | ||
| int getDigestLength() { |
There was a problem hiding this comment.
Same issues as for getKeySizeFixed above: "we should unify getDigestLength and getKeySizeFixed as well as the Length/Size/Fixed/ terminology" and concerns about inconsistent modelling paths.
| override Expr getIVArg() { result = this.(Call).getArgument(4) } | ||
| } | ||
|
|
||
| abstract class EVP_EX2_Initializer extends EVP_Cipher_Inititalizer { |
There was a problem hiding this comment.
Does this need to be abstract? Where are we using that functionality?
| import experimental.Quantum.Language | ||
| import semmle.code.cpp.dataflow.new.DataFlow | ||
|
|
||
| abstract class OpenSSLAlgorithmValueConsumer extends Crypto::AlgorithmValueConsumer instanceof Call { |
There was a problem hiding this comment.
Why is this necessary as opposed to defining abstract classes for specific types of algorithm value consumers, as done in the JCA, where we define specific abstract library-specific AVC classes for specific algorithm types, e.g., HashAlgorithmValueConsumer?
There was a problem hiding this comment.
I think there should be a giant "to-do" here at the very least: we should restrict source and sinks to analysis-relevant sources and sinks, with intermediate function calls that have context parameters modeled as additional steps (with state if necessary). As-is, this configuration is overly broad and under-specified.
| abstract Expr getIVArg(); | ||
|
|
||
| // abstract Crypto::CipherOperationSubtype getCipherOperationSubtype(); | ||
| abstract Expr getOperataionSubtypeArg(); |
There was a problem hiding this comment.
| abstract Expr getOperataionSubtypeArg(); | |
| abstract Expr getOperationSubtypeArg(); |
Fixes a typo.
|
Merging the changes now regardless (to revisit feedback) |
2 participants
No description provided.