Ruby: Lift barrier guards to logical operations

2024-10-07 11:55:29 +02:00 · 2024-10-07 11:55:29 +02:00 · bd17a9eca7
--- a/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
@ -31,30 +31,6 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
  )
  or
  stringConstCaseCompare(guard, testedNode, branch)
-  or
-  exists(CfgNodes::ExprNodes::BinaryOperationCfgNode g |
-    g = guard and
-    stringConstCompareOr(guard, branch) and
-    stringConstCompare(g.getLeftOperand(), testedNode, _)
-  )
-}
-
-/**
- * Holds if `guard` is an `or` expression whose operands are string comparison guards.
- * For example:
- *
- * ```rb
- * x == "foo" or x == "bar"
- * ```
- */
-private predicate stringConstCompareOr(
-  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, boolean branch
-) {
-  guard.getExpr() instanceof LogicalOrExpr and
-  branch = true and
-  forall(CfgNode innerGuard | innerGuard = guard.getAnOperand() |
-    stringConstCompare(innerGuard, any(Ssa::Definition def).getARead(), branch)
-  )
 }

 /**
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll
@ -541,6 +541,7 @@ class ParameterExt extends TParameterExt {

 private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
  private import codeql.ruby.controlflow.internal.Guards as Guards
+  private import codeql.util.Boolean

  class Parameter = ParameterExt;

@ -560,6 +561,79 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
    predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
  }

+  abstract class LogicalOperationGuard extends Guard {
+    abstract Guard getOperand(int i);
+
+    abstract predicate lift(string id, int i, boolean operandBranch, boolean branch);
+  }
+
+  private class NotGuard extends LogicalOperationGuard,
+    Cfg::CfgNodes::ExprNodes::UnaryOperationCfgNode
+  {
+    NotGuard() { this.getExpr() instanceof NotExpr }
+
+    override Guard getOperand(int i) { i = 0 and result = this.getOperand() }
+
+    override predicate lift(string id, int i, boolean operandBranch, boolean branch) {
+      operandBranch instanceof Boolean and
+      id = operandBranch.toString() and
+      i = 0 and
+      branch = operandBranch.booleanNot()
+    }
+  }
+
+  private class StmtSequenceGuard extends LogicalOperationGuard,
+    Cfg::CfgNodes::ExprNodes::StmtSequenceCfgNode
+  {
+    override Guard getOperand(int i) { i = 0 and result = this.getLastStmt() }
+
+    override predicate lift(string id, int i, boolean operandBranch, boolean branch) {
+      operandBranch instanceof Boolean and
+      id = operandBranch.toString() and
+      i = 0 and
+      branch = operandBranch
+    }
+  }
+
+  abstract private class BinaryLogicalOperationGuard extends LogicalOperationGuard,
+    Cfg::CfgNodes::ExprNodes::BinaryOperationCfgNode
+  {
+    final override Guard getOperand(int i) {
+      i = 0 and result = this.getLeftOperand()
+      or
+      i = 1 and result = this.getRightOperand()
+    }
+
+    abstract predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch);
+
+    final override predicate lift(string id, int i, boolean operandBranch, boolean branch) {
+      exists(Boolean branchLeft, Boolean branchRight |
+        this.lift(branchLeft, branchRight, branch) and
+        id = branchLeft + "," + branchRight
+      |
+        i = 0 and operandBranch = branchLeft
+        or
+        i = 1 and operandBranch = branchRight
+      )
+    }
+  }
+
+  private class AndGuard extends BinaryLogicalOperationGuard {
+    AndGuard() { this.getExpr() instanceof LogicalAndExpr }
+
+    override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) {
+      branch = branchLeft.booleanOr(branchRight) // yes, should not be `booleanAnd`
+    }
+  }
+
+  private class OrGuard extends BinaryLogicalOperationGuard {
+    OrGuard() { this.getExpr() instanceof LogicalOrExpr }
+
+    override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) {
+      branch = branchLeft.booleanAnd(branchRight) // yes, should not be `booleanOr`
+    }
+  }
+
  /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
  predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) {
    Guards::guardControlsBlock(guard, bb, branch)
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected
@ -10,9 +10,7 @@ edges
 | barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:93:14:93:14 | x | provenance |  |
 | barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:99:14:99:14 | x | provenance |  |
 | barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:103:14:103:14 | x | provenance |  |
-| barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:105:14:105:14 | x | provenance |  |
 | barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:109:14:109:14 | x | provenance |  |
-| barrier_flow.rb:82:5:82:5 | x | barrier_flow.rb:111:14:111:14 | x | provenance |  |
 | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:82:5:82:5 | x | provenance |  |
 | barrier_flow.rb:116:5:116:5 | x | barrier_flow.rb:121:14:121:14 | x | provenance |  |
 | barrier_flow.rb:116:5:116:5 | x | barrier_flow.rb:125:14:125:14 | x | provenance |  |
@ -36,9 +34,7 @@ nodes
 | barrier_flow.rb:93:14:93:14 | x | semmle.label | x |
 | barrier_flow.rb:99:14:99:14 | x | semmle.label | x |
 | barrier_flow.rb:103:14:103:14 | x | semmle.label | x |
-| barrier_flow.rb:105:14:105:14 | x | semmle.label | x |
 | barrier_flow.rb:109:14:109:14 | x | semmle.label | x |
-| barrier_flow.rb:111:14:111:14 | x | semmle.label | x |
 | barrier_flow.rb:116:5:116:5 | x | semmle.label | x |
 | barrier_flow.rb:116:9:116:18 | call to source | semmle.label | call to source |
 | barrier_flow.rb:121:14:121:14 | x | semmle.label | x |
@ -48,8 +44,6 @@ nodes
 | barrier_flow.rb:143:14:143:14 | x | semmle.label | x |
 subpaths
 testFailures
-| barrier_flow.rb:105:14:105:14 | x | Unexpected result: hasValueFlow=10 |
-| barrier_flow.rb:111:14:111:14 | x | Unexpected result: hasValueFlow=10 |
 #select
 | barrier_flow.rb:4:10:4:10 | x | barrier_flow.rb:2:9:2:17 | call to source | barrier_flow.rb:4:10:4:10 | x | $@ | barrier_flow.rb:2:9:2:17 | call to source | call to source |
 | barrier_flow.rb:11:14:11:14 | x | barrier_flow.rb:8:9:8:17 | call to source | barrier_flow.rb:11:14:11:14 | x | $@ | barrier_flow.rb:8:9:8:17 | call to source | call to source |
@ -58,9 +52,7 @@ testFailures
 | barrier_flow.rb:93:14:93:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:93:14:93:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source |
 | barrier_flow.rb:99:14:99:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:99:14:99:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source |
 | barrier_flow.rb:103:14:103:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:103:14:103:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source |
-| barrier_flow.rb:105:14:105:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:105:14:105:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source |
 | barrier_flow.rb:109:14:109:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:109:14:109:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source |
-| barrier_flow.rb:111:14:111:14 | x | barrier_flow.rb:82:9:82:18 | call to source | barrier_flow.rb:111:14:111:14 | x | $@ | barrier_flow.rb:82:9:82:18 | call to source | call to source |
 | barrier_flow.rb:121:14:121:14 | x | barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:121:14:121:14 | x | $@ | barrier_flow.rb:116:9:116:18 | call to source | call to source |
 | barrier_flow.rb:125:14:125:14 | x | barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:125:14:125:14 | x | $@ | barrier_flow.rb:116:9:116:18 | call to source | call to source |
 | barrier_flow.rb:131:14:131:14 | x | barrier_flow.rb:116:9:116:18 | call to source | barrier_flow.rb:131:14:131:14 | x | $@ | barrier_flow.rb:116:9:116:18 | call to source | call to source |
--- a/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
+++ b/ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
@ -1,6 +1,4 @@
 testFailures
-| barrier_flow.rb:105:16:105:26 | # $ guarded | Missing result:guarded= |
-| barrier_flow.rb:111:16:111:26 | # $ guarded | Missing result:guarded= |
 failures
 newStyleBarrierGuards
 | barrier-guards.rb:3:16:4:19 | [input] SSA phi read(foo) |
@ -55,6 +53,8 @@ newStyleBarrierGuards
 | barrier_flow.rb:91:14:91:14 | x |
 | barrier_flow.rb:96:24:96:24 | x |
 | barrier_flow.rb:97:14:97:14 | x |
+| barrier_flow.rb:105:14:105:14 | x |
+| barrier_flow.rb:111:14:111:14 | x |
 | barrier_flow.rb:118:8:118:19 | [input] SSA phi read(x) |
 | barrier_flow.rb:118:24:118:35 | [input] SSA phi read(x) |
 | barrier_flow.rb:119:14:119:14 | x |