From d7bd8c7ffd5edca4cfa014cfc37793c9b4058400 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 20 Sep 2023 10:19:33 +0200 Subject: [PATCH 1/5] Shared/TypeTracking: Add support for flow from non-LocalSourceNode source and bugfix in smallstep. --- .../codeql/typetracking/TypeTracking.qll | 57 +++++++++++++++---- 1 file changed, 47 insertions(+), 10 deletions(-) diff --git a/shared/typetracking/codeql/typetracking/TypeTracking.qll b/shared/typetracking/codeql/typetracking/TypeTracking.qll index 24411405c657..4f338f59378f 100644 --- a/shared/typetracking/codeql/typetracking/TypeTracking.qll +++ b/shared/typetracking/codeql/typetracking/TypeTracking.qll @@ -478,16 +478,14 @@ module TypeTracking { } pragma[nomagic] - private predicate smallStepProj(LocalSourceNode nodeFrom, StepSummary summary) { + private predicate smallStepProj(Node nodeFrom, StepSummary summary) { smallStep(nodeFrom, _, summary) } bindingset[t, nodeFrom] pragma[inline_late] pragma[noopt] - private TypeTracker smallStepInlineLate( - TypeTracker t, LocalSourceNode nodeFrom, LocalSourceNode nodeTo - ) { + private TypeTracker smallStepInlineLate(TypeTracker t, Node nodeFrom, LocalSourceNode nodeTo) { exists(StepSummary summary | smallStepProj(nodeFrom, summary) and result = append(t, summary) and @@ -657,7 +655,7 @@ module TypeTracking { pragma[inline_late] pragma[noopt] private TypeBackTracker backSmallStepInlineLate( - TypeBackTracker t, LocalSourceNode nodeFrom, LocalSourceNode nodeTo + TypeBackTracker t, Node nodeFrom, LocalSourceNode nodeTo ) { exists(StepSummary summary | backSmallStepProj(nodeTo, summary) and @@ -803,16 +801,35 @@ module TypeTracking { * those sources. */ module TypeTrack { + pragma[nomagic] + private predicate sourceSimpleLocalSmallSteps(Node src, Node n) { + source(src) and + not src instanceof LocalSourceNode and + simpleLocalSmallStep*(src, n) + } + + private predicate firstStep(TypeTracker tt, Node src, LocalSourceNode n2) { + exists(Node n1, TypeTracker tt0 | + sourceSimpleLocalSmallSteps(src, n1) and + tt0.start() and + tt = smallStepInlineLate(tt0, n1, n2) + ) + } + private Node flow(TypeTracker tt) { tt.start() and source(result) or + firstStep(tt, _, result) + or exists(TypeTracker ttMid | tt = ttMid.step(flow(ttMid), result)) } /** - * Holds if the given source flows to `n`. + * Holds if a source flows to `n`. */ - predicate flowsTo(Node n) { flowsTo(flow(TypeTracker::end()), n) } + predicate flowsTo(Node n) { + flowsTo(flow(TypeTracker::end()), n) or sourceSimpleLocalSmallSteps(_, n) + } /** * Given a sink definition, constructs the relation of edges that can be used @@ -849,7 +866,10 @@ module TypeTracking { string toString() { result = this.getNode().toString() + this.ppContent() } /** Holds if this is a source. */ - predicate isSource() { source(this.getNode()) and this.getTypeTracker().start() } + predicate isSource() { + source(this.getNode()) and + (this.getTypeTracker().start() or this instanceof TPathNodeSink) + } /** Holds if this is a sink. */ predicate isSink() { this instanceof TPathNodeSink } @@ -857,7 +877,11 @@ module TypeTracking { private predicate edgeCand(Node n1, TypeTracker tt1, Node n2, TypeTracker tt2) { n1 = flow(tt1) and - tt2 = tt1.step(n1, n2) + ( + tt2 = tt1.step(n1, n2) + or + tt1.start() and firstStep(tt2, n1, n2) + ) } private predicate edgeCand(PathNodeFwd n1, PathNodeFwd n2) { @@ -871,7 +895,13 @@ module TypeTracking { or n1.getTypeTracker().end() and flowsTo(n1.getNode(), n2.getNode()) and + n1.getNode() != n2.getNode() and n2 instanceof TPathNodeSink + or + sourceSimpleLocalSmallSteps(n1.getNode(), n2.getNode()) and + n1.getNode() != n2.getNode() and + n1.isSource() and + n2.isSink() } private predicate reachRev(PathNodeFwd n) { @@ -896,8 +926,15 @@ module TypeTracking { private predicate stepPlus(PathNode n1, PathNode n2) = fastTC(edges/2)(n1, n2) + /** + * DEPRECATED: Use `flowPath` instead. + * + * Holds if there is a path between `source` and `sink`. + */ + deprecated predicate hasFlow(PathNode source, PathNode sink) { flowPath(source, sink) } + /** Holds if there is a path between `source` and `sink`. */ - predicate hasFlow(PathNode source, PathNode sink) { + predicate flowPath(PathNode source, PathNode sink) { source.isSource() and sink.isSink() and (source = sink or stepPlus(source, sink)) From d7e965f863df8ee98713ffaf0e25fe0e58538bee Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 20 Sep 2023 10:21:21 +0200 Subject: [PATCH 2/5] Dataflow: Add lightweight api based on TypeTracking. --- .../dataflow/internal/DataFlowImplCommon.qll | 75 +++++++++++++++++++ shared/dataflow/qlpack.yml | 1 + 2 files changed, 76 insertions(+) diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index 6fb53e04331a..c0b0973f0db0 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll @@ -1,4 +1,5 @@ private import codeql.dataflow.DataFlow +private import codeql.typetracking.TypeTracking as Tt module MakeImplCommon { private import Lang @@ -52,8 +53,82 @@ module MakeImplCommon { class FeatureEqualSourceSinkCallContext extends FlowFeature, TFeatureEqualSourceSinkCallContext { override string toString() { result = "FeatureEqualSourceSinkCallContext" } } + + signature predicate sourceNode(Node n); + + /** + * EXPERIMENTAL: This API is subject to change without notice. + * + * Given a source definition, this constructs a simple forward flow + * computation with an access path limit of 1. + */ + module SimpleGlobal { + import TypeTracking::TypeTrack + } } + private module TypeTrackingInput implements Tt::TypeTrackingInput { + final class Node = Lang::Node; + + class LocalSourceNode extends Node { + LocalSourceNode() { + storeStep(_, this, _) or + loadStep(_, this, _) or + jumpStepCached(_, this) or + this instanceof ParamNode or + this instanceof OutNodeExt + } + } + + final private class LangContentSet = Lang::ContentSet; + + class Content extends LangContentSet { + string toString() { result = "Content" } + } + + class ContentFilter extends Content { + Content getAMatchingContent() { result = this } + } + + predicate compatibleContents(Content storeContents, Content loadContents) { + storeContents.getAStoreContent() = loadContents.getAReadContent() + } + + predicate simpleLocalSmallStep = simpleLocalFlowStepExt/2; + + predicate levelStepNoCall(Node n1, LocalSourceNode n2) { none() } + + predicate levelStepCall(Node n1, LocalSourceNode n2) { + argumentValueFlowsThrough(n1, TReadStepTypesNone(), n2) + } + + predicate storeStep(Node n1, Node n2, Content f) { storeSet(n1, f, n2, _, _) } + + predicate loadStep(Node n1, LocalSourceNode n2, Content f) { + readSet(n1, f, n2) + or + argumentValueFlowsThrough(n1, TReadStepTypesSome(_, f, _), n2) + } + + predicate loadStoreStep(Node nodeFrom, Node nodeTo, Content f1, Content f2) { none() } + + predicate withContentStep(Node nodeFrom, LocalSourceNode nodeTo, ContentFilter f) { none() } + + predicate withoutContentStep(Node nodeFrom, LocalSourceNode nodeTo, ContentFilter f) { none() } + + predicate jumpStep(Node n1, LocalSourceNode n2) { jumpStepCached(n1, n2) } + + predicate callStep(Node n1, LocalSourceNode n2) { viableParamArg(_, n2, n1) } + + predicate returnStep(Node n1, LocalSourceNode n2) { + viableReturnPosOut(_, getReturnPosition(n1), n2) + } + + predicate hasFeatureBacktrackStoreTarget() { none() } + } + + private module TypeTracking = Tt::TypeTracking; + /** * The cost limits for the `AccessPathFront` to `AccessPathApprox` expansion. * diff --git a/shared/dataflow/qlpack.yml b/shared/dataflow/qlpack.yml index 62a35f1ccc85..42bdbb50379f 100644 --- a/shared/dataflow/qlpack.yml +++ b/shared/dataflow/qlpack.yml @@ -4,5 +4,6 @@ groups: shared library: true dependencies: codeql/ssa: ${workspace} + codeql/typetracking: ${workspace} codeql/util: ${workspace} warnOnImplicitThis: true From 5c40d553b479587af427ba3d9b30a22b0b42c6d9 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 20 Sep 2023 10:21:53 +0200 Subject: [PATCH 3/5] Java: Switch XmlParsers lib to lightweight data flow. --- java/ql/lib/ext/java.lang.model.yml | 1 + .../semmle/code/java/security/XmlParsers.qll | 238 +++++++----------- .../CWE-611/DocumentBuilderTests.java | 2 +- 3 files changed, 92 insertions(+), 149 deletions(-) diff --git a/java/ql/lib/ext/java.lang.model.yml b/java/ql/lib/ext/java.lang.model.yml index 6022da5a4860..7974a234f279 100644 --- a/java/ql/lib/ext/java.lang.model.yml +++ b/java/ql/lib/ext/java.lang.model.yml @@ -130,6 +130,7 @@ extensions: - ["java.lang", "Thread", True, "getName", "()", "", "Argument[this].SyntheticField[java.lang.Thread.name]", "ReturnValue", "value", "manual"] - ["java.lang", "ThreadLocal", True, "get", "()", "", "Argument[this].SyntheticField[java.lang.ThreadLocal.value]", "ReturnValue", "value", "manual"] - ["java.lang", "ThreadLocal", True, "set", "(Object)", "", "Argument[0]", "Argument[this].SyntheticField[java.lang.ThreadLocal.value]", "value", "manual"] + - ["java.lang", "ThreadLocal", False, "withInitial", "(Supplier)", "", "Argument[0].ReturnValue", "ReturnValue.SyntheticField[java.lang.ThreadLocal.value]", "value", "manual"] - ["java.lang", "Throwable", False, "Throwable", "(Throwable)", "", "Argument[0]", "Argument[this].SyntheticField[java.lang.Throwable.cause]", "value", "manual"] - ["java.lang", "Throwable", False, "Throwable", "(String)", "", "Argument[0]", "Argument[this].SyntheticField[java.lang.Throwable.message]", "value", "manual"] - ["java.lang", "Throwable", True, "getCause", "()", "", "Argument[this].SyntheticField[java.lang.Throwable.cause]", "ReturnValue", "value", "manual"] diff --git a/java/ql/lib/semmle/code/java/security/XmlParsers.qll b/java/ql/lib/semmle/code/java/security/XmlParsers.qll index 71f896ef2559..cf8ef5e8775b 100644 --- a/java/ql/lib/semmle/code/java/security/XmlParsers.qll +++ b/java/ql/lib/semmle/code/java/security/XmlParsers.qll @@ -82,41 +82,16 @@ class DocumentBuilderParse extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } override predicate isSafe() { - SafeDocumentBuilderToDocumentBuilderParseFlow::flowToExpr(this.getQualifier()) + SafeDocumentBuilderToDocumentBuilderParseFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } -private module SafeDocumentBuilderToDocumentBuilderParseFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeDocumentBuilder } - - predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(DocumentBuilderParse dbp).getQualifier() - } - - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(RefType t, ReturnStmt ret, Method m | - node2.asExpr().(ClassInstanceExpr).getConstructedType().getSourceDeclaration() = t and - t.getASourceSupertype+().hasQualifiedName("java.lang", "ThreadLocal") and - ret.getResult() = node1.asExpr() and - ret.getEnclosingCallable() = m and - m.hasName("initialValue") and - m.getDeclaringType() = t - ) - or - exists(MethodAccess ma, Method m | - ma = node2.asExpr() and - ma.getQualifier() = node1.asExpr() and - ma.getMethod() = m and - m.hasName("get") and - m.getDeclaringType().getSourceDeclaration().hasQualifiedName("java.lang", "ThreadLocal") - ) - } - - int fieldFlowBranchLimit() { result = 0 } +private predicate safeDocumentBuilderNode(DataFlow::Node src) { + src.asExpr() instanceof SafeDocumentBuilder } private module SafeDocumentBuilderToDocumentBuilderParseFlow = - DataFlow::Global; + DataFlow::SimpleGlobal; /** * A `ParserConfig` specific to `DocumentBuilderFactory`. @@ -176,27 +151,19 @@ private class DocumentBuilderConstruction extends MethodAccess { } } -private module SafeDocumentBuilderFactoryToDocumentBuilderConstructionFlowConfig implements - DataFlow::ConfigSig -{ - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeDocumentBuilderFactory } - - predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(DocumentBuilderConstruction dbc).getQualifier() - } - - int fieldFlowBranchLimit() { result = 0 } +private predicate safeDocumentBuilderFactoryNode(DataFlow::Node src) { + src.asExpr() instanceof SafeDocumentBuilderFactory } private module SafeDocumentBuilderFactoryToDocumentBuilderConstructionFlow = - DataFlow::Global; + DataFlow::SimpleGlobal; /** * A `DocumentBuilder` created from a safely configured `DocumentBuilderFactory`. */ class SafeDocumentBuilder extends DocumentBuilderConstruction { SafeDocumentBuilder() { - SafeDocumentBuilderFactoryToDocumentBuilderConstructionFlow::flowToExpr(this.getQualifier()) + SafeDocumentBuilderFactoryToDocumentBuilderConstructionFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } @@ -226,23 +193,16 @@ class XmlInputFactoryStreamReader extends XmlParserCall { } override predicate isSafe() { - SafeXmlInputFactoryToXmlInputFactoryReaderFlow::flowToExpr(this.getQualifier()) + SafeXmlInputFactoryToXmlInputFactoryReaderFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } -private module SafeXmlInputFactoryToXmlInputFactoryReaderFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeXmlInputFactory } - - predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(XmlInputFactoryStreamReader xifsr).getQualifier() or - sink.asExpr() = any(XmlInputFactoryEventReader xifer).getQualifier() - } - - int fieldFlowBranchLimit() { result = 0 } +private predicate safeXmlInputFactoryNode(DataFlow::Node src) { + src.asExpr() instanceof SafeXmlInputFactory } private module SafeXmlInputFactoryToXmlInputFactoryReaderFlow = - DataFlow::Global; + DataFlow::SimpleGlobal; /** A call to `XMLInputFactory.createEventReader`. */ class XmlInputFactoryEventReader extends XmlParserCall { @@ -261,7 +221,7 @@ class XmlInputFactoryEventReader extends XmlParserCall { } override predicate isSafe() { - SafeXmlInputFactoryToXmlInputFactoryReaderFlow::flowToExpr(this.getQualifier()) + SafeXmlInputFactoryToXmlInputFactoryReaderFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } @@ -353,20 +313,13 @@ class SaxBuilderParse extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } override predicate isSafe() { - SafeSaxBuilderToSaxBuilderParseFlow::flowToExpr(this.getQualifier()) + SafeSaxBuilderToSaxBuilderParseFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } -private module SafeSaxBuilderToSaxBuilderParseFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeSaxBuilder } +private predicate safeSaxBuilderNode(DataFlow::Node src) { src.asExpr() instanceof SafeSaxBuilder } - predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(SaxBuilderParse sax).getQualifier() } - - int fieldFlowBranchLimit() { result = 0 } -} - -private module SafeSaxBuilderToSaxBuilderParseFlow = - DataFlow::Global; +private module SafeSaxBuilderToSaxBuilderParseFlow = DataFlow::SimpleGlobal; /** * A `ParserConfig` specific to `SAXBuilder`. @@ -426,7 +379,9 @@ class SaxParserParse extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } - override predicate isSafe() { SafeSaxParserFlow::flowToExpr(this.getQualifier()) } + override predicate isSafe() { + SafeSaxParserFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) + } } /** A `ParserConfig` that is specific to `SaxParserFactory`. */ @@ -473,44 +428,23 @@ class SafeSaxParserFactory extends VarAccess { } } -private module SafeSaxParserFactoryToNewSaxParserFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeSaxParserFactory } - - predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma, Method m | - sink.asExpr() = ma.getQualifier() and - ma.getMethod() = m and - m.getDeclaringType() instanceof SaxParserFactory and - m.hasName("newSAXParser") - ) - } - - int fieldFlowBranchLimit() { result = 0 } +private predicate safeSaxParserFactoryNode(DataFlow::Node src) { + src.asExpr() instanceof SafeSaxParserFactory } private module SafeSaxParserFactoryToNewSaxParserFlow = - DataFlow::Global; + DataFlow::SimpleGlobal; -private module SafeSaxParserFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeSaxParser } +private predicate safeSaxParserNode(DataFlow::Node src) { src.asExpr() instanceof SafeSaxParser } - predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - sink.asExpr() = ma.getQualifier() and ma.getMethod().getDeclaringType() instanceof SaxParser - ) - } - - int fieldFlowBranchLimit() { result = 0 } -} - -private module SafeSaxParserFlow = DataFlow::Global; +private module SafeSaxParserFlow = DataFlow::SimpleGlobal; /** A `SaxParser` created from a safely configured `SaxParserFactory`. */ class SafeSaxParser extends MethodAccess { SafeSaxParser() { this.getMethod().getDeclaringType() instanceof SaxParserFactory and this.getMethod().hasName("newSAXParser") and - SafeSaxParserFactoryToNewSaxParserFlow::flowToExpr(this.getQualifier()) + SafeSaxParserFactoryToNewSaxParserFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } @@ -534,7 +468,9 @@ class SaxReaderRead extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } - override predicate isSafe() { SafeSaxReaderFlow::flowToExpr(this.getQualifier()) } + override predicate isSafe() { + SafeSaxReaderFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) + } } /** A `ParserConfig` specific to `SaxReader`. */ @@ -548,19 +484,9 @@ class SaxReaderConfig extends ParserConfig { } } -private module SafeSaxReaderFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeSaxReader } +private predicate safeSaxReaderNode(DataFlow::Node src) { src.asExpr() instanceof SafeSaxReader } - predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - sink.asExpr() = ma.getQualifier() and ma.getMethod().getDeclaringType() instanceof SaxReader - ) - } - - int fieldFlowBranchLimit() { result = 0 } -} - -private module SafeSaxReaderFlow = DataFlow::Global; +private module SafeSaxReaderFlow = DataFlow::SimpleGlobal; /** A safely configured `SaxReader`. */ class SafeSaxReader extends VarAccess { @@ -612,8 +538,8 @@ class XmlReaderParse extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } override predicate isSafe() { - exists(ExplicitlySafeXmlReader sr | sr.flowsTo(this.getQualifier())) or - exists(CreatedSafeXmlReader cr | cr.flowsTo(this.getQualifier())) + ExplicitlySafeXmlReaderFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) or + CreatedSafeXmlReaderFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } @@ -628,7 +554,7 @@ class XmlReaderConfig extends ParserConfig { } } -private module ExplicitlySafeXmlReaderFlowConfig implements DataFlow::ConfigSig { +deprecated private module ExplicitlySafeXmlReaderFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ExplicitlySafeXmlReader } predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof SafeXmlReaderFlowSink } @@ -636,7 +562,14 @@ private module ExplicitlySafeXmlReaderFlowConfig implements DataFlow::ConfigSig int fieldFlowBranchLimit() { result = 0 } } -private module ExplicitlySafeXmlReaderFlow = DataFlow::Global; +private predicate explicitlySafeXmlReaderNode(DataFlow::Node src) { + src.asExpr() instanceof ExplicitlySafeXmlReader +} + +deprecated private module ExplicitlySafeXmlReaderFlowDeprecated = + DataFlow::Global; + +private module ExplicitlySafeXmlReaderFlow = DataFlow::SimpleGlobal; /** An argument to a safe XML reader. */ class SafeXmlReaderFlowSink extends Expr { @@ -680,13 +613,13 @@ class ExplicitlySafeXmlReader extends VarAccess { ) } - /** Holds if `SafeXmlReaderFlowSink` detects flow from this to `sink` */ - predicate flowsTo(SafeXmlReaderFlowSink sink) { - ExplicitlySafeXmlReaderFlow::flow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) + /** DEPRECATED. Holds if `SafeXmlReaderFlowSink` detects flow from this to `sink` */ + deprecated predicate flowsTo(SafeXmlReaderFlowSink sink) { + ExplicitlySafeXmlReaderFlowDeprecated::flow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) } } -private module CreatedSafeXmlReaderFlowConfig implements DataFlow::ConfigSig { +deprecated private module CreatedSafeXmlReaderFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { src.asExpr() instanceof CreatedSafeXmlReader } predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof SafeXmlReaderFlowSink } @@ -694,7 +627,14 @@ private module CreatedSafeXmlReaderFlowConfig implements DataFlow::ConfigSig { int fieldFlowBranchLimit() { result = 0 } } -private module CreatedSafeXmlReaderFlow = DataFlow::Global; +private predicate createdSafeXmlReaderNode(DataFlow::Node src) { + src.asExpr() instanceof CreatedSafeXmlReader +} + +deprecated private module CreatedSafeXmlReaderFlowDeprecated = + DataFlow::Global; + +private module CreatedSafeXmlReaderFlow = DataFlow::SimpleGlobal; /** An `XmlReader` that is obtained from a safe source. */ class CreatedSafeXmlReader extends Call { @@ -702,12 +642,12 @@ class CreatedSafeXmlReader extends Call { //Obtained from SAXParser this.(MethodAccess).getMethod().getDeclaringType() instanceof SaxParser and this.(MethodAccess).getMethod().hasName("getXMLReader") and - SafeSaxParserFlow::flowToExpr(this.getQualifier()) + SafeSaxParserFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) or //Obtained from SAXReader this.(MethodAccess).getMethod().getDeclaringType() instanceof SaxReader and this.(MethodAccess).getMethod().hasName("getXMLReader") and - SafeSaxReaderFlow::flowToExpr(this.getQualifier()) + SafeSaxReaderFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) or exists(RefType secureReader, string package | this.(ClassInstanceExpr).getConstructedType() = secureReader and @@ -716,9 +656,9 @@ class CreatedSafeXmlReader extends Call { ) } - /** Holds if `CreatedSafeXmlReaderFlowConfig` detects flow from this to `sink` */ - predicate flowsTo(SafeXmlReaderFlowSink sink) { - CreatedSafeXmlReaderFlow::flow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) + /** DEPRECATED. Holds if `CreatedSafeXmlReaderFlowConfig` detects flow from this to `sink` */ + deprecated predicate flowsTo(SafeXmlReaderFlowSink sink) { + CreatedSafeXmlReaderFlowDeprecated::flow(DataFlow::exprNode(this), DataFlow::exprNode(sink)) } } @@ -747,8 +687,8 @@ class ConstructedSaxSource extends ClassInstanceExpr { /** Holds if the resulting `SaxSource` is safe. */ predicate isSafe() { - exists(CreatedSafeXmlReader safeReader | safeReader.flowsTo(this.getArgument(0))) or - exists(ExplicitlySafeXmlReader safeReader | safeReader.flowsTo(this.getArgument(0))) + CreatedSafeXmlReaderFlow::flowsTo(DataFlow::exprNode(this.getArgument(0))) or + ExplicitlySafeXmlReaderFlow::flowsTo(DataFlow::exprNode(this.getArgument(0))) } } @@ -769,8 +709,8 @@ class SafeSaxSource extends Expr { exists(Variable v | v = this.(VarAccess).getVariable() | exists(SaxSourceSetReader s | s.getQualifier() = v.getAnAccess() | ( - exists(CreatedSafeXmlReader safeReader | safeReader.flowsTo(s.getArgument(0))) or - exists(ExplicitlySafeXmlReader safeReader | safeReader.flowsTo(s.getArgument(0))) + CreatedSafeXmlReaderFlow::flowsTo(DataFlow::exprNode(s.getArgument(0))) or + ExplicitlySafeXmlReaderFlow::flowsTo(DataFlow::exprNode(s.getArgument(0))) ) ) ) @@ -859,22 +799,16 @@ class TransformerTransform extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } override predicate isSafe() { - SafeTransformerToTransformerTransformFlow::flowToExpr(this.getQualifier()) + SafeTransformerToTransformerTransformFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } -private module SafeTransformerToTransformerTransformFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeTransformer } - - predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(TransformerTransform tt).getQualifier() - } - - int fieldFlowBranchLimit() { result = 0 } +private predicate safeTransformerNode(DataFlow::Node src) { + src.asExpr() instanceof SafeTransformer } private module SafeTransformerToTransformerTransformFlow = - DataFlow::Global; + DataFlow::SimpleGlobal; /** A call to `Transformer.newTransformer` with source. */ class TransformerFactorySource extends XmlParserCall { @@ -888,7 +822,9 @@ class TransformerFactorySource extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } - override predicate isSafe() { SafeTransformerFactoryFlow::flowToExpr(this.getQualifier()) } + override predicate isSafe() { + SafeTransformerFactoryFlow2::flowsTo(DataFlow::exprNode(this.getQualifier())) + } } /** A `ParserConfig` specific to `TransformerFactory`. */ @@ -903,7 +839,7 @@ class TransformerFactoryConfig extends TransformerConfig { } /** - * DEPRECATED: Use `SafeTransformerFactoryFlow` instead. + * DEPRECATED. * * A dataflow configuration that identifies `TransformerFactory` and `SAXTransformerFactory` * instances that have been safely configured. @@ -924,10 +860,12 @@ deprecated class SafeTransformerFactoryFlowConfig extends DataFlow3::Configurati } /** + * DEPRECATED. + * * A dataflow configuration that identifies `TransformerFactory` and `SAXTransformerFactory` * instances that have been safely configured. */ -module SafeTransformerFactoryFlowConfig implements DataFlow::ConfigSig { +deprecated module SafeTransformerFactoryFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeTransformerFactory } predicate isSink(DataFlow::Node sink) { @@ -940,11 +878,19 @@ module SafeTransformerFactoryFlowConfig implements DataFlow::ConfigSig { int fieldFlowBranchLimit() { result = 0 } } +private predicate safeTransformerFactoryNode(DataFlow::Node src) { + src.asExpr() instanceof SafeTransformerFactory +} + /** + * DEPRECATED. + * * Identifies `TransformerFactory` and `SAXTransformerFactory` * instances that have been safely configured. */ -module SafeTransformerFactoryFlow = DataFlow::Global; +deprecated module SafeTransformerFactoryFlow = DataFlow::Global; + +private module SafeTransformerFactoryFlow2 = DataFlow::SimpleGlobal; /** A safely configured `TransformerFactory`. */ class SafeTransformerFactory extends VarAccess { @@ -967,7 +913,7 @@ class SafeTransformer extends MethodAccess { this.getMethod() = m and m.getDeclaringType() instanceof TransformerFactory and m.hasName("newTransformer") and - SafeTransformerFactoryFlow::flowToExpr(this.getQualifier()) + SafeTransformerFactoryFlow2::flowsTo(DataFlow::exprNode(this.getQualifier())) ) } } @@ -989,7 +935,9 @@ class SaxTransformerFactoryNewXmlFilter extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } - override predicate isSafe() { SafeTransformerFactoryFlow::flowToExpr(this.getQualifier()) } + override predicate isSafe() { + SafeTransformerFactoryFlow2::flowsTo(DataFlow::exprNode(this.getQualifier())) + } } /* Schema: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#schemafactory */ @@ -1022,22 +970,16 @@ class SchemaFactoryNewSchema extends XmlParserCall { override Expr getSink() { result = this.getArgument(0) } override predicate isSafe() { - SafeSchemaFactoryToSchemaFactoryNewSchemaFlow::flowToExpr(this.getQualifier()) + SafeSchemaFactoryToSchemaFactoryNewSchemaFlow::flowsTo(DataFlow::exprNode(this.getQualifier())) } } -private module SafeSchemaFactoryToSchemaFactoryNewSchemaFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeSchemaFactory } - - predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(SchemaFactoryNewSchema sfns).getQualifier() - } - - int fieldFlowBranchLimit() { result = 0 } +private predicate safeSchemaFactoryNode(DataFlow::Node src) { + src.asExpr() instanceof SafeSchemaFactory } private module SafeSchemaFactoryToSchemaFactoryNewSchemaFlow = - DataFlow::Global; + DataFlow::SimpleGlobal; /** A safely configured `SchemaFactory`. */ class SafeSchemaFactory extends VarAccess { diff --git a/java/ql/test/query-tests/security/CWE-611/DocumentBuilderTests.java b/java/ql/test/query-tests/security/CWE-611/DocumentBuilderTests.java index 98d95686301c..5761bb531363 100644 --- a/java/ql/test/query-tests/security/CWE-611/DocumentBuilderTests.java +++ b/java/ql/test/query-tests/security/CWE-611/DocumentBuilderTests.java @@ -129,7 +129,7 @@ protected DocumentBuilder initialValue() { public void disableExternalEntities2(Socket sock) throws Exception { DocumentBuilder builder = XML_DOCUMENT_BUILDER.get(); - builder.parse(sock.getInputStream()); // safe + builder.parse(sock.getInputStream()); // $ SPURIOUS: hasTaintFlow } } From d285afba08f9be936266ff9b644c1b34824d3bfc Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 20 Sep 2023 14:52:49 +0200 Subject: [PATCH 4/5] Typetracking: minor perf fix. --- shared/typetracking/codeql/typetracking/TypeTracking.qll | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/shared/typetracking/codeql/typetracking/TypeTracking.qll b/shared/typetracking/codeql/typetracking/TypeTracking.qll index 4f338f59378f..94cca417a863 100644 --- a/shared/typetracking/codeql/typetracking/TypeTracking.qll +++ b/shared/typetracking/codeql/typetracking/TypeTracking.qll @@ -805,7 +805,12 @@ module TypeTracking { private predicate sourceSimpleLocalSmallSteps(Node src, Node n) { source(src) and not src instanceof LocalSourceNode and - simpleLocalSmallStep*(src, n) + src = n + or + exists(Node mid | + sourceSimpleLocalSmallSteps(src, mid) and + simpleLocalSmallStep(mid, n) + ) } private predicate firstStep(TypeTracker tt, Node src, LocalSourceNode n2) { From 3dadfa224374c38c735282a966d21ca4818112f5 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 21 Sep 2023 11:50:04 +0200 Subject: [PATCH 5/5] Dataflow: review fixes --- .../codeql/dataflow/internal/DataFlowImplCommon.qll | 13 +++++++++---- shared/util/codeql/util/Unit.qll | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index c0b0973f0db0..ac5b5a243a86 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll @@ -1,5 +1,6 @@ private import codeql.dataflow.DataFlow private import codeql.typetracking.TypeTracking as Tt +private import codeql.util.Unit module MakeImplCommon { private import Lang @@ -54,7 +55,10 @@ module MakeImplCommon { override string toString() { result = "FeatureEqualSourceSinkCallContext" } } - signature predicate sourceNode(Node n); + /** + * Holds if `source` is a relevant data flow source. + */ + signature predicate sourceNode(Node source); /** * EXPERIMENTAL: This API is subject to change without notice. @@ -86,8 +90,8 @@ module MakeImplCommon { string toString() { result = "Content" } } - class ContentFilter extends Content { - Content getAMatchingContent() { result = this } + class ContentFilter extends Unit { + Content getAMatchingContent() { none() } } predicate compatibleContents(Content storeContents, Content loadContents) { @@ -102,6 +106,7 @@ module MakeImplCommon { argumentValueFlowsThrough(n1, TReadStepTypesNone(), n2) } + // TODO: support setters predicate storeStep(Node n1, Node n2, Content f) { storeSet(n1, f, n2, _, _) } predicate loadStep(Node n1, LocalSourceNode n2, Content f) { @@ -720,7 +725,7 @@ module MakeImplCommon { * If a read step was taken, then `read` captures the `Content`, the * container type, and the content type. */ - pragma[nomagic] + cached predicate argumentValueFlowsThrough(ArgNode arg, ReadStepTypesOption read, Node out) { exists(DataFlowCall call, ReturnKind kind | argumentValueFlowsThrough0(call, arg, kind, read) and diff --git a/shared/util/codeql/util/Unit.qll b/shared/util/codeql/util/Unit.qll index e9611ed3df4e..a0db9d7030f7 100644 --- a/shared/util/codeql/util/Unit.qll +++ b/shared/util/codeql/util/Unit.qll @@ -4,7 +4,7 @@ private newtype TUnit = TMkUnit() /** The trivial type with a single element. */ -class Unit extends TUnit { +final class Unit extends TUnit { /** Gets a textual representation of this element. */ string toString() { result = "unit" } }