From 93ba865b3209d95059701d10b6720dc7313e1e7e Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 7 Oct 2024 13:20:55 +0200 Subject: [PATCH 1/7] Java: Diff-informed CleartextStorageCookie.ql This query shares implementation with several other queries about cleartext storage, but it's the only one of them that's in the code-scanning suite. The sharing mechanism remains the same as before, but now each query has to override `getASelectedLocation` to become diff-informed. Two other data-flow configurations are used in this query, but they can't easily be made diff-informed. --- .../security/CleartextStorageCookieQuery.qll | 12 ++++++++++- .../java/security/CleartextStorageQuery.qll | 20 ++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll index 1f262ad57d61..1c99821386da 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll @@ -7,7 +7,17 @@ private import semmle.code.java.dataflow.FlowSinks private import semmle.code.java.dataflow.FlowSources private class CookieCleartextStorageSink extends CleartextStorageSink { - CookieCleartextStorageSink() { this.asExpr() = cookieInput(_) } + Cookie cookie; + + CookieCleartextStorageSink() { this.asExpr() = cookieInput(cookie) } + + override Location getASelectedLocation() { + result = this.getLocation() + or + result = cookie.getLocation() + or + result = cookie.getAStore().getLocation() + } } /** The instantiation of a cookie, which can act as storage. */ diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll index a607fd8c8d2b..21d82bef657e 100644 --- a/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll +++ b/java/ql/lib/semmle/code/java/security/CleartextStorageQuery.qll @@ -5,7 +5,14 @@ private import semmle.code.java.dataflow.TaintTracking private import semmle.code.java.security.SensitiveActions /** A sink representing persistent storage that saves data in clear text. */ -abstract class CleartextStorageSink extends DataFlow::Node { } +abstract class CleartextStorageSink extends DataFlow::Node { + /** + * Gets a location that will be selected in the diff-informed query where + * this sink is found. If this has no results for any sink, that's taken to + * mean the query is not diff-informed. + */ + Location getASelectedLocation() { none() } +} /** A sanitizer for flows tracking sensitive data being stored in persistent storage. */ abstract class CleartextStorageSanitizer extends DataFlow::Node { } @@ -46,6 +53,17 @@ private module SensitiveSourceFlowConfig implements DataFlow::ConfigSig { predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { any(CleartextStorageAdditionalTaintStep c).step(n1, n2) } + + predicate observeDiffInformedIncrementalMode() { + // This configuration is used by several queries. A query can opt in to + // diff-informed mode by implementing `getASelectedLocation` on its sinks, + // indicating that it has considered which sinks are selected. + exists(CleartextStorageSink sink | exists(sink.getASelectedLocation())) + } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + result = sink.(CleartextStorageSink).getASelectedLocation() + } } private module SensitiveSourceFlow = TaintTracking::Global; From 964a0a54143d44df2c083fcfeb6ad6046b4319f9 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 10 Oct 2024 09:07:45 +0200 Subject: [PATCH 2/7] DataFlow: Add has{Source,Sink}InDiffRange --- .../codeql/dataflow/internal/DataFlowImpl.qll | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll index a13c71f554cc..989318691afc 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll @@ -8,6 +8,7 @@ private import codeql.util.Unit private import codeql.util.Option private import codeql.util.Boolean private import codeql.util.Location +private import codeql.util.AlertFiltering private import codeql.dataflow.DataFlow private import DataFlowImplStage1 @@ -19,6 +20,8 @@ module MakeImpl Lang> { private import DataFlowImplCommonPublic private import Aliases + private module AlertFiltering = AlertFilteringImpl; + /** * An input configuration for data flow using flow state. This signature equals * `StateConfigSig`, but requires explicit implementation of all predicates. @@ -3389,6 +3392,38 @@ module MakeImpl Lang> { */ predicate flowToExpr(Expr sink) { flowTo(exprNode(sink)) } + /** + * Holds if the configuration has at least one source in the diff range as + * determined by `AlertFiltering`. This predicate is independent of whether + * diff-informed mode is observed by the configuration and is also + * independent whether there was flow. + */ + pragma[nomagic] + predicate hasSourceInDiffRange() { + exists(Node source | + Config::isSource(source, _) and + AlertFiltering::filterByLocation(Config::getASelectedSourceLocation(source)) + // TODO: also require `Config::isSink(_, _)`? Because if the source + // isn't reachable, it may as well not be there. + ) + } + + /** + * Holds if the configuration has at least one sink in the diff range as + * determined by `AlertFiltering`. This predicate is independent of whether + * diff-informed mode is observed by the configuration and is also + * independent whether there was flow. + */ + pragma[nomagic] + predicate hasSinkInDiffRange() { + exists(Node sink | + Config::isSink(sink, _) and + AlertFiltering::filterByLocation(Config::getASelectedSinkLocation(sink)) + // TODO: also require `Config::isSource(_, _)`? Because if the sink + // isn't reachable, it may as well not be there. + ) + } + /** * INTERNAL: Only for debugging. * From 793446b2a5ec6699911ee4ee2838efbba7e4c9a6 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 4 Oct 2024 10:59:02 +0200 Subject: [PATCH 3/7] Java: Diff-informed XSS.ql This change makes the XSS query fully diff-informed, including the discovery of sinks. This involved making a helper data-flow analysis diff-informed, which required punching through some abstraction layers. An alternative would have been to use `DataFlow::SimpleGlobal` or other means to make the analysis faster, but I didn't want to risk removing good taint steps such as wrapping one `OutputStream` in another. --- .../lib/semmle/code/java/frameworks/JaxWS.qll | 2 +- .../java/frameworks/spring/SpringHttp.qll | 2 +- java/ql/lib/semmle/code/java/security/XSS.qll | 125 +++++++++++++----- .../code/java/security/XssLocalQuery.qll | 4 +- .../semmle/code/java/security/XssQuery.qll | 4 +- 5 files changed, 102 insertions(+), 35 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/JaxWS.qll b/java/ql/lib/semmle/code/java/frameworks/JaxWS.qll index a0f891fd36ea..e9f1cea4b330 100644 --- a/java/ql/lib/semmle/code/java/frameworks/JaxWS.qll +++ b/java/ql/lib/semmle/code/java/frameworks/JaxWS.qll @@ -417,7 +417,7 @@ class JaxRSConsumesAnnotation extends JaxRSAnnotation { } /** A default sink representing methods susceptible to XSS attacks. */ -private class JaxRSXssSink extends XssSink { +private class JaxRSXssSink extends AbstractXssSink { JaxRSXssSink() { exists(JaxRsResourceMethod resourceMethod, ReturnStmt rs | resourceMethod = any(JaxRsResourceClass resourceClass).getAResourceMethod() and diff --git a/java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll b/java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll index e12e2b2643a0..25f7fdc3068d 100644 --- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll +++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll @@ -46,7 +46,7 @@ private predicate specifiesContentType(SpringRequestMappingMethod method) { exists(method.getAProducesExpr()) } -private class SpringXssSink extends XSS::XssSink { +private class SpringXssSink extends XSS::AbstractXssSink { SpringXssSink() { exists(SpringRequestMappingMethod requestMappingMethod, ReturnStmt rs | requestMappingMethod = rs.getEnclosingCallable() and diff --git a/java/ql/lib/semmle/code/java/security/XSS.qll b/java/ql/lib/semmle/code/java/security/XSS.qll index cc584033e4fc..da48c056a07c 100644 --- a/java/ql/lib/semmle/code/java/security/XSS.qll +++ b/java/ql/lib/semmle/code/java/security/XSS.qll @@ -13,17 +13,36 @@ private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSources private import semmle.code.java.dataflow.FlowSinks -/** A sink that represent a method that outputs data without applying contextual output encoding. */ -abstract class XssSink extends ApiSinkNode { } - /** A sanitizer that neutralizes dangerous characters that can be used to perform a XSS attack. */ abstract class XssSanitizer extends DataFlow::Node { } +/** + * A sink that represent a method that outputs data without applying contextual + * output encoding. Extend this class to add more sinks that should be + * considered XSS sinks by every query. To find the full set of XSS sinks, use + * `XssSink` instead. + */ +abstract class AbstractXssSink extends DataFlow::Node { } + +/** A default sink representing methods susceptible to XSS attacks. */ +private class DefaultXssSink extends AbstractXssSink { + DefaultXssSink() { sinkNode(this, ["html-injection", "js-injection"]) } +} + +/** + * A sink that represent a method that outputs data without applying contextual + * output encoding. To add more sinks, extend `AbstractXssSink` rather than + * this class. To find XSS sinks efficiently for a diff-informed query, use the + * `XssDiffInformed` module instead. + */ +final class XssSink extends ApiSinkNode instanceof XssDiffInformed::XssSink { +} + /** * A sink that represent a method that outputs data without applying contextual output encoding, * and which should truncate flow paths such that downstream sinks are not flagged as well. */ -abstract class XssSinkBarrier extends XssSink { } +abstract class XssSinkBarrier extends AbstractXssSink { } /** * A unit class for adding additional taint steps. @@ -39,19 +58,6 @@ class XssAdditionalTaintStep extends Unit { abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); } -/** A default sink representing methods susceptible to XSS attacks. */ -private class DefaultXssSink extends XssSink { - DefaultXssSink() { - sinkNode(this, ["html-injection", "js-injection"]) - or - exists(MethodCall ma | - ma.getMethod() instanceof WritingMethod and - XssVulnerableWriterSourceToWritingMethodFlow::flowToExpr(ma.getQualifier()) and - this.asExpr() = ma.getArgument(_) - ) - } -} - /** A default sanitizer that considers numeric and boolean typed data safe for writing to output. */ private class DefaultXssSanitizer extends XssSanitizer { DefaultXssSanitizer() { @@ -62,20 +68,6 @@ private class DefaultXssSanitizer extends XssSanitizer { } } -/** A configuration that tracks data from a servlet writer to an output method. */ -private module XssVulnerableWriterSourceToWritingMethodFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src instanceof XssVulnerableWriterSourceNode } - - predicate isSink(DataFlow::Node sink) { - exists(MethodCall ma | - sink.asExpr() = ma.getQualifier() and ma.getMethod() instanceof WritingMethod - ) - } -} - -private module XssVulnerableWriterSourceToWritingMethodFlow = - TaintTracking::Global; - /** A method that can be used to output data to an output stream or writer. */ private class WritingMethod extends Method { WritingMethod() { @@ -113,6 +105,77 @@ class XssVulnerableWriterSourceNode extends ApiSourceNode { XssVulnerableWriterSourceNode() { this.asExpr() instanceof XssVulnerableWriterSource } } +/** A nullary predicate. */ +signature predicate xssNullaryPredicate(); + +/** Holds always. Use this predicate as parameter to `XssDiffInformed` to disable diff-informed mode. */ +predicate xssNotDiffInformed() { any() } + +/** + * A module for finding XSS sinks faster in a diff-informed query. The + * `hasSourceInDiffRange` parameter should hold if the overall data-flow + * configuration of the query has any sources in the diff range. + */ +module XssDiffInformed { + final private class Node = DataFlow::Node; + + /** + * A diff-informed replacement for the top-level `XssSink`, omitting for + * efficiency some sinks that would never be reported by a diff-informed + * query. + */ + final class XssSink extends Node { + XssSink() { + this instanceof AbstractXssSink + or + isResponseWriterSink(this) + } + } + + predicate isResponseWriterSink(DataFlow::Node sink) { + exists(MethodCall ma | + // This code mirrors `getASelectedSinkLocation`. + ma.getMethod() instanceof WritingMethod and + XssVulnerableWriterSourceToWritingMethodFlow::flowToExpr(ma.getQualifier()) and + sink.asExpr() = ma.getArgument(_) + ) + } + + /** A configuration that tracks data from a servlet writer to an output method. */ + private module XssVulnerableWriterSourceToWritingMethodFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src instanceof XssVulnerableWriterSourceNode } + + predicate isSink(DataFlow::Node sink) { + exists(MethodCall ma | + sink.asExpr() = ma.getQualifier() and ma.getMethod() instanceof WritingMethod + ) + } + + predicate observeDiffInformedIncrementalMode() { + // Since this configuration is for finding sinks to be used in a main + // data-flow configuration, this configuration should only restrict the + // sinks to be found if there are no main-configuration sources in the + // diff range. That's because if there is such a source, we need to + // report query results for it even with sinks outside the diff range. + not hasSourceInDiffRange() + } + + // The sources are not exposed outside this file module, so we know the + // query will not select them. + Location getASelectedSourceLocation(DataFlow::Node source) { none() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + // This code mirrors `isResponseWriterSink`. + exists(MethodCall ma | result = ma.getAnArgument().getLocation() | + sink.asExpr() = ma.getQualifier() and ma.getMethod() instanceof WritingMethod + ) + } + } + + private module XssVulnerableWriterSourceToWritingMethodFlow = + TaintTracking::Global; +} + /** * Holds if `s` is an HTTP Content-Type vulnerable to XSS. */ diff --git a/java/ql/lib/semmle/code/java/security/XssLocalQuery.qll b/java/ql/lib/semmle/code/java/security/XssLocalQuery.qll index 5e1098865aa6..d13583d942d1 100644 --- a/java/ql/lib/semmle/code/java/security/XssLocalQuery.qll +++ b/java/ql/lib/semmle/code/java/security/XssLocalQuery.qll @@ -11,7 +11,9 @@ private import semmle.code.java.security.XSS deprecated module XssLocalConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput } - predicate isSink(DataFlow::Node sink) { sink instanceof XssSink } + predicate isSink(DataFlow::Node sink) { + sink instanceof XssDiffInformed::XssSink + } predicate isBarrier(DataFlow::Node node) { node instanceof XssSanitizer } diff --git a/java/ql/lib/semmle/code/java/security/XssQuery.qll b/java/ql/lib/semmle/code/java/security/XssQuery.qll index c0d7035a4f9a..2576c043f48f 100644 --- a/java/ql/lib/semmle/code/java/security/XssQuery.qll +++ b/java/ql/lib/semmle/code/java/security/XssQuery.qll @@ -11,7 +11,9 @@ import semmle.code.java.security.XSS module XssConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource } - predicate isSink(DataFlow::Node sink) { sink instanceof XssSink } + predicate isSink(DataFlow::Node sink) { + sink instanceof XssDiffInformed::XssSink + } predicate isBarrier(DataFlow::Node node) { node instanceof XssSanitizer } From 05b8e1844a4400f3b547fced9eec2673bc4f4395 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 4 Oct 2024 15:00:44 +0200 Subject: [PATCH 4/7] Java: Diff-informed UnsafeDeserialization.ql With this change, the slowest data-flow analysis in this query is made diff-informed with the same approach as for XSS. --- .../security/UnsafeDeserializationQuery.qll | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll b/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll index b16770c222b8..7c9dfd058045 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll @@ -379,8 +379,12 @@ predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNo /** A sink representing an argument of a deserialization method */ private class UnsafeTypeSink extends DataFlow::Node { + MethodCall ma; + + MethodCall getMethodCall() { result = ma } + UnsafeTypeSink() { - exists(MethodCall ma, int i, Expr arg | i > 0 and ma.getArgument(i) = arg | + exists(int i, Expr arg | i > 0 and ma.getArgument(i) = arg | ( ma.getMethod() instanceof ObjectMapperReadMethod or @@ -425,6 +429,25 @@ module UnsafeTypeConfig implements DataFlow::ConfigSig { predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) { isUnsafeTypeAdditionalTaintStep(fromNode, toNode) } + + predicate observeDiffInformedIncrementalMode() { + // Since this configuration is for finding sinks to be used in a main + // data-flow configuration, this configuration should only restrict the + // sinks to be found if there are no main-configuration sources in the diff + // range. That's because if there is such a source, we need to report query + // results for it even with sinks outside the diff range. + not UnsafeDeserializationFlow::hasSourceInDiffRange() + } + + // The query does not select the sources of this configuration + Location getASelectedSourceLocation(DataFlow::Node source) { none() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + // Match by the surrounding method call since the sink of the overall + // query will be contained in that (see the body of + // `unsafeDeserialization/2`). + result = sink.(UnsafeTypeSink).getMethodCall().getLocation() + } } /** From a2fd4eee275c0b5b323a59e2bc2f25b5d4d284e4 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Sat, 5 Oct 2024 21:25:19 +0200 Subject: [PATCH 5/7] Java: Diff-informed information leak queries --- .../code/java/security/InformationLeak.qll | 43 ++++++++++++++++--- ...veDataExposureThroughErrorMessageQuery.qll | 7 ++- .../java/security/StackTraceExposureQuery.qll | 19 +++++++- 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/InformationLeak.qll b/java/ql/lib/semmle/code/java/security/InformationLeak.qll index 8fe7d2151650..e95d00edc8bf 100644 --- a/java/ql/lib/semmle/code/java/security/InformationLeak.qll +++ b/java/ql/lib/semmle/code/java/security/InformationLeak.qll @@ -5,13 +5,44 @@ import semmle.code.java.dataflow.DataFlow private import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.security.XSS -/** A sink that represent a method that outputs data to an HTTP response. */ -abstract class InformationLeakSink extends DataFlow::Node { } +/** + * A sink that represent a method that outputs data to an HTTP response. Extend + * this class to add more sinks that should be considered information leak + * points by every query. To find the full set of information-leak sinks, use + * `InformationLeakSink` instead. + */ +abstract class AbstractInformationLeakSink extends DataFlow::Node { } + +/** + * A sink that represent a method that outputs data to an HTTP response. To add + * more sinks, extend `AbstractInformationLeakSink` rather than this class. + */ +final class InformationLeakSink extends DataFlow::Node instanceof InformationLeakDiffInformed::InformationLeakSink +{ } /** A default sink representing methods outputing data to an HTTP response. */ -private class DefaultInformationLeakSink extends InformationLeakSink { - DefaultInformationLeakSink() { - sinkNode(this, "information-leak") or - this instanceof XssSink +private class DefaultInformationLeakSink extends AbstractInformationLeakSink { + DefaultInformationLeakSink() { sinkNode(this, "information-leak") } +} + +/** + * A module for finding information-leak sinks faster in a diff-informed query. + * The `hasSourceInDiffRange` parameter should hold if the overall data-flow + * configuration of the query has any sources in the diff range. + */ +module InformationLeakDiffInformed { + final private class Node = DataFlow::Node; + + /** + * A diff-informed replacement for the top-level `InformationLeakSink`, + * omitting for efficiency some sinks that would never be reported by a + * diff-informed query. + */ + final class InformationLeakSink extends Node { + InformationLeakSink() { + this instanceof AbstractInformationLeakSink + or + this instanceof XssDiffInformed::XssSink + } } } diff --git a/java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll b/java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll index 8644ef415b3e..3c3bdd7dfc72 100644 --- a/java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll +++ b/java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll @@ -20,7 +20,12 @@ private class GetMessageFlowSource extends ApiSourceNode { private module GetMessageFlowSourceToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { src instanceof GetMessageFlowSource } - predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink } + predicate isSink(DataFlow::Node sink) { + sink instanceof + InformationLeakDiffInformed::InformationLeakSink + } + + predicate observeDiffInformedIncrementalMode() { any() } } private module GetMessageFlowSourceToHttpResponseSinkFlow = diff --git a/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll b/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll index 0eb069a06c20..25cb4b141522 100644 --- a/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll +++ b/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll @@ -25,6 +25,14 @@ private module ServletWriterSourceToPrintStackTraceMethodFlowConfig implements D sink.asExpr() = ma.getAnArgument() and ma.getMethod() instanceof PrintStackTraceMethod ) } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + exists(MethodCall ma | result = ma.getLocation() | + sink.asExpr() = ma.getAnArgument() and ma.getMethod() instanceof PrintStackTraceMethod + ) + } } private module ServletWriterSourceToPrintStackTraceMethodFlow = @@ -69,7 +77,16 @@ private predicate stackTraceExpr(Expr exception, MethodCall stackTraceString) { private module StackTraceStringToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { stackTraceExpr(_, src.asExpr()) } - predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink } + predicate isSink(DataFlow::Node sink) { + sink instanceof + InformationLeakDiffInformed::InformationLeakSink + } + + predicate observeDiffInformedIncrementalMode() { any() } + + Location getASelectedSourceLocation(DataFlow::Node source) { + exists(Expr e | stackTraceExpr(e, source.asExpr()) and result = e.getLocation()) + } } private module StackTraceStringToHttpResponseSinkFlow = From 8c8f3ed89d99af23fa42a3cca8b452ead973f3d1 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 9 Oct 2024 14:55:40 +0200 Subject: [PATCH 6/7] Java: Diff-informed regex queries An alternative to dynamic dispatch would have been to use parameterised modules. I tried that but abandoned it because it led to cascading changes and noise in too many places. The parameterisation used here has to be propagated through three different library files, and that propagation becomes invisible (for better or worse) when using dynamic dispatch. --- .../code/java/regex/RegexDiffInformed.qll | 28 +++++++++++++++++++ .../code/java/regex/RegexFlowConfigs.qll | 9 ++++++ .../security/regexp/PolynomialReDoSQuery.qll | 9 ++++++ .../Security/CWE/CWE-020/OverlyLargeRange.ql | 8 ++++++ java/ql/src/Security/CWE/CWE-730/ReDoS.ql | 8 ++++++ 5 files changed, 62 insertions(+) create mode 100644 java/ql/lib/semmle/code/java/regex/RegexDiffInformed.qll diff --git a/java/ql/lib/semmle/code/java/regex/RegexDiffInformed.qll b/java/ql/lib/semmle/code/java/regex/RegexDiffInformed.qll new file mode 100644 index 000000000000..004a70bf7a25 --- /dev/null +++ b/java/ql/lib/semmle/code/java/regex/RegexDiffInformed.qll @@ -0,0 +1,28 @@ +private import java +private import semmle.code.java.dataflow.DataFlow +private import codeql.util.Unit + +/** + * An extension point to allow a query to detect only the regular expressions + * it needs in diff-informed incremental mode. The data-flow analysis that's + * modified by this class has its sources as (certain) string literals and its + * sinks as regular-expression matches. + */ +class RegexDiffInformedConfig instanceof Unit { + /** + * Holds if discovery of regular expressions should be diff-informed, which + * is possible when there cannot be any elements selected by the query in the + * diff range except the regular expressions and (locations derived from) the + * places where they are matched against. + */ + abstract predicate observeDiffInformedIncrementalMode(); + + /** + * Gets a location of a regex match that will be part of the query results. + * If the query does not select the match locations, this predicate can be + * `none()` for performance. + */ + abstract Location getASelectedSinkLocation(DataFlow::Node sink); + + string toString() { result = "RegexDiffInformedConfig" } +} diff --git a/java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll b/java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll index 763b96f5a02d..e04f849c16a5 100644 --- a/java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll +++ b/java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll @@ -6,6 +6,7 @@ import java import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.security.SecurityTests +private import RegexDiffInformed private class ExploitableStringLiteral extends StringLiteral { ExploitableStringLiteral() { this.getValue().matches(["%+%", "%*%", "%{%}%"]) } @@ -157,6 +158,14 @@ private module RegexFlowConfig implements DataFlow::ConfigSig { } int fieldFlowBranchLimit() { result = 1 } + + predicate observeDiffInformedIncrementalMode() { + exists(RegexDiffInformedConfig c | c.observeDiffInformedIncrementalMode()) + } + + Location getASelectedSinkLocation(DataFlow::Node sink) { + exists(RegexDiffInformedConfig c | result = c.getASelectedSinkLocation(sink)) + } } private module RegexFlow = DataFlow::Global; 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 767ebc97437b..e8eddaebf54c 100644 --- a/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll +++ b/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll @@ -4,6 +4,7 @@ private import semmle.code.java.regex.RegexTreeView::RegexTreeView as TreeView import codeql.regex.nfa.SuperlinearBackTracking::Make as SuperlinearBackTracking import semmle.code.java.dataflow.DataFlow import semmle.code.java.regex.RegexFlowConfigs +import semmle.code.java.regex.RegexDiffInformed import semmle.code.java.dataflow.FlowSources private import semmle.code.java.security.Sanitizers @@ -33,6 +34,14 @@ private class LengthRestrictedMethod extends Method { } } +class PolynomialRedDosDiffInformed extends RegexDiffInformedConfig { + override predicate observeDiffInformedIncrementalMode() { + not PolynomialRedosFlow::hasSourceInDiffRange() + } + + override Location getASelectedSinkLocation(DataFlow::Node sink) { result = sink.getLocation() } +} + /** A configuration for Polynomial ReDoS queries. */ module PolynomialRedosConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { src instanceof ActiveThreatModelSource } diff --git a/java/ql/src/Security/CWE/CWE-020/OverlyLargeRange.ql b/java/ql/src/Security/CWE/CWE-020/OverlyLargeRange.ql index b8ea3e52dbd0..8d5cfab03055 100644 --- a/java/ql/src/Security/CWE/CWE-020/OverlyLargeRange.ql +++ b/java/ql/src/Security/CWE/CWE-020/OverlyLargeRange.ql @@ -12,9 +12,17 @@ * external/cwe/cwe-020 */ +import semmle.code.java.regex.RegexDiffInformed +import semmle.code.java.dataflow.DataFlow private import semmle.code.java.regex.RegexTreeView::RegexTreeView as TreeView import codeql.regex.OverlyLargeRangeQuery::Make +class OverlyLargeRangeDiffInformed extends RegexDiffInformedConfig { + override predicate observeDiffInformedIncrementalMode() { any() } + + override Location getASelectedSinkLocation(DataFlow::Node sink) { none() } +} + TreeView::RegExpCharacterClass potentialMisparsedCharClass() { // nested char classes are currently misparsed result.getAChild().(TreeView::RegExpNormalChar).getValue() = "[" diff --git a/java/ql/src/Security/CWE/CWE-730/ReDoS.ql b/java/ql/src/Security/CWE/CWE-730/ReDoS.ql index ca4750fc8588..057da9fd3545 100644 --- a/java/ql/src/Security/CWE/CWE-730/ReDoS.ql +++ b/java/ql/src/Security/CWE/CWE-730/ReDoS.ql @@ -14,9 +14,17 @@ * external/cwe/cwe-400 */ +import semmle.code.java.regex.RegexDiffInformed +import semmle.code.java.dataflow.DataFlow private import semmle.code.java.regex.RegexTreeView::RegexTreeView as TreeView import codeql.regex.nfa.ExponentialBackTracking::Make as ExponentialBackTracking +class ReDoSDiffInformed extends RegexDiffInformedConfig { + override predicate observeDiffInformedIncrementalMode() { any() } + + override Location getASelectedSinkLocation(DataFlow::Node sink) { none() } +} + from TreeView::RegExpTerm t, string pump, ExponentialBackTracking::State s, string prefixMsg where ExponentialBackTracking::hasReDoSResult(t, pump, s, prefixMsg) and From 0fc6b5ef287bd204f23d78f90b798b2e72dacf67 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 24 Feb 2025 11:32:14 +0100 Subject: [PATCH 7/7] Java: diff-informed StaticInitializationVector This query was made diff-informed by shifting the secondary configuration to run after the primary. The test had to change because the query now does more than just present the results of data flow. --- .../StaticInitializationVectorQuery.qll | 5 +- .../CWE-1204/StaticInitializationVector.ql | 4 +- .../StaticInitializationVector.expected | 97 +++++++++++++++++++ .../CWE-1204/StaticInitializationVector.java | 26 ++--- .../CWE-1204/StaticInitializationVector.qlref | 2 + .../StaticInitializationVectorTest.expected | 0 .../StaticInitializationVectorTest.ql | 18 ---- 7 files changed, 117 insertions(+), 35 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.expected create mode 100644 java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.qlref delete mode 100644 java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected delete mode 100644 java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql diff --git a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll index 282133ec5c67..9e74c1f04ce2 100644 --- a/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll +++ b/java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll @@ -81,7 +81,7 @@ private class ArrayUpdate extends Expr { } private predicate arrayUpdateSrc(DataFlow::Node source) { - source.asExpr() instanceof StaticByteArrayCreation + StaticInitializationVectorFlow::flow(source, _) } private predicate arrayUpdateSink(DataFlow::Node sink) { @@ -92,7 +92,7 @@ private module ArrayUpdateFlowFwd = DataFlow::SimpleGlobal; private module ArrayUpdateFlow = ArrayUpdateFlowFwd::Graph; -private predicate arrayReachesUpdate(StaticByteArrayCreation array) { +predicate arrayReachesUpdate(StaticByteArrayCreation array) { exists(ArrayUpdateFlow::PathNode src | src.isSource() and src.getNode().asExpr() = array) } @@ -102,7 +102,6 @@ private predicate arrayReachesUpdate(StaticByteArrayCreation array) { private class StaticInitializationVectorSource extends DataFlow::Node { StaticInitializationVectorSource() { exists(StaticByteArrayCreation array | array = this.asExpr() | - not arrayReachesUpdate(array) and // Reduce FPs from utility methods that return an empty array in an exceptional case not exists(ReturnStmt ret | array.getADimension().(CompileTimeConstantExpr).getIntValue() = 0 and diff --git a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql index 258e0f871123..0ad9275899af 100644 --- a/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql +++ b/java/ql/src/Security/CWE/CWE-1204/StaticInitializationVector.ql @@ -16,6 +16,8 @@ import semmle.code.java.security.StaticInitializationVectorQuery import StaticInitializationVectorFlow::PathGraph from StaticInitializationVectorFlow::PathNode source, StaticInitializationVectorFlow::PathNode sink -where StaticInitializationVectorFlow::flowPath(source, sink) +where + StaticInitializationVectorFlow::flowPath(source, sink) and + not arrayReachesUpdate(source.getNode().asExpr()) select sink.getNode(), source, sink, "A $@ should not be used for encryption.", source.getNode(), "static initialization vector" diff --git a/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.expected b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.expected new file mode 100644 index 000000000000..1438764d7972 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.expected @@ -0,0 +1,97 @@ +#select +| StaticInitializationVector.java:19:51:19:56 | ivSpec | StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | StaticInitializationVector.java:19:51:19:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:13:21:13:81 | new byte[] | static initialization vector | +| StaticInitializationVector.java:32:51:32:56 | ivSpec | StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | StaticInitializationVector.java:32:51:32:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:26:21:26:32 | new byte[] | static initialization vector | +| StaticInitializationVector.java:48:51:48:56 | ivSpec | StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | StaticInitializationVector.java:48:51:48:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:39:21:39:32 | new byte[] | static initialization vector | +| StaticInitializationVector.java:64:51:64:56 | ivSpec | StaticInitializationVector.java:55:30:58:9 | new byte[][] : byte[][] | StaticInitializationVector.java:64:51:64:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:55:30:58:9 | new byte[][] | static initialization vector | +| StaticInitializationVector.java:80:51:80:56 | ivSpec | StaticInitializationVector.java:71:30:74:9 | new byte[][] : byte[][] | StaticInitializationVector.java:80:51:80:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:71:30:74:9 | new byte[][] | static initialization vector | +| StaticInitializationVector.java:96:51:96:56 | ivSpec | StaticInitializationVector.java:88:13:88:23 | new byte[] : byte[] | StaticInitializationVector.java:96:51:96:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:88:13:88:23 | new byte[] | static initialization vector | +| StaticInitializationVector.java:96:51:96:56 | ivSpec | StaticInitializationVector.java:89:13:89:24 | new byte[] : byte[] | StaticInitializationVector.java:96:51:96:56 | ivSpec | A $@ should not be used for encryption. | StaticInitializationVector.java:89:13:89:24 | new byte[] | static initialization vector | +edges +| StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | StaticInitializationVector.java:15:61:15:62 | iv : byte[] | provenance | | +| StaticInitializationVector.java:15:35:15:63 | new GCMParameterSpec(...) : GCMParameterSpec | StaticInitializationVector.java:19:51:19:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:15:61:15:62 | iv : byte[] | StaticInitializationVector.java:15:35:15:63 | new GCMParameterSpec(...) : GCMParameterSpec | provenance | MaD:45875 | +| StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | StaticInitializationVector.java:28:61:28:62 | iv : byte[] | provenance | | +| StaticInitializationVector.java:28:35:28:63 | new GCMParameterSpec(...) : GCMParameterSpec | StaticInitializationVector.java:32:51:32:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:28:61:28:62 | iv : byte[] | StaticInitializationVector.java:28:35:28:63 | new GCMParameterSpec(...) : GCMParameterSpec | provenance | MaD:45875 | +| StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | StaticInitializationVector.java:44:54:44:55 | iv : byte[] | provenance | | +| StaticInitializationVector.java:44:34:44:56 | new IvParameterSpec(...) : IvParameterSpec | StaticInitializationVector.java:48:51:48:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:44:54:44:55 | iv : byte[] | StaticInitializationVector.java:44:34:44:56 | new IvParameterSpec(...) : IvParameterSpec | provenance | MaD:45874 | +| StaticInitializationVector.java:55:30:58:9 | new byte[][] : byte[][] | StaticInitializationVector.java:60:61:60:72 | ...[...] : byte[] | provenance | | +| StaticInitializationVector.java:60:35:60:73 | new GCMParameterSpec(...) : GCMParameterSpec | StaticInitializationVector.java:64:51:64:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:60:61:60:72 | ...[...] : byte[] | StaticInitializationVector.java:60:35:60:73 | new GCMParameterSpec(...) : GCMParameterSpec | provenance | MaD:45875 | +| StaticInitializationVector.java:71:30:74:9 | new byte[][] : byte[][] | StaticInitializationVector.java:76:61:76:72 | ...[...] : byte[] | provenance | | +| StaticInitializationVector.java:76:35:76:73 | new GCMParameterSpec(...) : GCMParameterSpec | StaticInitializationVector.java:80:51:80:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:76:61:76:72 | ...[...] : byte[] | StaticInitializationVector.java:76:35:76:73 | new GCMParameterSpec(...) : GCMParameterSpec | provenance | MaD:45875 | +| StaticInitializationVector.java:87:24:90:9 | {...} : byte[][] [[]] : byte[] | StaticInitializationVector.java:92:61:92:63 | ivs : byte[][] [[]] : byte[] | provenance | | +| StaticInitializationVector.java:88:13:88:23 | new byte[] : byte[] | StaticInitializationVector.java:87:24:90:9 | {...} : byte[][] [[]] : byte[] | provenance | | +| StaticInitializationVector.java:89:13:89:24 | new byte[] : byte[] | StaticInitializationVector.java:87:24:90:9 | {...} : byte[][] [[]] : byte[] | provenance | | +| StaticInitializationVector.java:92:35:92:67 | new GCMParameterSpec(...) : GCMParameterSpec | StaticInitializationVector.java:96:51:96:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:92:61:92:63 | ivs : byte[][] [[]] : byte[] | StaticInitializationVector.java:92:61:92:66 | ...[...] : byte[] | provenance | | +| StaticInitializationVector.java:92:61:92:66 | ...[...] : byte[] | StaticInitializationVector.java:92:35:92:67 | new GCMParameterSpec(...) : GCMParameterSpec | provenance | MaD:45875 | +| StaticInitializationVector.java:103:21:103:32 | new byte[] : byte[] | StaticInitializationVector.java:108:61:108:62 | iv : byte[] | provenance | | +| StaticInitializationVector.java:108:35:108:63 | new GCMParameterSpec(...) : GCMParameterSpec | StaticInitializationVector.java:112:51:112:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:108:61:108:62 | iv : byte[] | StaticInitializationVector.java:108:35:108:63 | new GCMParameterSpec(...) : GCMParameterSpec | provenance | MaD:45875 | +| StaticInitializationVector.java:120:21:120:32 | new byte[] : byte[] | StaticInitializationVector.java:125:61:125:62 | iv : byte[] | provenance | | +| StaticInitializationVector.java:125:35:125:63 | new GCMParameterSpec(...) : GCMParameterSpec | StaticInitializationVector.java:129:51:129:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:125:61:125:62 | iv : byte[] | StaticInitializationVector.java:125:35:125:63 | new GCMParameterSpec(...) : GCMParameterSpec | provenance | MaD:45875 | +| StaticInitializationVector.java:136:30:136:41 | new byte[] : byte[] | StaticInitializationVector.java:140:26:140:36 | randomBytes : byte[] | provenance | | +| StaticInitializationVector.java:139:21:139:32 | new byte[] : byte[] | StaticInitializationVector.java:142:61:142:62 | iv : byte[] | provenance | | +| StaticInitializationVector.java:140:26:140:36 | randomBytes : byte[] | StaticInitializationVector.java:140:42:140:43 | iv [post update] : byte[] | provenance | MaD:44199 | +| StaticInitializationVector.java:140:42:140:43 | iv [post update] : byte[] | StaticInitializationVector.java:142:61:142:62 | iv : byte[] | provenance | | +| StaticInitializationVector.java:142:35:142:63 | new GCMParameterSpec(...) : GCMParameterSpec | StaticInitializationVector.java:146:51:146:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:142:61:142:62 | iv : byte[] | StaticInitializationVector.java:142:35:142:63 | new GCMParameterSpec(...) : GCMParameterSpec | provenance | MaD:45875 | +| StaticInitializationVector.java:172:30:172:43 | new byte[] : byte[] | StaticInitializationVector.java:174:16:174:26 | randomBytes : byte[] | provenance | | +| StaticInitializationVector.java:174:16:174:26 | randomBytes : byte[] | StaticInitializationVector.java:179:21:179:32 | generate(...) : byte[] | provenance | | +| StaticInitializationVector.java:179:21:179:32 | generate(...) : byte[] | StaticInitializationVector.java:181:54:181:55 | iv : byte[] | provenance | | +| StaticInitializationVector.java:181:34:181:56 | new IvParameterSpec(...) : IvParameterSpec | StaticInitializationVector.java:185:51:185:56 | ivSpec | provenance | Sink:MaD:45855 | +| StaticInitializationVector.java:181:54:181:55 | iv : byte[] | StaticInitializationVector.java:181:34:181:56 | new IvParameterSpec(...) : IvParameterSpec | provenance | MaD:45874 | +nodes +| StaticInitializationVector.java:13:21:13:81 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:15:35:15:63 | new GCMParameterSpec(...) : GCMParameterSpec | semmle.label | new GCMParameterSpec(...) : GCMParameterSpec | +| StaticInitializationVector.java:15:61:15:62 | iv : byte[] | semmle.label | iv : byte[] | +| StaticInitializationVector.java:19:51:19:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:26:21:26:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:28:35:28:63 | new GCMParameterSpec(...) : GCMParameterSpec | semmle.label | new GCMParameterSpec(...) : GCMParameterSpec | +| StaticInitializationVector.java:28:61:28:62 | iv : byte[] | semmle.label | iv : byte[] | +| StaticInitializationVector.java:32:51:32:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:39:21:39:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:44:34:44:56 | new IvParameterSpec(...) : IvParameterSpec | semmle.label | new IvParameterSpec(...) : IvParameterSpec | +| StaticInitializationVector.java:44:54:44:55 | iv : byte[] | semmle.label | iv : byte[] | +| StaticInitializationVector.java:48:51:48:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:55:30:58:9 | new byte[][] : byte[][] | semmle.label | new byte[][] : byte[][] | +| StaticInitializationVector.java:60:35:60:73 | new GCMParameterSpec(...) : GCMParameterSpec | semmle.label | new GCMParameterSpec(...) : GCMParameterSpec | +| StaticInitializationVector.java:60:61:60:72 | ...[...] : byte[] | semmle.label | ...[...] : byte[] | +| StaticInitializationVector.java:64:51:64:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:71:30:74:9 | new byte[][] : byte[][] | semmle.label | new byte[][] : byte[][] | +| StaticInitializationVector.java:76:35:76:73 | new GCMParameterSpec(...) : GCMParameterSpec | semmle.label | new GCMParameterSpec(...) : GCMParameterSpec | +| StaticInitializationVector.java:76:61:76:72 | ...[...] : byte[] | semmle.label | ...[...] : byte[] | +| StaticInitializationVector.java:80:51:80:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:87:24:90:9 | {...} : byte[][] [[]] : byte[] | semmle.label | {...} : byte[][] [[]] : byte[] | +| StaticInitializationVector.java:88:13:88:23 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:89:13:89:24 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:92:35:92:67 | new GCMParameterSpec(...) : GCMParameterSpec | semmle.label | new GCMParameterSpec(...) : GCMParameterSpec | +| StaticInitializationVector.java:92:61:92:63 | ivs : byte[][] [[]] : byte[] | semmle.label | ivs : byte[][] [[]] : byte[] | +| StaticInitializationVector.java:92:61:92:66 | ...[...] : byte[] | semmle.label | ...[...] : byte[] | +| StaticInitializationVector.java:96:51:96:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:103:21:103:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:108:35:108:63 | new GCMParameterSpec(...) : GCMParameterSpec | semmle.label | new GCMParameterSpec(...) : GCMParameterSpec | +| StaticInitializationVector.java:108:61:108:62 | iv : byte[] | semmle.label | iv : byte[] | +| StaticInitializationVector.java:112:51:112:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:120:21:120:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:125:35:125:63 | new GCMParameterSpec(...) : GCMParameterSpec | semmle.label | new GCMParameterSpec(...) : GCMParameterSpec | +| StaticInitializationVector.java:125:61:125:62 | iv : byte[] | semmle.label | iv : byte[] | +| StaticInitializationVector.java:129:51:129:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:136:30:136:41 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:139:21:139:32 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:140:26:140:36 | randomBytes : byte[] | semmle.label | randomBytes : byte[] | +| StaticInitializationVector.java:140:42:140:43 | iv [post update] : byte[] | semmle.label | iv [post update] : byte[] | +| StaticInitializationVector.java:142:35:142:63 | new GCMParameterSpec(...) : GCMParameterSpec | semmle.label | new GCMParameterSpec(...) : GCMParameterSpec | +| StaticInitializationVector.java:142:61:142:62 | iv : byte[] | semmle.label | iv : byte[] | +| StaticInitializationVector.java:146:51:146:56 | ivSpec | semmle.label | ivSpec | +| StaticInitializationVector.java:172:30:172:43 | new byte[] : byte[] | semmle.label | new byte[] : byte[] | +| StaticInitializationVector.java:174:16:174:26 | randomBytes : byte[] | semmle.label | randomBytes : byte[] | +| StaticInitializationVector.java:179:21:179:32 | generate(...) : byte[] | semmle.label | generate(...) : byte[] | +| StaticInitializationVector.java:181:34:181:56 | new IvParameterSpec(...) : IvParameterSpec | semmle.label | new IvParameterSpec(...) : IvParameterSpec | +| StaticInitializationVector.java:181:54:181:55 | iv : byte[] | semmle.label | iv : byte[] | +| StaticInitializationVector.java:185:51:185:56 | ivSpec | semmle.label | ivSpec | +subpaths diff --git a/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java index dca6eb261ca1..4da986c4a3df 100644 --- a/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java +++ b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.java @@ -10,33 +10,33 @@ public class StaticInitializationVector { // BAD: AES-GCM with static IV from a byte array public byte[] encryptWithStaticIvByteArrayWithInitializer(byte[] key, byte[] plaintext) throws Exception { - byte[] iv = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5 }; + byte[] iv = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5 }; // $Source GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $Alert cipher.update(plaintext); return cipher.doFinal(); } // BAD: AES-GCM with static IV from zero-initialized byte array public byte[] encryptWithZeroStaticIvByteArray(byte[] key, byte[] plaintext) throws Exception { - byte[] iv = new byte[16]; + byte[] iv = new byte[16]; // $Source GCMParameterSpec ivSpec = new GCMParameterSpec(128, iv); SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $Alert cipher.update(plaintext); return cipher.doFinal(); } // BAD: AES-CBC with static IV from zero-initialized byte array public byte[] encryptWithStaticIvByteArray(byte[] key, byte[] plaintext) throws Exception { - byte[] iv = new byte[16]; + byte[] iv = new byte[16]; // $Source for (byte i = 0; i < iv.length; i++) { iv[i] = 1; } @@ -45,7 +45,7 @@ public byte[] encryptWithStaticIvByteArray(byte[] key, byte[] plaintext) throws SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $Alert cipher.update(plaintext); return cipher.doFinal(); } @@ -55,13 +55,13 @@ public byte[] encryptWithOneOfStaticIvs01(byte[] key, byte[] plaintext) throws E byte[][] staticIvs = new byte[][] { { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5 }, { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 42 } - }; + }; // $Source GCMParameterSpec ivSpec = new GCMParameterSpec(128, staticIvs[1]); SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $Alert cipher.update(plaintext); return cipher.doFinal(); } @@ -71,13 +71,13 @@ public byte[] encryptWithOneOfStaticIvs02(byte[] key, byte[] plaintext) throws E byte[][] staticIvs = new byte[][] { new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5 }, new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 42 } - }; + }; // $Source GCMParameterSpec ivSpec = new GCMParameterSpec(128, staticIvs[1]); SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $Alert cipher.update(plaintext); return cipher.doFinal(); } @@ -85,15 +85,15 @@ public byte[] encryptWithOneOfStaticIvs02(byte[] key, byte[] plaintext) throws E // BAD: AES-GCM with static IV from a multidimensional byte array public byte[] encryptWithOneOfStaticZeroIvs(byte[] key, byte[] plaintext) throws Exception { byte[][] ivs = new byte[][] { - new byte[8], - new byte[16] + new byte[8], // $Source + new byte[16] // $Source }; GCMParameterSpec ivSpec = new GCMParameterSpec(128, ivs[1]); SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); Cipher cipher = Cipher.getInstance("AES/GCM/PKCS5PADDING"); - cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $staticInitializationVector + cipher.init(Cipher.ENCRYPT_MODE, keySpec, ivSpec); // $Alert cipher.update(plaintext); return cipher.doFinal(); } diff --git a/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.qlref b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.qlref new file mode 100644 index 000000000000..2686a2637cb5 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVector.qlref @@ -0,0 +1,2 @@ +query: Security/CWE/CWE-1204/StaticInitializationVector.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.expected deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql b/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql deleted file mode 100644 index 5996cccdd4f4..000000000000 --- a/java/ql/test/query-tests/security/CWE-1204/StaticInitializationVectorTest.ql +++ /dev/null @@ -1,18 +0,0 @@ -import java -import semmle.code.java.security.StaticInitializationVectorQuery -import utils.test.InlineExpectationsTest - -module StaticInitializationVectorTest implements TestSig { - string getARelevantTag() { result = "staticInitializationVector" } - - predicate hasActualResult(Location location, string element, string tag, string value) { - tag = "staticInitializationVector" and - exists(DataFlow::Node sink | StaticInitializationVectorFlow::flowTo(sink) | - sink.getLocation() = location and - element = sink.toString() and - value = "" - ) - } -} - -import MakeTest