Merge pull request github#10760 from hvitved/ruby/regex-taint-flow-restrict

Ruby: Restrict regexp taint flow to `String` summaries

Author: hvitved

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": [

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll

@@ -735,6 +735,9 @@ class SummaryNode extends NodeImpl, TSummaryNode {
 
   SummaryNode() { this = TSummaryNode(c, state) }
 
+  /** Gets the summarized callable that this node belongs to. */
+  FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = c }
+
   override CfgScope getCfgScope() { none() }
 
   override DataFlowCallable getEnclosingCallable() { result.asLibraryCallable() = c }

ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingImpl.qll

@@ -3,7 +3,7 @@
 private import codeql.ruby.AST
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.DataFlow
-private import codeql.ruby.dataflow.FlowSummary
+private import codeql.ruby.dataflow.FlowSummary as FlowSummary
 private import codeql.ruby.dataflow.internal.DataFlowDispatch
 private import codeql.ruby.controlflow.CfgNodes
 private import codeql.ruby.Regexp as RE
@@ -107,6 +107,18 @@ module String {
     preservesValue = false
   }
 
+  /** A `String` callable with a flow summary. */
+  abstract class SummarizedCallable extends FlowSummary::SummarizedCallable {
+    bindingset[this]
+    SummarizedCallable() { any() }
+  }
+
+  abstract private class SimpleSummarizedCallable extends SummarizedCallable,
+    FlowSummary::SimpleSummarizedCallable {
+    bindingset[this]
+    SimpleSummarizedCallable() { any() }
+  }
+
   private class NewSummary extends SummarizedCallable {
     NewSummary() { this = "String.new" }
 

ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforregexp/TaintTrackingParameter.qll

@@ -1,10 +1,15 @@
 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
+private import codeql.ruby.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
+private import codeql.ruby.dataflow.FlowSummary as FlowSummary
+private import codeql.ruby.frameworks.core.String
 
 class RegExpConfiguration extends Configuration {
   RegExpConfiguration() { this = "RegExpConfiguration" }
@@ -20,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
@@ -31,6 +36,24 @@ class RegExpConfiguration extends Configuration {
       mce.getReceiver() = trackRegexpType()
     )
   }
+
+  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()
+  }
 }
 
 private DataFlow::LocalSourceNode trackRegexpType(TypeTracker t) {