diff --git a/java/ql/lib/experimental/Quantum/JCA.qll b/java/ql/lib/experimental/Quantum/JCA.qll index b2698cabee24..54c754b45807 100644 --- a/java/ql/lib/experimental/Quantum/JCA.qll +++ b/java/ql/lib/experimental/Quantum/JCA.qll @@ -4,6 +4,7 @@ import semmle.code.java.controlflow.Dominance module JCAModel { import Language + import KeyConcepts // TODO: Verify that the PBEWith% case works correctly bindingset[algo] diff --git a/java/ql/lib/experimental/Quantum/KeyConcepts.qll b/java/ql/lib/experimental/Quantum/KeyConcepts.qll new file mode 100644 index 000000000000..f34bd6196508 --- /dev/null +++ b/java/ql/lib/experimental/Quantum/KeyConcepts.qll @@ -0,0 +1,227 @@ +import Language + +/** + * Key Material Concept + * any class that implements `java.security.spec.KeySpec` + */ +class KeyMaterialObject extends Class { + KeyMaterialObject() { + exists(RefType t | + this.extendsOrImplements*(t) and + t.hasQualifiedName("java.security.spec", "KeySpec") + ) + } +} + +/** + * KeyMaterial + * ie some plain material that gets used to generate a Key + */ +class KeyMaterialInstantiation extends Crypto::KeyMaterialInstance instanceof ClassInstanceExpr { + KeyMaterialInstantiation() { + this.(ClassInstanceExpr).getConstructedType() instanceof KeyMaterialObject + } + + override DataFlow::Node getOutputNode() { result.asExpr() = this } + + override DataFlow::Node getInputNode() { + result.asExpr() = this.(ClassInstanceExpr).getArgument(0) + } + + //TODO + override predicate flowsTo(Crypto::FlowAwareElement other) { none() } +} + +abstract class JCAKeyGenerationCall extends Call { } + +private class JCAKeyGenerationCallWithMaterialArg extends JCAKeyGenerationCall { + JCAKeyGenerationCallWithMaterialArg() { + exists(string s | s in ["generatePrivate", "generatePublic", "translateKey"] | + this.getCallee().hasQualifiedName("java.security", "KeyFactory", s) + ) + } + + DataFlow::Node getInputData() { result.asExpr() = this.getArgument(0) } +} + +private class JCAKeyGenerationCallWithoutMaterialArg extends JCAKeyGenerationCall { + JCAKeyGenerationCallWithoutMaterialArg() { + this.getCallee().hasQualifiedName("javax.crypto", "KeyGenerator", "generateKey") + } + + DataFlow::Node getInputData() { result.asExpr() = this.getArgument(0) } +} + +abstract class KeyGenerators extends Class { + Method getInstance() { result = this.getAMethod() and result.hasName("getInstance") } + + abstract Method keySizeSetter(); +} + +class KeyGenerator extends KeyGenerators { + KeyGenerator() { this.hasQualifiedName("javax.crypto", "KeyGenerator") } + + override Method keySizeSetter() { result = this.getAMethod() and result.hasName("init") } +} + +class KeyFactory extends KeyGenerators { + KeyFactory() { this.hasQualifiedName("java.security", "KeyFactory") } + + //todo this requires flow from a keyspec + override Method keySizeSetter() { none() } +} + +class KeyPairGenerator extends KeyGenerators { + KeyPairGenerator() { this.hasQualifiedName("java.security", "KeyPairGenerator") } + + override Method keySizeSetter() { result = this.getAMethod() and result.hasName("initialize") } +} + +/** + * Data-flow configuration modelling flow from a string literal to a `KeyGeneratorsGetInstanceCall` argument. + */ +private module KeyGenAlgorithmStringToFetchFlow implements DataFlow::ConfigSig { + //TODO narrow this type based on JCA standard names doc for whats possible here, like is done for the cipher algs + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof StringLiteral } + + predicate isSink(DataFlow::Node sink) { + exists(KeyGenerators gen, Call c | + c.getCallee() = gen.getInstance() and + c.getArgument(0) = sink.asExpr() + ) + } +} + +module KeyGenAlgorithmStringToFetchFlowIns = DataFlow::Global; + +/** + * The key generation algorithm argument to a `KeyGeneratorsGetInstanceCall `. + * + * For example, in `KeyGenerator.getInstance(algorithm)`, this class represents `algorithm`. + * + * the algorithm is both what the resulting key is meant to be used for but also informs how it + * was created + */ +class KeyDerivationAlgorithmArg extends Crypto::KeyDerivationAlgorithmInstance instanceof Expr { + Call getInstanceCall; + + KeyDerivationAlgorithmArg() { + exists(KeyGenerators gen | + getInstanceCall.getCallee() = gen.getInstance() and this = getInstanceCall.getArgument(0) + ) + } + + /** + * Returns the `StringLiteral` from which this argument is derived, if known. + */ + StringLiteral getOrigin() { + KeyGenAlgorithmStringToFetchFlowIns::flow(DataFlow::exprNode(result), + DataFlow::exprNode(this.(Expr).getAChildExpr*())) + } + + Call getCall() { result = getInstanceCall } +} + +/** + * A data-flow configuration to track flow from a integer value to + * the keysize argument of the various `init` methods of the KeyGeneratorsGetInstanceCall types + */ +private module IntToKeyGeneratorInitConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr() instanceof IntegerLiteral } + + predicate isSink(DataFlow::Node sink) { + exists(KeyGenerators c, Call call | + call.getCallee() = c.keySizeSetter() and call.getArgument(0) = sink.asExpr() + ) + } +} + +module IntToKeyGeneratorInitConfigFlow = DataFlow::Global; + +/** + * Data-flow configuration modelling flow from a keygenerator type getinstance call + * to generation step + */ +module KeyDerivationOperationInstanceConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { + exists(Call getInstanceCall, KeyGenerators kg | + kg.getInstance() = getInstanceCall.getCallee() and + src.asExpr() = getInstanceCall + ) + } + + predicate isSink(DataFlow::Node sink) { + exists(JCAKeyGenerationCall call | sink.asExpr() = call.getQualifier()) + } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(Call getInstanceCall, KeyGenerators kg, Call keySizeSetterCall | + kg.getInstance() = getInstanceCall.getCallee() and + node1.asExpr() = getInstanceCall.getQualifier() and + kg.keySizeSetter() = keySizeSetterCall.getCallee() and + node2.asExpr() = keySizeSetterCall.getQualifier() + ) + } +} + +module KeyDerivationOperationInstanceConfigFlow = + DataFlow::Global; + +class KeyDerivationOperationInstance extends Crypto::KeyDerivationOperationInstance instanceof Call { + Crypto::KeyDerivationAlgorithmInstance algorithm; + JCAKeyGenerationCall generateCall; + + KeyDerivationOperationInstance() { + exists(KeyDerivationAlgorithmArg arg, DataFlow::Node source, DataFlow::Node sink | + this = generateCall and + generateCall.getQualifier() = sink.asExpr() and + KeyDerivationOperationInstanceConfigFlow::flow(source, sink) and + algorithm = arg and + source.asExpr() = arg.getCall() + ) + } + + //TODO fill these out using various flows defined above + override Crypto::KeyDerivationAlgorithmInstance getAlgorithm() { result = algorithm } + + override Crypto::KeyMaterialInstance getInputKeyMaterial() { none() } + + override Crypto::KeyArtifactInstance getOutputKey() { none() } + //exists(KeyObject key | result = key and key.getOutputNode() = this) } +} + +/** + * A Key Object + */ +class KeyObject extends Crypto::KeyArtifactInstance instanceof Variable { + Crypto::KeyDerivationOperationInstance instantiation; + + KeyObject() { + exists(RefType t | + instantiation = this.getInitializer() and + this.getType().(RefType).extendsOrImplements*(t) and + t.hasQualifiedName("java.security", "Key") + ) + } + + override DataFlow::Node getOutputNode() { result.asExpr() = instantiation } + + //TODO fix by using the KeyDerivationOperationInstance type in this type + override DataFlow::Node getInputNode() { + result.asExpr() = instantiation + } + + override DataFlow::Node getKeySize() { exists(DataFlow::Node src, DataFlow::Node sink, Call initcall + | + IntToKeyGeneratorInitConfigFlow::flow(src, sink) + and result = src + and initcall.getArgument(0) = sink.asExpr() + //init qualifier same as keygen qualifier, we need to use more of the connected concepts tho, this is re-inventing them every time we need something +and initcall.getQualifier() = instantiation.(JCAKeyGenerationCall).getQualifier().(VarAccess).getVariable().getAnAccess() + +) +} + + //TODO + override predicate flowsTo(Crypto::FlowAwareElement other) { none() } +} diff --git a/java/ql/src/experimental/Quantum/ConstantPassword.ql b/java/ql/src/experimental/Quantum/ConstantPassword.ql new file mode 100644 index 000000000000..aa339843b4de --- /dev/null +++ b/java/ql/src/experimental/Quantum/ConstantPassword.ql @@ -0,0 +1,38 @@ +/** + * @name Constant password + * @description Using constant passwords is not secure, because potential attackers can easily recover them from the source code. + * @kind problem + * @problem.severity error + * @security-severity 6.8 + * @precision high + * @id java/constant-password-new-model + * @tags security + * external/cwe/cwe-259 + */ + +//this query is a replica of the concept in: https://github.com/github/codeql/blob/main/swift/ql/src/queries/Security/CWE-259/ConstantPassword.ql +//but uses the **NEW MODELLING** +import experimental.Quantum.Language +import semmle.code.java.dataflow.TaintTracking + +/** + * A constant password is created through either a byte array or string literals. + */ +class ConstantPasswordSource extends Expr { + ConstantPasswordSource() { + this instanceof CharacterLiteral or + this instanceof StringLiteral + } +} + +module ConstantToKeyDerivationFlow implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source.asExpr() instanceof ConstantPasswordSource } + + predicate isSink(DataFlow::Node sink) { any(Crypto::KeyMaterialInstance i).getInputNode() = sink } +} + +module ConstantToKeyDerivationFlowInit = TaintTracking::Global; + +from DataFlow::Node source, DataFlow::Node sink +where ConstantToKeyDerivationFlowInit::flow(source, sink) +select sink, "Constant password $@ is used.", source, source.toString() diff --git a/java/ql/src/experimental/Quantum/InsuficientKeySize.ql b/java/ql/src/experimental/Quantum/InsuficientKeySize.ql new file mode 100644 index 000000000000..aa6cd36cf9fa --- /dev/null +++ b/java/ql/src/experimental/Quantum/InsuficientKeySize.ql @@ -0,0 +1,28 @@ +/** + * @name Use of a cryptographic algorithm with insufficient key size + * @description Using cryptographic algorithms with too small a key size can + * allow an attacker to compromise security. + * @kind path-problem + * @problem.severity warning + * @security-severity 7.5 + * @precision high + * @id java/insufficient-key-size-new-model + * @tags security + * external/cwe/cwe-326 + */ + +//this query is a replica of the concept in: https://github.com/github/codeql/blob/main/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql +//but uses the **NEW MODELLING** +//AND is actually only a subset of the logic - +//precisely this currently detects insuffficient key size for AES +import experimental.Quantum.Language + +/** Returns the minimum recommended key size for AES. */ +int minSecureKeySizeAes() { result = 128 } + +//todo make this actually check to match the alg to AES (some JCA model improve required) +//currently looks for any key size lower than... +from Crypto::KeyArtifactInstance key +where key.getKeySize().asExpr().(IntegerLiteral).getIntValue() < minSecureKeySizeAes() +select key, "This $@ is less than the recommended key size of " + minSecureKeySizeAes() + " bits.", + key.getKeySize(), "key size" diff --git a/shared/cryptography/codeql/cryptography/Model.qll b/shared/cryptography/codeql/cryptography/Model.qll index f5f57d245ba1..9fff36eeea11 100644 --- a/shared/cryptography/codeql/cryptography/Model.qll +++ b/shared/cryptography/codeql/cryptography/Model.qll @@ -368,11 +368,16 @@ module CryptographyBase Input> { * Gets the type of this digest algorithm, e.g., "SHA1", "SHA2", "MD5" etc. */ abstract THashType getHashFamily(); - // abstract int getHashSize(); } - abstract class KeyDerivationOperationInstance extends KnownElement { } + abstract class KeyDerivationOperationInstance extends KnownElement { + abstract KeyDerivationAlgorithmInstance getAlgorithm(); + + abstract KeyMaterialInstance getInputKeyMaterial(); + + abstract KeyArtifactInstance getOutputKey(); + } abstract class KeyDerivationAlgorithmInstance extends KnownElement { } @@ -395,12 +400,17 @@ module CryptographyBase Input> { abstract class CipherInputConsumer extends ArtifactConsumerAndInstance { } // Other artifacts - abstract class KeyArtifactInstance extends ArtifactElement { } // TODO: implement and categorize + abstract class KeyMaterialInstance extends ArtifactElement { } + + abstract class KeyArtifactInstance extends ArtifactElement { + abstract DataFlowNode getKeySize(); + } newtype TNode = // Artifacts (data that is not an operation or algorithm, e.g., a key) TDigest(DigestArtifactInstance e) or TKey(KeyArtifactInstance e) or + TKeyMaterial(KeyMaterialInstance e) or TNonce(NonceArtifactConsumer e) or TCipherInput(CipherInputConsumer e) or TCipherOutput(CipherOutputArtifactInstance e) or @@ -1085,7 +1095,9 @@ module CryptographyBase Input> { */ abstract class HashAlgorithmNode extends AlgorithmNode, THashAlgorithm { HashAlgorithmInstance instance; + HashAlgorithmNode() { this = THashAlgorithm(instance) } + override string getInternalType() { result = "HashAlgorithm" } final predicate hashTypeToNameMapping(THashType type, string name) { @@ -1200,6 +1212,8 @@ module CryptographyBase Input> { * For known algorithms, use the specialized classes, e.g., `HKDF` and `PKCS12KDF`. */ abstract class KeyDerivationAlgorithmNode extends AlgorithmNode, TKeyDerivationAlgorithm { + final LocatableElement getInstance() { this = TKeyDerivationAlgorithm(result) } + final override Location getLocation() { exists(LocatableElement le | this = TKeyDerivationAlgorithm(le) and result = le.getLocation()) } @@ -1471,4 +1485,58 @@ module CryptographyBase Input> { abstract class KEMAlgorithm extends TKeyEncapsulationAlgorithm, AlgorithmNode { final override string getInternalType() { result = "KeyEncapsulationAlgorithm" } } + + /** + * A Key Material Object + */ + private class KeyMaterialImpl extends ArtifactNode, TKeyMaterial { + KeyMaterialInstance instance; + + KeyMaterialImpl() { this = TKeyMaterial(instance) } + + final override string getInternalType() { result = "KeyMaterial" } + + override Location getLocation() { result = instance.getLocation() } + + override LocatableElement asElement() { result = instance } + } + + final class KeyMaterial = KeyMaterialImpl; + + /** + * A Key Object + */ + private class KeyArtifactImpl extends ArtifactNode, TKey { + KeyArtifactInstance instance; + + KeyArtifactImpl() { this = TKey(instance) } + + final override string getInternalType() { result = "KeyArtifact" } + + override Location getLocation() { result = instance.getLocation() } + + override LocatableElement asElement() { result = instance } + } + + final class KeyArtifact = KeyArtifactImpl; + + /** + * A Key Derivation operation + * todo: decide if derivation or generation is the right term here for the most general concept + */ + private class KeyDerivationOperationImpl extends OperationNode, TKeyDerivationOperation { + KeyDerivationOperationInstance instance; + + KeyDerivationOperationImpl() { this = TKeyDerivationOperation(instance) } + + final override string getInternalType() { result = "KeyDerivationOperation" } + + override Location getLocation() { result = instance.getLocation() } + + KeyDerivationAlgorithmNode getAlgorithm() { result.getInstance() = instance.getAlgorithm() } + + override LocatableElement asElement() { result = instance } + } + + final class KeyDerivationOperation = KeyDerivationOperationImpl; }