Merge pull request #11478 from erik-krogh/more-shell-taint

Rb: more taint-steps for shell-command-construction

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

@@ -1186,6 +1186,16 @@ module Array {
     }
   }
 
+  private class JoinSummary extends SimpleSummarizedCallable {
+    JoinSummary() { this = ["join"] }
+
+    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+      input = "Argument[self].Element[any]" and
+      output = "ReturnValue" and
+      preservesValue = false
+    }
+  }
+
   private class PushSummary extends SimpleSummarizedCallable {
     // `append` is an alias for `push`
     PushSummary() { this = ["push", "append"] }

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

@@ -197,4 +197,38 @@ module Kernel {
       super.getMethodName() = ["autoload", "autoload?"]
     }
   }
+
+  private import codeql.ruby.ast.internal.Module as Module
+
+  /**
+   * A call to `Array()`, that converts it's singular argument to an array.
+   * This summary is based on https://ruby-doc.org/3.2.1/Kernel.html#method-i-Array
+   */
+  private class KernelArraySummary extends SummarizedCallable {
+    KernelArraySummary() { this = "Array()" }
+
+    override MethodCall getACallSimple() {
+      result.getMethodName() = "Array" and
+      // I have to have a simplified "KernelMethodCall" implementation inlined here, because relying on `UnknownMethodCall` results in non-monotonic recursion (even if using `getACall`).
+      (
+        // similar to `getAStaticArrayCall` from Array.qll
+        Module::resolveConstantReadAccess(result.getReceiver()) = Module::TResolved("Kernel")
+        or
+        result.getReceiver() instanceof SelfVariableAccess
+      )
+    }
+
+    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+      (
+        // already an array
+        input = "Argument[0].WithElement[0..]" and
+        output = "ReturnValue"
+        or
+        // not already an array
+        input = "Argument[0].WithoutElement[0..]" and
+        output = "ReturnValue.Element[0]"
+      ) and
+      preservesValue = true
+    }
+  }
 }

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

@@ -31,19 +31,11 @@ class Configuration extends TaintTracking::Configuration {
     result instanceof DataFlow::FeatureHasSourceCallContext
   }
 
-  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-    // if an array element gets tainted, then we treat the entire array as tainted
-    exists(DataFlow::CallNode call |
-      call.getMethodName() = ["<<", "push", "append"] and
-      call.getReceiver() = succ and
-      pred = call.getArgument(0) and
-      call.getNumberOfArguments() = 1
-    )
-    or
-    exists(DataFlow::CallNode call |
-      call.getMethodName() = "[]" and
-      succ = call and
-      pred = call.getArgument(_)
-    )
+  override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
+    // allow implicit reads of array elements
+    this.isSink(node) and
+    set.isKnownOrUnknownElement(any(DataFlow::Content::KnownElementContent content |
+        content.getIndex().getValueType() = "int"
+      ))
   }
 }

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

@@ -32,4 +32,12 @@ 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.isKnownOrUnknownElement(any(DataFlow::Content::KnownElementContent content |
+        content.getIndex().getValueType() = "int"
+      ))
+  }
 }