diff --git a/ruby/ql/lib/change-notes/2022-10-26-case-expression-barrier-guard.md b/ruby/ql/lib/change-notes/2022-10-26-case-expression-barrier-guard.md new file mode 100644 index 000000000000..6f3df870eed6 --- /dev/null +++ b/ruby/ql/lib/change-notes/2022-10-26-case-expression-barrier-guard.md @@ -0,0 +1,4 @@ +--- + category: minorAnalysis +--- + * String literals and arrays of string literals in case expression patterns are now recognised as barrier guards. diff --git a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll index 9ce0bf32fd71..e3e28f5d4669 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll @@ -313,6 +313,58 @@ private module Cached { predicate immediatelyControls(ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s) { succ = cb.getASuccessor(s) and forall(BasicBlock pred | pred = succ.getAPredecessor() and pred != cb | succ.dominates(pred)) + or + // special case for `when` clauses in case expressions + whenClauseImmediatelyControls(cb, succ, s) + } + + /** + * Holds if all control flow paths reaching `succ` first exit `cb` with + * successor type `s`, assuming that the last node of `cb` is a `when` clause. + * + * ``` + * case foo + * | + * +---+ + * | + * v + * when "foo" ---> "bar" ----+ + * | | | | + * no-match| |match |match |no-match + * | v | | + * | foo <----+ | + * | | + * end <-+--------------------+ + * ``` + * + * The read of `foo` in the `then` block is not technically controlled by + * either `"foo"` or `"bar"` because it can be reached from either of them. + * However if we consider the `when` clause as a whole, then it is + * controlled because _at least one of the patterns_ must match. + * + * We determine this by finding a common successor `succ` of each pattern + * with the same successor type `s`. We check that all predecessors of + * `succ` are patterns in the clause, or are dominated by a pattern in the + * clause. + */ + cached + private predicate whenClauseImmediatelyControls( + ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s + ) { + exists(ExprNodes::WhenClauseCfgNode when | + cb = when.getBasicBlock() and + forall(ExprCfgNode pattern | pattern = when.getPattern(_) | + succ = pattern.getBasicBlock().getASuccessor(s) and + forall(BasicBlock pred | + pred = succ.getAPredecessor() and + pred != cb + | + pred = when.getPattern(_).getBasicBlock() + or + when.getPattern(_).getBasicBlock().dominates(pred) + ) + ) + ) } cached diff --git a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll index f7be488d0fde..030bb2cf2458 100644 --- a/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll +++ b/ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll @@ -234,7 +234,7 @@ module ExprNodes { override predicate relevantChild(AstNode n) { none() } } - /** A control-flow node that wraps an `ArrayLiteral` AST expression. */ + /** A control-flow node that wraps a `Literal` AST expression. */ class LiteralCfgNode extends ExprCfgNode { override string getAPrimaryQlClass() { result = "LiteralCfgNode" } @@ -866,6 +866,17 @@ module ExprNodes { final override RelationalOperation getExpr() { result = super.getExpr() } } + /** A control-flow node that wraps a `SplatExpr` AST expression. */ + class SplatExprCfgNode extends ExprCfgNode { + override string getAPrimaryQlClass() { result = "SplatExprCfgNode" } + + SplatExprCfgNode() { e instanceof SplatExpr } + + final override SplatExpr getExpr() { result = super.getExpr() } + + final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() } + } + /** A control-flow node that wraps an `ElementReference` AST expression. */ class ElementReferenceCfgNode extends MethodCallCfgNode { override string getAPrimaryQlClass() { result = "ElementReferenceCfgNode" } diff --git a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll index c22aa4ca1cfd..ac6138a74f51 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll @@ -8,9 +8,9 @@ private import codeql.ruby.dataflow.SSA private import codeql.ruby.ast.internal.Constant private import codeql.ruby.InclusionTests -private predicate stringConstCompare(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) { +private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch) { exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c | - c = g and + c = guard and exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode | c.getExpr() instanceof EqExpr and branch = true or @@ -18,11 +18,13 @@ private predicate stringConstCompare(CfgNodes::ExprCfgNode g, CfgNode e, boolean or c.getExpr() instanceof NEExpr and branch = false | - c.getLeftOperand() = strLitNode and c.getRightOperand() = e + c.getLeftOperand() = strLitNode and c.getRightOperand() = testedNode or - c.getLeftOperand() = e and c.getRightOperand() = strLitNode + c.getLeftOperand() = testedNode and c.getRightOperand() = strLitNode ) ) + or + stringConstCaseCompare(guard, testedNode, branch) } /** @@ -72,10 +74,12 @@ deprecated class StringConstCompare extends DataFlow::BarrierGuard, } } -private predicate stringConstArrayInclusionCall(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) { +private predicate stringConstArrayInclusionCall( + CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch +) { exists(InclusionTest t | - t.asExpr() = g and - e = t.getContainedNode().asExpr() and + t.asExpr() = guard and + testedNode = t.getContainedNode().asExpr() and branch = t.getPolarity() | exists(ExprNodes::ArrayLiteralCfgNode arr | @@ -132,3 +136,53 @@ deprecated class StringConstArrayInclusionCall extends DataFlow::BarrierGuard, override predicate checks(CfgNode expr, boolean branch) { expr = checkedNode and branch = true } } + +/** + * A validation of a value by comparing with a constant string via a `case` + * expression. For example: + * + * ```rb + * name = params[:user_name] + * case name + * when "alice" + * User.find_by("username = #{name}") + * end + * ``` + */ +private predicate stringConstCaseCompare( + CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch +) { + branch = true and + exists(CfgNodes::ExprNodes::CaseExprCfgNode case | + case.getValue() = testedNode and + exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode | + branchNode = case.getBranch(_) and + guard = branchNode.getPattern(_) and + // For simplicity, consider patterns that contain only string literals or arrays of string literals + forall(ExprCfgNode pattern | pattern = branchNode.getPattern(_) | + // when "foo" + // when "foo", "bar" + pattern instanceof ExprNodes::StringLiteralCfgNode + or + // array literals behave weirdly in the CFG so we need to drop down to the AST level for this bit + // specifically: `SplatExprCfgNode.getOperand()` does not return results for array literals + exists(CfgNodes::ExprNodes::SplatExprCfgNode splat | splat = pattern | + // when *["foo", "bar"] + exists(ArrayLiteral arr | + splat.getExpr().getOperand() = arr and + forall(Expr elem | elem = arr.getAnElement() | elem instanceof StringLiteral) + ) + or + // when *some_var + // when *SOME_CONST + exists(ExprNodes::ArrayLiteralCfgNode arr | + isArrayConstant(splat.getOperand(), arr) and + forall(ExprCfgNode elem | elem = arr.getAnArgument() | + elem instanceof ExprNodes::StringLiteralCfgNode + ) + ) + ) + ) + ) + ) +} diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index 7b56f2e6a935..2bd377c64f25 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -431,7 +431,7 @@ class ContentSet extends TContentSet { * For example, the guard `g` might be a call `isSafe(x)` and the expression `e` * the argument `x`. */ -signature predicate guardChecksSig(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch); +signature predicate guardChecksSig(CfgNodes::AstCfgNode g, CfgNode e, boolean branch); /** * Provides a set of barrier nodes for a guard that validates an expression. @@ -441,13 +441,13 @@ signature predicate guardChecksSig(CfgNodes::ExprCfgNode g, CfgNode e, boolean b */ module BarrierGuard { pragma[nomagic] - private predicate guardChecksSsaDef(CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def) { + private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) { guardChecks(g, def.getARead(), branch) } pragma[nomagic] private predicate guardControlsSsaDef( - CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def, Node n + CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, Node n ) { def.getARead() = n.asExpr() and guardControlsBlock(g, n.asExpr().getBasicBlock(), branch) @@ -455,7 +455,7 @@ module BarrierGuard { /** Gets a node that is safely guarded by the given guard check. */ Node getABarrierNode() { - exists(CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def | + exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def | guardChecksSsaDef(g, branch, def) and guardControlsSsaDef(g, branch, def, result) ) @@ -485,8 +485,8 @@ module BarrierGuard { } /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ -private predicate guardControlsBlock(CfgNodes::ExprCfgNode guard, BasicBlock bb, boolean branch) { - exists(ConditionBlock conditionBlock, SuccessorTypes::BooleanSuccessor s | +private predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean branch) { + exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s | guard = conditionBlock.getLastNode() and s.getValue() = branch and conditionBlock.controls(bb, s) diff --git a/ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSCustomizations.qll index 8c05642fc838..c2f27992ff83 100644 --- a/ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/regexp/PolynomialReDoSCustomizations.qll @@ -127,7 +127,7 @@ module PolynomialReDoS { override DataFlow::Node getHighlight() { result = matchNode } } - private predicate lengthGuard(CfgNodes::ExprCfgNode g, CfgNode node, boolean branch) { + private predicate lengthGuard(CfgNodes::AstCfgNode g, CfgNode node, boolean branch) { exists(DataFlow::Node input, DataFlow::CallNode length, DataFlow::ExprNode operand | length.asExpr().getExpr().(Ast::MethodCall).getMethodName() = "length" and length.getReceiver() = input and diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected index 783d414dad6b..08d6e87948a9 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected @@ -20,3 +20,12 @@ newStyleBarrierGuards | barrier-guards.rb:71:5:71:7 | foo | | barrier-guards.rb:83:5:83:7 | foo | | barrier-guards.rb:91:5:91:7 | foo | +| barrier-guards.rb:108:5:108:7 | foo | +| barrier-guards.rb:115:5:115:7 | foo | +| barrier-guards.rb:117:5:117:7 | foo | +| barrier-guards.rb:122:5:122:7 | foo | +| barrier-guards.rb:124:5:124:7 | foo | +| barrier-guards.rb:131:5:131:7 | foo | +| barrier-guards.rb:136:5:136:7 | foo | +| barrier-guards.rb:141:5:141:7 | foo | +| barrier-guards.rb:146:5:146:7 | foo | diff --git a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb index bc9599fd9269..62bd74870cd2 100644 --- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb +++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb @@ -102,3 +102,72 @@ else foo end + +case foo +when "foo" + foo +else + foo +end + +case foo +when "foo" + foo +when "bar" + foo +end + +case foo +when "foo", "bar" + foo +when "baz", "quux" + foo +else + foo +end + +case foo +when *["foo", "bar"] + foo +end + +case foo +when *%w[foo bar] + foo +end + +case foo +when *FOO + foo +end + +case foo +when *foos + foo +end + +x = nil + +case foo +when *["foo", x] + foo +end + +case foo +when "foo", x + foo +end + +foo_and_x = ["foo", x] + +case foo +when *foo_and_x + foo +end + +FOO_AND_X = ["foo", x] + +case foo +when *FOO_AND_X + foo +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected index b2e4990daec2..750315fc9d9b 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected @@ -10,11 +10,13 @@ edges | CommandInjection.rb:6:15:6:26 | ...[...] : | CommandInjection.rb:34:39:34:51 | "grep #{...}" | | CommandInjection.rb:46:15:46:20 | call to params : | CommandInjection.rb:46:15:46:26 | ...[...] : | | CommandInjection.rb:46:15:46:26 | ...[...] : | CommandInjection.rb:50:24:50:36 | "echo #{...}" | -| CommandInjection.rb:64:18:64:23 | number : | CommandInjection.rb:65:14:65:29 | "echo #{...}" | -| CommandInjection.rb:72:23:72:33 | blah_number : | CommandInjection.rb:73:14:73:34 | "echo #{...}" | -| CommandInjection.rb:81:20:81:25 | **args : | CommandInjection.rb:82:22:82:25 | args : | -| CommandInjection.rb:82:22:82:25 | args : | CommandInjection.rb:82:22:82:37 | ...[...] : | -| CommandInjection.rb:82:22:82:37 | ...[...] : | CommandInjection.rb:82:14:82:39 | "echo #{...}" | +| CommandInjection.rb:54:13:54:18 | call to params : | CommandInjection.rb:54:13:54:24 | ...[...] : | +| CommandInjection.rb:54:13:54:24 | ...[...] : | CommandInjection.rb:59:14:59:16 | cmd | +| CommandInjection.rb:73:18:73:23 | number : | CommandInjection.rb:74:14:74:29 | "echo #{...}" | +| CommandInjection.rb:81:23:81:33 | blah_number : | CommandInjection.rb:82:14:82:34 | "echo #{...}" | +| CommandInjection.rb:90:20:90:25 | **args : | CommandInjection.rb:91:22:91:25 | args : | +| CommandInjection.rb:91:22:91:25 | args : | CommandInjection.rb:91:22:91:37 | ...[...] : | +| CommandInjection.rb:91:22:91:37 | ...[...] : | CommandInjection.rb:91:14:91:39 | "echo #{...}" | nodes | CommandInjection.rb:6:15:6:20 | call to params : | semmle.label | call to params : | | CommandInjection.rb:6:15:6:26 | ...[...] : | semmle.label | ...[...] : | @@ -29,14 +31,17 @@ nodes | CommandInjection.rb:46:15:46:20 | call to params : | semmle.label | call to params : | | CommandInjection.rb:46:15:46:26 | ...[...] : | semmle.label | ...[...] : | | CommandInjection.rb:50:24:50:36 | "echo #{...}" | semmle.label | "echo #{...}" | -| CommandInjection.rb:64:18:64:23 | number : | semmle.label | number : | -| CommandInjection.rb:65:14:65:29 | "echo #{...}" | semmle.label | "echo #{...}" | -| CommandInjection.rb:72:23:72:33 | blah_number : | semmle.label | blah_number : | -| CommandInjection.rb:73:14:73:34 | "echo #{...}" | semmle.label | "echo #{...}" | -| CommandInjection.rb:81:20:81:25 | **args : | semmle.label | **args : | -| CommandInjection.rb:82:14:82:39 | "echo #{...}" | semmle.label | "echo #{...}" | -| CommandInjection.rb:82:22:82:25 | args : | semmle.label | args : | -| CommandInjection.rb:82:22:82:37 | ...[...] : | semmle.label | ...[...] : | +| CommandInjection.rb:54:13:54:18 | call to params : | semmle.label | call to params : | +| CommandInjection.rb:54:13:54:24 | ...[...] : | semmle.label | ...[...] : | +| CommandInjection.rb:59:14:59:16 | cmd | semmle.label | cmd | +| CommandInjection.rb:73:18:73:23 | number : | semmle.label | number : | +| CommandInjection.rb:74:14:74:29 | "echo #{...}" | semmle.label | "echo #{...}" | +| CommandInjection.rb:81:23:81:33 | blah_number : | semmle.label | blah_number : | +| CommandInjection.rb:82:14:82:34 | "echo #{...}" | semmle.label | "echo #{...}" | +| CommandInjection.rb:90:20:90:25 | **args : | semmle.label | **args : | +| CommandInjection.rb:91:14:91:39 | "echo #{...}" | semmle.label | "echo #{...}" | +| CommandInjection.rb:91:22:91:25 | args : | semmle.label | args : | +| CommandInjection.rb:91:22:91:37 | ...[...] : | semmle.label | ...[...] : | subpaths #select | CommandInjection.rb:7:10:7:15 | #{...} | CommandInjection.rb:6:15:6:20 | call to params : | CommandInjection.rb:7:10:7:15 | #{...} | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value | @@ -48,6 +53,7 @@ subpaths | CommandInjection.rb:33:24:33:36 | "echo #{...}" | CommandInjection.rb:6:15:6:20 | call to params : | CommandInjection.rb:33:24:33:36 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value | | CommandInjection.rb:34:39:34:51 | "grep #{...}" | CommandInjection.rb:6:15:6:20 | call to params : | CommandInjection.rb:34:39:34:51 | "grep #{...}" | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value | | CommandInjection.rb:50:24:50:36 | "echo #{...}" | CommandInjection.rb:46:15:46:20 | call to params : | CommandInjection.rb:50:24:50:36 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:46:15:46:20 | call to params | user-provided value | -| CommandInjection.rb:65:14:65:29 | "echo #{...}" | CommandInjection.rb:64:18:64:23 | number : | CommandInjection.rb:65:14:65:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:64:18:64:23 | number | user-provided value | -| CommandInjection.rb:73:14:73:34 | "echo #{...}" | CommandInjection.rb:72:23:72:33 | blah_number : | CommandInjection.rb:73:14:73:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:72:23:72:33 | blah_number | user-provided value | -| CommandInjection.rb:82:14:82:39 | "echo #{...}" | CommandInjection.rb:81:20:81:25 | **args : | CommandInjection.rb:82:14:82:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:20:81:25 | **args | user-provided value | +| CommandInjection.rb:59:14:59:16 | cmd | CommandInjection.rb:54:13:54:18 | call to params : | CommandInjection.rb:59:14:59:16 | cmd | This command depends on a $@. | CommandInjection.rb:54:13:54:18 | call to params | user-provided value | +| CommandInjection.rb:74:14:74:29 | "echo #{...}" | CommandInjection.rb:73:18:73:23 | number : | CommandInjection.rb:74:14:74:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:73:18:73:23 | number | user-provided value | +| CommandInjection.rb:82:14:82:34 | "echo #{...}" | CommandInjection.rb:81:23:81:33 | blah_number : | CommandInjection.rb:82:14:82:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:23:81:33 | blah_number | user-provided value | +| CommandInjection.rb:91:14:91:39 | "echo #{...}" | CommandInjection.rb:90:20:90:25 | **args : | CommandInjection.rb:91:14:91:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:90:20:90:25 | **args | user-provided value | diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb index ed9750128cca..f64c5823d8a8 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb +++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb @@ -49,6 +49,15 @@ def index end Open3.capture2("echo #{cmd}") end + + def update + cmd = params[:key] + case cmd + when "foo" + system(cmd) + end + system(cmd) + end end module Types