Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 1 addition & 5 deletions config/identical-files.json
Original file line number Diff line number Diff line change
Expand Up @@ -462,10 +462,6 @@
"ruby/ql/lib/codeql/ruby/security/internal/SensitiveDataHeuristics.qll",
"swift/ql/lib/codeql/swift/security/internal/SensitiveDataHeuristics.qll"
],
"TypeTracker": [
"python/ql/lib/semmle/python/dataflow/new/internal/TypeTracker.qll",
"ruby/ql/lib/codeql/ruby/typetracking/TypeTracker.qll"
],
"SummaryTypeTracker": [
"python/ql/lib/semmle/python/dataflow/new/internal/SummaryTypeTracker.qll",
"ruby/ql/lib/codeql/ruby/typetracking/internal/SummaryTypeTracker.qll"
Expand Down Expand Up @@ -534,4 +530,4 @@
"python/ql/test/experimental/dataflow/model-summaries/InlineTaintTest.ext.yml",
"python/ql/test/experimental/dataflow/model-summaries/NormalDataflowTest.ext.yml"
]
}
}
8 changes: 8 additions & 0 deletions ruby/ql/consistency-queries/TypeTrackingConsistency.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import codeql.ruby.DataFlow
import codeql.ruby.typetracking.internal.TypeTrackingImpl

private module ConsistencyChecksInput implements ConsistencyChecksInputSig {
predicate unreachableNodeExclude(DataFlow::Node n) { n instanceof DataFlow::PostUpdateNode }
}

