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

Make barrier guards more specific

Author: hmac

ql/lib/codeql/ruby/dataflow/BarrierGuards.qll

@@ -20,19 +20,26 @@ 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
       this.getLeftOperand() = checkedNode and this.getRightOperand() = strLitNode
     )
   }
 
-  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 }
 }

ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll

@@ -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)
+    )
+  }
 }

ql/lib/codeql/ruby/regexp/PolynomialReDoSCustomizations.qll

@@ -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
+    }
   }
 }

ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected

@@ -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 |

ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql

@@ -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

ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb

@@ -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

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

@@ -71,8 +71,15 @@ def some_other_request_handler
   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

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

@@ -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 |