From ba6039946bcca5620bd5d781247760bb89fb6ee6 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Mon, 5 Feb 2024 14:43:19 +0100 Subject: [PATCH] Go: Add alert provenance plumbing. --- go/ql/lib/semmle/go/dataflow/ExternalFlow.qll | 99 ++++++++++++------- .../go/dataflow/internal/DataFlowPrivate.qll | 5 + .../go/dataflow/internal/DataFlowUtil.qll | 12 ++- .../internal/ExternalFlowExtensions.qll | 6 +- .../go/dataflow/internal/FlowSummaryImpl.qll | 16 +-- .../dataflow/internal/TaintTrackingUtil.qll | 34 ++++--- .../semmle/go/frameworks/stdlib/NetHttp.qll | 2 +- .../IncorrectIntegerConversionLib.qll | 2 +- 8 files changed, 108 insertions(+), 68 deletions(-) diff --git a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll index 9df20606d27..26572e27a78 100644 --- a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll +++ b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll @@ -82,9 +82,9 @@ private import codeql.mad.ModelValidation as SharedModelVal /** Holds if `package` have MaD framework coverage. */ private predicate packageHasMaDCoverage(string package) { - sourceModel(package, _, _, _, _, _, _, _, _) or - sinkModel(package, _, _, _, _, _, _, _, _) or - summaryModel(package, _, _, _, _, _, _, _, _, _) + sourceModel(package, _, _, _, _, _, _, _, _, _) or + sinkModel(package, _, _, _, _, _, _, _, _, _) or + summaryModel(package, _, _, _, _, _, _, _, _, _, _) } /** @@ -128,7 +128,7 @@ predicate modelCoverage(string package, int pkgs, string kind, string part, int strictcount(string subpkg, string type, boolean subtypes, string name, string signature, string ext, string output, string provenance | canonicalPackageHasASubpackage(package, subpkg) and - sourceModel(subpkg, type, subtypes, name, signature, ext, output, kind, provenance) + sourceModel(subpkg, type, subtypes, name, signature, ext, output, kind, provenance, _) ) or part = "sink" and @@ -136,7 +136,7 @@ predicate modelCoverage(string package, int pkgs, string kind, string part, int strictcount(string subpkg, string type, boolean subtypes, string name, string signature, string ext, string input, string provenance | canonicalPackageHasASubpackage(package, subpkg) and - sinkModel(subpkg, type, subtypes, name, signature, ext, input, kind, provenance) + sinkModel(subpkg, type, subtypes, name, signature, ext, input, kind, provenance, _) ) or part = "summary" and @@ -144,7 +144,8 @@ predicate modelCoverage(string package, int pkgs, string kind, string part, int strictcount(string subpkg, string type, boolean subtypes, string name, string signature, string ext, string input, string output, string provenance | canonicalPackageHasASubpackage(package, subpkg) and - summaryModel(subpkg, type, subtypes, name, signature, ext, input, output, kind, provenance) + summaryModel(subpkg, type, subtypes, name, signature, ext, input, output, kind, provenance, + _) ) ) } @@ -153,9 +154,9 @@ predicate modelCoverage(string package, int pkgs, string kind, string part, int module ModelValidation { private string getInvalidModelInput() { exists(string pred, AccessPath input, string part | - sinkModel(_, _, _, _, _, _, input, _, _) and pred = "sink" + sinkModel(_, _, _, _, _, _, input, _, _, _) and pred = "sink" or - summaryModel(_, _, _, _, _, _, input, _, _, _) and pred = "summary" + summaryModel(_, _, _, _, _, _, input, _, _, _, _) and pred = "summary" | ( invalidSpecComponent(input, part) and @@ -171,9 +172,9 @@ module ModelValidation { private string getInvalidModelOutput() { exists(string pred, string output, string part | - sourceModel(_, _, _, _, _, _, output, _, _) and pred = "source" + sourceModel(_, _, _, _, _, _, output, _, _, _) and pred = "source" or - summaryModel(_, _, _, _, _, _, _, output, _, _) and pred = "summary" + summaryModel(_, _, _, _, _, _, _, output, _, _, _) and pred = "summary" | invalidSpecComponent(output, part) and not part = "" and @@ -183,11 +184,11 @@ module ModelValidation { } private module KindValConfig implements SharedModelVal::KindValidationConfigSig { - predicate summaryKind(string kind) { summaryModel(_, _, _, _, _, _, _, _, kind, _) } + predicate summaryKind(string kind) { summaryModel(_, _, _, _, _, _, _, _, kind, _, _) } - predicate sinkKind(string kind) { sinkModel(_, _, _, _, _, _, _, kind, _) } + predicate sinkKind(string kind) { sinkModel(_, _, _, _, _, _, _, kind, _, _) } - predicate sourceKind(string kind) { sourceModel(_, _, _, _, _, _, _, kind, _) } + predicate sourceKind(string kind) { sourceModel(_, _, _, _, _, _, _, kind, _, _) } } private module KindVal = SharedModelVal::KindValidation; @@ -197,11 +198,12 @@ module ModelValidation { string pred, string package, string type, string name, string signature, string ext, string provenance | - sourceModel(package, type, _, name, signature, ext, _, _, provenance) and pred = "source" + sourceModel(package, type, _, name, signature, ext, _, _, provenance, _) and pred = "source" or - sinkModel(package, type, _, name, signature, ext, _, _, provenance) and pred = "sink" + sinkModel(package, type, _, name, signature, ext, _, _, provenance, _) and pred = "sink" or - summaryModel(package, type, _, name, signature, ext, _, _, _, provenance) and pred = "summary" + summaryModel(package, type, _, name, signature, ext, _, _, _, provenance, _) and + pred = "summary" | not package.replaceAll("$ANYVERSION", "").regexpMatch("[a-zA-Z0-9_\\./-]*") and result = "Dubious package \"" + package + "\" in " + pred + " model." @@ -237,9 +239,9 @@ pragma[nomagic] private predicate elementSpec( string package, string type, boolean subtypes, string name, string signature, string ext ) { - sourceModel(package, type, subtypes, name, signature, ext, _, _, _) or - sinkModel(package, type, subtypes, name, signature, ext, _, _, _) or - summaryModel(package, type, subtypes, name, signature, ext, _, _, _, _) + sourceModel(package, type, subtypes, name, signature, ext, _, _, _, _) or + sinkModel(package, type, subtypes, name, signature, ext, _, _, _, _) or + summaryModel(package, type, subtypes, name, signature, ext, _, _, _, _, _) } private string paramsStringPart(Function f, int i) { @@ -297,8 +299,8 @@ predicate hasExternalSpecification(Function f) { f = any(SummarizedCallable sc).asFunction() or exists(SourceSinkInterpretationInput::SourceOrSinkElement e | f = e.asEntity() | - SourceSinkInterpretationInput::sourceElement(e, _, _, _) or - SourceSinkInterpretationInput::sinkElement(e, _, _, _) + SourceSinkInterpretationInput::sourceElement(e, _, _, _, _) or + SourceSinkInterpretationInput::sinkElement(e, _, _, _, _) ) } @@ -351,9 +353,9 @@ private module Cached { * model. */ cached - predicate sourceNode(DataFlow::Node node, string kind) { + predicate sourceNode(DataFlow::Node node, string kind, string model) { exists(SourceSinkInterpretationInput::InterpretNode n | - isSourceNode(n, kind) and n.asNode() = node + isSourceNode(n, kind, model) and n.asNode() = node ) } @@ -362,57 +364,78 @@ private module Cached { * model. */ cached - predicate sinkNode(DataFlow::Node node, string kind) { + predicate sinkNode(DataFlow::Node node, string kind, string model) { exists(SourceSinkInterpretationInput::InterpretNode n | - isSinkNode(n, kind) and n.asNode() = node + isSinkNode(n, kind, model) and n.asNode() = node ) } } import Cached +/** + * Holds if `node` is specified as a source with the given kind in a MaD flow + * model. + */ +predicate sourceNode(DataFlow::Node node, string kind) { sourceNode(node, kind, _) } + +/** + * Holds if `node` is specified as a sink with the given kind in a MaD flow + * model. + */ +predicate sinkNode(DataFlow::Node node, string kind) { sinkNode(node, kind, _) } + private predicate interpretSummary( - Callable c, string input, string output, string kind, string provenance + Callable c, string input, string output, string kind, string provenance, string model ) { exists( - string namespace, string type, boolean subtypes, string name, string signature, string ext + string namespace, string type, boolean subtypes, string name, string signature, string ext, + QlBuiltins::ExtensionId madId | - summaryModel(namespace, type, subtypes, name, signature, ext, input, output, kind, provenance) and + summaryModel(namespace, type, subtypes, name, signature, ext, input, output, kind, provenance, + madId) and + model = "MaD:" + madId.toString() and c.asFunction() = interpretElement(namespace, type, subtypes, name, signature, ext).asEntity() ) } // adapter class for converting Mad summaries to `SummarizedCallable`s private class SummarizedCallableAdapter extends SummarizedCallable { - SummarizedCallableAdapter() { interpretSummary(this, _, _, _, _) } + SummarizedCallableAdapter() { interpretSummary(this, _, _, _, _, _) } - private predicate relevantSummaryElementManual(string input, string output, string kind) { + private predicate relevantSummaryElementManual( + string input, string output, string kind, string model + ) { exists(Provenance provenance | - interpretSummary(this, input, output, kind, provenance) and + interpretSummary(this, input, output, kind, provenance, model) and provenance.isManual() ) } - private predicate relevantSummaryElementGenerated(string input, string output, string kind) { + private predicate relevantSummaryElementGenerated( + string input, string output, string kind, string model + ) { exists(Provenance provenance | - interpretSummary(this, input, output, kind, provenance) and + interpretSummary(this, input, output, kind, provenance, model) and provenance.isGenerated() ) } - override predicate propagatesFlow(string input, string output, boolean preservesValue) { + override predicate propagatesFlow( + string input, string output, boolean preservesValue, string model + ) { exists(string kind | - this.relevantSummaryElementManual(input, output, kind) + this.relevantSummaryElementManual(input, output, kind, model) or - not this.relevantSummaryElementManual(_, _, _) and - this.relevantSummaryElementGenerated(input, output, kind) + not this.relevantSummaryElementManual(_, _, _, _) and + this.relevantSummaryElementGenerated(input, output, kind, model) | if kind = "value" then preservesValue = true else preservesValue = false ) } override predicate hasProvenance(Provenance provenance) { - interpretSummary(this, _, _, _, provenance) + interpretSummary(this, _, _, _, provenance, _) } } diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll index f750214010f..ebfc08a797b 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll @@ -4,6 +4,7 @@ private import DataFlowImplCommon private import ContainerFlow private import FlowSummaryImpl as FlowSummaryImpl private import semmle.go.dataflow.FlowSummary as FlowSummary +private import semmle.go.dataflow.ExternalFlow private import codeql.util.Unit import DataFlowNodes::Private @@ -410,6 +411,10 @@ predicate lambdaCall(DataFlowCall call, LambdaCallKind kind, Node receiver) { no /** Extra data-flow steps needed for lambda flow analysis. */ predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preservesValue) { none() } +predicate knownSourceModel(Node source, string model) { sourceNode(source, _, model) } + +predicate knownSinkModel(Node sink, string model) { sinkNode(sink, _, model) } + /** * Holds if flow is allowed to pass from parameter `p` and back to itself as a * side-effect, resulting in a summary from `p` to itself. diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll index 5a76d8592a8..9f76e7c7c95 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -104,7 +104,7 @@ predicate isReturnedWithError(Node node) { * (intra-procedural) step. */ predicate localFlowStep(Node nodeFrom, Node nodeTo) { - simpleLocalFlowStep(nodeFrom, nodeTo) + simpleLocalFlowStep(nodeFrom, nodeTo, _) or // Simple flow through library code is included in the exposed local // step relation, even though flow is technically inter-procedural @@ -118,14 +118,16 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) { * data flow. It may have less flow than the `localFlowStep` predicate. */ cached -predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) { - basicLocalFlowStep(nodeFrom, nodeTo) +predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo, string model) { + basicLocalFlowStep(nodeFrom, nodeTo) and + model = "" or // step through function model - any(FunctionModel m).flowStep(nodeFrom, nodeTo) + any(FunctionModel m).flowStep(nodeFrom, nodeTo) and + model = "FunctionModel" or FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom.(FlowSummaryNode).getSummaryNode(), - nodeTo.(FlowSummaryNode).getSummaryNode(), true) + nodeTo.(FlowSummaryNode).getSummaryNode(), true, model) } /** diff --git a/go/ql/lib/semmle/go/dataflow/internal/ExternalFlowExtensions.qll b/go/ql/lib/semmle/go/dataflow/internal/ExternalFlowExtensions.qll index 24a8ddf4b8d..327cd65df87 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/ExternalFlowExtensions.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/ExternalFlowExtensions.qll @@ -7,7 +7,7 @@ */ extensible predicate sourceModel( string package, string type, boolean subtypes, string name, string signature, string ext, - string output, string kind, string provenance + string output, string kind, string provenance, QlBuiltins::ExtensionId madId ); /** @@ -15,7 +15,7 @@ extensible predicate sourceModel( */ extensible predicate sinkModel( string package, string type, boolean subtypes, string name, string signature, string ext, - string input, string kind, string provenance + string input, string kind, string provenance, QlBuiltins::ExtensionId madId ); /** @@ -23,5 +23,5 @@ extensible predicate sinkModel( */ extensible predicate summaryModel( string package, string type, boolean subtypes, string name, string signature, string ext, - string input, string output, string kind, string provenance + string input, string output, string kind, string provenance, QlBuiltins::ExtensionId madId ); diff --git a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll index 1a51af5c3fb..d5f7d72b2d0 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll @@ -104,12 +104,14 @@ module SourceSinkInterpretationInput implements * `output`, kind `kind`, and provenance `provenance`. */ predicate sourceElement( - SourceOrSinkElement e, string output, string kind, Public::Provenance provenance + SourceOrSinkElement e, string output, string kind, Public::Provenance provenance, string model ) { exists( - string package, string type, boolean subtypes, string name, string signature, string ext + string package, string type, boolean subtypes, string name, string signature, string ext, + QlBuiltins::ExtensionId madId | - sourceModel(package, type, subtypes, name, signature, ext, output, kind, provenance) and + sourceModel(package, type, subtypes, name, signature, ext, output, kind, provenance, madId) and + model = "MaD:" + madId.toString() and e = interpretElement(package, type, subtypes, name, signature, ext) ) } @@ -119,12 +121,14 @@ module SourceSinkInterpretationInput implements * `input`, kind `kind` and provenance `provenance`. */ predicate sinkElement( - SourceOrSinkElement e, string input, string kind, Public::Provenance provenance + SourceOrSinkElement e, string input, string kind, Public::Provenance provenance, string model ) { exists( - string package, string type, boolean subtypes, string name, string signature, string ext + string package, string type, boolean subtypes, string name, string signature, string ext, + QlBuiltins::ExtensionId madId | - sinkModel(package, type, subtypes, name, signature, ext, input, kind, provenance) and + sinkModel(package, type, subtypes, name, signature, ext, input, kind, provenance, madId) and + model = "MaD:" + madId.toString() and e = interpretElement(package, type, subtypes, name, signature, ext) ) } diff --git a/go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll b/go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll index 77b9d867121..9c645bac1da 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll @@ -28,7 +28,7 @@ predicate localExprTaint(Expr src, Expr sink) { */ predicate localTaintStep(DataFlow::Node src, DataFlow::Node sink) { DataFlow::localFlowStep(src, sink) or - localAdditionalTaintStep(src, sink) or + localAdditionalTaintStep(src, sink, _) or // Simple flow through library code is included in the exposed local // step relation, even though flow is technically inter-procedural FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(src, sink, _) @@ -86,18 +86,24 @@ class AdditionalTaintStep extends Unit { * Holds if the additional step from `pred` to `succ` should be included in all * global taint flow configurations. */ -predicate localAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - referenceStep(pred, succ) or - elementWriteStep(pred, succ) or - fieldReadStep(pred, succ) or - elementStep(pred, succ) or - tupleStep(pred, succ) or - stringConcatStep(pred, succ) or - sliceStep(pred, succ) or - any(FunctionModel fm).taintStep(pred, succ) or - any(AdditionalTaintStep a).step(pred, succ) or +predicate localAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ, string model) { + ( + referenceStep(pred, succ) or + elementWriteStep(pred, succ) or + fieldReadStep(pred, succ) or + elementStep(pred, succ) or + tupleStep(pred, succ) or + stringConcatStep(pred, succ) or + sliceStep(pred, succ) + ) and + model = "" + or + any(FunctionModel fm).taintStep(pred, succ) and model = "FunctionModel" + or + any(AdditionalTaintStep a).step(pred, succ) and model = "AdditionalTaintStep" + or FlowSummaryImpl::Private::Steps::summaryLocalStep(pred.(DataFlowPrivate::FlowSummaryNode) - .getSummaryNode(), succ.(DataFlowPrivate::FlowSummaryNode).getSummaryNode(), false) + .getSummaryNode(), succ.(DataFlowPrivate::FlowSummaryNode).getSummaryNode(), false, model) } /** @@ -207,8 +213,8 @@ abstract class FunctionModel extends Function { * Holds if the additional step from `src` to `sink` should be included in all * global taint flow configurations. */ -predicate defaultAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) { - localAdditionalTaintStep(src, sink) +predicate defaultAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink, string model) { + localAdditionalTaintStep(src, sink, model) } /** diff --git a/go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll b/go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll index 787bf40e9cc..c63cf71737b 100644 --- a/go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll +++ b/go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll @@ -159,7 +159,7 @@ module NetHttp { | this = call.getASyntacticArgument() and callable = call.getACalleeIncludingExternals() and - callable.propagatesFlow(input, output, _) + callable.propagatesFlow(input, output, _, _) | // A modeled function conveying taint from some input to the response writer, // e.g. `io.Copy(responseWriter, someTaintedReader)` diff --git a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll index 3447f2f6dbe..df417397b13 100644 --- a/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll +++ b/go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll @@ -509,7 +509,7 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf ) { // Create additional flow steps for `BarrierFlowStateTransformer`s state2 = node2.(BarrierFlowStateTransformer).transform(state1) and - DataFlow::simpleLocalFlowStep(node1, node2) + DataFlow::simpleLocalFlowStep(node1, node2, _) } }