From 2daa3457d79906b53353ad8a5cc0c475e770f5bd Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Thu, 13 Oct 2022 17:57:56 -0400 Subject: [PATCH] combine three configs into one --- .../java/security/InsufficientKeySize.qll | 58 ++++--- .../security/InsufficientKeySizeQuery.qll | 141 ++++-------------- .../CWE/CWE-326/InsufficientKeySize.ql | 11 +- .../CWE-326/InsufficientKeySizeTest.ql | 10 +- 4 files changed, 79 insertions(+), 141 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll b/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll index 53efe07b490..dde014fe6de 100644 --- a/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll +++ b/java/ql/lib/semmle/code/java/security/InsufficientKeySize.qll @@ -3,30 +3,41 @@ private import semmle.code.java.security.Encryption private import semmle.code.java.dataflow.DataFlow +// TODO: only update key sizes (and key size strings in one place in the code) /** A source for an insufficient key size. */ -abstract class InsufficientKeySizeSource extends DataFlow::Node { } +abstract class InsufficientKeySizeSource extends DataFlow::Node { + /** Holds if this source has the specified `state`. */ + predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty } +} /** A sink for an insufficient key size. */ -abstract class InsufficientKeySizeSink extends DataFlow::Node { } - -// TODO: Consider if below 3 sources should be private and if it's possible to only use InsufficientKeySizeSource in the configs -// TODO: add QLDocs if keeping non-private -/** A source for an insufficient key size (asymmetric-non-ec). */ -class AsymmetricNonECSource extends InsufficientKeySizeSource { - AsymmetricNonECSource() { getNodeIntValue(this) < 2048 } +abstract class InsufficientKeySizeSink extends DataFlow::Node { + /** Holds if this sink has the specified `state`. */ + predicate hasState(DataFlow::FlowState state) { state instanceof DataFlow::FlowStateEmpty } } -/** A source for an insufficient key size (asymmetric-ec). */ -class AsymmetricECSource extends InsufficientKeySizeSource { +/** A source for an insufficient key size (asymmetric-non-ec: RSA, DSA, DH). */ +private class AsymmetricNonECSource extends InsufficientKeySizeSource { + AsymmetricNonECSource() { getNodeIntValue(this) < 2048 } + + override predicate hasState(DataFlow::FlowState state) { state = "2048" } +} + +/** A source for an insufficient key size (asymmetric-ec: EC%). */ +private class AsymmetricECSource extends InsufficientKeySizeSource { AsymmetricECSource() { getNodeIntValue(this) < 256 or - getECKeySize(this.asExpr().(StringLiteral).getValue()) < 256 // need this for the cases when the key size is embedded in the curve name. + getECKeySize(this.asExpr().(StringLiteral).getValue()) < 256 // for cases when the key size is embedded in the curve name } + + override predicate hasState(DataFlow::FlowState state) { state = "256" } } -/** A source for an insufficient key size (symmetric). */ -class SymmetricSource extends InsufficientKeySizeSource { +/** A source for an insufficient key size (symmetric: AES). */ +private class SymmetricSource extends InsufficientKeySizeSource { SymmetricSource() { getNodeIntValue(this) < 128 } + + override predicate hasState(DataFlow::FlowState state) { state = "128" } } // TODO: use `typeFlag` like with sinks below to include the size numbers in this predicate? @@ -34,6 +45,7 @@ private int getNodeIntValue(DataFlow::Node node) { result = node.asExpr().(IntegerLiteral).getIntValue() } +// TODO: check if any other regex should be added based on some MRVA results. /** Returns the key size in the EC algorithm string */ bindingset[algorithm] private int getECKeySize(string algorithm) { @@ -47,18 +59,17 @@ private int getECKeySize(string algorithm) { result = algorithm.regexpCapture(".*[a-zA-Z](\\d+)[a-zA-Z].*", 1).toInt() } -// TODO: Consider if below 3 sinks should be private and if it's possible to only use InsufficientKeySizeSink in the configs -// TODO: add QLDocs if keeping non-private /** A sink for an insufficient key size (asymmetric-non-ec). */ -class AsymmetricNonECSink extends InsufficientKeySizeSink { +private class AsymmetricNonECSink extends InsufficientKeySizeSink { AsymmetricNonECSink() { hasKeySizeInInitMethod(this, "asymmetric-non-ec") or hasKeySizeInSpec(this, "asymmetric-non-ec") } + + override predicate hasState(DataFlow::FlowState state) { state = "2048" } } -// TODO: move to Encryption.qll? or keep here since specific to this query? private class AsymmetricNonECSpec extends RefType { AsymmetricNonECSpec() { this instanceof RsaKeyGenParameterSpec or @@ -68,17 +79,21 @@ private class AsymmetricNonECSpec extends RefType { } /** A sink for an insufficient key size (asymmetric-ec). */ -class AsymmetricECSink extends InsufficientKeySizeSink { +private class AsymmetricECSink extends InsufficientKeySizeSink { AsymmetricECSink() { hasKeySizeInInitMethod(this, "asymmetric-ec") or hasKeySizeInSpec(this, "asymmetric-ec") } + + override predicate hasState(DataFlow::FlowState state) { state = "256" } } /** A sink for an insufficient key size (symmetric). */ -class SymmetricSink extends InsufficientKeySizeSink { +private class SymmetricSink extends InsufficientKeySizeSink { SymmetricSink() { hasKeySizeInInitMethod(this, "symmetric") } + + override predicate hasState(DataFlow::FlowState state) { state = "128" } } // TODO: rethink the predicate name; also think about whether this could/should be a class instead; or a predicate within the sink class so can do sink.predicate()... @@ -113,7 +128,7 @@ private string getAlgoName(JavaxCryptoAlgoSpec jca) { } // TODO: rethink the predicate name; also think about whether this could/should be a class instead; or a predicate within the sink class so can do sink.predicate()... -// TODO: can prbly re-work way using the typeFlag to be better and less repetitive +// TODO: can prbly re-work way using the typeFlag to be better and less repetitive... private predicate hasKeySizeInSpec(DataFlow::Node node, string typeFlag) { exists(ClassInstanceExpr paramSpec | ( @@ -126,7 +141,8 @@ private predicate hasKeySizeInSpec(DataFlow::Node node, string typeFlag) { node.asExpr() = paramSpec.getArgument(0) ) } + +class SpecWithKeySize extends RefType { } // TODO: // todo #0: look into use of specs without keygen objects; should spec not be a sink in these cases? // todo #3: make list of algo names more easily reusable (either as constant-type variable at top of file, or model as own class to share, etc.) -// todo #4: use flowstate (or even just result predicate) to pass source int size to sink? diff --git a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll index 51800aa8df9..08388c345d6 100644 --- a/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsufficientKeySizeQuery.qll @@ -6,120 +6,43 @@ import semmle.code.java.dataflow.TaintTracking import semmle.code.java.security.InsufficientKeySize /** - * A data flow configuration for tracking non-elliptic curve asymmetric algorithms + * A data flow configuration for tracking non-elliptic curve asymmetric algorithm * (RSA, DSA, and DH) key sizes. */ -class AsymmetricNonECKeyTrackingConfiguration extends DataFlow::Configuration { - AsymmetricNonECKeyTrackingConfiguration() { this = "AsymmetricNonECKeyTrackingConfiguration" } +class KeySizeConfiguration extends DataFlow::Configuration { + KeySizeConfiguration() { this = "KeySizeConfiguration" } - override predicate isSource(DataFlow::Node source) { source instanceof AsymmetricNonECSource } + override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { + source.(InsufficientKeySizeSource).hasState(state) + //source instanceof InsufficientKeySizeSource + } - override predicate isSink(DataFlow::Node sink) { sink instanceof AsymmetricNonECSink } + override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { + sink.(InsufficientKeySizeSink).hasState(state) + //sink instanceof InsufficientKeySizeSink + } } - -/** - * A data flow configuration for tracking elliptic curve (EC) asymmetric - * algorithm key sizes. - */ -class AsymmetricECKeyTrackingConfiguration extends DataFlow::Configuration { - AsymmetricECKeyTrackingConfiguration() { this = "AsymmetricECKeyTrackingConfiguration" } - - override predicate isSource(DataFlow::Node source) { source instanceof AsymmetricECSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof AsymmetricECSink } -} - -/** A data flow configuration for tracking symmetric algorithm (AES) key sizes. */ -class SymmetricKeyTrackingConfiguration extends DataFlow::Configuration { - SymmetricKeyTrackingConfiguration() { this = "SymmetricKeyTrackingConfiguration" } - - override predicate isSource(DataFlow::Node source) { source instanceof SymmetricSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof SymmetricSink } -} -// ******* 3 DATAFLOW CONFIGS ABOVE ************************************************************************* -// ******* SINGLE CONFIG ATTEMPT BELOW ************************************************************************* // /** -// * A key length data flow tracking configuration. +// * A data flow configuration for tracking non-elliptic curve asymmetric algorithm +// * (RSA, DSA, and DH) key sizes. // */ -// class KeyTrackingConfiguration extends DataFlow::Configuration { -// KeyTrackingConfiguration() { this = "KeyTrackingConfiguration" } -// override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { -// //state instanceof DataFlow::FlowStateEmpty and -// // SYMMETRIC -// source.asExpr().(IntegerLiteral).getIntValue() < 128 and state = "128" -// or -// // ASYMMETRIC -// source.asExpr().(IntegerLiteral).getIntValue() < 2048 and state = "2048" -// or -// source.asExpr().(IntegerLiteral).getIntValue() < 256 and state = "256" -// or -// getECKeySize(source.asExpr().(StringLiteral).getValue()) < 256 and state = "256" // need this for the cases when the key size is embedded in the curve name. -// } -// override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { -// // SYMMETRIC -// exists(MethodAccess ma, JavaxCryptoKeyGenerator jcg | -// ma.getMethod() instanceof KeyGeneratorInitMethod and -// jcg.getAlgoSpec().(StringLiteral).getValue().toUpperCase() = "AES" and -// DataFlow::localExprFlow(jcg, ma.getQualifier()) and -// sink.asExpr() = ma.getArgument(0) and -// state = "128" -// ) -// or -// // ASYMMETRIC -// exists(MethodAccess ma, JavaSecurityKeyPairGenerator jpg | -// ma.getMethod() instanceof KeyPairGeneratorInitMethod and -// ( -// jpg.getAlgoSpec().(StringLiteral).getValue().toUpperCase().matches(["RSA", "DSA", "DH"]) and -// DataFlow::localExprFlow(jpg, ma.getQualifier()) and -// sink.asExpr() = ma.getArgument(0) and -// //ma.getArgument(0).(LocalSourceNode).flowsTo(sink) and -// //ma.getArgument(0).(CompileTimeConstantExpr).getIntValue() < 2048 and -// state = "2048" -// ) -// or -// jpg.getAlgoSpec().(StringLiteral).getValue().toUpperCase().matches("EC%") and -// DataFlow::localExprFlow(jpg, ma.getQualifier()) and -// sink.asExpr() = ma.getArgument(0) and -// //ma.getArgument(0).(CompileTimeConstantExpr).getIntValue() < 256 and -// state = "256" -// ) -// or -// // TODO: combine below three for less duplicated code -// exists(ClassInstanceExpr rsaKeyGenParamSpec | -// rsaKeyGenParamSpec.getConstructedType() instanceof RsaKeyGenParameterSpec and -// sink.asExpr() = rsaKeyGenParamSpec.getArgument(0) and -// state = "2048" -// ) -// or -// exists(ClassInstanceExpr dsaGenParamSpec | -// dsaGenParamSpec.getConstructedType() instanceof DsaGenParameterSpec and -// sink.asExpr() = dsaGenParamSpec.getArgument(0) and -// state = "2048" -// ) -// or -// exists(ClassInstanceExpr dhGenParamSpec | -// dhGenParamSpec.getConstructedType() instanceof DhGenParameterSpec and -// sink.asExpr() = dhGenParamSpec.getArgument(0) and -// state = "2048" -// ) -// or -// exists(ClassInstanceExpr ecGenParamSpec | -// ecGenParamSpec.getConstructedType() instanceof EcGenParameterSpec and -// sink.asExpr() = ecGenParamSpec.getArgument(0) and -// state = "256" -// ) -// } -// // ! FlowStates seem to work without even including a step like the below... hmmm -// override predicate isAdditionalFlowStep( -// DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, -// DataFlow::FlowState state2 -// ) { -// exists(IntegerLiteral intLiteral | -// state1 = "" and -// state2 = intLiteral.toString() and -// node1.asExpr() = intLiteral and -// node2.asExpr() = intLiteral -// ) -// } +// class AsymmetricNonECKeyTrackingConfiguration extends DataFlow::Configuration { +// AsymmetricNonECKeyTrackingConfiguration() { this = "AsymmetricNonECKeyTrackingConfiguration" } +// override predicate isSource(DataFlow::Node source) { source instanceof AsymmetricNonECSource } +// override predicate isSink(DataFlow::Node sink) { sink instanceof AsymmetricNonECSink } +// } +// /** +// * A data flow configuration for tracking elliptic curve (EC) asymmetric +// * algorithm key sizes. +// */ +// class AsymmetricECKeyTrackingConfiguration extends DataFlow::Configuration { +// AsymmetricECKeyTrackingConfiguration() { this = "AsymmetricECKeyTrackingConfiguration" } +// override predicate isSource(DataFlow::Node source) { source instanceof AsymmetricECSource } +// override predicate isSink(DataFlow::Node sink) { sink instanceof AsymmetricECSink } +// } +// /** A data flow configuration for tracking symmetric algorithm (AES) key sizes. */ +// class SymmetricKeyTrackingConfiguration extends DataFlow::Configuration { +// SymmetricKeyTrackingConfiguration() { this = "SymmetricKeyTrackingConfiguration" } +// override predicate isSource(DataFlow::Node source) { source instanceof SymmetricSource } +// override predicate isSink(DataFlow::Node sink) { sink instanceof SymmetricSink } // } diff --git a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql index f9d3d3baa0f..8f0f54c7d9b 100644 --- a/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql +++ b/java/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql @@ -16,10 +16,9 @@ import semmle.code.java.security.InsufficientKeySizeQuery import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink -where - //exists(KeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) - //or - exists(AsymmetricNonECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or - exists(AsymmetricECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or - exists(SymmetricKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) +where exists(KeySizeConfiguration config1 | config1.hasFlowPath(source, sink)) +//or +// exists(AsymmetricNonECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or +// exists(AsymmetricECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or +// exists(SymmetricKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) select sink.getNode(), source, sink, "This $@ is too small.", source.getNode(), "key size" diff --git a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql index db925935d5c..5dc7ecdf467 100644 --- a/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql +++ b/java/ql/test/query-tests/security/CWE-326/InsufficientKeySizeTest.ql @@ -11,12 +11,12 @@ class InsufficientKeySizeTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasInsufficientKeySize" and exists(DataFlow::PathNode source, DataFlow::PathNode sink | - //exists(KeyTrackingConfiguration config1 | config1.hasFlowPath(source, sink)) - //or - exists(AsymmetricNonECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or - exists(AsymmetricECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or - exists(SymmetricKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) + exists(KeySizeConfiguration config1 | config1.hasFlowPath(source, sink)) | + //or + // exists(AsymmetricNonECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or + // exists(AsymmetricECKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) or + // exists(SymmetricKeyTrackingConfiguration cfg | cfg.hasFlowPath(source, sink)) sink.getNode().getLocation() = location and element = sink.getNode().toString() and value = ""