From 40529d1e51b57f079a16ac6a4265ca71b0303222 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 7 Oct 2024 13:16:10 +0200 Subject: [PATCH 01/10] DataFlow: Add the concept of selected locations This extension allows queries to be diff-informed even when the elements they select are different from the sources and sinks found by data flow. --- shared/dataflow/codeql/dataflow/DataFlow.qll | 44 +++++++++++++++++++ .../codeql/dataflow/internal/DataFlowImpl.qll | 8 +++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll index 400ed5746ebd..7c437adabb84 100644 --- a/shared/dataflow/codeql/dataflow/DataFlow.qll +++ b/shared/dataflow/codeql/dataflow/DataFlow.qll @@ -442,6 +442,28 @@ module Configs Lang> { * are used directly in a query result. */ default predicate observeDiffInformedIncrementalMode() { none() } + + /** + * Gets a location that will be associated with the given `source` in a + * diff-informed query that uses this configuration (see + * `observeDiffInformedIncrementalMode`). By default, this is the location + * of the source itself, but this predicate should include any locations + * that are reported as the primary-location of the query or as an + * additional location ("$@" interpolation). For a query that doesn't + * report the source at all, this predicate can be `none()`. + */ + default Location getASelectedSourceLocation(Node source) { result = source.getLocation() } + + /** + * Gets a location that will be associated with the given `sink` in a + * diff-informed query that uses this configuration (see + * `observeDiffInformedIncrementalMode`). By default, this is the location + * of the sink itself, but this predicate should include any locations + * that are reported as the primary-location of the query or as an + * additional location ("$@" interpolation). For a query that doesn't + * report the sink at all, this predicate can be `none()`. + */ + default Location getASelectedSinkLocation(Node sink) { result = sink.getLocation() } } /** An input configuration for data flow using flow state. */ @@ -569,6 +591,28 @@ module Configs Lang> { * are used directly in a query result. */ default predicate observeDiffInformedIncrementalMode() { none() } + + /** + * Gets a location that will be associated with the given `source` in a + * diff-informed query that uses this configuration (see + * `observeDiffInformedIncrementalMode`). By default, this is the location + * of the source itself, but this predicate should include any locations + * that are reported as the primary-location of the query or as an + * additional location ("$@" interpolation). For a query that doesn't + * report the source at all, this predicate can be `none()`. + */ + default Location getASelectedSourceLocation(Node source) { result = source.getLocation() } + + /** + * Gets a location that will be associated with the given `sink` in a + * diff-informed query that uses this configuration (see + * `observeDiffInformedIncrementalMode`). By default, this is the location + * of the sink itself, but this predicate should include any locations + * that are reported as the primary-location of the query or as an + * additional location ("$@" interpolation). For a query that doesn't + * report the sink at all, this predicate can be `none()`. + */ + default Location getASelectedSinkLocation(Node sink) { result = sink.getLocation() } } } diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll index 1373345423f7..6534aaa640c6 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll @@ -137,6 +137,10 @@ module MakeImpl Lang> { * are used directly in a query result. */ predicate observeDiffInformedIncrementalMode(); + + Location getASelectedSourceLocation(Node source); + + Location getASelectedSinkLocation(Node sink); } /** @@ -177,7 +181,7 @@ module MakeImpl Lang> { private predicate isFilteredSource(Node source) { Config::isSource(source, _) and if Config::observeDiffInformedIncrementalMode() - then AlertFiltering::filterByLocation(source.getLocation()) + then AlertFiltering::filterByLocation(Config::getASelectedSourceLocation(source)) else any() } @@ -188,7 +192,7 @@ module MakeImpl Lang> { Config::isSink(sink) ) and if Config::observeDiffInformedIncrementalMode() - then AlertFiltering::filterByLocation(sink.getLocation()) + then AlertFiltering::filterByLocation(Config::getASelectedSinkLocation(sink)) else any() } From 2561cec80c2df4588055ed41a8e33d12f124f70b Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 4 Oct 2024 19:19:37 +0200 Subject: [PATCH 02/10] Java: Diff-informed CommandLineQuery --- java/ql/lib/semmle/code/java/security/CommandLineQuery.qll | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/CommandLineQuery.qll b/java/ql/lib/semmle/code/java/security/CommandLineQuery.qll index af5476f8b3df..468ab9506a5f 100644 --- a/java/ql/lib/semmle/code/java/security/CommandLineQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CommandLineQuery.qll @@ -58,6 +58,13 @@ module InputToArgumentToExecFlowConfig implements DataFlow::ConfigSig { predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { any(CommandInjectionAdditionalTaintStep s).step(n1, n2) } + + // It's valid to use diff-informed data flow for this configuration because + // the location of the selected element in the query is contained inside the + // location of the sink. The query, as a predicate, is used negated in + // another query, but that's only to prevent overlapping results between two + // queries. + predicate observeDiffInformedIncrementalMode() { any() } } /** From eac1a4c002d29c6a56a3bb44b988436d8e1e50cf Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Sat, 5 Oct 2024 21:32:25 +0200 Subject: [PATCH 03/10] Java: Diff-informed SqlTainted.ql --- java/ql/lib/semmle/code/java/security/SqlInjectionQuery.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/SqlInjectionQuery.qll b/java/ql/lib/semmle/code/java/security/SqlInjectionQuery.qll index 0aaf46cf2dd5..67f0f1220433 100644 --- a/java/ql/lib/semmle/code/java/security/SqlInjectionQuery.qll +++ b/java/ql/lib/semmle/code/java/security/SqlInjectionQuery.qll @@ -24,6 +24,8 @@ module QueryInjectionFlowConfig implements DataFlow::ConfigSig { predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { any(AdditionalQueryInjectionTaintStep s).step(node1, node2) } + + predicate observeDiffInformedIncrementalMode() { any() } } /** Tracks flow of unvalidated user input that is used in SQL queries. */ From 8224ef69291e69c60f56b383218b6ab6da333889 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Sat, 5 Oct 2024 21:49:05 +0200 Subject: [PATCH 04/10] Java: Diff-informed InsecureTrustManager.ql --- .../code/java/security/InsecureTrustManagerQuery.qll | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/InsecureTrustManagerQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureTrustManagerQuery.qll index d732716ec2e5..39420807a27e 100644 --- a/java/ql/lib/semmle/code/java/security/InsecureTrustManagerQuery.qll +++ b/java/ql/lib/semmle/code/java/security/InsecureTrustManagerQuery.qll @@ -18,6 +18,17 @@ module InsecureTrustManagerConfig implements DataFlow::ConfigSig { node.getType() instanceof Array and c instanceof DataFlow::ArrayContent } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { + isSource(source) and + ( + result = source.getLocation() + or + result = source.asExpr().(ClassInstanceExpr).getConstructedType().getLocation() + ) + } } module InsecureTrustManagerFlow = DataFlow::Global; From fea260bd55e496e867aa78173b3d05d9927236d6 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Sat, 5 Oct 2024 21:54:14 +0200 Subject: [PATCH 05/10] Java: Diff-informed UnsafeHostnameVerification.ql This commit also adds a test case that would fail under `codeql test run --check-diff-informed` if not for the override of `getASelectedSourceLocation`. There was no existing such test since all the existing tests used anonymous classes whose location was on the same line as the source. --- .../security/UnsafeHostnameVerificationQuery.qll | 11 +++++++++++ .../CWE-297/UnsafeHostnameVerification.expected | 2 ++ .../CWE-297/UnsafeHostnameVerification.java | 16 ++++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll b/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll index 1b44121591c6..60829f426f75 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll @@ -65,6 +65,17 @@ module TrustAllHostnameVerifierConfig implements DataFlow::ConfigSig { "|(set)?(accept|trust|ignore|allow)(all|every|any)" + "|(use|do|enable)insecure|(set|do|use)?no.*(check|validation|verify|verification)|disable).*$") } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { + isSource(source) and + ( + result = source.getLocation() + or + result = source.asExpr().(ClassInstanceExpr).getConstructedType().getLocation() + ) + } } /** Data flow to model the flow of a `TrustAllHostnameVerifier` to a `set(Default)HostnameVerifier` call. */ diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected index ed248caef2a6..9064a7cf151d 100644 --- a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.expected @@ -4,6 +4,7 @@ | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | The $@ defined by $@ always accepts any certificate, even if the hostname does not match. | UnsafeHostnameVerification.java:47:55:47:71 | ...->... | hostname verifier | UnsafeHostnameVerification.java:47:55:47:71 | new HostnameVerifier(...) { ... } | this type | | UnsafeHostnameVerification.java:81:55:81:62 | verifier | UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:81:55:81:62 | verifier | The $@ defined by $@ always accepts any certificate, even if the hostname does not match. | UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | hostname verifier | UnsafeHostnameVerification.java:66:41:66:56 | new HostnameVerifier(...) { ... } | this type | | UnsafeHostnameVerification.java:94:55:94:62 | verifier | UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:94:55:94:62 | verifier | The $@ defined by $@ always accepts any certificate, even if the hostname does not match. | UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | hostname verifier | UnsafeHostnameVerification.java:88:41:88:56 | new HostnameVerifier(...) { ... } | this type | +| UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | The $@ defined by $@ always accepts any certificate, even if the hostname does not match. | UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | hostname verifier | UnsafeHostnameVerification.java:104:26:104:43 | AlwaysTrueVerifier | this type | edges | UnsafeHostnameVerification.java:66:37:80:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:81:55:81:62 | verifier | provenance | Sink:MaD:1 | | UnsafeHostnameVerification.java:88:37:93:9 | new (...) : new HostnameVerifier(...) { ... } | UnsafeHostnameVerification.java:94:55:94:62 | verifier | provenance | Sink:MaD:1 | @@ -23,4 +24,5 @@ nodes | UnsafeHostnameVerification.java:94:55:94:62 | verifier | semmle.label | verifier | | UnsafeHostnameVerification.java:97:42:97:68 | ALLOW_ALL_HOSTNAME_VERIFIER : new HostnameVerifier(...) { ... } | semmle.label | ALLOW_ALL_HOSTNAME_VERIFIER : new HostnameVerifier(...) { ... } | | UnsafeHostnameVerification.java:97:72:102:5 | new (...) : new HostnameVerifier(...) { ... } | semmle.label | new (...) : new HostnameVerifier(...) { ... } | +| UnsafeHostnameVerification.java:116:55:116:78 | new AlwaysTrueVerifier(...) | semmle.label | new AlwaysTrueVerifier(...) | subpaths diff --git a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java index 8343f50ace1f..09fdf89e0f06 100644 --- a/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java +++ b/java/ql/test/query-tests/security/CWE-297/UnsafeHostnameVerification.java @@ -100,4 +100,20 @@ public boolean verify(String hostname, SSLSession session) { return true; // BAD, always returns true } }; + + private static class AlwaysTrueVerifier implements HostnameVerifier { + @Override + public boolean verify(String hostname, SSLSession session) { + return true; // BAD, always returns true + } + } + + /** + * Same as testTrustAllHostnameOfAnonymousClass, but with a named class. + * This is for testing the diff-informed functionality of the query. + */ + public void testTrustAllHostnameOfNamedClass() { + HttpsURLConnection.setDefaultHostnameVerifier(new AlwaysTrueVerifier()); + } + } From a928a0d2b5b15d85ca4dfecbecea25fc69d065b3 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 7 Oct 2024 13:31:08 +0200 Subject: [PATCH 06/10] Java: Diff-informed BrokenCryptoAlgorithm.ql --- .../code/java/security/BrokenCryptoAlgorithmQuery.qll | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/BrokenCryptoAlgorithmQuery.qll b/java/ql/lib/semmle/code/java/security/BrokenCryptoAlgorithmQuery.qll index bfd48b24e80a..4f9e39b23f2d 100644 --- a/java/ql/lib/semmle/code/java/security/BrokenCryptoAlgorithmQuery.qll +++ b/java/ql/lib/semmle/code/java/security/BrokenCryptoAlgorithmQuery.qll @@ -31,6 +31,12 @@ module InsecureCryptoConfig implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node n) { exists(CryptoAlgoSpec c | n.asExpr() = c.getAlgoSpec()) } predicate isBarrier(DataFlow::Node node) { node instanceof SimpleTypeSanitizer } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + exists(CryptoAlgoSpec c | sink.asExpr() = c.getAlgoSpec() | result = c.getLocation()) + } } /** From 011d667f06fd35d26690ca9ffc646c239c8c3f8a Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 7 Oct 2024 15:35:19 +0200 Subject: [PATCH 07/10] Java: Diff-informed PredictableSeed.ql --- .../ql/lib/semmle/code/java/security/RandomQuery.qll | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/RandomQuery.qll b/java/ql/lib/semmle/code/java/security/RandomQuery.qll index 6be76411ce94..49174a1f61ac 100644 --- a/java/ql/lib/semmle/code/java/security/RandomQuery.qll +++ b/java/ql/lib/semmle/code/java/security/RandomQuery.qll @@ -37,6 +37,18 @@ private module PredictableSeedFlowConfig implements DataFlow::ConfigSig { predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { predictableCalcStep(node1.asExpr(), node2.asExpr()) } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + // This predicate matches `PredictableSeed.ql`, which is the only place + // where `PredictableSeedFlowConfig` is used. + exists(GetRandomData da, VarRead use | + result = da.getLocation() and + da.getQualifier() = use and + isSeeding(sink.asExpr(), use) + ) + } } private module PredictableSeedFlow = DataFlow::Global; From e799bff7449b57b6b43c4ec936c81e9e868f76f8 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 7 Oct 2024 15:35:40 +0200 Subject: [PATCH 08/10] Java: Diff-informed TaintedPermissionsCheck.ql --- .../code/java/security/TaintedPermissionsCheckQuery.qll | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/TaintedPermissionsCheckQuery.qll b/java/ql/lib/semmle/code/java/security/TaintedPermissionsCheckQuery.qll index eb5b589a98ba..bbec7d4f4e6c 100644 --- a/java/ql/lib/semmle/code/java/security/TaintedPermissionsCheckQuery.qll +++ b/java/ql/lib/semmle/code/java/security/TaintedPermissionsCheckQuery.qll @@ -59,6 +59,15 @@ module TaintedPermissionsCheckFlowConfig implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(PermissionsConstruction p).getInput() } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + exists(PermissionsConstruction p | + sink.asExpr() = p.getInput() and + result = p.getLocation() + ) + } } /** Tracks flow from user input to a permissions check. */ From 5bebae9abf3542b59e5a680320e1d7996e4fbfd8 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 7 Oct 2024 15:58:43 +0200 Subject: [PATCH 09/10] Java: Diff-informed ImproperIntentVerification.ql --- .../ImproperIntentVerificationQuery.qll | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll b/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll index bca045bc8e42..995f10ad3c9a 100644 --- a/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll +++ b/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll @@ -25,6 +25,25 @@ private module VerifiedIntentConfig implements DataFlow::ConfigSig { sink.asExpr() = ma.getQualifier() ) } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node src) { + exists(AndroidReceiverXmlElement rec, OnReceiveMethod orm, SystemActionName sa | + src.asParameter() = orm.getIntentParameter() and + anySystemReceiver(rec, orm, sa) + | + result = rec.getLocation() + or + result = orm.getLocation() + or + result = sa.getLocation() + ) + } + + // All sinks are set to have no locations because sinks aren't selected in + // the query. This effectively means that we're filtering on sources only. + Location getASelectedSinkLocation(DataFlow::Node sink) { none() } } private module VerifiedIntentFlow = DataFlow::Global; @@ -67,9 +86,8 @@ class SystemActionName extends AndroidActionXmlElement { string getSystemActionName() { result = name } } -/** Holds if the XML element `rec` declares a receiver `orm` to receive the system action named `sa` that doesn't verify intents it receives. */ -predicate unverifiedSystemReceiver( - AndroidReceiverXmlElement rec, UnverifiedOnReceiveMethod orm, SystemActionName sa +private predicate anySystemReceiver( + AndroidReceiverXmlElement rec, OnReceiveMethod orm, SystemActionName sa ) { exists(Class ormty | ormty = orm.getDeclaringType() and @@ -77,3 +95,11 @@ predicate unverifiedSystemReceiver( rec.getAnIntentFilterElement().getAnActionElement() = sa ) } + +/** Holds if the XML element `rec` declares a receiver `orm` to receive the system action named `sa` that doesn't verify intents it receives. */ +predicate unverifiedSystemReceiver( + AndroidReceiverXmlElement rec, UnverifiedOnReceiveMethod orm, SystemActionName sa +) { + // The type of `orm` is different in these two predicates + anySystemReceiver(rec, orm, sa) +} From 2b1c70c33b8c7b21743fdd8c14914cd34b2a2691 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 8 Oct 2024 09:45:01 +0200 Subject: [PATCH 10/10] Java: Diff-informed PolynomialReDoS.ql This and other queries would also benefit from making `RegexFlow` diff-informed. That will come later. --- .../java/security/regexp/PolynomialReDoSQuery.qll | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll b/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll index ba65e13dd611..767ebc97437b 100644 --- a/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll +++ b/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll @@ -47,6 +47,18 @@ module PolynomialRedosConfig implements DataFlow::ConfigSig { node instanceof SimpleTypeSanitizer or node.asExpr().(MethodCall).getMethod() instanceof LengthRestrictedMethod } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + exists(SuperlinearBackTracking::PolynomialBackTrackingTerm regexp | + regexp.getRootTerm() = sink.(PolynomialRedosSink).getRegExp() + | + result = sink.getLocation() + or + result = regexp.getLocation() + ) + } } module PolynomialRedosFlow = TaintTracking::Global;