From 31336b09c43394bb022161707c558ecf45c486d8 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Wed, 1 Mar 2023 12:53:13 +0100
Subject: [PATCH] add summary for the `Array` method on `Kernel`

---
 .../codeql/ruby/frameworks/core/Kernel.qll    | 31 +++++++++++++++++++
 .../UnsafeCodeConstruction.expected           | 18 +++++++++++
 .../UnsafeCodeConstruction/impl/unsafeCode.rb |  8 ++---
 3 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll
index bf487904134..f7e6817a3fc 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll
@@ -197,4 +197,35 @@ module Kernel {
       super.getMethodName() = ["autoload", "autoload?"]
     }
   }
+
+  /**
+   * 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`).
+      (
+        result = API::getTopLevelMember("Kernel").getAMethodCall(_).asExpr().getExpr()
+        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
+    }
+  }
 }
diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected
index 51529e3855e..1b55dd3c999 100644
--- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected
+++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected
@@ -11,6 +11,13 @@ edges
 | impl/unsafeCode.rb:39:14:39:14 | x :  | impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] :  |
 | impl/unsafeCode.rb:47:15:47:15 | x :  | impl/unsafeCode.rb:49:9:49:12 | #{...} |
 | impl/unsafeCode.rb:54:21:54:21 | x :  | impl/unsafeCode.rb:55:22:55:22 | x |
+| impl/unsafeCode.rb:59:21:59:21 | x :  | impl/unsafeCode.rb:60:17:60:17 | x :  |
+| impl/unsafeCode.rb:59:24:59:24 | y :  | impl/unsafeCode.rb:63:30:63:30 | y :  |
+| impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] :  | impl/unsafeCode.rb:61:10:61:12 | arr |
+| impl/unsafeCode.rb:60:17:60:17 | x :  | impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] :  |
+| impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] :  | impl/unsafeCode.rb:63:13:63:42 | call to join :  |
+| impl/unsafeCode.rb:63:13:63:42 | call to join :  | impl/unsafeCode.rb:64:10:64:13 | arr2 |
+| impl/unsafeCode.rb:63:30:63:30 | y :  | impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] :  |
 nodes
 | impl/unsafeCode.rb:2:12:2:17 | target :  | semmle.label | target :  |
 | impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} |
@@ -32,6 +39,15 @@ nodes
 | impl/unsafeCode.rb:49:9:49:12 | #{...} | semmle.label | #{...} |
 | impl/unsafeCode.rb:54:21:54:21 | x :  | semmle.label | x :  |
 | impl/unsafeCode.rb:55:22:55:22 | x | semmle.label | x |
+| impl/unsafeCode.rb:59:21:59:21 | x :  | semmle.label | x :  |
+| impl/unsafeCode.rb:59:24:59:24 | y :  | semmle.label | y :  |
+| impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] :  | semmle.label | call to Array [element 0] :  |
+| impl/unsafeCode.rb:60:17:60:17 | x :  | semmle.label | x :  |
+| impl/unsafeCode.rb:61:10:61:12 | arr | semmle.label | arr |
+| impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] :  | semmle.label | call to Array [element 1] :  |
+| impl/unsafeCode.rb:63:13:63:42 | call to join :  | semmle.label | call to join :  |
+| impl/unsafeCode.rb:63:30:63:30 | y :  | semmle.label | y :  |
+| impl/unsafeCode.rb:64:10:64:13 | arr2 | semmle.label | arr2 |
 subpaths
 #select
 | impl/unsafeCode.rb:3:17:3:25 | #{...} | impl/unsafeCode.rb:2:12:2:17 | target :  | impl/unsafeCode.rb:3:17:3:25 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:2:12:2:17 | target | library input | impl/unsafeCode.rb:3:5:3:27 | call to eval | interpreted as code |
@@ -43,3 +59,5 @@ subpaths
 | impl/unsafeCode.rb:44:10:44:12 | arr | impl/unsafeCode.rb:37:15:37:15 | x :  | impl/unsafeCode.rb:44:10:44:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:37:15:37:15 | x | library input | impl/unsafeCode.rb:44:5:44:24 | call to eval | interpreted as code |
 | impl/unsafeCode.rb:49:9:49:12 | #{...} | impl/unsafeCode.rb:47:15:47:15 | x :  | impl/unsafeCode.rb:49:9:49:12 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:47:15:47:15 | x | library input | impl/unsafeCode.rb:51:5:51:13 | call to eval | interpreted as code |
 | impl/unsafeCode.rb:55:22:55:22 | x | impl/unsafeCode.rb:54:21:54:21 | x :  | impl/unsafeCode.rb:55:22:55:22 | x | This string concatenation which depends on $@ is later $@. | impl/unsafeCode.rb:54:21:54:21 | x | library input | impl/unsafeCode.rb:56:5:56:13 | call to eval | interpreted as code |
+| impl/unsafeCode.rb:61:10:61:12 | arr | impl/unsafeCode.rb:59:21:59:21 | x :  | impl/unsafeCode.rb:61:10:61:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:59:21:59:21 | x | library input | impl/unsafeCode.rb:61:5:61:23 | call to eval | interpreted as code |
+| impl/unsafeCode.rb:64:10:64:13 | arr2 | impl/unsafeCode.rb:59:24:59:24 | y :  | impl/unsafeCode.rb:64:10:64:13 | arr2 | This array which depends on $@ is later $@. | impl/unsafeCode.rb:59:24:59:24 | y | library input | impl/unsafeCode.rb:64:5:64:25 | call to eval | interpreted as code |
diff --git a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb
index 6ce7baee02d..b69048f6328 100644
--- a/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb
+++ b/ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb
@@ -57,10 +57,10 @@ class Foobar
   end
 
   def join_indirect(x, y) 
-    arr = Array("foo = ", x)
-    eval(arr.join(" ")) # NOT OK - but not currently flagged by the query
+    arr = Array(x)
+    eval(arr.join(" ")) # NOT OK
 
-    arr2 = [Array("foo = ", y).join(" ")]
-    eval(arr2.join("\n")) # NOT OK - but not currently flagged by the query
+    arr2 = [Array(["foo = ", y]).join(" ")]
+    eval(arr2.join("\n")) # NOT OK
   end
 end