Ruby: Add case string comparison barrier guard #11114

hmac · 2022-11-03T22:21:47Z

This recognises barriers of the form

STRINGS = ["foo", "bar"]

case foo
when "some string literal"
  foo
when *["other", "strings"]
  foo
when *STRINGS
  foo
end

where the reads of foo inside each when are guarded by the comparison
of foo with the string literals.

We don't yet recognise this construct:

case foo
when "foo", "bar"
  foo
end

This is due to a limitation in the shared barrier guard logic.

aibaars · 2022-11-07T12:21:57Z

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

+
+    final override SplatExpr getExpr() { result = super.getExpr() }
+
+    final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() }


Please add a class SplatExprChildMapping and change this to

Suggested change

final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() }

final ExprCfgNode getLhs() { e.hasCfgChild(e.getOperand(), this, result) }

Sure, but why (the extra class)?

Well regardless of why, I think that's fixed the issue below with SplatExprCfgNode.getAnOperand() not giving results for array literals. Still, in the absence of any documentation comments, it would be nice to know why.

There may be multiple CFG nodes for the same AST node due to control flow graph splitting. The e.hasCfgChild predicate picks the CfgNode that matches the AstNode child node and is connected to the current CfgNode.

aibaars · 2022-11-07T12:26:45Z

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

