Merge pull request #10862 from erik-krogh/unsafeCodeConstruction

Rb: Add an `unsafe-code-construction` query

Author: erik-krogh

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

@@ -0,0 +1,119 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * code constructed from library input vulnerabilities, as
+ * well as extension points for adding your own.
+ */
+
+private import ruby
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.frameworks.core.Gem::Gem as Gem
+private import codeql.ruby.Concepts as Concepts
+
+/**
+ * Module containing sources, sinks, and sanitizers for code constructed from library input.
+ */
+module UnsafeCodeConstruction {
+  /** A source for code constructed from library input vulnerabilities. */
+  abstract class Source extends DataFlow::Node { }
+
+  /** An input parameter to a gem seen as a source. */
+  private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
+    LibraryInputAsSource() {
+      this = Gem::getALibraryInput() and
+      not this.getName() = "code"
+    }
+  }
+
+  /** A sink for code constructed from library input vulnerabilities. */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets the node where the unsafe code is executed.
+     */
+    abstract DataFlow::Node getCodeSink();
+
+    /**
+     * Gets the type of sink.
+     */
+    string getSinkType() { result = "code construction" }
+  }
+
+  /** Gets a node that is eventually executed as code at `codeExec`. */
+  DataFlow::Node getANodeExecutedAsCode(Concepts::CodeExecution codeExec) {
+    result = getANodeExecutedAsCode(TypeTracker::TypeBackTracker::end(), codeExec)
+  }
+
+  import codeql.ruby.typetracking.TypeTracker as TypeTracker
+
+  /** Gets a node that is eventually executed as code at `codeExec`, type-tracked with `t`. */
+  private DataFlow::LocalSourceNode getANodeExecutedAsCode(
+    TypeTracker::TypeBackTracker t, Concepts::CodeExecution codeExec
+  ) {
+    t.start() and
+    result = codeExec.getCode().getALocalSource() and
+    codeExec.runsArbitraryCode() // methods like `Object.send` is benign here, because of the string-construction the attacker cannot control the entire method name
+    or
+    exists(TypeTracker::TypeBackTracker t2 |
+      result = getANodeExecutedAsCode(t2, codeExec).backtrack(t2, t)
+    )
+  }
+
+  /**
+   * A string constructed using a `.join(...)` call, where the resulting string ends up being executed as code.
+   */
+  class ArrayJoin extends Sink {
+    Concepts::CodeExecution s;
+
+    ArrayJoin() {
+      exists(DataFlow::CallNode call |
+        call.getMethodName() = "join" and
+        call.getNumberOfArguments() = 1 and // any string. E.g. ";" or "\n".
+        call = getANodeExecutedAsCode(s) and
+        this = call.getReceiver()
+      )
+    }
+
+    override DataFlow::Node getCodeSink() { result = s }
+
+    override string getSinkType() { result = "array" }
+  }
+
+  /**
+   * A string constructed from a string-literal (e.g. `"foo #{sink}"`),
+   * where the resulting string ends up being executed as a code.
+   */
+  class StringInterpolationAsSink extends Sink {
+    Concepts::CodeExecution s;
+
+    StringInterpolationAsSink() {
+      exists(Ast::StringlikeLiteral lit |
+        any(DataFlow::Node n | n.asExpr().getExpr() = lit) = getANodeExecutedAsCode(s) and
+        this.asExpr().getExpr() = lit.getComponent(_)
+      )
+    }
+
+    override DataFlow::Node getCodeSink() { result = s }
+
+    override string getSinkType() { result = "string interpolation" }
+  }
+
+  import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
+
+  /**
+   * A string constructed from a printf-style call,
+   * where the resulting string ends up being executed as a code.
+   */
+  class TaintedFormatStringAsSink extends Sink {
+    Concepts::CodeExecution s;
+
+    TaintedFormatStringAsSink() {
+      exists(TaintedFormat::PrintfStyleCall call |
+        call = getANodeExecutedAsCode(s) and
+        this = [call.getFormatArgument(_), call.getFormatString()]
+      )
+    }
+
+    override DataFlow::Node getCodeSink() { result = s }
+
+    override string getSinkType() { result = "string format" }
+  }
+}

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

