From e419fc9599e951f88a5fde9ec95096ed5ca0a178 Mon Sep 17 00:00:00 2001 From: Harry Maclean Date: Tue, 5 Oct 2021 10:24:17 +0100 Subject: [PATCH] Make Code execution query more specific Only the first argument to eval, instance_eval, send, class_send and module_send is interpreted as Ruby code. --- .../ruby/frameworks/StandardLibrary.qll | 20 +++++++++---------- ql/test/library-tests/frameworks/Eval.rb | 7 +++++-- .../frameworks/StandardLibrary.expected | 12 ++++++++--- .../frameworks/StandardLibrary.ql | 17 ++++++++++++++-- .../security/cwe-094/CodeInjection.expected | 12 +++++------ .../security/cwe-094/CodeInjection.rb | 9 +++++++++ 6 files changed, 54 insertions(+), 23 deletions(-) diff --git a/ql/lib/codeql/ruby/frameworks/StandardLibrary.qll b/ql/lib/codeql/ruby/frameworks/StandardLibrary.qll index a66750aa97b..3eb7320c90f 100644 --- a/ql/lib/codeql/ruby/frameworks/StandardLibrary.qll +++ b/ql/lib/codeql/ruby/frameworks/StandardLibrary.qll @@ -277,7 +277,7 @@ class Open3PipelineCall extends SystemCommandExecution::Range { } /** - * A call to `Kernel.eval`, which executes its argument as Ruby code. + * A call to `Kernel.eval`, which executes its first argument as Ruby code. * ```ruby * a = 1 * Kernel.eval("a = 2") @@ -291,11 +291,11 @@ class EvalCallCodeExecution extends CodeExecution::Range { this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "eval" } - override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() } + override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) } } /** - * A call to `Kernel#send`, which executes its arguments as a Ruby method call. + * A call to `Kernel#send`, which executes its first argument as a Ruby method call. * ```ruby * arr = [] * arr.send("push", 1) @@ -309,11 +309,11 @@ class SendCallCodeExecution extends CodeExecution::Range { this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "send" } - override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() } + override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) } } /** - * A call to `BasicObject#instance_eval`, which executes its argument as Ruby code. + * A call to `BasicObject#instance_eval`, which executes its first argument as Ruby code. */ class InstanceEvalCallCodeExecution extends CodeExecution::Range { BasicObjectInstanceMethodCall methodCall; @@ -322,11 +322,11 @@ class InstanceEvalCallCodeExecution extends CodeExecution::Range { this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "instance_eval" } - override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() } + override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) } } /** - * A call to `Module#class_eval`, which executes its argument as Ruby code. + * A call to `Module#class_eval`, which executes its first argument as Ruby code. */ class ClassEvalCallCodeExecution extends CodeExecution::Range { UnknownMethodCall methodCall; @@ -335,11 +335,11 @@ class ClassEvalCallCodeExecution extends CodeExecution::Range { this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "class_eval" } - override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() } + override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) } } /** - * A call to `Module#module_eval`, which executes its argument as Ruby code. + * A call to `Module#module_eval`, which executes its first argument as Ruby code. */ class ModuleEvalCallCodeExecution extends CodeExecution::Range { UnknownMethodCall methodCall; @@ -348,5 +348,5 @@ class ModuleEvalCallCodeExecution extends CodeExecution::Range { this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "module_eval" } - override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() } + override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) } } diff --git a/ql/test/library-tests/frameworks/Eval.rb b/ql/test/library-tests/frameworks/Eval.rb index 5f7237265ac..b75e57934bb 100644 --- a/ql/test/library-tests/frameworks/Eval.rb +++ b/ql/test/library-tests/frameworks/Eval.rb @@ -1,10 +1,10 @@ # Uses of eval and send -eval("raise \"error\"") +eval("raise \"error\"", binding, "file", 1) send("raise", "error") a = [] -a.send("raise", "error") +a.send("push", "1") class Foo def eval(x) @@ -21,3 +21,6 @@ class Foo end Foo.new.send("exit", 1) +Foo.new.instance_eval("self.class", "file.rb", 3) +Foo.class_eval("def foo; 1; end", "file.rb", 1) +Foo.module_eval("def bar; 1; end", "other_file.rb", 2) \ No newline at end of file diff --git a/ql/test/library-tests/frameworks/StandardLibrary.expected b/ql/test/library-tests/frameworks/StandardLibrary.expected index d6fbf3a44fc..594d4c299ad 100644 --- a/ql/test/library-tests/frameworks/StandardLibrary.expected +++ b/ql/test/library-tests/frameworks/StandardLibrary.expected @@ -59,7 +59,13 @@ open3PipelineCallExecutions | 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 | +| Eval.rb:3:1:3:43 | call to eval | Eval.rb:3:6:3:22 | "raise \\"error\\"" | sendCallCodeExecutions -| Eval.rb:4:1:4:22 | call to send | -| Eval.rb:7:1:7:24 | call to send | +| Eval.rb:4:1:4:22 | call to send | Eval.rb:4:6:4:12 | "raise" | +| Eval.rb:7:1:7:19 | call to send | Eval.rb:7:8:7:13 | "push" | +instanceEvalCallCodeExecutions +| Eval.rb:24:1:24:49 | call to instance_eval | Eval.rb:24:23:24:34 | "self.class" | +classEvalCallCodeExecutions +| Eval.rb:25:1:25:47 | call to class_eval | Eval.rb:25:16:25:32 | "def foo; 1; end" | +moduleEvalCallCodeExecutions +| Eval.rb:26:1:26:54 | call to module_eval | Eval.rb:26:17:26:33 | "def bar; 1; end" | diff --git a/ql/test/library-tests/frameworks/StandardLibrary.ql b/ql/test/library-tests/frameworks/StandardLibrary.ql index ca04ebb4826..791d55b0f8d 100644 --- a/ql/test/library-tests/frameworks/StandardLibrary.ql +++ b/ql/test/library-tests/frameworks/StandardLibrary.ql @@ -1,4 +1,5 @@ import codeql.ruby.frameworks.StandardLibrary +import codeql.ruby.DataFlow query predicate subshellLiteralExecutions(SubshellLiteralExecution e) { any() } @@ -14,6 +15,18 @@ query predicate open3CallExecutions(Open3Call c) { any() } query predicate open3PipelineCallExecutions(Open3PipelineCall c) { any() } -query predicate evalCallCodeExecutions(EvalCallCodeExecution e) { any() } +query DataFlow::Node evalCallCodeExecutions(EvalCallCodeExecution e) { result = e.getCode() } -query predicate sendCallCodeExecutions(SendCallCodeExecution e) { any() } +query DataFlow::Node sendCallCodeExecutions(SendCallCodeExecution e) { result = e.getCode() } + +query DataFlow::Node instanceEvalCallCodeExecutions(InstanceEvalCallCodeExecution e) { + result = e.getCode() +} + +query DataFlow::Node classEvalCallCodeExecutions(ClassEvalCallCodeExecution e) { + result = e.getCode() +} + +query DataFlow::Node moduleEvalCallCodeExecutions(ModuleEvalCallCodeExecution e) { + result = e.getCode() +} diff --git a/ql/test/query-tests/security/cwe-094/CodeInjection.expected b/ql/test/query-tests/security/cwe-094/CodeInjection.expected index 44f85644b0a..834fd1d1db7 100644 --- a/ql/test/query-tests/security/cwe-094/CodeInjection.expected +++ b/ql/test/query-tests/security/cwe-094/CodeInjection.expected @@ -1,16 +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 | +| CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:18:20:18:23 | code | +| CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:21:21:21: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 | +| CodeInjection.rb:18:20:18:23 | code | semmle.label | code | +| CodeInjection.rb:21:21:21: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 | +| CodeInjection.rb:18:20:18:23 | code | CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:18:20:18:23 | code | This code execution depends on $@. | CodeInjection.rb:3:12:3:17 | call to params | a user-provided value | +| CodeInjection.rb:21:21:21:24 | code | CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:21:21:21:24 | code | This code execution depends on $@. | CodeInjection.rb:3:12:3:17 | call to params | a user-provided value | diff --git a/ql/test/query-tests/security/cwe-094/CodeInjection.rb b/ql/test/query-tests/security/cwe-094/CodeInjection.rb index ae9625ecd21..80ee5bee133 100644 --- a/ql/test/query-tests/security/cwe-094/CodeInjection.rb +++ b/ql/test/query-tests/security/cwe-094/CodeInjection.rb @@ -8,6 +8,9 @@ class UsersController < ActionController::Base # BAD eval(params) + # GOOD - user input is in second argument, which is not evaluated as Ruby code + send(:sanitize, params[:code]) + # GOOD Foo.new.bar(code) @@ -25,6 +28,12 @@ class UsersController < ActionController::Base # GOOD eval("foo") end + + private + + def sanitize(code) + true + end end class Foo