hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-26 09:15:12 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #288 from github/hmac-barrier-guard-checks

Browse Source 
Make barrier guards more specific
This commit is contained in:
Harry Maclean
2021-09-14 16:16:20 +01:00
committed by GitHub
parent f4e2c30d86 4763312e55
commit 12723f0f13
8 changed files with 113 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
View File

@@ -20,11 +20,16 @@ private import codeql.ruby.CFG
class StringConstCompare extends DataFlow::BarrierGuard,
CfgNodes::ExprNodes::ComparisonOperationCfgNode {
private CfgNode checkedNode;
// The value of the condition that results in the node being validated.
private boolean checkedBranch;
StringConstCompare() {
exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
this.getExpr() instanceof EqExpr or
this.getExpr() instanceof CaseEqExpr
this.getExpr() instanceof EqExpr and checkedBranch = true
or
this.getExpr() instanceof CaseEqExpr and checkedBranch = true
or
this.getExpr() instanceof NEExpr and checkedBranch = false
|
this.getLeftOperand() = strLitNode and this.getRightOperand() = checkedNode
or
@@ -32,7 +37,9 @@ class StringConstCompare extends DataFlow::BarrierGuard,
)
}
override DataFlow::Node getAGuardedNode() { result.asExpr() = checkedNode }
override predicate checks(CfgNode expr, boolean branch) {
expr = checkedNode and branch = checkedBranch
}
}
/**
@@ -64,5 +71,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 }
}

41
ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
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
@@ -160,7 +161,45 @@ class Content extends TContent {
/**
* 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 CfgNodes::ExprCfgNode {
Node getAGuardedNode() { none() }
private ConditionBlock conditionBlock;
BarrierGuard() { this = conditionBlock.getLastNode() }
/** Holds if this guard controls block `b` upon evaluating to `branch`. */
private predicate controlsBlock(BasicBlock bb, boolean branch) {
exists(SuccessorTypes::BooleanSuccessor s | s.getValue() = branch |
conditionBlock.controls(bb, s)
)
}
/**
* 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)
)
}
}

4
ql/lib/codeql/ruby/regexp/PolynomialReDoSCustomizations.qll
View File

@@ -124,6 +124,8 @@ module PolynomialReDoS {
)
}
override DataFlow::Node getAGuardedNode() { result = input }
override predicate checks(CfgNode node, boolean branch) {
node = input.asExpr() and branch = true
}
}
}

5
ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected Normal file
View File

@@ -0,0 +1,5 @@
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:4:3:6 | foo | true |
| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | barrier-guards.rb:9:21:9:23 | foo | true |
| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | barrier-guards.rb:15:4:15:6 | foo | false |
| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | barrier-guards.rb:21:8:21:10 | foo | true |
| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | barrier-guards.rb:27:8:27:10 | foo | false |

7
ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql Normal file
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

33
ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb Normal file
View File

@@ -0,0 +1,33 @@
foo = "foo"
if foo == "foo"
foo
else
foo
end
if ["foo"].include?(foo)
foo
else
foo
end
if foo != "foo"
foo
else
foo
end
unless foo == "foo"
foo
else
foo
end
unless foo != "foo"
foo
else
foo
end
foo

11
ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb
View File

@@ -71,8 +71,15 @@ class BarController < ApplicationController
def safe_paths
dir = params[:order]
# GOOD: barrier guard prevents taint flow
dir = "DESC" unless dir == "ASC"
User.order("name #{dir}")
if dir == "ASC"
User.order("name #{dir}")
else
dir = "DESC"
User.order("name #{dir}")
end
# TODO: a more idiomatic form of this guard is the following:
# dir = "DESC" unless dir == "ASC"
# but our taint tracking can't (yet) handle that properly
name = params[:user_name]
# GOOD: barrier guard prevents taint flow

10
ql/test/query-tests/security/cwe-089/SqlInjection.expected
View File

@@ -12,8 +12,8 @@ edges
| ActiveRecordInjection.rb:56:38:56:43 | call to params : | ActiveRecordInjection.rb:56:38:56:50 | ...[...] : |
| ActiveRecordInjection.rb:56:38:56:50 | ...[...] : | ActiveRecordInjection.rb:8:31:8:34 | pass : |
| ActiveRecordInjection.rb:62:10:62:15 | call to params : | ActiveRecordInjection.rb:68:21:68:33 | ... + ... |
| ActiveRecordInjection.rb:94:22:94:27 | call to params : | ActiveRecordInjection.rb:94:22:94:45 | ...[...] : |
| ActiveRecordInjection.rb:94:22:94:45 | ...[...] : | ActiveRecordInjection.rb:20:23:20:31 | condition : |
| ActiveRecordInjection.rb:101:22:101:27 | call to params : | ActiveRecordInjection.rb:101:22:101:45 | ...[...] : |
| ActiveRecordInjection.rb:101:22:101:45 | ...[...] : | ActiveRecordInjection.rb:20:23:20:31 | condition : |
nodes
| ActiveRecordInjection.rb:8:25:8:28 | name : | semmle.label | name : |
| ActiveRecordInjection.rb:8:31:8:34 | pass : | semmle.label | pass : |
@@ -36,12 +36,12 @@ nodes
| ActiveRecordInjection.rb:56:38:56:50 | ...[...] : | semmle.label | ...[...] : |
| ActiveRecordInjection.rb:62:10:62:15 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:68:21:68:33 | ... + ... | semmle.label | ... + ... |
| ActiveRecordInjection.rb:94:22:94:27 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:94:22:94:45 | ...[...] : | semmle.label | ...[...] : |
| ActiveRecordInjection.rb:101:22:101:27 | call to params : | semmle.label | call to params : |
| ActiveRecordInjection.rb:101:22:101:45 | ...[...] : | semmle.label | ...[...] : |
#select
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:56:23:56:28 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:56:23:56:28 | call to params | a user-provided value |
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:56:38:56:43 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:56:38:56:43 | call to params | a user-provided value |
| ActiveRecordInjection.rb:23:17:23:25 | condition | ActiveRecordInjection.rb:94:22:94:27 | call to params : | ActiveRecordInjection.rb:23:17:23:25 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:94:22:94:27 | call to params | a user-provided value |
| ActiveRecordInjection.rb:23:17:23:25 | condition | ActiveRecordInjection.rb:101:22:101:27 | call to params : | ActiveRecordInjection.rb:23:17:23:25 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:101:22:101:27 | call to params | a user-provided value |
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] | ActiveRecordInjection.rb:35:30:35:35 | call to params : | ActiveRecordInjection.rb:35:30:35:44 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:35:30:35:35 | call to params | a user-provided value |
| ActiveRecordInjection.rb:39:21:39:43 | "id = '#{...}'" | ActiveRecordInjection.rb:39:30:39:35 | call to params : | ActiveRecordInjection.rb:39:21:39:43 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:39:30:39:35 | call to params | a user-provided value |
| ActiveRecordInjection.rb:43:23:43:45 | "id = '#{...}'" | ActiveRecordInjection.rb:43:32:43:37 | call to params : | ActiveRecordInjection.rb:43:23:43:45 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:43:32:43:37 | call to params | a user-provided value |