diff --git a/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpTracking.qll b/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpTracking.qll index 5d70b7d164a2..6ad57a4f90a3 100644 --- a/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpTracking.qll +++ b/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpTracking.qll @@ -55,7 +55,7 @@ DataFlow::Node stringSink() { /** Gets a node where regular expressions that flow to the node are used. */ DataFlow::Node regSink() { result = any(RegexExecution exec).getRegex() } -private signature module TypeTrackInputSig { +private signature module TypeTrackInputSig2 { DataFlow::LocalSourceNode start(TypeTracker t, DataFlow::Node start); predicate end(DataFlow::Node n); @@ -67,7 +67,7 @@ private signature module TypeTrackInputSig { * Provides a version of type tracking where we first prune for reachable nodes, * before doing the type tracking computation. */ -private module PrunedTypeTrack { +private module PrunedTypeTrack { private predicate additionalStep( DataFlow::LocalSourceNode nodeFrom, DataFlow::LocalSourceNode nodeTo ) { @@ -162,7 +162,7 @@ private predicate regFromString(DataFlow::LocalSourceNode inputStr, DataFlow::Ca ) } -private module StringTypeTrackInput implements TypeTrackInputSig { +private module StringTypeTrackInput implements TypeTrackInputSig2 { DataFlow::LocalSourceNode start(TypeTracker t, DataFlow::Node start) { start = strStart() and t.start() and result = start } @@ -204,7 +204,7 @@ private predicate regFromStringStart(DataFlow::Node strConst, TypeTracker t, Dat exists(t.continue()) } -private module RegTypeTrackInput implements TypeTrackInputSig { +private module RegTypeTrackInput implements TypeTrackInputSig2 { DataFlow::LocalSourceNode start(TypeTracker t, DataFlow::Node start) { start = regStart() and t.start() and diff --git a/ruby/ql/lib/codeql/ruby/security/XSS.qll b/ruby/ql/lib/codeql/ruby/security/XSS.qll index 26aa90fe8311..964880d0af96 100644 --- a/ruby/ql/lib/codeql/ruby/security/XSS.qll +++ b/ruby/ql/lib/codeql/ruby/security/XSS.qll @@ -279,10 +279,7 @@ module ReflectedXss { } private module OrmTracking { - /** - * A data flow configuration to track flow from finder calls to field accesses. - */ - private module Config implements DataFlow::ConfigSig { + private module Conf implements DataFlow::SimpleGlobalInputSig { predicate isSource(DataFlow::Node source) { // We currently only use ORM instances that come from a call site, so restrict the sources // to calls. This works around a performance issue that would arise from using 'self' as a source @@ -291,17 +288,42 @@ private module OrmTracking { source instanceof OrmInstantiation and source instanceof DataFlow::CallNode } - // Select any call receiver and narrow down later - predicate isSink(DataFlow::Node sink) { sink = any(DataFlow::CallNode c).getReceiver() } - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { Shared::isAdditionalXssFlowStep(node1, node2) } - - predicate isBarrierIn(DataFlow::Node node) { node instanceof DataFlow::SelfParameterNode } + // predicate isBarrierIn(DataFlow::Node node) { node instanceof DataFlow::SelfParameterNode } } - import DataFlow::Global + // private predicate isSource(DataFlow::Node source) { + // // We currently only use ORM instances that come from a call site, so restrict the sources + // // to calls. This works around a performance issue that would arise from using 'self' as a source + // // in ActiveRecord models. Over time, library models should stop relying on OrmInstantiation and instead + // // use API graphs or type-tracking the same way we track other types. + // source instanceof OrmInstantiation and source instanceof DataFlow::CallNode + // } + // Select any call receiver and narrow down later + predicate isSink(DataFlow::Node sink) { sink = any(DataFlow::CallNode c).getReceiver() } + + import DataFlow::SimpleGlobalExt::Graph + // /** + // * A data flow configuration to track flow from finder calls to field accesses. + // */ + // private module Config implements DataFlow::ConfigSig { + // predicate isSource(DataFlow::Node source) { + // // We currently only use ORM instances that come from a call site, so restrict the sources + // // to calls. This works around a performance issue that would arise from using 'self' as a source + // // in ActiveRecord models. Over time, library models should stop relying on OrmInstantiation and instead + // // use API graphs or type-tracking the same way we track other types. + // source instanceof OrmInstantiation and source instanceof DataFlow::CallNode + // } + // // Select any call receiver and narrow down later + // predicate isSink(DataFlow::Node sink) { sink = any(DataFlow::CallNode c).getReceiver() } + // predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + // Shared::isAdditionalXssFlowStep(node1, node2) + // } + // predicate isBarrierIn(DataFlow::Node node) { node instanceof DataFlow::SelfParameterNode } + // } + // import DataFlow::Global } /** Provides default sources, sinks and sanitizers for detecting stored cross-site scripting (XSS) vulnerabilities. */ @@ -322,9 +344,14 @@ module StoredXss { private class OrmFieldAsSource extends Source instanceof DataFlow::CallNode { OrmFieldAsSource() { - exists(DataFlow::CallNode subSrc | - OrmTracking::flow(subSrc, this.getReceiver()) and - subSrc.(OrmInstantiation).methodCallMayAccessField(this.getMethodName()) + // exists(DataFlow::CallNode subSrc | + // OrmTracking::flow(subSrc, this.getReceiver()) and + // subSrc.(OrmInstantiation).methodCallMayAccessField(this.getMethodName()) + // ) + exists(OrmTracking::PathNode src, OrmTracking::PathNode sink | + OrmTracking::flowPath(src, sink) and + sink.getNode() = this.getReceiver() and + src.getNode().(OrmInstantiation).methodCallMayAccessField(this.getMethodName()) ) } } diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll index 21040c2d0d92..f3f985950259 100644 --- a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll +++ b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll @@ -61,6 +61,39 @@ module MakeImplCommon { override string toString() { result = "FeatureEqualSourceSinkCallContext" } } + signature module SimpleGlobalInputSig { + predicate isSource(Node n); + + /** + * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps. + */ + default predicate isAdditionalFlowStep(Node node1, Node node2) { none() } + } + + /** + * 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 SimpleGlobalExt { + private module Inp implements TypeTracking::TypeTrackInputSig { + predicate source = Input::isSource/1; + + predicate isAdditionalLevelStep(Node node1, Node node2) { + Input::isAdditionalFlowStep(pragma[only_bind_into](node1), pragma[only_bind_into](node2)) and + getNodeEnclosingCallable(node1) = getNodeEnclosingCallable(node2) + } + + predicate isAdditionalJumpStep(Node node1, Node node2) { + Input::isAdditionalFlowStep(pragma[only_bind_into](node1), pragma[only_bind_into](node2)) and + getNodeEnclosingCallable(node1) != getNodeEnclosingCallable(node2) + } + } + + import TypeTracking::TypeTrackExt + } + /** * Holds if `source` is a relevant data flow source. */ diff --git a/shared/typetracking/codeql/typetracking/TypeTracking.qll b/shared/typetracking/codeql/typetracking/TypeTracking.qll index 044b672fe85e..238a6d9b4b3f 100644 --- a/shared/typetracking/codeql/typetracking/TypeTracking.qll +++ b/shared/typetracking/codeql/typetracking/TypeTracking.qll @@ -142,6 +142,32 @@ module TypeTracking { module TypeBackTracker = MkImpl::TypeBackTracker; + signature module TypeTrackInputSig { + predicate source(I::Node n); + + /** + * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps. + */ + default predicate isAdditionalLevelStep(I::Node node1, I::Node node2) { none() } + + /** + * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps. + */ + default predicate isAdditionalJumpStep(I::Node node1, I::Node node2) { none() } + } + + /** + * Given a source definition, constructs the default forward type tracking from + * those sources. + */ + module TypeTrackExt { + private module Inp implements MkImpl::TypeTrackInputSig { + import Input + } + + import MkImpl::TypeTrack + } + signature predicate endpoint(I::Node node); /** @@ -149,6 +175,16 @@ module TypeTracking { * those sources. */ module TypeTrack { - import MkImpl::TypeTrack + private predicate sourceAlias = source/1; + + private module Inp implements MkImpl::TypeTrackInputSig { + predicate source = sourceAlias/1; + + predicate isAdditionalLevelStep(I::Node node1, I::Node node2) { none() } + + predicate isAdditionalJumpStep(I::Node node1, I::Node node2) { none() } + } + + import MkImpl::TypeTrack } } diff --git a/shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll b/shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll index f5efc6c6b563..cd218ded54b0 100644 --- a/shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll +++ b/shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll @@ -17,6 +17,9 @@ private import codeql.typetracking.TypeTracking module TypeTracking { private import I + pragma[inline] + private predicate isLocalSourceNode(LocalSourceNode n) { any() } + signature module ConsistencyChecksInputSig { /** Holds if `n` should be excluded from the consistency test `unreachableNode`. */ default predicate unreachableNodeExclude(Node n) { none() } @@ -246,9 +249,6 @@ module TypeTracking { returnStep(nodeFrom, nodeTo) and summary = ReturnStep() } - pragma[inline] - private predicate isLocalSourceNode(LocalSourceNode n) { any() } - cached predicate standardFlowsTo(Node localSource, Node dst) { not nonStandardFlowsTo(_, _) and @@ -690,22 +690,41 @@ module TypeTracking { TypeBackTracker end() { result.end() } } - signature predicate endpoint(Node node); + signature predicate endpoint(Node n); + + signature module TypeTrackInputSig { + predicate source(Node n); + + /** + * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps. + */ + predicate isAdditionalLevelStep(Node node1, Node node2); + + /** + * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps. + */ + predicate isAdditionalJumpStep(Node node1, Node node2); + } /** * Given a source definition, constructs the default forward type tracking from * those sources. */ - module TypeTrack { + module TypeTrack { + private import Input + pragma[nomagic] private predicate sourceSimpleLocalSmallSteps(Node src, Node n) { source(src) and not src instanceof LocalSourceNode and src = n or - exists(Node mid | - sourceSimpleLocalSmallSteps(src, mid) and + exists(Node mid | sourceSimpleLocalSmallSteps(src, mid) | simpleLocalSmallStep(mid, n) + or + isAdditionalLevelStep(mid, n) + or + isAdditionalJumpStep(mid, n) ) } @@ -717,19 +736,72 @@ module TypeTracking { ) } + predicate additionalStepFlowsTo(Node add, Node dst) { + isAdditionalLevelStep(_, add) and + dst = add + or + isAdditionalJumpStep(_, add) and + dst = add + or + exists(Node mid | + additionalStepFlowsTo(add, mid) and + simpleLocalSmallStep(mid, dst) + ) + } + + predicate additionalLevelStep(LocalSourceNode node1, LocalSourceNode node2) { + exists(Node n1, Node n2 | + flowsTo(node1, n1) and + isAdditionalLevelStep(n1, n2) and + additionalStepFlowsTo(n2, node2) + ) + } + + predicate additionalJumpStep(LocalSourceNode node1, LocalSourceNode node2) { + exists(Node n1, Node n2 | + flowsTo(node1, n1) and + isAdditionalJumpStep(n1, n2) and + additionalStepFlowsTo(n2, node2) + ) + } + + pragma[inline] + TypeTracker stepExt(TypeTracker tt, LocalSourceNode nodeFrom, LocalSourceNode nodeTo) { + result = tt.step(nodeFrom, nodeTo) + or + result = append(tt, LevelStep()) and + additionalLevelStep(nodeFrom, nodeTo) + or + result = append(tt, JumpStep()) and + additionalJumpStep(nodeFrom, nodeTo) + } + private Node flow(TypeTracker tt) { tt.start() and source(result) or firstStep(tt, _, result) or - exists(TypeTracker ttMid | tt = ttMid.step(flow(ttMid), result)) + exists(TypeTracker ttMid | tt = stepExt(ttMid, flow(ttMid), result)) + } + + private predicate flowsToExt(Node localSource, Node dst) { + not nonStandardFlowsTo(_, _) and + // explicit type check in base case to avoid repeated type tests in recursive case + isLocalSourceNode(localSource) and + dst = localSource + or + exists(Node mid | flowsToExt(localSource, mid) | + simpleLocalSmallStep(mid, dst) or + isAdditionalLevelStep(mid, dst) or + isAdditionalJumpStep(mid, dst) + ) } /** * Holds if a source flows to `n`. */ predicate flowsTo(Node n) { - flowsTo(flow(TypeTracker::end()), n) or sourceSimpleLocalSmallSteps(_, n) + flowsToExt(flow(TypeTracker::end()), n) or sourceSimpleLocalSmallSteps(_, n) } /** @@ -779,7 +851,7 @@ module TypeTracking { private predicate edgeCand(Node n1, TypeTracker tt1, Node n2, TypeTracker tt2) { n1 = flow(tt1) and ( - tt2 = tt1.step(n1, n2) + tt2 = stepExt(tt1, n1, n2) or tt1.start() and firstStep(tt2, n1, n2) ) @@ -799,7 +871,7 @@ module TypeTracking { ) or n1.getTypeTracker().end() and - flowsTo(getNodeMid(n1), getNodeSink(n2)) and + flowsToExt(getNodeMid(n1), getNodeSink(n2)) and getNodeMid(n1) != getNodeSink(n2) or sourceSimpleLocalSmallSteps(n1.getNode(), getNodeSink(n2)) and