Skip to content

Ruby: Use type tracking instead of dataflow in OrmTracking #15830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hvitved wants to merge 1 commit into from
Closed

Ruby: Use type tracking instead of dataflow in OrmTracking #15830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions ruby/ql/lib/codeql/ruby/regexp/internal/RegExpTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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<TypeTrackInputSig Input> {
private module PrunedTypeTrack<TypeTrackInputSig2 Input> {
private predicate additionalStep(
DataFlow::LocalSourceNode nodeFrom, DataFlow::LocalSourceNode nodeTo
) {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
53 changes: 40 additions & 13 deletions ruby/ql/lib/codeql/ruby/security/XSS.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<Config>
// 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<Conf>::Graph<isSink/1>
// /**
// * 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<Config>
}

/** Provides default sources, sinks and sanitizers for detecting stored cross-site scripting (XSS) vulnerabilities. */
Expand All @@ -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())
)
}
}
Expand Down
33 changes: 33 additions & 0 deletions shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,39 @@ module MakeImplCommon<InputSig Lang> {
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<SimpleGlobalInputSig Input> {
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<Inp>
}

/**
* Holds if `source` is a relevant data flow source.
*/
Expand Down
38 changes: 37 additions & 1 deletion shared/typetracking/codeql/typetracking/TypeTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,49 @@ module TypeTracking<TypeTrackingInput I> {

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<TypeTrackInputSig Input> {
private module Inp implements MkImpl::TypeTrackInputSig {
import Input
}

import MkImpl::TypeTrack<Inp>
}

signature predicate endpoint(I::Node node);

/**
* Given a source definition, constructs the default forward type tracking from
* those sources.
*/
module TypeTrack<endpoint/1 source> {
import MkImpl::TypeTrack<source/1>
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<Inp>
}
}
94 changes: 83 additions & 11 deletions shared/typetracking/codeql/typetracking/internal/TypeTrackingImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ private import codeql.typetracking.TypeTracking
module TypeTracking<TypeTrackingInput I> {
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() }
Expand Down Expand Up @@ -246,9 +249,6 @@ module TypeTracking<TypeTrackingInput I> {
returnStep(nodeFrom, nodeTo) and summary = ReturnStep()
}

pragma[inline]
private predicate isLocalSourceNode(LocalSourceNode n) { any() }

cached
predicate standardFlowsTo(Node localSource, Node dst) {
not nonStandardFlowsTo(_, _) and
Expand Down Expand Up @@ -690,22 +690,41 @@ module TypeTracking<TypeTrackingInput I> {
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<endpoint/1 source> {
module TypeTrack<TypeTrackInputSig Input> {
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)
)
}

Expand All @@ -717,19 +736,72 @@ module TypeTracking<TypeTrackingInput I> {
)
}

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)
}

/**
Expand Down Expand Up @@ -779,7 +851,7 @@ module TypeTracking<TypeTrackingInput I> {
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)
)
Expand All @@ -799,7 +871,7 @@ module TypeTracking<TypeTrackingInput I> {
)
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
Expand Down