Skip to content

Commit 0fcb079

Browse files
hmachmac
authored
Merge pull request #326 from github/hmac/eval-fixes
Make Code execution query more specific
2 parents b955fdb + e419fc9 commit 0fcb079

File tree

6 files changed

+54
-23
lines changed

6 files changed

+54
-23
lines changed

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

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ class Open3PipelineCall extends SystemCommandExecution::Range {
277277
}
278278

279279
/**
280-
* A call to `Kernel.eval`, which executes its argument as Ruby code.
280+
* A call to `Kernel.eval`, which executes its first argument as Ruby code.
281281
* ```ruby
282282
* a = 1
283283
* Kernel.eval("a = 2")
@@ -291,11 +291,11 @@ class EvalCallCodeExecution extends CodeExecution::Range {
291291
this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "eval"
292292
}
293293

294-
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
294+
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
295295
}
296296

297297
/**
298-
* A call to `Kernel#send`, which executes its arguments as a Ruby method call.
298+
* A call to `Kernel#send`, which executes its first argument as a Ruby method call.
299299
* ```ruby
300300
* arr = []
301301
* arr.send("push", 1)
@@ -309,11 +309,11 @@ class SendCallCodeExecution extends CodeExecution::Range {
309309
this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "send"
310310
}
311311

312-
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
312+
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
313313
}
314314

315315
/**
316-
* A call to `BasicObject#instance_eval`, which executes its argument as Ruby code.
316+
* A call to `BasicObject#instance_eval`, which executes its first argument as Ruby code.
317317
*/
318318
class InstanceEvalCallCodeExecution extends CodeExecution::Range {
319319
BasicObjectInstanceMethodCall methodCall;
@@ -322,11 +322,11 @@ class InstanceEvalCallCodeExecution extends CodeExecution::Range {
322322
this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "instance_eval"
323323
}
324324

325-
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
325+
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
326326
}
327327

328328
/**
329-
* A call to `Module#class_eval`, which executes its argument as Ruby code.
329+
* A call to `Module#class_eval`, which executes its first argument as Ruby code.
330330
*/
331331
class ClassEvalCallCodeExecution extends CodeExecution::Range {
332332
UnknownMethodCall methodCall;
@@ -335,11 +335,11 @@ class ClassEvalCallCodeExecution extends CodeExecution::Range {
335335
this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "class_eval"
336336
}
337337

338-
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
338+
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
339339
}
340340

341341
/**
342-
* A call to `Module#module_eval`, which executes its argument as Ruby code.
342+
* A call to `Module#module_eval`, which executes its first argument as Ruby code.
343343
*/
344344
class ModuleEvalCallCodeExecution extends CodeExecution::Range {
345345
UnknownMethodCall methodCall;
@@ -348,5 +348,5 @@ class ModuleEvalCallCodeExecution extends CodeExecution::Range {
348348
this.asExpr().getExpr() = methodCall and methodCall.getMethodName() = "module_eval"
349349
}
350350

351-
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getAnArgument() }
351+
override DataFlow::Node getCode() { result.asExpr().getExpr() = methodCall.getArgument(0) }
352352
}

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
# Uses of eval and send
22

3-
eval("raise \"error\"")
3+
eval("raise \"error\"", binding, "file", 1)
44
send("raise", "error")
55

66
a = []
7-
a.send("raise", "error")
7+
a.send("push", "1")
88

99
class Foo
1010
def eval(x)
@@ -21,3 +21,6 @@ def run
2121
end
2222

