Merge pull request #12672 from hvitved/ruby/implicit-array-reads-at-sinks

Ruby: Allow for implicit array reads at all sinks during taint tracking

Author: hvitved

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

@@ -17,7 +17,10 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
  * of `c` at sinks and inputs to additional taint steps.
  */
 bindingset[node]
-predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) { none() }
+predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) {
+  exists(node) and
+  c.isElementOfTypeOrUnknown("int")
+}
 
 private CfgNodes::ExprNodes::VariableWriteAccessCfgNode variablesInPattern(
   CfgNodes::ExprNodes::CasePatternCfgNode p

ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll

@@ -201,11 +201,8 @@ private predicate sqlFragmentArgumentInner(DataFlow::CallNode call, DataFlow::No
 }
 
 private predicate sqlFragmentArgument(DataFlow::CallNode call, DataFlow::Node sink) {
-  exists(DataFlow::Node arg |
-    sqlFragmentArgumentInner(call, arg) and
-    sink = [arg, arg.(DataFlow::ArrayLiteralNode).getElement(0)] and
-    unsafeSqlExpr(sink.asExpr().getExpr())
-  )
+  sqlFragmentArgumentInner(call, sink) and
+  unsafeSqlExpr(sink.asExpr().getExpr())
 }
 
 // An expression that, if tainted by unsanitized input, should not be used as

ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll

@@ -210,9 +210,28 @@ module Array {
     }
   }
 
+  private predicate isKnownRange(RangeLiteral rl, int start, int end) {
+    (
+      // Either an explicit, positive beginning index...
+      start = rl.getBegin().getConstantValue().getInt() and start >= 0
+      or
+      // Or a begin-less one, since `..n` is equivalent to `0..n`
+      not exists(rl.getBegin()) and start = 0
+    ) and
+    // There must be an explicit end. An end-less range like `2..` is not
+    // treated as a known range, since we don't track the length of the array.
+    exists(int e | e = rl.getEnd().getConstantValue().getInt() and e >= 0 |
+      rl.isInclusive() and end = e
+      or
+      rl.isExclusive() and end = e - 1
+    )
+  }
+
   /**
    * A call to `[]` with an unknown argument, which could be either an index or
-   * a range.
+   * a range. To avoid spurious flow, we are going to ignore the possibility
+   * that the argument might be a range (unless it is an explicit range literal,
+   * see `ElementReferenceRangeReadUnknownSummary`).
    */
   private class ElementReferenceReadUnknownSummary extends ElementReferenceReadSummary {
     ElementReferenceReadUnknownSummary() {
@@ -223,7 +242,7 @@ module Array {
 
     override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
       input = "Argument[self].Element[any]" and
-      output = ["ReturnValue", "ReturnValue.Element[?]"] and
+      output = "ReturnValue" and
       preservesValue = true
     }
   }
@@ -242,24 +261,8 @@ module Array {
       )
       or
       mc.getNumberOfArguments() = 1 and
-      exists(RangeLiteral rl |
-        rl = mc.getArgument(0) and
-        (
-          // Either an explicit, positive beginning index...
-          start = rl.getBegin().getConstantValue().getInt() and start >= 0
-          or
-          // Or a begin-less one, since `..n` is equivalent to `0..n`
-          not exists(rl.getBegin()) and start = 0
-        ) and
-        // There must be an explicit end. An end-less range like `2..` is not
-        // treated as a known range, since we don't track the length of the array.
-        exists(int e | e = rl.getEnd().getConstantValue().getInt() and e >= 0 |
-          rl.isInclusive() and end = e
-          or
-          rl.isExclusive() and end = e - 1
-        ) and
-        this = methodName + "(" + start + ".." + end + ")"
-      )
+      isKnownRange(mc.getArgument(0), start, end) and
+      this = methodName + "(" + start + ".." + end + ")"
     }
 
     override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
@@ -291,12 +294,7 @@ module Array {
         )
         or
         mc.getNumberOfArguments() = 1 and
-        exists(RangeLiteral rl | rl = mc.getArgument(0) |
-          exists(rl.getBegin()) and
-          not exists(int b | b = rl.getBegin().getConstantValue().getInt() and b >= 0)
-          or
-          not exists(int e | e = rl.getEnd().getConstantValue().getInt() and e >= 0)
-        )
+        mc.getArgument(0) = any(RangeLiteral range | not isKnownRange(range, _, _))
       )
     }
 

ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll

@@ -32,12 +32,6 @@ deprecated class Configuration extends TaintTracking::Configuration {
   override DataFlow::FlowFeature getAFeature() {
     result instanceof DataFlow::FeatureHasSourceCallContext
   }
-
-  override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
-    // allow implicit reads of array elements
-    this.isSink(node) and
-    set.isElementOfTypeOrUnknown("int")
-  }
 }
 
 private module UnsafeCodeConstructionConfig implements DataFlow::ConfigSig {

ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll

@@ -34,12 +34,6 @@ deprecated class Configuration extends TaintTracking::Configuration {
   override DataFlow::FlowFeature getAFeature() {
     result instanceof DataFlow::FeatureHasSourceCallContext
   }
-
-  override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
-    // allow implicit reads of array elements
-    this.isSink(node) and
-    set.isElementOfTypeOrUnknown("int")
-  }
 }
 
 private module UnsafeShellCommandConstructionConfig implements DataFlow::ConfigSig {