From e8dce25cc2836d303dab012a4aed82160e381d99 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 25 Oct 2022 14:44:23 +0200 Subject: [PATCH] fix rb/code-injection --- .../ruby/security/CodeInjectionCustomizations.qll | 2 +- .../src/queries/security/cwe-094/CodeInjection.ql | 5 +++-- .../security/cwe-094/CodeInjection.expected | 14 ++++++++++++++ .../query-tests/security/cwe-094/CodeInjection.rb | 6 ++++++ 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll index 8814bea0890..82b385abb8c 100644 --- a/ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll @@ -70,7 +70,7 @@ module CodeInjection { /** Gets a flow state for which this is a sink. */ override DataFlow::FlowState getAFlowState() { if c.runsArbitraryCode() - then result = [FlowState::substring(), FlowState::full()] // If it runs immediately, then it's always vulnerable. + then result = [FlowState::substring(), FlowState::full()] // If it runs arbitrary code then it's always vulnerable. else result = FlowState::full() // If it "just" loads something, then it's only vulnerable if the attacker controls the entire string. } } diff --git a/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql b/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql index 2abdc4652fa..e74f68c1a8b 100644 --- a/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql +++ b/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql @@ -25,9 +25,10 @@ where // removing duplications of the same path, but different flow-labels. sink = min(DataFlow::PathNode otherSink | - config.hasFlowPath(any(DataFlow::PathNode s | s.getNode() = source.getNode()), otherSink) + config.hasFlowPath(any(DataFlow::PathNode s | s.getNode() = sourceNode), otherSink) and + otherSink.getNode() = sink.getNode() | otherSink order by otherSink.getState() ) -select sink.getNode(), source, sink, "This code execution depends on a $@.", source.getNode(), +select sink.getNode(), source, sink, "This code execution depends on a $@.", sourceNode, "user-provided value" diff --git a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected index 7dd8035b41f..2e56377db71 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected @@ -15,7 +15,12 @@ edges | CodeInjection.rb:38:24:38:27 | code : | CodeInjection.rb:38:10:38:28 | call to escape | | CodeInjection.rb:38:24:38:27 | code : | CodeInjection.rb:38:10:38:28 | call to escape | | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:78:12:78:24 | ...[...] : | +| CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:78:12:78:24 | ...[...] : | | CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:80:16:80:19 | code | +| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:86:10:86:37 | ... + ... | +| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | +| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:90:10:90:13 | code | +| CodeInjection.rb:78:12:78:24 | ...[...] : | CodeInjection.rb:90:10:90:13 | code | nodes | CodeInjection.rb:5:12:5:17 | call to params : | semmle.label | call to params : | | CodeInjection.rb:5:12:5:17 | call to params : | semmle.label | call to params : | @@ -37,8 +42,14 @@ nodes | CodeInjection.rb:38:24:38:27 | code : | semmle.label | code : | | CodeInjection.rb:41:40:41:43 | code | semmle.label | code | | CodeInjection.rb:78:12:78:17 | call to params : | semmle.label | call to params : | +| CodeInjection.rb:78:12:78:17 | call to params : | semmle.label | call to params : | +| CodeInjection.rb:78:12:78:24 | ...[...] : | semmle.label | ...[...] : | | CodeInjection.rb:78:12:78:24 | ...[...] : | semmle.label | ...[...] : | | CodeInjection.rb:80:16:80:19 | code | semmle.label | code | +| CodeInjection.rb:86:10:86:37 | ... + ... | semmle.label | ... + ... | +| CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | semmle.label | "prefix_#{...}_suffix" | +| CodeInjection.rb:90:10:90:13 | code | semmle.label | code | +| CodeInjection.rb:90:10:90:13 | code | semmle.label | code | subpaths #select | CodeInjection.rb:8:10:8:13 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:8:10:8:13 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value | @@ -50,3 +61,6 @@ subpaths | CodeInjection.rb:38:10:38:28 | call to escape | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:38:10:38:28 | call to escape | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value | | CodeInjection.rb:41:40:41:43 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:41:40:41:43 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value | | CodeInjection.rb:80:16:80:19 | code | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:80:16:80:19 | code | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value | +| CodeInjection.rb:86:10:86:37 | ... + ... | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:86:10:86:37 | ... + ... | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value | +| CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value | +| CodeInjection.rb:90:10:90:13 | code | CodeInjection.rb:78:12:78:17 | call to params : | CodeInjection.rb:90:10:90:13 | code | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value | diff --git a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb index 8bb2d8de946..dfa3a3d411b 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb +++ b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb @@ -82,5 +82,11 @@ class UsersController < ActionController::Base obj().send("prefix_" + code + "_suffix", "foo"); # GOOD obj().send("prefix_#{code}_suffix", "foo"); # GOOD + + eval("prefix_" + code + "_suffix"); # BAD + + eval("prefix_#{code}_suffix"); # BAD + + eval(code); # BAD end end \ No newline at end of file