Merge pull request #293 from github/hmac-code-injection

Add query for Code Injection

Author: hmac

ql/lib/codeql/ruby/Concepts.qll

@@ -461,6 +461,7 @@ class SystemCommandExecution extends DataFlow::Node instanceof SystemCommandExec
   DataFlow::Node getAnArgument() { result = super.getAnArgument() }
 }
 
+/** Provides a class for modeling new operating system command APIs. */
 module SystemCommandExecution {
   /**
    * A data flow node that executes an operating system command, for instance by spawning a new
@@ -477,3 +478,28 @@ module SystemCommandExecution {
     predicate isShellInterpreted(DataFlow::Node arg) { none() }
   }
 }
+
+/**
+ * A data-flow node that dynamically executes Ruby code.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `CodeExecution::Range` instead.
+ */
+class CodeExecution extends DataFlow::Node instanceof CodeExecution::Range {
+  /** Gets the argument that specifies the code to be executed. */
+  DataFlow::Node getCode() { result = super.getCode() }
+}
+
+/** Provides a class for modeling new dynamic code execution APIs. */
+module CodeExecution {
+  /**
+   * A data-flow node that dynamically executes Ruby code.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `CodeExecution` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the argument that specifies the code to be executed. */
+    abstract DataFlow::Node getCode();
+  }
+}

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

@@ -239,3 +239,39 @@ class Open3PipelineCall extends SystemCommandExecution::Range {
     arg.asExpr().getExpr() = methodCall.getAnArgument()
   }
 }
+
+/**
+ * A call to `Kernel.eval`, which executes its argument as Ruby code.
+ * ```ruby
+ * a = 1
+ * Kernel.eval("a = 2")
+ * a # => 2
+ * ```
+ */
+class EvalCallCodeExecution extends CodeExecution::Range {
+  KernelMethodCall methodCall;
+
+  EvalCallCodeExecution() {
+    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "eval"
+  }
+
+  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
+}
+
+/**
+ * A call to `Kernel#send`, which executes its arguments as a Ruby method call.
+ * ```ruby
+ * arr = []
+ * arr.send("push", 1)
+ * arr # => [1]
+ * ```
+ */
+class SendCallCodeExecution extends CodeExecution::Range {
+  KernelMethodCall methodCall;
+
+  SendCallCodeExecution() {
+    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "send"
+  }
+
+  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
+}

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

@@ -0,0 +1,40 @@
+private import ruby
+private import codeql.ruby.DataFlow
+private import codeql.ruby.Concepts
+private import codeql.ruby.Frameworks
+private import codeql.ruby.dataflow.RemoteFlowSources
+private import codeql.ruby.dataflow.BarrierGuards
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "Code injection" vulnerabilities, as well as extension points for
+ * adding your own.
+ */
+module CodeInjection {
+  /**
+   * A data flow source for "Code injection" vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for "Code injection" vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for "Code injection" vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * A call that evaluates its arguments as Ruby code, considered as a flow sink.
+   */
+  class CodeExecutionAsSink extends Sink {
+    CodeExecutionAsSink() { this = any(CodeExecution c).getCode() }
+  }
+}

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

