From 2b75562037dcda454e3ffe0d98adc1619de2ee6d Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 11 Oct 2022 11:39:45 +0200 Subject: [PATCH] Ruby: Use `DataFlow::Configuration` in `RegExpConfiguration.qll` --- config/identical-files.json | 1 - .../TaintTrackingImpl.qll | 191 ------------------ .../TaintTrackingParameter.qll | 6 - .../regexp/internal/RegExpConfiguration.qll | 28 ++- 4 files changed, 21 insertions(+), 205 deletions(-) delete mode 100644 ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingImpl.qll delete mode 100644 ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingParameter.qll diff --git a/config/identical-files.json b/config/identical-files.json index c168f540f1e..832fac7741c 100644 --- a/config/identical-files.json +++ b/config/identical-files.json @@ -70,7 +70,6 @@ "python/ql/lib/semmle/python/dataflow/new/internal/tainttracking3/TaintTrackingImpl.qll", "python/ql/lib/semmle/python/dataflow/new/internal/tainttracking4/TaintTrackingImpl.qll", "ruby/ql/lib/codeql/ruby/dataflow/internal/tainttracking1/TaintTrackingImpl.qll", - "ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingImpl.qll", "swift/ql/lib/codeql/swift/dataflow/internal/tainttracking1/TaintTrackingImpl.qll" ], "DataFlow Java/C++/C#/Python Consistency checks": [ diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingImpl.qll deleted file mode 100644 index bf937b6de31..00000000000 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingImpl.qll +++ /dev/null @@ -1,191 +0,0 @@ -/** - * Provides an implementation of global (interprocedural) taint tracking. - * This file re-exports the local (intraprocedural) taint-tracking analysis - * from `TaintTrackingParameter::Public` and adds a global analysis, mainly - * exposed through the `Configuration` class. For some languages, this file - * exists in several identical copies, allowing queries to use multiple - * `Configuration` classes that depend on each other without introducing - * mutual recursion among those configurations. - */ - -import TaintTrackingParameter::Public -private import TaintTrackingParameter::Private - -/** - * A configuration of interprocedural taint tracking analysis. This defines - * sources, sinks, and any other configurable aspect of the analysis. Each - * use of the taint tracking library must define its own unique extension of - * this abstract class. - * - * A taint-tracking configuration is a special data flow configuration - * (`DataFlow::Configuration`) that allows for flow through nodes that do not - * necessarily preserve values but are still relevant from a taint tracking - * perspective. (For example, string concatenation, where one of the operands - * is tainted.) - * - * To create a configuration, extend this class with a subclass whose - * characteristic predicate is a unique singleton string. For example, write - * - * ```ql - * class MyAnalysisConfiguration extends TaintTracking::Configuration { - * MyAnalysisConfiguration() { this = "MyAnalysisConfiguration" } - * // Override `isSource` and `isSink`. - * // Optionally override `isSanitizer`. - * // Optionally override `isSanitizerIn`. - * // Optionally override `isSanitizerOut`. - * // Optionally override `isSanitizerGuard`. - * // Optionally override `isAdditionalTaintStep`. - * } - * ``` - * - * Then, to query whether there is flow between some `source` and `sink`, - * write - * - * ```ql - * exists(MyAnalysisConfiguration cfg | cfg.hasFlow(source, sink)) - * ``` - * - * Multiple configurations can coexist, but it is unsupported to depend on - * another `TaintTracking::Configuration` or a `DataFlow::Configuration` in the - * overridden predicates that define sources, sinks, or additional steps. - * Instead, the dependency should go to a `TaintTracking2::Configuration` or a - * `DataFlow2::Configuration`, `DataFlow3::Configuration`, etc. - */ -abstract class Configuration extends DataFlow::Configuration { - bindingset[this] - Configuration() { any() } - - /** - * Holds if `source` is a relevant taint source. - * - * The smaller this predicate is, the faster `hasFlow()` will converge. - */ - // overridden to provide taint-tracking specific qldoc - override predicate isSource(DataFlow::Node source) { none() } - - /** - * Holds if `source` is a relevant taint source with the given initial - * `state`. - * - * The smaller this predicate is, the faster `hasFlow()` will converge. - */ - // overridden to provide taint-tracking specific qldoc - override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { none() } - - /** - * Holds if `sink` is a relevant taint sink - * - * The smaller this predicate is, the faster `hasFlow()` will converge. - */ - // overridden to provide taint-tracking specific qldoc - override predicate isSink(DataFlow::Node sink) { none() } - - /** - * Holds if `sink` is a relevant taint sink accepting `state`. - * - * The smaller this predicate is, the faster `hasFlow()` will converge. - */ - // overridden to provide taint-tracking specific qldoc - override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { none() } - - /** Holds if the node `node` is a taint sanitizer. */ - predicate isSanitizer(DataFlow::Node node) { none() } - - final override predicate isBarrier(DataFlow::Node node) { - this.isSanitizer(node) or - defaultTaintSanitizer(node) - } - - /** - * Holds if the node `node` is a taint sanitizer when the flow state is - * `state`. - */ - predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) { none() } - - final override predicate isBarrier(DataFlow::Node node, DataFlow::FlowState state) { - this.isSanitizer(node, state) - } - - /** Holds if taint propagation into `node` is prohibited. */ - predicate isSanitizerIn(DataFlow::Node node) { none() } - - final override predicate isBarrierIn(DataFlow::Node node) { this.isSanitizerIn(node) } - - /** Holds if taint propagation out of `node` is prohibited. */ - predicate isSanitizerOut(DataFlow::Node node) { none() } - - final override predicate isBarrierOut(DataFlow::Node node) { this.isSanitizerOut(node) } - - /** - * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead. - * - * Holds if taint propagation through nodes guarded by `guard` is prohibited. - */ - deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() } - - deprecated final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) { - this.isSanitizerGuard(guard) - } - - /** - * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead. - * - * Holds if taint propagation through nodes guarded by `guard` is prohibited - * when the flow state is `state`. - */ - deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) { - none() - } - - deprecated final override predicate isBarrierGuard( - DataFlow::BarrierGuard guard, DataFlow::FlowState state - ) { - this.isSanitizerGuard(guard, state) - } - - /** - * Holds if taint may propagate from `node1` to `node2` in addition to the normal data-flow and taint steps. - */ - predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { none() } - - final override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - this.isAdditionalTaintStep(node1, node2) or - defaultAdditionalTaintStep(node1, node2) - } - - /** - * Holds if taint may propagate from `node1` to `node2` in addition to the normal data-flow and taint steps. - * This step is only applicable in `state1` and updates the flow state to `state2`. - */ - predicate isAdditionalTaintStep( - DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, - DataFlow::FlowState state2 - ) { - none() - } - - final override predicate isAdditionalFlowStep( - DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, - DataFlow::FlowState state2 - ) { - this.isAdditionalTaintStep(node1, state1, node2, state2) - } - - override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) { - ( - this.isSink(node) or - this.isSink(node, _) or - this.isAdditionalTaintStep(node, _) or - this.isAdditionalTaintStep(node, _, _, _) - ) and - defaultImplicitTaintRead(node, c) - } - - /** - * Holds if taint may flow from `source` to `sink` for this configuration. - */ - // overridden to provide taint-tracking specific qldoc - override predicate hasFlow(DataFlow::Node source, DataFlow::Node sink) { - super.hasFlow(source, sink) - } -} diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingParameter.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingParameter.qll deleted file mode 100644 index 77949aa5ccf..00000000000 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingParameter.qll +++ /dev/null @@ -1,6 +0,0 @@ -import codeql.ruby.dataflow.internal.TaintTrackingPublic as Public - -module Private { - import codeql.ruby.dataflow.internal.DataFlowImplForRegExp as DataFlow - import codeql.ruby.dataflow.internal.TaintTrackingPrivate -} diff --git a/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll b/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll index 310844fbbd9..3c451b15b78 100644 --- a/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll +++ b/ruby/ql/lib/codeql/ruby/regexp/internal/RegExpConfiguration.qll @@ -1,8 +1,9 @@ private import codeql.ruby.Regexp -private import codeql.ruby.ast.Literal as Ast +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.dataflow.internal.tainttrackingforregexp.TaintTrackingImpl +private import codeql.ruby.dataflow.internal.DataFlowImplForRegExp private import codeql.ruby.typetracking.TypeTracker private import codeql.ruby.ApiGraphs private import codeql.ruby.dataflow.internal.DataFlowPrivate as DataFlowPrivate @@ -24,7 +25,7 @@ class RegExpConfiguration extends Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof RegExpInterpretation::Range } - override predicate isSanitizer(DataFlow::Node node) { + override predicate isBarrier(DataFlow::Node node) { exists(DataFlow::CallNode mce | mce.getMethodName() = ["match", "match?"] | // receiver of https://ruby-doc.org/core-2.4.0/String.html#method-i-match node = mce.getReceiver() and @@ -34,11 +35,24 @@ class RegExpConfiguration extends Configuration { node = mce.getArgument(0) and mce.getReceiver() = trackRegexpType() ) - or - // only include taint flow through `String` summaries - FlowSummaryImpl::Private::Steps::summaryLocalStep(_, node, false) and - not node.(DataFlowPrivate::SummaryNode).getSummarizedCallable() instanceof + } + + override predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { + // include taint flow through `String` summaries, + FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, false) and + nodeFrom.(DataFlowPrivate::SummaryNode).getSummarizedCallable() instanceof String::SummarizedCallable + or + // string concatenations, and + exists(CfgNodes::ExprNodes::OperationCfgNode op | + op = nodeTo.asExpr() and + op.getAnOperand() = nodeFrom.asExpr() and + op.getExpr().(Ast::BinaryOperation).getOperator() = "+" + ) + or + // string interpolations + nodeFrom.asExpr() = + nodeTo.asExpr().(CfgNodes::ExprNodes::StringlikeLiteralCfgNode).getAComponent() } }