import ConsistencyChecks<ConsistencyChecksInput>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Ruby now makes use of the shared type tracking library, exposed as `codeql.ruby.typetracking.TypeTracking`. The existing type tracking library, `codeql.ruby.typetracking.TypeTracker`, has consequently been deprecated.
17 changes: 7 additions & 10 deletions ruby/ql/lib/codeql/ruby/ApiGraphs.qll
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@
private import codeql.ruby.AST
private import codeql.ruby.DataFlow
private import codeql.ruby.typetracking.ApiGraphShared
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.TypeTrackerSpecific as TypeTrackerSpecific
private import codeql.ruby.typetracking.internal.TypeTrackingImpl
private import codeql.ruby.controlflow.CfgNodes
private import codeql.ruby.dataflow.internal.DataFlowPrivate as DataFlowPrivate
private import codeql.ruby.dataflow.internal.DataFlowDispatch as DataFlowDispatch
Expand Down Expand Up @@ -1050,9 +1049,9 @@ module API {

/** INTERNAL USE ONLY. */
module Internal {
private module Shared = ApiGraphShared<SharedArg>;
private module MkShared = ApiGraphShared<SharedArg>;

import Shared
import MkShared

/** Gets the API node corresponding to the module/class object for `mod`. */
bindingset[mod]
Expand Down Expand Up @@ -1093,7 +1092,7 @@ module API {
private predicate needsSinkNode(DataFlow::Node node) {
node instanceof DataFlowPrivate::ArgumentNode
or
TypeTrackerSpecific::basicStoreStep(node, _, _)
TypeTrackingInput::storeStep(node, _, _)
or
node = any(DataFlow::CallableNode callable).getAReturnNode()
or
Expand Down Expand Up @@ -1203,18 +1202,16 @@ module API {

cached
predicate contentEdge(Node pred, DataFlow::Content content, Node succ) {
exists(
DataFlow::Node object, DataFlow::Node value, TypeTrackerSpecific::TypeTrackerContent c
|
TypeTrackerSpecific::basicLoadStep(object, value, c) and
exists(DataFlow::Node object, DataFlow::Node value, DataFlow::ContentSet c |
TypeTrackingInput::loadStep(object, value, c) and
content = c.getAStoreContent() and
not c.isSingleton(any(DataFlow::Content::AttributeNameContent k)) and
// `x -> x.foo` with content "foo"
pred = getForwardOrBackwardEndNode(getALocalSourceStrict(object)) and
succ = getForwardStartNode(value)
or
// Based on `object.c = value` generate `object -> value` with content `c`
TypeTrackerSpecific::basicStoreStep(value, object, c) and
TypeTrackingInput::storeStep(value, object, c) and
content = c.getAStoreContent() and
pred = getForwardOrBackwardEndNode(getALocalSourceStrict(object)) and
succ = MkSinkNode(value)
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/dataflow/FlowSummary.qll
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import codeql.ruby.AST
private import codeql.ruby.CFG
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.TypeTracking
import codeql.ruby.DataFlow
private import internal.FlowSummaryImpl as Impl
private import internal.DataFlowDispatch
Expand Down
13 changes: 6 additions & 7 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
private import codeql.ruby.AST
private import codeql.ruby.CFG
private import DataFlowPrivate
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.TypeTrackerSpecific as TypeTrackerSpecific
private import codeql.ruby.typetracking.internal.TypeTrackingImpl
private import codeql.ruby.ast.internal.Module
private import FlowSummaryImpl as FlowSummaryImpl
private import FlowSummaryImplSpecific as FlowSummaryImplSpecific
Expand Down Expand Up @@ -657,7 +656,7 @@ private module TrackInstanceInput implements CallGraphConstruction::InputSig {
predicate stepNoCall(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, StepSummary summary) {
// We exclude steps into `self` parameters. For those, we instead rely on the type of
// the enclosing module
StepSummary::smallstepNoCall(nodeFrom, nodeTo, summary) and
smallStepNoCall(nodeFrom, nodeTo, summary) and
isNotSelf(nodeTo)
or
// We exclude steps into type checked variables. For those, we instead rely on the
Expand All @@ -667,7 +666,7 @@ private module TrackInstanceInput implements CallGraphConstruction::InputSig {
}

predicate stepCall(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, StepSummary summary) {
StepSummary::smallstepCall(nodeFrom, nodeTo, summary)
smallStepCall(nodeFrom, nodeTo, summary)
}

class StateProj = Unit;
Expand Down Expand Up @@ -941,7 +940,7 @@ private module TrackSingletonMethodOnInstanceInput implements CallGraphConstruct
RelevantCall call, DataFlow::Node arg, DataFlow::ParameterNode p,
CfgNodes::ExprCfgNode nodeFromPreExpr
|
TypeTrackerSpecific::callStep(call, arg, p) and
callStep(call, arg, p) and
nodeTo.getPreUpdateNode() = arg and
summary.toString() = "return" and
(
Expand All @@ -965,13 +964,13 @@ private module TrackSingletonMethodOnInstanceInput implements CallGraphConstruct
}

predicate stepNoCall(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, StepSummary summary) {
StepSummary::smallstepNoCall(nodeFrom, nodeTo, summary)
smallStepNoCall(nodeFrom, nodeTo, summary)
or
localFlowStep(nodeFrom, nodeTo, summary)
}

predicate stepCall(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, StepSummary summary) {
StepSummary::smallstepCall(nodeFrom, nodeTo, summary)
smallStepCall(nodeFrom, nodeTo, summary)
or
paramReturnFlow(nodeFrom, nodeTo, summary)
}
Expand Down
34 changes: 19 additions & 15 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ module LocalFlow {
SsaImpl::lastRefBeforeRedefExt(def, bb, i, next.getDefinitionExt()) and
exprFrom = bb.getNode(i) and
exprFrom.getExpr() instanceof VariableReadAccess and
exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr()]
exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode().asExpr()]
)
}

Expand Down Expand Up @@ -159,7 +159,7 @@ module LocalFlow {
firstReadExt(def, nodeTo.asExpr())
or
// Flow from post-update read to next read
localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode(), nodeTo)
or
// Flow into phi (read) SSA definition node from def
localFlowSsaInputFromDef(nodeFrom, def, nodeTo)
Expand Down Expand Up @@ -456,7 +456,7 @@ private predicate splatArgumentAt(CfgNodes::ExprNodes::CallCfgNode c, int pos) {
/** A collection of cached types and predicates to be evaluated in the same stage. */
cached
private module Cached {
private import codeql.ruby.typetracking.TypeTrackerSpecific as TypeTrackerSpecific
private import codeql.ruby.typetracking.internal.TypeTrackingImpl

cached
newtype TNode =
Expand Down Expand Up @@ -575,14 +575,10 @@ private module Cached {
VariableCapture::flowInsensitiveStep(nodeFrom, nodeTo)
}

/** Holds if `n` wraps an SSA definition without ingoing flow. */
private predicate entrySsaDefinition(SsaDefinitionExtNode n) {
n = LocalFlow::getParameterDefNode(_)
or
exists(SsaImpl::DefinitionExt def | def = n.getDefinitionExt() |
def instanceof Ssa::SelfDefinition
or
def instanceof Ssa::CapturedEntryDefinition
)
n.getDefinitionExt() =
any(SsaImpl::WriteDefinition def | not def.(Ssa::WriteDefinition).assigns(_))
}

pragma[nomagic]
Expand All @@ -599,6 +595,16 @@ private module Cached {
)
}

private predicate isStoreTargetNode(Node n) {
TypeTrackingInput::storeStep(_, n, _)
or
TypeTrackingInput::loadStoreStep(_, n, _, _)
or
TypeTrackingInput::withContentStepImpl(_, n, _)
or
TypeTrackingInput::withoutContentStepImpl(_, n, _)
}

cached
predicate isLocalSourceNode(Node n) {
n instanceof TSourceParameterNode
Expand All @@ -614,11 +620,9 @@ private module Cached {
entrySsaDefinition(n) and
not LocalFlow::localFlowSsaParamInput(_, n)
or
TypeTrackerSpecific::basicStoreStep(_, n, _)
or
TypeTrackerSpecific::basicLoadStep(_, n, _)
isStoreTargetNode(n)
or
TypeTrackerSpecific::basicLoadStoreStep(_, n, _, _)
TypeTrackingInput::loadStep(_, n, _)
}

cached
Expand All @@ -633,7 +637,7 @@ private module Cached {
TElementContentOfTypeContent(string type, Boolean includeUnknown) {
type = any(Content::KnownElementContent content).getIndex().getValueType()
} or
TNoContentSet() // Only used by type-tracking
deprecated TNoContentSet() // Only used by type-tracking

cached
class TContentSet =
Expand Down
35 changes: 17 additions & 18 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@ private import codeql.ruby.AST
private import DataFlowDispatch
private import DataFlowPrivate
private import codeql.ruby.CFG
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.internal.TypeTrackingImpl
private import codeql.ruby.dataflow.SSA
private import FlowSummaryImpl as FlowSummaryImpl
private import SsaImpl as SsaImpl
private import codeql.ruby.ApiGraphs

/**
Expand Down Expand Up @@ -245,7 +244,7 @@ class LocalSourceNode extends Node {

/** Holds if this `LocalSourceNode` can flow to `nodeTo` in one or more local flow steps. */
pragma[inline]
predicate flowsTo(Node nodeTo) { hasLocalSource(nodeTo, this) }
predicate flowsTo(Node nodeTo) { flowsTo(this, nodeTo) }

/**
* Gets a node that this node may flow to using one heap and/or interprocedural step.
Expand All @@ -261,14 +260,14 @@ class LocalSourceNode extends Node {
* See `TypeBackTracker` for more details about how to use this.
*/
pragma[inline]
LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) }
LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t = t2.step(result, this) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change correct? Were the parameters of backtrack swapped? We might want to check that all the places where we use backtrack are still correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is because the shared library (which defines step) has the arguments reversed. I decided to keep the order as-is in backtrack.


/**
* Gets a node to which data may flow from this node in zero or
* more local data-flow steps.
*/
pragma[inline]
Node getALocalUse() { hasLocalSource(result, this) }
Node getALocalUse() { flowsTo(this, result) }

/** Gets a method call where this node flows to the receiver. */
CallNode getAMethodCall() { Cached::hasMethodCall(this, result, _) }
Expand Down Expand Up @@ -354,21 +353,21 @@ class PostUpdateNode extends Node instanceof PostUpdateNodeImpl {
Node getPreUpdateNode() { result = super.getPreUpdateNode() }
}

/** An SSA definition, viewed as a node in a data flow graph. */
class SsaDefinitionNode extends Node instanceof SsaDefinitionExtNode {
Ssa::Definition def;

SsaDefinitionNode() { this = TSsaDefinitionExtNode(def) }

/** Gets the underlying SSA definition. */
Ssa::Definition getDefinition() { result = def }

/** Gets the underlying variable. */
Variable getVariable() { result = def.getSourceVariable() }
}

cached
private module Cached {
cached
predicate hasLocalSource(Node sink, Node source) {
// Declaring `source` to be a `SourceNode` currently causes a redundant check in the
// recursive case, so instead we check it explicitly here.
source = sink and
source instanceof LocalSourceNode
or
exists(Node mid |
hasLocalSource(mid, source) and
localFlowStepTypeTracker(mid, sink)
)
}

cached
predicate hasMethodCall(LocalSourceNode source, CallNode call, string name) {
source.flowsTo(call.getReceiver()) and
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/XmlParsing.qll
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
private import codeql.ruby.Concepts
private import codeql.ruby.AST
private import codeql.ruby.DataFlow
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.TypeTracking
private import codeql.ruby.ApiGraphs
private import codeql.ruby.controlflow.CfgNodes as CfgNodes

Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ private import codeql.ruby.AST
private import codeql.ruby.ApiGraphs
private import codeql.ruby.Concepts
private import codeql.ruby.DataFlow
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.TypeTracking
private import Response::Private as RP

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ private import codeql.ruby.ApiGraphs
private import codeql.ruby.Concepts
private import codeql.ruby.controlflow.CfgNodes::ExprNodes
private import codeql.ruby.DataFlow
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.TypeTracking
private import App as A

/** Contains implementation details for modeling `Rack::Response`. */
Expand Down
12 changes: 6 additions & 6 deletions ruby/ql/lib/codeql/ruby/regexp/internal/RegExpTracking.qll
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ private import codeql.ruby.AST as Ast
private import codeql.ruby.CFG
private import codeql.ruby.DataFlow
private import codeql.ruby.controlflow.CfgNodes
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.internal.TypeTrackingImpl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really need to import the Impl library? I think ideally the Impl module is only used when instantiating the library. The modules that are built on top should ideally only use the "public facing" bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, because the code refers to StepSummary, which is considered internal in the new library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I guess we cannot avoid that, can we? Or should StepSummary perhaps be part of the public interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep it internal, at least for now. It is not something I expect we need to use very often.

private import codeql.ruby.ApiGraphs
private import codeql.ruby.Concepts
private import codeql.ruby.dataflow.internal.DataFlowPrivate as DataFlowPrivate
Expand Down Expand Up @@ -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 TypeTrack<TypeTrackInputSig Input> {
private module PrunedTypeTrack<TypeTrackInputSig Input> {
private predicate additionalStep(
DataFlow::LocalSourceNode nodeFrom, DataFlow::LocalSourceNode nodeTo
) {
Expand Down Expand Up @@ -130,10 +130,10 @@ private module TypeTrack<TypeTrackInputSig Input> {
TypeTracker t, DataFlow::LocalSourceNode nodeFrom, DataFlow::LocalSourceNode nodeTo
) {
exists(StepSummary summary |
StepSummary::step(nodeFrom, nodeTo, summary) and
step(nodeFrom, nodeTo, summary) and
reached(nodeFrom, t) and
reached(nodeTo, result) and
result = t.append(summary)
result = append(t, summary)
)
or
additionalStep(nodeFrom, nodeTo) and
Expand Down Expand Up @@ -195,7 +195,7 @@ private module StringTypeTrackInput implements TypeTrackInputSig {
* This is used to figure out where `start` is evaluated as a regular expression against an input string,
* or where `start` is compiled into a regular expression.
*/
private predicate trackStrings = TypeTrack<StringTypeTrackInput>::track/2;
private predicate trackStrings = PrunedTypeTrack<StringTypeTrackInput>::track/2;

/** Holds if `strConst` flows to a regex compilation (tracked by `t`), where the resulting regular expression is stored in `reg`. */
pragma[nomagic]
Expand All @@ -222,7 +222,7 @@ private module RegTypeTrackInput implements TypeTrackInputSig {
* Gets a node that has been tracked from the regular expression `start` to some node.
* This is used to figure out where `start` is executed against an input string.
*/
private predicate trackRegs = TypeTrack<RegTypeTrackInput>::track/2;
private predicate trackRegs = PrunedTypeTrack<RegTypeTrackInput>::track/2;

/** Gets a node that references a regular expression. */
private DataFlow::LocalSourceNode trackRegexpType(TypeTracker t) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
private import codeql.ruby.AST
private import codeql.ruby.DataFlow
private import codeql.ruby.Concepts
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.TypeTracking
private import codeql.ruby.frameworks.Files
private import codeql.ruby.frameworks.core.IO

Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/security/OpenSSL.qll
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ private import internal.CryptoAlgorithmNames
private import codeql.ruby.Concepts
private import codeql.ruby.DataFlow
private import codeql.ruby.ApiGraphs
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.typetracking.TypeTracking

bindingset[algorithmString]
private string algorithmRegex(string algorithmString) {
Expand Down
Loading