@@ -0,0 +1,29 @@
+/**
+ * Provides a taint-tracking configuration for detecting "Code injection" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if `Configuration` is needed,
+ * otherwise `CodeInjectionCustomizations` should be imported instead.
+ */
+
+import codeql.ruby.DataFlow::DataFlow::PathGraph
+import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+import CodeInjectionCustomizations::CodeInjection
+import codeql.ruby.dataflow.BarrierGuards
+
+/**
+ * A taint-tracking configuration for detecting "Code injection" vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "CodeInjection" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof SanitizerGuard or
+    guard instanceof StringConstCompare or
+    guard instanceof StringConstArrayInclusionCall
+  }
+}

ql/src/queries/security/cwe-094/CodeInjection.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Directly evaluating user input (for example, an HTTP request parameter) as code without first
+sanitizing the input allows an attacker arbitrary code execution. This can occur when user
+input is passed to code that interprets it as an expression to be
+evaluated, using methods such as <code>Kernel.eval</code> or <code>Kernel.send</code>.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Avoid including user input in any expression that may be dynamically evaluated. If user input must
+be included, use context-specific escaping before including it.
+It is important that the correct escaping is used for the type of evaluation that will occur.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows two functions setting a name from a request.
+The first function uses <code>eval</code> to execute the <code>set_name</code> method.
+This is dangerous as it can allow a malicious user to execute arbitrary code on the server.
+For example, the user could supply the value <code>"' + exec('rm -rf') + '"</code>
+to destroy the server's file system.
+The second function calls the <code>set_name</code> method directly and is thus safe.
+
+</p>
+
+<sample src="examples/code_injection.rb" />
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.
+</li>
+<li>
+Wikipedia: <a href="https://en.wikipedia.org/wiki/Code_injection">Code Injection</a>.
+</li>
+</references>
+</qhelp>

ql/src/queries/security/cwe-094/CodeInjection.ql

@@ -0,0 +1,27 @@
+/**
+ * @name Code injection
+ * @description Interpreting unsanitized user input as code allows a malicious user to perform arbitrary
+ *              code execution.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.3
+ * @sub-severity high
+ * @precision high
+ * @id rb/code-injection
+ * @tags security
+ *       external/owasp/owasp-a1
+ *       external/cwe/cwe-094
+ *       external/cwe/cwe-095
+ *       external/cwe/cwe-116
+ */
+
+import ruby
+import codeql.ruby.security.CodeInjectionQuery
+import DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Source sourceNode
+where
+  config.hasFlowPath(source, sink) and
+  sourceNode = source.getNode()
+select sink.getNode(), source, sink, "This code execution depends on $@.", sourceNode,
+  "a user-provided value"

ql/src/queries/security/cwe-094/examples/code_injection.rb

@@ -0,0 +1,17 @@
+class UsersController < ActionController::Base
+  # BAD - Allow user to define code to be run.
+  def create_bad
+    first_name = params[:first_name]
+    eval("set_name(#{first_name})")
+  end
+
+  # GOOD - Call code directly
+  def create_good
+    first_name = params[:first_name]
+    set_name(first_name)
+  end
+
+  def set_name(name)
+    @name = name
+  end
+end

ql/test/library-tests/frameworks/Eval.rb

@@ -0,0 +1,23 @@
+# Uses of eval and send
+
+eval("raise \"error\"")
+send("raise", "error")
+
+a = []
+a.send("raise", "error")
+
+class Foo
+  def eval(x)
+    x + 1
+  end
+
+  def send(*args)
+    2
+  end
+
+  def run
+    eval("exit 1")
+  end
+end
+
+Foo.new.send("exit", 1)

ql/test/library-tests/frameworks/StandardLibrary.expected

@@ -58,3 +58,8 @@ open3PipelineCallExecutions
 | CommandExecution.rb:63:1:63:40 | call to pipeline_w |
 | CommandExecution.rb:64:1:64:44 | call to pipeline_start |
 | CommandExecution.rb:65:1:65:38 | call to pipeline |
+evalCallCodeExecutions
+| Eval.rb:3:1:3:23 | call to eval |
+sendCallCodeExecutions
+| Eval.rb:4:1:4:22 | call to send |
+| Eval.rb:7:1:7:24 | call to send |

ql/test/library-tests/frameworks/StandardLibrary.ql

@@ -13,3 +13,7 @@ query predicate kernelSpawnCallExecutions(KernelSpawnCall c) { any() }
 query predicate open3CallExecutions(Open3Call c) { any() }
 
 query predicate open3PipelineCallExecutions(Open3PipelineCall c) { any() }
+
+query predicate evalCallCodeExecutions(EvalCallCodeExecution e) { any() }
+
+query predicate sendCallCodeExecutions(SendCallCodeExecution e) { any() }