Merge pull request #11909 from erik-krogh/concatCode

Rb: recognize string concatenations as sinks for unsafe-code-construction

ruby/ql/lib/codeql/ruby/ast/Operation.qll

@@ -158,6 +158,27 @@ class AddExpr extends BinaryArithmeticOperation, TAddExpr {
   final override string getAPrimaryQlClass() { result = "AddExpr" }
 }
 
+/**
+ * A series of add expressions, e.g. `1 + 2 + 3`.
+ * This class is used to represent the root of such a series, and
+ * the `getALeaf` predicate can be used to get the leaf nodes.
+ */
+class AddExprRoot extends AddExpr {
+  AddExprRoot() { not this.getParent() instanceof AddExpr }
+
+  private AstNode getALeafOrAdd() {
+    result = this.getAChild()
+    or
+    result = this.getALeafOrAdd().(AddExpr).getAChild()
+  }
+
+  /** Gets a leaf node of this add expression. */
+  AstNode getALeaf() {
+    result = this.getALeafOrAdd() and
+    not result instanceof AddExpr
+  }
+}
+
 /**
  * A subtract expression.
  * ```rb

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

@@ -96,6 +96,25 @@ module UnsafeCodeConstruction {
     override string getSinkType() { result = "string interpolation" }
   }
 
+  /**
+   * A component of a string-concatenation (e.g. `"foo " + sink`),
+   * where the resulting string ends up being executed as a code.
+   */
+  class StringConcatAsSink extends Sink {
+    Concepts::CodeExecution s;
+
+    StringConcatAsSink() {
+      exists(Ast::AddExprRoot add |
+        any(DataFlow::Node n | n.asExpr().getExpr() = add) = getANodeExecutedAsCode(s) and
+        this.asExpr().getExpr() = add.getALeaf()
+      )
+    }
+
+    override DataFlow::Node getCodeSink() { result = s }
+
+    override string getSinkType() { result = "string concatenation" }
+  }
+
   import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
 
   /**

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

@@ -81,6 +81,26 @@ module UnsafeShellCommandConstruction {
     override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = lit }
   }
 
+  /**
+   * A component of a string-concatenation (e.g. `"foo " + sink`),
+   * where the resulting string ends up being executed as a shell command.
+   */
+  class StringConcatAsSink extends Sink {
+    Concepts::SystemCommandExecution s;
+    Ast::AddExprRoot add;
+
+    StringConcatAsSink() {
+      isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = add), s) and
+      this.asExpr().getExpr() = add.getALeaf()
+    }
+
+    override DataFlow::Node getCommandExecution() { result = s }
+
+    override string describe() { result = "string concatenation" }
+
+    override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = add }
+  }
+
   /**
    * A string constructed using a `.join(" ")` call, where the resulting string ends up being executed as a shell command.
    */

ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected

@@ -11,6 +11,7 @@ edges
 | impl/unsafeShell.rb:47:16:47:21 | target :  | impl/unsafeShell.rb:48:19:48:27 | #{...} |
 | impl/unsafeShell.rb:51:17:51:17 | x :  | impl/unsafeShell.rb:52:14:52:14 | x |
 | impl/unsafeShell.rb:51:17:51:17 | x :  | impl/unsafeShell.rb:54:29:54:29 | x |
+| impl/unsafeShell.rb:57:21:57:21 | x :  | impl/unsafeShell.rb:58:23:58:23 | x |
 nodes
 | impl/sub/notImported.rb:2:12:2:17 | target :  | semmle.label | target :  |
 | impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
@@ -35,6 +36,8 @@ nodes
 | impl/unsafeShell.rb:51:17:51:17 | x :  | semmle.label | x :  |
 | impl/unsafeShell.rb:52:14:52:14 | x | semmle.label | x |
 | impl/unsafeShell.rb:54:29:54:29 | x | semmle.label | x |
+| impl/unsafeShell.rb:57:21:57:21 | x :  | semmle.label | x :  |
+| impl/unsafeShell.rb:58:23:58:23 | x | semmle.label | x |
 subpaths
 #select
 | impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target :  | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command |
@@ -49,3 +52,4 @@ subpaths
 | impl/unsafeShell.rb:48:14:48:28 | "cat #{...}" | impl/unsafeShell.rb:47:16:47:21 | target :  | impl/unsafeShell.rb:48:19:48:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:47:16:47:21 | target | library input | impl/unsafeShell.rb:48:5:48:34 | call to popen | shell command |
 | impl/unsafeShell.rb:52:14:52:24 | call to join | impl/unsafeShell.rb:51:17:51:17 | x :  | impl/unsafeShell.rb:52:14:52:14 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:52:5:52:30 | call to popen | shell command |
 | impl/unsafeShell.rb:54:14:54:40 | call to join | impl/unsafeShell.rb:51:17:51:17 | x :  | impl/unsafeShell.rb:54:29:54:29 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:54:5:54:46 | call to popen | shell command |
+| impl/unsafeShell.rb:58:14:58:23 | ... + ... | impl/unsafeShell.rb:57:21:57:21 | x :  | impl/unsafeShell.rb:58:23:58:23 | x | This string concatenation which depends on $@ is later used in a $@. | impl/unsafeShell.rb:57:21:57:21 | x | library input | impl/unsafeShell.rb:58:5:58:29 | call to popen | shell command |

ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb

@@ -53,4 +53,8 @@ class Foobar2
 
     IO.popen(["foo", "bar", x].join(' '), "w") # NOT OK
   end
+
+  def string_concat(x) 
+    IO.popen("cat " + x, "w") # NOT OK
+  end
 end

ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected

@@ -7,6 +7,7 @@ edges
 | impl/unsafeCode.rb:37:15:37:15 | x :  | impl/unsafeCode.rb:40:10:40:12 | arr |
 | impl/unsafeCode.rb:37:15:37:15 | x :  | impl/unsafeCode.rb:44:10:44:12 | arr |
 | 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 |
 nodes
 | impl/unsafeCode.rb:2:12:2:17 | target :  | semmle.label | target :  |
 | impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} |
@@ -23,6 +24,8 @@ nodes
 | impl/unsafeCode.rb:44:10:44:12 | arr | semmle.label | arr |
 | impl/unsafeCode.rb:47:15:47:15 | x :  | semmle.label | x :  |
 | 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 |
 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 |
@@ -33,3 +36,4 @@ subpaths
 | impl/unsafeCode.rb:40:10:40:12 | arr | impl/unsafeCode.rb:37:15:37:15 | x :  | impl/unsafeCode.rb:40:10:40:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:37:15:37:15 | x | library input | impl/unsafeCode.rb:40:5:40:24 | call to eval | interpreted as code |
 | 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 |

ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb

@@ -50,4 +50,9 @@ class Foobar
     HERE
     eval(foo) # NOT OK
   end
+
+  def string_concat(x)
+    foo = "foo = " + x
+    eval(foo) # NOT OK
+  end
 end