+  branch = true and
+  exists(CfgNodes::ExprNodes::CaseExprCfgNode case |
+    case.getValue() = testedNode and
+    exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |


What about InClauses?

This doesn't handle in clauses. It looks much harder to be sure that an expression is guarded by an in pattern, because the patterns can be complex. I guess we'd have to check that there are no new variable bindings in the pattern or something. Anyway, I think that's something to tackle in the future.

I've pushed a commit that handles very simple in clauses where the pattern is a single string literal, such as

case foo in "foo" foo end

aibaars · 2022-11-07T12:28:03Z

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

+) {
+  branch = true and
+  exists(CfgNodes::ExprNodes::CaseExprCfgNode case |
+    case.getValue() = testedNode and


Should we also handle the case where getValue does not exist?

case when foo == "foo" then ... end

Weird, I didn't know you could do this. This appears to already be covered by our existing StringConstCompareBarrier. I'll add a test for it.

aibaars · 2022-11-07T12:32:32Z

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

+        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


I'm a bit surprised by this comment. It could be that there is a bug in the CFG library, but other than that getOperand should return an array literal.

Your suggested change for SplatExprCfgNode appears to have fixed this. I've pushed an update.

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

This recognises barriers of the form STRINGS = ["foo", "bar"] case foo when "some string literal" foo when *["other", "strings"] foo when *STRINGS foo end where the reads of `foo` inside each `when` are guarded by the comparison of `foo` with the string literals. We don't yet recognise this construct: case foo when "foo", "bar" foo end This is due to a limitation in the shared barrier guard logic.

aibaars · 2022-11-09T07:47:45Z

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

+    foo
+end
+
+if foo == "foo" or foo == "bar" # not recognised


I think this should be fixed by #11153

aibaars · 2022-11-09T08:03:05Z

@hmac I trivially fixed all merge conflicts in the test cases through the GitHub UI. You probably need to refresh the expected output, though.

Recognise if statements with conditionals made up or logical `and` or `or` clauses as barrier guards.

The `when` node now acts as a join point for patterns in the when clause, with match/no-match completions. This is similar to how `or` expressions work. The result of this is that the `when` clause "controls" the body of the `when`, which allows us to model barrier guards for multi-pattern when clauses. For this code case x when 1, 2 y end The old CFG was x --> when --> 1 --no-match--> 2 ---no-match---> case \ \ ^ \ \ | \ --match----+ | \ | | \ | | ------match---------> y --+ The new CFG is x --> 1 --no-match--> 2 --no-match--> [no-match] when --no-match--> case \ \ ^ \ \ | \ --match--> [match] when --match--> y -----+ \ / \ / -------match----- i.e. all patterns flow to the `when` node, which is split based on whether the pattern matched or not. The body of the when clause then has a single predecessor `[match] when`, which acts as condition block that controls `y`.

The `or` case ran extremely slowly before this change. Also exclude string interpolations from consideration, for correctness, and add some more tests.

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

hmac · 2022-11-11T07:50:38Z

I've updated this with quite a lot more stuff:

We now recognise guards of the form if x and y, if x or y
We recognise guards from multi-pattern when clauses, such as when "a", "b", "c"

To support multi-pattern when clauses I reworked the CFG for when clauses so that control flows from patterns to the when with the correct match/no-match edge. This is a bit weird, because it means that the when controlflow tree is neither post-order nor pre-order - instead, the last node is sometimes the when (if there's no match) and sometimes the body of the when (if there's a match).

This means we get one edge to the body instead of one per pattern, and makes it easy to define a barrier guard for when clauses. The downside is it is a bit weird, and if there's a nicer way to do it I would love to know.

Another result of this is a strange failure in one of the dataflow tests (see CI). This seems to imply that dataflow through case expressions is no longer working correctly, but I haven't found any obvious problems apart from that test failure, so I don't know what's going on there.

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

hvitved · 2022-11-17T09:34:21Z

Another result of this is a strange failure in one of the dataflow tests (see CI). This seems to imply that dataflow through case expressions is no longer working correctly, but I haven't found any obvious problems apart from that test failure, so I don't know what's going on there.

I have pushed some CFG fixes that resolve this.

The existing barrier guard machinery recognises guards such as `if x and y`, so there's no need to explicitly model them.

hmac · 2022-11-21T08:59:46Z

@hvitved is there anything remaining to change on this PR or do you think it's ready to go?

hvitved · 2022-11-21T13:05:45Z

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

+  }
+
+  /** A control-flow node that wraps a `SplatExpr` AST expression. */
+  class SplatExprCfgNode extends ExprCfgNode {


May as well extend UnaryOperationCfgNode.

hvitved · 2022-11-21T13:07:30Z

ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll


-  private class WhenTree extends PreOrderTree, WhenClause {
+  private class WhenTree extends ControlFlowTree, WhenClause {
    final override predicate propagatesAbnormal(AstNode child) { child = this.getAPattern() }


I think we should add getBody() here as well.

hvitved · 2022-11-21T13:07:48Z

ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

      or
-      last(this.getBody(), last, c)
+      last(this.getBody(), last, c) and
+      c instanceof NormalCompletion


Not needed.

hvitved · 2022-11-21T13:11:47Z

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

+ * ```
+ */
+private predicate stringConstCompareOr(
+  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, Ssa::Definition def, boolean branch


def is not used by the caller, so can be removed from the parameter list.

hvitved · 2022-11-21T13:13:01Z

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

+  exists(CfgNodes::ExprNodes::CaseExprCfgNode case |
+    case.getValue() = testedNode and
+    (
+      exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |


I prefer guard = any(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |

hvitved · 2022-11-21T13:13:46Z

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

+          // when "foo", "bar"
+          pattern instanceof ExprNodes::StringLiteralCfgNode
+          or
+          exists(CfgNodes::ExprNodes::SplatExprCfgNode splat | splat = pattern |


Again, I prefer pattern = any(CfgNodes::ExprNodes::SplatExprCfgNode splat |

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

+ * x == "foo" or x == "bar"
+ * ```
+ */
+private predicate stringConstCompareOr(


hvitved · 2022-11-24T08:10:51Z

ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

      c.(ConditionalCompletion).getValue() = false
-      or
-      last(this.getBody(), last, c) and
-      c instanceof NormalCompletion


Sorry; I was not clear: What I meant was that the c instanceof NormalCompletion should be removed; we still need last(this.getBody(), last, c) (I wonder why the CFG tests don't fail).

I checked this and thought it was OK to remove because there's a clause in the succ predicate for case trees that I thought might cover it. On my phone so I can't link easily, but L431 of this file on main.

Happy to re-add this bit I deleted though. I'll do that tomorrow.

Ah yes, it works because of the (somewhat non-compositional)

last(this.getABranch().(WhenClause).getBody(), pred, c)

I still think we should add this back, though.

I'll also see if we can fix up that line to something like last(this.getABranch(), pred, c)

Changing ^ caused some problems, so I've just left it as-is for now. The change in WhenTree is fixed though.

hmac mentioned this pull request Nov 3, 2022

Ruby: Case barrier guards #10981

Closed

github-actions bot added documentation Ruby labels Nov 3, 2022

hmac marked this pull request as ready for review November 4, 2022 00:49

hmac requested a review from a team as a code owner November 4, 2022 00:49

calumgrant requested a review from aibaars November 7, 2022 09:29

aibaars reviewed Nov 7, 2022

View reviewed changes

github-advanced-security bot found potential problems Nov 7, 2022

View reviewed changes

ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll Fixed Show fixed Hide fixed

asgerf mentioned this pull request Nov 8, 2022

Ruby: handle knownOrUnkown in default taint step #11170

Merged

hmac added 6 commits November 9, 2022 15:03

Ruby: Fix SplatExprCfgNode

25ceeaf

Ruby: Add test for another case guard variant

87944a3

Ruby: Handle simple in clauses in barrier guard

0ab88c2

Ruby: Fix in clause barrier guard

f1b63c4

Ruby: Add inline barrier guard test

ad7b5ae

aibaars reviewed Nov 9, 2022

View reviewed changes

hmac added 3 commits November 10, 2022 16:38

Ruby: More string comparison guards

a8b0d29

Recognise if statements with conditionals made up or logical `and` or `or` clauses as barrier guards.

Ruby: Fix performance of string comparison guard

62ea1f0

The `or` case ran extremely slowly before this change. Also exclude string interpolations from consideration, for correctness, and add some more tests.

hmac force-pushed the case-barrier-guard-3 branch 2 times, most recently from a2fe67c to f7d17d6 Compare November 11, 2022 05:29

github-advanced-security bot found potential problems Nov 11, 2022

View reviewed changes

ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll Fixed Show fixed Hide fixed

ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll Fixed Show fixed Hide fixed

ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll Fixed Show fixed Hide fixed

hmac added 2 commits November 11, 2022 18:41

Ruby: Add missing doc

b16cecc

Ruby: Update test fixture

2b4217b

hmac force-pushed the case-barrier-guard-3 branch from f7d17d6 to 2b4217b Compare November 11, 2022 05:42

hvitved reviewed Nov 17, 2022

View reviewed changes

ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll Outdated Show resolved Hide resolved

hvitved reviewed Nov 17, 2022

View reviewed changes

ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll Show resolved Hide resolved

Adjust CFG

f24fa40

hmac added 2 commits November 18, 2022 18:14

Ruby: Remove redundant predicate

5deb16e

The existing barrier guard machinery recognises guards such as `if x and y`, so there's no need to explicitly model them.

Ruby: Cache some barrier guard predicates

376d4e0

hvitved mentioned this pull request Nov 21, 2022

CFG: Workaround in test output for origin/target pairs with multiple edges #11341

Merged

hvitved reviewed Nov 21, 2022

View reviewed changes

Ruby: SplatExprCfgNode extends UnaryOperationCfgNode

57f6894

github-advanced-security bot found potential problems Nov 24, 2022

View reviewed changes

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

* x == "foo" or x == "bar"

* ```

*/

private predicate stringConstCompareOr(

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter

The QLDoc has no documentation for branch, but the QLDoc mentions or

hvitved reviewed Nov 24, 2022

View reviewed changes

hmac added 2 commits November 25, 2022 09:12

Ruby: Clean up WhenClause CFG

6897fb4

Ruby: Minor refactor of barrier guard code

2822c94

hmac force-pushed the case-barrier-guard-3 branch from 3e974fe to 2822c94 Compare November 24, 2022 20:13

hvitved approved these changes Nov 28, 2022

View reviewed changes

hmac merged commit 375403f into github:main Nov 29, 2022

hmac deleted the case-barrier-guard-3 branch November 29, 2022 22:21


		final override SplatExpr getExpr() { result = super.getExpr() }

		final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() }

	final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() }
	final ExprCfgNode getLhs() { e.hasCfgChild(e.getOperand(), this, result) }

Ruby: Add case string comparison barrier guard #11114

Ruby: Add case string comparison barrier guard #11114

Uh oh!

Conversation

hmac commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmac Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmac Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aibaars commented Nov 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hmac commented Nov 11, 2022

Uh oh!

Uh oh!

Uh oh!

hvitved commented Nov 17, 2022

Uh oh!

hmac commented Nov 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check warning

Choose a reason for hiding this comment

Uh oh!

hmac Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

hmac commented Nov 3, 2022 •

edited

Loading

hmac Nov 7, 2022 •

edited

Loading

hmac Nov 7, 2022 •

edited

Loading

hvitved Nov 21, 2022 •

edited

Loading

hmac Nov 24, 2022 •

edited

Loading

hvitved Nov 24, 2022 •

edited

Loading