2323
Foo.new.send("exit", 1)
24+
Foo.new.instance_eval("self.class", "file.rb", 3)
25+
Foo.class_eval("def foo; 1; end", "file.rb", 1)
26+
Foo.module_eval("def bar; 1; end", "other_file.rb", 2)

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,13 @@ open3PipelineCallExecutions
5959
| CommandExecution.rb:64:1:64:44 | call to pipeline_start |
6060
| CommandExecution.rb:65:1:65:38 | call to pipeline |
6161
evalCallCodeExecutions
62-
| Eval.rb:3:1:3:23 | call to eval |
62+
| Eval.rb:3:1:3:43 | call to eval | Eval.rb:3:6:3:22 | "raise \\"error\\"" |
6363
sendCallCodeExecutions
64-
| Eval.rb:4:1:4:22 | call to send |
65-
| Eval.rb:7:1:7:24 | call to send |
64+
| Eval.rb:4:1:4:22 | call to send | Eval.rb:4:6:4:12 | "raise" |
65+
| Eval.rb:7:1:7:19 | call to send | Eval.rb:7:8:7:13 | "push" |
66+
instanceEvalCallCodeExecutions
67+
| Eval.rb:24:1:24:49 | call to instance_eval | Eval.rb:24:23:24:34 | "self.class" |
68+
classEvalCallCodeExecutions
69+
| Eval.rb:25:1:25:47 | call to class_eval | Eval.rb:25:16:25:32 | "def foo; 1; end" |
70+
moduleEvalCallCodeExecutions
71+
| Eval.rb:26:1:26:54 | call to module_eval | Eval.rb:26:17:26:33 | "def bar; 1; end" |

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

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import codeql.ruby.frameworks.StandardLibrary
2+
import codeql.ruby.DataFlow
23

34
query predicate subshellLiteralExecutions(SubshellLiteralExecution e) { any() }
45

@@ -14,6 +15,18 @@ query predicate open3CallExecutions(Open3Call c) { any() }
1415

1516
query predicate open3PipelineCallExecutions(Open3PipelineCall c) { any() }
1617

17-
query predicate evalCallCodeExecutions(EvalCallCodeExecution e) { any() }
18+
query DataFlow::Node evalCallCodeExecutions(EvalCallCodeExecution e) { result = e.getCode() }
1819

19-
query predicate sendCallCodeExecutions(SendCallCodeExecution e) { any() }
20+
query DataFlow::Node sendCallCodeExecutions(SendCallCodeExecution e) { result = e.getCode() }
21+
22+
query DataFlow::Node instanceEvalCallCodeExecutions(InstanceEvalCallCodeExecution e) {
23+
result = e.getCode()
24+
}
25+
26+
query DataFlow::Node classEvalCallCodeExecutions(ClassEvalCallCodeExecution e) {
27+
result = e.getCode()
28+
}
29+
30+
query DataFlow::Node moduleEvalCallCodeExecutions(ModuleEvalCallCodeExecution e) {
31+
result = e.getCode()
32+
}

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
edges
22
| CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:6:10:6:13 | code |
3-
| CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:15:20:15:23 | code |
4-
| CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:18:21:18:24 | code |
3+
| CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:18:20:18:23 | code |
4+
| CodeInjection.rb:3:12:3:17 | call to params : | CodeInjection.rb:21:21:21:24 | code |
55
nodes
66
| CodeInjection.rb:3:12:3:17 | call to params : | semmle.label | call to params : |
77
| CodeInjection.rb:6:10:6:13 | code | semmle.label | code |
88
| CodeInjection.rb:9:10:9:15 | call to params | semmle.label | call to params |
9-
| CodeInjection.rb:15:20:15:23 | code | semmle.label | code |
10-
| CodeInjection.rb:18:21:18:24 | code | semmle.label | code |
9+
| CodeInjection.rb:18:20:18:23 | code | semmle.label | code |
10+
| CodeInjection.rb:21:21:21:24 | code | semmle.label | code |
1111
subpaths
1212
#select
1313
| 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 |
1414
| 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 |
15-
| 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 |
16-
| 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 |
15+
| 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 |
16+
| 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 |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ def create
88
# BAD
99
eval(params)
1010

11+
# GOOD - user input is in second argument, which is not evaluated as Ruby code
12+
send(:sanitize, params[:code])
13+
1114
# GOOD
1215
Foo.new.bar(code)
1316

@@ -25,6 +28,12 @@ def update
2528
# GOOD
2629
eval("foo")
2730
end
31+
32+
private
33+
34+
def sanitize(code)
35+
true
36+
end
2837
end
2938

3039
class Foo

0 commit comments

Comments
 (0)