Merge pull request #321 from github/hmac-more-eval

Identify more instances of code injection

Author: hmac

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

@@ -12,10 +12,7 @@ class KernelMethodCall extends MethodCall {
   KernelMethodCall() {
     this = API::getTopLevelMember("Kernel").getAMethodCall(_).asExpr().getExpr()
     or
-    // we assume that if there's no obvious target for this method call
-    // and the method name matches a Kernel method, then it is a Kernel method call.
-    // TODO: ApiGraphs should ideally handle this case
-    not exists(this.(Call).getATarget()) and
+    this instanceof UnknownMethodCall and
     (
       this.getReceiver() instanceof Self and isPrivateKernelMethod(this.getMethodName())
       or
@@ -58,6 +55,45 @@ private predicate isPrivateKernelMethod(string method) {
     ]
 }
 
+/**
+ * Instance methods on `BasicObject`, which are available to all classes.
+ */
+class BasicObjectInstanceMethodCall extends UnknownMethodCall {
+  BasicObjectInstanceMethodCall() {
+    this.getMethodName() in [
+        "equal?", "instance_eval", "instance_exec", "method_missing", "singleton_method_added",
+        "singleton_method_removed", "singleton_method_undefined"
+      ]
+  }
+}
+
+/**
+ * Instance methods on `Object`, which are available to all classes except `BasicObject`.
+ */
+class ObjectInstanceMethodCall extends UnknownMethodCall {
+  ObjectInstanceMethodCall() {
+    this.getMethodName() in [
+        "!~", "<=>", "===", "=~", "callable_methods", "define_singleton_method", "display",
+        "do_until", "do_while", "dup", "enum_for", "eql?", "extend", "f", "freeze", "h", "hash",
+        "inspect", "instance_of?", "instance_variable_defined?", "instance_variable_get",
+        "instance_variable_set", "instance_variables", "is_a?", "itself", "kind_of?",
+        "matching_methods", "method", "method_missing", "methods", "nil?", "object_id",
+        "private_methods", "protected_methods", "public_method", "public_methods", "public_send",
+        "remove_instance_variable", "respond_to?", "respond_to_missing?", "send",
+        "shortest_abbreviation", "singleton_class", "singleton_method", "singleton_methods",
+        "taint", "tainted?", "to_enum", "to_s", "trust", "untaint", "untrust", "untrusted?"
+      ]
+  }
+}
+
+/**
+ * Method calls which have no known target.
+ * These will typically be calls to methods inherited from a superclass.
+ */
+class UnknownMethodCall extends MethodCall {
+  UnknownMethodCall() { not exists(this.(Call).getATarget()) }
+}
+
 /**
  * A system command executed via subshell literal syntax.
  * E.g.
@@ -275,3 +311,42 @@ class SendCallCodeExecution extends CodeExecution::Range {
 
   override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
 }
+
+/**
+ * A call to `BasicObject#instance_eval`, which executes its argument as Ruby code.
+ */
+class InstanceEvalCallCodeExecution extends CodeExecution::Range {
+  BasicObjectInstanceMethodCall methodCall;
+
+  InstanceEvalCallCodeExecution() {
+    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "instance_eval"
+  }
+
+  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
+}
+
+/**
+ * A call to `Module#class_eval`, which executes its argument as Ruby code.
+ */
+class ClassEvalCallCodeExecution extends CodeExecution::Range {
+  UnknownMethodCall methodCall;
+
+  ClassEvalCallCodeExecution() {
+    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "class_eval"
+  }
+
+  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
+}
+
+/**
+ * A call to `Module#module_eval`, which executes its argument as Ruby code.
+ */
+class ModuleEvalCallCodeExecution extends CodeExecution::Range {
+  UnknownMethodCall methodCall;
+
+  ModuleEvalCallCodeExecution() {
+    this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "module_eval"
+  }
+
+  override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
+}

ql/test/query-tests/security/cwe-094/CodeInjection.expected

@@ -1,10 +1,16 @@
 edges
 | CodeInjection.rb:3:12:3:17 | call to params :  | CodeInjection.rb:6:10:6:13 | code |
+| CodeInjection.rb:3:12:3:17 | call to params :  | CodeInjection.rb:15:20:15:23 | code |
+| CodeInjection.rb:3:12:3:17 | call to params :  | CodeInjection.rb:18:21:18:24 | code |
 nodes
 | CodeInjection.rb:3:12:3:17 | call to params :  | semmle.label | call to params :  |
 | CodeInjection.rb:6:10:6:13 | code | semmle.label | code |
 | CodeInjection.rb:9:10:9:15 | call to params | semmle.label | call to params |
+| CodeInjection.rb:15:20:15:23 | code | semmle.label | code |
+| CodeInjection.rb:18:21:18:24 | code | semmle.label | code |
 subpaths
 #select
 | CodeInjection.rb:6:10:6:13 | code | CodeInjection.rb:3:12:3:17 | call to params :  | CodeInjection.rb:6:10:6:13 | code | This code execution depends on $@. | CodeInjection.rb:3:12:3:17 | call to params | a user-provided value |
 | CodeInjection.rb:9:10:9:15 | call to params | CodeInjection.rb:9:10:9:15 | call to params | CodeInjection.rb:9:10:9:15 | call to params | This code execution depends on $@. | CodeInjection.rb:9:10:9:15 | call to params | a user-provided value |
+| CodeInjection.rb:15:20:15:23 | code | CodeInjection.rb:3:12:3:17 | call to params :  | CodeInjection.rb:15:20:15:23 | code | This code execution depends on $@. | CodeInjection.rb:3:12:3:17 | call to params | a user-provided value |
+| CodeInjection.rb:18:21:18:24 | code | CodeInjection.rb:3:12:3:17 | call to params :  | CodeInjection.rb:18:21:18:24 | code | This code execution depends on $@. | CodeInjection.rb:3:12:3:17 | call to params | a user-provided value |

ql/test/query-tests/security/cwe-094/CodeInjection.rb

@@ -10,6 +10,15 @@ def create
 
     # GOOD
     Foo.new.bar(code)
+
+    # BAD
+    Foo.class_eval(code)
+
+    # BAD
+    Foo.module_eval(code)
+
+    # GOOD
+    Bar.class_eval(code)
   end
 
   def update
@@ -27,3 +36,9 @@ def bar(x)
     eval(x)
   end
 end
+
+class Bar
+  def self.class_eval(x)
+    true
+  end
+end