From 95544e20b2715e7cc108f4ec16fbeecee3fce160 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 24 Feb 2025 13:09:38 -0500 Subject: [PATCH 1/3] Add model and sample query for constants used in key specs --- java/ql/lib/experimental/Quantum/JCA.qll | 27 +++++++++++++ .../experimental/Quantum/ConstantPassword.ql | 38 +++++++++++++++++++ .../codeql/cryptography/Model.qll | 22 +++++++++++ 3 files changed, 87 insertions(+) create mode 100644 java/ql/src/experimental/Quantum/ConstantPassword.ql diff --git a/java/ql/lib/experimental/Quantum/JCA.qll b/java/ql/lib/experimental/Quantum/JCA.qll index 9892b445312a..3e2d6b53d944 100644 --- a/java/ql/lib/experimental/Quantum/JCA.qll +++ b/java/ql/lib/experimental/Quantum/JCA.qll @@ -532,4 +532,31 @@ module JCAModel { this.getMethod().getParameterType(2).hasName("AlgorithmParameterSpec") } } + + /** + * 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 asOutputData() { result.asExpr() = this } + + override DataFlow::Node getInput() { result.asExpr() = this.(ClassInstanceExpr).getArgument(0) } + } } diff --git a/java/ql/src/experimental/Quantum/ConstantPassword.ql b/java/ql/src/experimental/Quantum/ConstantPassword.ql new file mode 100644 index 000000000000..4d895ac8c0f8 --- /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).getInput() = 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/shared/cryptography/codeql/cryptography/Model.qll b/shared/cryptography/codeql/cryptography/Model.qll index e5b91e198eaf..1b96ba4cf3f4 100644 --- a/shared/cryptography/codeql/cryptography/Model.qll +++ b/shared/cryptography/codeql/cryptography/Model.qll @@ -118,6 +118,8 @@ module CryptographyBase Input> { abstract class DigestArtifactInstance extends ArtifactLocatableElement { } + abstract class KeyMaterialInstance extends ArtifactLocatableElement { } + abstract class KeyArtifactInstance extends ArtifactLocatableElement { } abstract class NonceArtifactInstance extends ArtifactLocatableElement { } @@ -130,6 +132,7 @@ module CryptographyBase Input> { // 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(NonceArtifactInstance e) or TRandomNumberGeneration(RandomNumberGenerationInstance e) or // Operations (e.g., hashing, encryption) @@ -1000,4 +1003,23 @@ module CryptographyBase Input> { abstract class KEMAlgorithm extends TKeyEncapsulationAlgorithm, Algorithm { final override string getAlgorithmType() { result = "KeyEncapsulationAlgorithm" } } + + /** + * A Key Material Object + */ + private class KeyMaterialImpl extends Artifact, TKeyMaterial { + KeyMaterialInstance instance; + + KeyMaterialImpl() { this = TKeyMaterial(instance) } + + final override string getInternalType() { result = "KeyMaterial" } + + override Location getLocation() { result = instance.getLocation() } + + override DataFlowNode asOutputData() { result = instance.asOutputData() } + + override DataFlowNode getInputData() { result = instance.getInput() } + } + + final class KeyMaterial = KeyMaterialImpl; } From 4bcd69e501df8c8341c8e60bc4a43723669d8fbd Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Tue, 25 Feb 2025 15:08:29 -0500 Subject: [PATCH 2/3] Add more key concepts to key model JCA --- java/ql/lib/experimental/Quantum/JCA.qll | 32 ++++++++++ .../codeql/cryptography/Model.qll | 63 +++++++++++++++---- 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/java/ql/lib/experimental/Quantum/JCA.qll b/java/ql/lib/experimental/Quantum/JCA.qll index 3e2d6b53d944..bc37ff97ee08 100644 --- a/java/ql/lib/experimental/Quantum/JCA.qll +++ b/java/ql/lib/experimental/Quantum/JCA.qll @@ -559,4 +559,36 @@ module JCAModel { override DataFlow::Node getInput() { result.asExpr() = this.(ClassInstanceExpr).getArgument(0) } } + + /** + * A Key Object + */ + class KeyInstantiation extends Crypto::KeyArtifactInstance instanceof Expr { + KeyInstantiation() { + exists(RefType t, Variable v | + this = v.getInitializer() and + v.getType().(RefType).extendsOrImplements*(t) and + t.hasQualifiedName("java.security", "Key") + ) + } + + override DataFlow::Node asOutputData() { result.asExpr() = this } + + override DataFlow::Node getInput() { result.asExpr() = this.(ClassInstanceExpr).getArgument(0) } + + override DataFlow::Node getKeySize() { none() } + } + + /** + * Key Factory + * any class that implements `java.security.KeyFactory` + */ + class KeyFactoryObject extends Class { + KeyFactoryObject() { + exists(RefType t | + this.extendsOrImplements*(t) and + t.hasQualifiedName("java.security", "KeyFactory") + ) + } + } } diff --git a/shared/cryptography/codeql/cryptography/Model.qll b/shared/cryptography/codeql/cryptography/Model.qll index 1b96ba4cf3f4..15ae51632306 100644 --- a/shared/cryptography/codeql/cryptography/Model.qll +++ b/shared/cryptography/codeql/cryptography/Model.qll @@ -82,7 +82,13 @@ module CryptographyBase Input> { abstract class HashAlgorithmInstance extends LocatableElement { } - abstract class KeyDerivationOperationInstance extends LocatableElement { } + abstract class KeyDerivationOperationInstance extends LocatableElement { + abstract KeyDerivationAlgorithmInstance getAlgorithm(); + + abstract KeyMaterialInstance getInputKeyMaterial(); + + abstract KeyArtifactInstance getOutputKey(); + } abstract class KeyDerivationAlgorithmInstance extends LocatableElement { } @@ -120,7 +126,9 @@ module CryptographyBase Input> { abstract class KeyMaterialInstance extends ArtifactLocatableElement { } - abstract class KeyArtifactInstance extends ArtifactLocatableElement { } + abstract class KeyArtifactInstance extends ArtifactLocatableElement { + abstract DataFlowNode getKeySize(); + } abstract class NonceArtifactInstance extends ArtifactLocatableElement { } @@ -410,16 +418,6 @@ module CryptographyBase Input> { } } - /** - * An operation that derives one or more keys from an input value. - */ - abstract class KeyDerivationOperation extends Operation, TKeyDerivationOperation { - final override Location getLocation() { - exists(LocatableElement le | this = TKeyDerivationOperation(le) and result = le.getLocation()) - } - //override string getOperationType() { result = "KeyDerivationOperation" } - } - /** * An algorithm that derives one or more keys from an input value. * @@ -428,6 +426,8 @@ module CryptographyBase Input> { * For known algorithms, use the specialized classes, e.g., `HKDF` and `PKCS12KDF`. */ abstract class KeyDerivationAlgorithm extends Algorithm, TKeyDerivationAlgorithm { + final LocatableElement getInstance() { this = TKeyDerivationAlgorithm(result) } + final override Location getLocation() { exists(LocatableElement le | this = TKeyDerivationAlgorithm(le) and result = le.getLocation()) } @@ -1022,4 +1022,43 @@ module CryptographyBase Input> { } final class KeyMaterial = KeyMaterialImpl; + + /** + * A Key Object + */ + private class KeyArtifactImpl extends Artifact, TKey { + KeyArtifactInstance instance; + + KeyArtifactImpl() { this = TKey(instance) } + + final override string getInternalType() { result = "KeyMaterial" } + + override Location getLocation() { result = instance.getLocation() } + + override DataFlowNode asOutputData() { result = instance.asOutputData() } + + override DataFlowNode getInputData() { result = instance.getInput() } + } + + 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 Operation, TKeyDerivationOperation { + KeyDerivationOperationInstance instance; + + KeyDerivationOperationImpl() { this = TKeyDerivationOperation(instance) } + + final override string getInternalType() { result = "KeyMaterial" } + + override Location getLocation() { result = instance.getLocation() } + + override KeyDerivationAlgorithm getAlgorithm() { + result.getInstance() = instance.getAlgorithm() + } + } + + final class KeyDerivationOperation = KeyDerivationOperationImpl; } From 20fb8b872698bdc3d264b30612ab2b9533f76c1d Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Thu, 27 Feb 2025 22:40:36 -0500 Subject: [PATCH 3/3] Add more key concepts to JCA model key gen, artifact and size --- java/ql/lib/experimental/Quantum/JCA.qll | 172 +++++++++++++++++++++-- 1 file changed, 159 insertions(+), 13 deletions(-) diff --git a/java/ql/lib/experimental/Quantum/JCA.qll b/java/ql/lib/experimental/Quantum/JCA.qll index bc37ff97ee08..59aadd3652b3 100644 --- a/java/ql/lib/experimental/Quantum/JCA.qll +++ b/java/ql/lib/experimental/Quantum/JCA.qll @@ -560,6 +560,164 @@ module JCAModel { override DataFlow::Node getInput() { result.asExpr() = this.(ClassInstanceExpr).getArgument(0) } } + 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() { none() } + + override Crypto::KeyMaterialInstance getInputKeyMaterial() { none() } + + override Crypto::KeyArtifactInstance getOutputKey() { none() } + } + /** * A Key Object */ @@ -574,21 +732,9 @@ module JCAModel { override DataFlow::Node asOutputData() { result.asExpr() = this } + //TODO fix by using the KeyDerivationOperationInstance type in this type override DataFlow::Node getInput() { result.asExpr() = this.(ClassInstanceExpr).getArgument(0) } override DataFlow::Node getKeySize() { none() } } - - /** - * Key Factory - * any class that implements `java.security.KeyFactory` - */ - class KeyFactoryObject extends Class { - KeyFactoryObject() { - exists(RefType t | - this.extendsOrImplements*(t) and - t.hasQualifiedName("java.security", "KeyFactory") - ) - } - } }