diff --git a/ruby/ql/lib/codeql/ruby/Concepts.qll b/ruby/ql/lib/codeql/ruby/Concepts.qll index dd7c75565b71..18995f39b349 100644 --- a/ruby/ql/lib/codeql/ruby/Concepts.qll +++ b/ruby/ql/lib/codeql/ruby/Concepts.qll @@ -701,6 +701,9 @@ module SystemCommandExecution { class CodeExecution extends DataFlow::Node instanceof CodeExecution::Range { /** Gets the argument that specifies the code to be executed. */ DataFlow::Node getCode() { result = super.getCode() } + + /** Holds if this execution runs arbitrary code, as opposed to some restricted subset. E.g. `Object.send` will only run any method on an object. */ + predicate runsArbitraryCode() { super.runsArbitraryCode() } } /** Provides a class for modeling new dynamic code execution APIs. */ @@ -714,6 +717,9 @@ module CodeExecution { abstract class Range extends DataFlow::Node { /** Gets the argument that specifies the code to be executed. */ abstract DataFlow::Node getCode(); + + /** Holds if this execution runs arbitrary code, as opposed to some restricted subset. E.g. `Object.send` will only run any method on an object. */ + predicate runsArbitraryCode() { any() } } } diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveJob.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveJob.qll index aad917ccb2eb..02b75b28072b 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveJob.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveJob.qll @@ -25,6 +25,8 @@ module ActiveJob { } override DataFlow::Node getCode() { result = this.getArgument(0) } + + override predicate runsArbitraryCode() { none() } } } } diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll index 360150efdbfb..389ac9ee64da 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll @@ -221,5 +221,7 @@ module ActiveStorage { } override DataFlow::Node getCode() { result = this.getArgument(0) } + + override predicate runsArbitraryCode() { none() } } } diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll index 0cf990ad00ac..32e3976216fd 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll @@ -35,6 +35,8 @@ module ActiveSupport { } override DataFlow::Node getCode() { result = this.getReceiver() } + + override predicate runsArbitraryCode() { none() } } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll index b47fe7a86e4c..71013f439367 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll @@ -166,6 +166,8 @@ module Kernel { SendCallCodeExecution() { this.getMethodName() = "send" } override DataFlow::Node getCode() { result = this.getArgument(0) } + + override predicate runsArbitraryCode() { none() } } private class TapSummary extends SimpleSummarizedCallable { diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Module.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Module.qll index 8e5098dd5838..e4e627a9a79f 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/core/Module.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Module.qll @@ -42,5 +42,7 @@ module Module { } override DataFlow::Node getCode() { result = this.getArgument(0) } + + override predicate runsArbitraryCode() { none() } } } diff --git a/ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll index 032915099777..8814bea0890c 100644 --- a/ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll @@ -11,20 +11,41 @@ private import codeql.ruby.dataflow.BarrierGuards * adding your own. */ module CodeInjection { + /** Flow states used to distinguish whether an attacker controls the entire string. */ + module FlowState { + /** Flow state used for normal tainted data, where an attacker might only control a substring. */ + DataFlow::FlowState substring() { result = "substring" } + + /** Flow state used for data that is entirely controlled by the attacker. */ + DataFlow::FlowState full() { result = "full" } + } + /** * A data flow source for "Code injection" vulnerabilities. */ - abstract class Source extends DataFlow::Node { } + abstract class Source extends DataFlow::Node { + /** Gets a flow state for which this is a source. */ + DataFlow::FlowState getAFlowState() { result = [FlowState::substring(), FlowState::full()] } + } /** * A data flow sink for "Code injection" vulnerabilities. */ - abstract class Sink extends DataFlow::Node { } + abstract class Sink extends DataFlow::Node { + /** Holds if this sink is safe for an attacker that only controls a substring. */ + DataFlow::FlowState getAFlowState() { result = [FlowState::substring(), FlowState::full()] } + } /** * A sanitizer for "Code injection" vulnerabilities. */ - abstract class Sanitizer extends DataFlow::Node { } + abstract class Sanitizer extends DataFlow::Node { + /** + * Gets a flow state for which this is a sanitizer. + * Sanitizes all states if the result is empty. + */ + DataFlow::FlowState getAFlowState() { none() } + } /** * DEPRECATED: Use `Sanitizer` instead. @@ -42,6 +63,35 @@ module CodeInjection { * A call that evaluates its arguments as Ruby code, considered as a flow sink. */ class CodeExecutionAsSink extends Sink { - CodeExecutionAsSink() { this = any(CodeExecution c).getCode() } + CodeExecution c; + + CodeExecutionAsSink() { this = c.getCode() } + + /** 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. + else result = FlowState::full() // If it "just" loads something, then it's only vulnerable if the attacker controls the entire string. + } + } + + private import codeql.ruby.AST as Ast + + /** + * A string-concatenation that sanitizes the `full()` state. + */ + class StringConcatenationSanitizer extends Sanitizer { + StringConcatenationSanitizer() { + // string concatenations sanitize the `full` state, as an attacker no longer controls the entire string + exists(Ast::AstNode str | + str instanceof Ast::StringLiteral + or + str instanceof Ast::AddExpr + | + this.asExpr().getExpr() = str + ) + } + + override DataFlow::FlowState getAFlowState() { result = FlowState::full() } } } diff --git a/ruby/ql/lib/codeql/ruby/security/CodeInjectionQuery.qll b/ruby/ql/lib/codeql/ruby/security/CodeInjectionQuery.qll index 8397bcf3fc63..d8a6b8c41b75 100644 --- a/ruby/ql/lib/codeql/ruby/security/CodeInjectionQuery.qll +++ b/ruby/ql/lib/codeql/ruby/security/CodeInjectionQuery.qll @@ -16,16 +16,26 @@ import codeql.ruby.dataflow.BarrierGuards class Configuration extends TaintTracking::Configuration { Configuration() { this = "CodeInjection" } - override predicate isSource(DataFlow::Node source) { source instanceof Source } + override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { + state = source.(Source).getAFlowState() + } - override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { + state = sink.(Sink).getAFlowState() + } override predicate isSanitizer(DataFlow::Node node) { - node instanceof Sanitizer or - node instanceof StringConstCompareBarrier or + node instanceof Sanitizer and not exists(node.(Sanitizer).getAFlowState()) + or + node instanceof StringConstCompareBarrier + or node instanceof StringConstArrayInclusionCallBarrier } + override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) { + node.(Sanitizer).getAFlowState() = state + } + deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { guard instanceof SanitizerGuard } diff --git a/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql b/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql index 1a23ea74aee5..2abdc4652fa1 100644 --- a/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql +++ b/ruby/ql/src/queries/security/cwe-094/CodeInjection.ql @@ -21,6 +21,13 @@ import DataFlow::PathGraph from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Source sourceNode where config.hasFlowPath(source, sink) and - sourceNode = source.getNode() + sourceNode = source.getNode() and + // 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) + | + otherSink order by otherSink.getState() + ) select sink.getNode(), source, sink, "This code execution depends on a $@.", source.getNode(), "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 f70ce6ca67e0..7dd8035b41f9 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected @@ -1,25 +1,44 @@ edges | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:5:12:5:24 | ...[...] : | +| CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:5:12:5:24 | ...[...] : | +| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:8:10:8:13 | code | | CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:8:10:8:13 | code | | CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:20:20:20:23 | code | +| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:20:20:20:23 | code | +| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:23:21:23:24 | code | | CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:23:21:23:24 | code | | CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:29:15:29:18 | code | | CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:32:19:32:22 | code | | CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:38:24:38:27 | code : | +| CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:38:24:38:27 | code : | | CodeInjection.rb:5:12:5:24 | ...[...] : | CodeInjection.rb:41:40:41:43 | code | | 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:24 | ...[...] : | CodeInjection.rb:80:16:80:19 | 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 : | | CodeInjection.rb:5:12:5:24 | ...[...] : | semmle.label | ...[...] : | +| CodeInjection.rb:5:12:5:24 | ...[...] : | semmle.label | ...[...] : | +| CodeInjection.rb:8:10:8:13 | code | semmle.label | code | | CodeInjection.rb:8:10:8:13 | code | semmle.label | code | | CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params | +| CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params | +| CodeInjection.rb:20:20:20:23 | code | semmle.label | code | | CodeInjection.rb:20:20:20:23 | code | semmle.label | code | | CodeInjection.rb:23:21:23:24 | code | semmle.label | code | +| CodeInjection.rb:23:21:23:24 | code | semmle.label | code | | CodeInjection.rb:29:15:29:18 | code | semmle.label | code | | CodeInjection.rb:32:19:32:22 | code | semmle.label | code | | CodeInjection.rb:38:10:38:28 | call to escape | semmle.label | call to escape | +| CodeInjection.rb:38:10:38:28 | call to escape | semmle.label | call to escape | +| CodeInjection.rb:38:24:38:27 | code : | semmle.label | code : | | 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:24 | ...[...] : | semmle.label | ...[...] : | +| CodeInjection.rb:80:16:80:19 | 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 | @@ -30,3 +49,4 @@ subpaths | CodeInjection.rb:32:19:32:22 | code | CodeInjection.rb:5:12:5:17 | call to params : | CodeInjection.rb:32:19:32:22 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value | | 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 | 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 8a1d52fa1761..8bb2d8de9467 100644 --- a/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb +++ b/ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb @@ -72,3 +72,15 @@ def self.const_get(x) true end end + +class UsersController < ActionController::Base + def create + code = params[:code] + + obj().send(code, "foo"); # BAD + + obj().send("prefix_" + code + "_suffix", "foo"); # GOOD + + obj().send("prefix_#{code}_suffix", "foo"); # GOOD + end +end \ No newline at end of file