@@ -0,0 +1,49 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about code
+ * constructed from library input vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if `Configuration` is needed,
+ * otherwise `UnsafeCodeConstructionCustomizations` should be imported instead.
+ */
+
+import codeql.ruby.DataFlow
+import UnsafeCodeConstructionCustomizations::UnsafeCodeConstruction
+private import codeql.ruby.TaintTracking
+private import codeql.ruby.dataflow.BarrierGuards
+
+/**
+ * A taint-tracking configuration for detecting code constructed from library input vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "UnsafeShellCommandConstruction" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node instanceof StringConstCompareBarrier or
+    node instanceof StringConstArrayInclusionCallBarrier
+  }
+
+  // override to require the path doesn't have unmatched return steps
+  override DataFlow::FlowFeature getAFeature() {
+    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(_)
+    )
+  }
+}

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

@@ -67,7 +67,7 @@ module UnsafeShellCommandConstruction {
    */
   class StringInterpolationAsSink extends Sink {
     Concepts::SystemCommandExecution s;
-    Ast::StringLiteral lit;
+    Ast::StringlikeLiteral lit;
 
     StringInterpolationAsSink() {
       isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and

ruby/ql/src/change-notes/2022-11-25-unsafe-code-construction.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/unsafe-code-construction`, to detect libraries that unsafely construct code from their inputs.

ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.qhelp

@@ -0,0 +1,111 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+When a library function dynamically constructs code in a potentially unsafe way, 
+it's important to document to clients of the library that the function should only be
+used with trusted inputs.
+
+If the function is not documented as being potentially unsafe, then a client may
+incorrectly use inputs containing unsafe code fragments, and thereby leave the
+client vulnerable to code-injection attacks.
+</p>
+
+</overview>
+
+<recommendation>
+<p>
+Properly document library functions that construct code from unsanitized 
+inputs, or avoid constructing code in the first place.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows two methods implemented using <code>eval</code>: a simple
+deserialization routine and a getter method.
+If untrusted inputs are used with these methods,
+then an attacker might be able to execute arbitrary code on the system. 
+</p>
+
+<sample src="examples/UnsafeCodeConstruction.rb" />
+
+<p>
+To avoid this problem, either properly document that the function is potentially
+unsafe, or use an alternative solution such as <code>JSON.parse</code> or another library 
+that does not allow arbitrary code to be executed.
+</p>
+
+<sample src="examples/UnsafeCodeConstructionSafe.rb" />
+
+</example>
+
+<example>
+<p>
+As another example, consider the below code which dynamically constructs 
+a class that has a getter method with a custom name.
+</p>
+
+<sample src="examples/UnsafeCodeConstruction2.rb" />
+
+<p>
+The example dynamically constructs a string which is then executed using <code>module_eval</code>.  
+This code will break if the specified name is not a valid Ruby identifier, and 
+if the value is controlled by an attacker, then this could lead to code-injection.
+</p>
+
+<p>
+A more robust implementation, that is also immune to code-injection, 
+can be made by using <code>module_eval</code> with a block and using <code>define_method</code>
+to define the getter method.
+</p>
+
+<sample src="examples/UnsafeCodeConstruction2Safe.rb" />
+</example>
+
+<example>
+<p>
+This example dynamically registers a method on another class which
+forwards its arguments to a target object. This approach uses
+<code>module_eval</code> and string interpolation to construct class variables
+and methods.
+</p>
+
+<sample src="examples/UnsafeCodeConstruction3.rb" />
+
+<p>
+A safer approach is to use <code>class_variable_set</code> and
+<code>class_variable_get</code> along with <code>define_method</code>. String
+interpolation is still used to construct the class variable name, but this is
+safe because <code>class_variable_set</code> is not susceptible to code-injection.
+</p>
+
+<p>
+<code>send</code> is used to dynamically call the method specified by <code>name</code>.  
+This is a more robust alternative than the previous example, because it does not allow
+arbitrary code to be executed, but it does still allow for any method to be called
+on the target object. 
+</p>
+
+<sample src="examples/UnsafeCodeConstruction3Safe.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>
+<li>
+Ruby documentation: <a href="https://docs.ruby-lang.org/en/3.2/Module.html#method-i-define_method">define_method</a>.
+</li>
+<li>
+Ruby documentation: <a href="https://docs.ruby-lang.org/en/3.2/Module.html#method-i-class_variable_set">class_variable_set</a>.
+</li>
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-094/UnsafeCodeConstruction.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Unsafe code constructed from library input
+ * @description Using externally controlled strings to construct code may allow a malicious
+ *              user to execute arbitrary code.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 6.1
+ * @precision medium
+ * @id rb/unsafe-code-construction
+ * @tags security
+ *       external/cwe/cwe-094
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import codeql.ruby.security.UnsafeCodeConstructionQuery
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
+where cfg.hasFlowPath(source, sink) and sinkNode = sink.getNode()
+select sink.getNode(), source, sink,
+  "This " + sinkNode.getSinkType() + " which depends on $@ is later $@.", source.getNode(),
+  "library input", sinkNode.getCodeSink(), "interpreted as code"

ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction.rb

@@ -0,0 +1,10 @@
+module MyLib
+    def unsafeDeserialize(value)
+        eval("foo = #{value}")
+        foo
+    end
+
+    def unsafeGetter(obj, path)
+        eval("obj.#{path}")
+    end
+end

ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2.rb

@@ -0,0 +1,17 @@
+require 'json'
+
+module BadMakeGetter
+  # Makes a class with a method named `getter_name` that returns `val`
+  def self.define_getter_class(getter_name, val)
+    new_class = Class.new
+    new_class.module_eval <<-END
+      def #{getter_name}
+        #{JSON.dump(val)}
+      end
+    END
+    new_class
+  end
+end
+
+one = BadMakeGetter.define_getter_class(:one, "foo")
+puts "One is #{one.new.one}"

ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction2Safe.rb

@@ -0,0 +1,13 @@
+# Uses `define_method` instead of constructing a string
+module GoodMakeGetter
+  def self.define_getter_class(getter_name, val)
+    new_class = Class.new
+    new_class.module_eval do
+      define_method(getter_name) { val }
+    end
+    new_class
+  end
+end
+
+two = GoodMakeGetter.define_getter_class(:two, "bar")
+puts "Two is #{two.new.two}"

ruby/ql/src/queries/security/cwe-094/examples/UnsafeCodeConstruction3.rb

@@ -0,0 +1,11 @@
+module Invoker
+  def attach(klass, name, target)
+    klass.module_eval <<-CODE
+      @@#{name} = target
+
+      def #{name}(*args)
+        @@#{name}.#{name}(*args)
+      end
+    CODE
+  end
+end