fix rb/code-injection

Author: erik-krogh

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.
     }
   }

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"

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 |

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

@@ -82,5 +82,11 @@ def create
     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