Make barrier guards more specific

Following examples from the other libraries, this change introduces a
member predicate `checks(CfgNode expr, boolean branch)` to
`BarrierGuard`, which holds if the guard validates `expr` for a
particular value of `branch`, which represents the value of the
condition in the guard.

For example, in the following guard...

    if foo == "foo"
      do_something foo
    else
      do_something_else foo
    end

...the variable `foo` is validated when the condition `foo == "foo"` is
true.

We also introduce the concept that a guard "controls" a code block based
on the value of `branch`. In the example above, the "then" branch of the
if statement is controlled when `branch` is true. The else branch is
not controlled because `foo` can take (almost) any value in that branch.

Based on these concepts, we define a guarded node to be a read of a
validated variable in a controlled block.

In the above example, the `foo` in `do_something foo` is guarded, but
the `foo` in `do_something_else foo` is not.
This commit is contained in:
Harry Maclean
2021-09-09 09:45:30 +01:00
parent a62aa2b1b2
commit b4c29425ea
5 changed files with 74 additions and 5 deletions

View File

@@ -32,7 +32,7 @@ class StringConstCompare extends DataFlow::BarrierGuard,
)
}
override DataFlow::Node getAGuardedNode() { result.asExpr() = checkedNode }
override predicate checks(CfgNode expr, boolean branch) { expr = checkedNode and branch = true }
}
/**
@@ -64,5 +64,5 @@ class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
)
}
override DataFlow::Node getAGuardedNode() { result.asExpr() = checkedNode }
override predicate checks(CfgNode expr, boolean branch) { expr = checkedNode and branch = true }
}

View File

@@ -3,6 +3,7 @@ private import DataFlowDispatch
private import DataFlowPrivate
private import codeql.ruby.CFG
private import codeql.ruby.typetracking.TypeTracker
private import codeql.ruby.dataflow.SSA
/**
* An element, viewed as a node in a data flow graph. Either an expression
@@ -159,8 +160,51 @@ class Content extends TContent {
}
/**
* A guard that validates some expression.
* A node that controls whether other nodes are evaluated.
*/
abstract class BarrierGuard extends CfgNodes::ExprCfgNode {
Node getAGuardedNode() { none() }
class GuardNode extends CfgNodes::ExprCfgNode {
ConditionBlock conditionBlock;
GuardNode() { this = conditionBlock.getLastNode() }
/** Holds if this guard controls block `b` upon evaluating to `branch`. */
predicate controlsBlock(BasicBlock bb, boolean branch) {
exists(SuccessorTypes::BooleanSuccessor s | s.getValue() = branch |
conditionBlock.controls(bb, s)
)
}
}
/**
* A guard that validates some expression.
*
* To use this in a configuration, extend the class and provide a
* characteristic predicate precisely specifying the guard, and override
* `checks` to specify what is being validated and in which branch.
*
* It is important that all extending classes in scope are disjoint.
*/
abstract class BarrierGuard extends GuardNode {
/**
* Holds if this guard validates `expr` upon evaluating to `branch`.
* For example, the following code validates `foo` when the condition
* `foo == "foo"` is true.
* ```ruby
* if foo == "foo"
* do_something
* else
* do_something_else
* end
* ```
*/
abstract predicate checks(CfgNode expr, boolean branch);
final Node getAGuardedNode() {
exists(boolean branch, CfgNodes::ExprCfgNode testedNode, Ssa::Definition def |
def.getARead() = testedNode and
def.getARead() = result.asExpr() and
this.checks(testedNode, branch) and
this.controlsBlock(result.asExpr().getBasicBlock(), branch)
)
}
}

View File

@@ -0,0 +1,2 @@
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:15:4:17 | foo | barrier-guards.rb:3:4:3:6 | foo | true |
| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:15:10:17 | foo | barrier-guards.rb:9:21:9:23 | foo | true |

View File

@@ -0,0 +1,7 @@
import codeql.ruby.dataflow.internal.DataFlowPublic
import codeql.ruby.dataflow.BarrierGuards
import codeql.ruby.controlflow.CfgNodes
from BarrierGuard g, boolean branch, ExprCfgNode expr
where g.checks(expr, branch)
select g, g.getAGuardedNode(), expr, branch

View File

@@ -0,0 +1,16 @@
foo = "foo"
if foo == "foo"
do_true_1 foo
else
do_false_1 foo
end
if ["foo"].include?(foo)
do_true_2 foo
else
do_false_2 foo
end
do_default foo