Skip to content

Ruby: Make array inclusion barrier more sensitive #11534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 9, 2023
Merged

Ruby: Make array inclusion barrier more sensitive #11534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fbb6bf
Ruby: Make array inclusion barrier more sensitive
hmac Dec 2, 2022
698a679
Ruby: add test
hmac Dec 12, 2022
9944252
Ruby: Add test for barrier guards
hmac Jan 3, 2023
4d228bc
Ruby: Recognise more string-valued variables
hmac Jan 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,18 @@ cached
private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch) {
exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
c = guard and
exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
// Only consider strings without any interpolations
not strLitNode.getExpr().getComponent(_) instanceof StringInterpolationComponent and
exists(ExprCfgNode strNode |
strNode.getConstantValue().isStringlikeValue(_) and
c.getExpr() instanceof EqExpr and
branch = true
or
c.getExpr() instanceof CaseEqExpr and branch = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strnode is not restricted in this disjunct and the one below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I missed this comment until recently. A fix for this is here: #12052

or
c.getExpr() instanceof NEExpr and branch = false
|
c.getLeftOperand() = strLitNode and c.getRightOperand() = testedNode
c.getLeftOperand() = strNode and c.getRightOperand() = testedNode
or
c.getLeftOperand() = testedNode and c.getRightOperand() = strLitNode
c.getLeftOperand() = testedNode and c.getRightOperand() = strNode
)
)
or
Expand Down Expand Up @@ -116,7 +115,7 @@ private predicate stringConstArrayInclusionCall(
isArrayConstant(t.getContainerNode().asExpr(), arr)
|
forall(ExprCfgNode elem | elem = arr.getAnArgument() |
elem instanceof ExprNodes::StringLiteralCfgNode
elem.getConstantValue().isStringlikeValue(_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing all occurrences of ExprNodes::StringLiteralCfgNode in this file.

Copy link
Contributor

@alexrford alexrford Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to make sense, I think it would allow us to cover guards like:

guardvar = "safe"
case input
  when guardvar
    # input is validated
  else
    # input is not validated
end

)
)
)
Expand Down Expand Up @@ -193,39 +192,45 @@ private predicate stringConstCaseCompare(
guard =
any(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
branchNode = case.getBranch(_) and
// For simplicity, consider patterns that contain only string literals or arrays of string literals
// For simplicity, consider patterns that contain only string literals, string-valued variables, or arrays of the same.
forall(ExprCfgNode pattern | pattern = branchNode.getPattern(_) |
// foo = "foo"
//
// when foo
// when foo, bar
// when "foo"
// when "foo", "bar"
pattern instanceof ExprNodes::StringLiteralCfgNode
pattern.getConstantValue().isStringlikeValue(_)
or
pattern =
any(CfgNodes::ExprNodes::SplatExprCfgNode splat |
// when *["foo", "bar"]
forex(ExprCfgNode elem |
elem = splat.getOperand().(ExprNodes::ArrayLiteralCfgNode).getAnArgument()
|
elem instanceof ExprNodes::StringLiteralCfgNode
elem.getConstantValue().isStringlikeValue(_)
)
or
// when *some_var
// when *SOME_CONST
exists(ExprNodes::ArrayLiteralCfgNode arr |
isArrayConstant(splat.getOperand(), arr) and
forall(ExprCfgNode elem | elem = arr.getAnArgument() |
elem instanceof ExprNodes::StringLiteralCfgNode
elem.getConstantValue().isStringlikeValue(_)
)
)
)
)
)
or
// foo = "foo"
//
// in foo
// in "foo"
exists(
CfgNodes::ExprNodes::InClauseCfgNode branchNode, ExprNodes::StringLiteralCfgNode pattern
|
exists(CfgNodes::ExprNodes::InClauseCfgNode branchNode, ExprCfgNode pattern |
branchNode = case.getBranch(_) and
pattern = branchNode.getPattern() and
pattern.getConstantValue().isStringlikeValue(_) and
guard = pattern
)
)
Expand Down
55 changes: 47 additions & 8 deletions ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,21 @@ WARNING: Type BarrierGuard has been deprecated and may be removed in future (bar
failures
oldStyleBarrierGuards
| 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:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:11:3:15 | "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:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | barrier-guards.rb:15:11:15:15 | "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:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | barrier-guards.rb:21:15:21:19 | "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 |
| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | barrier-guards.rb:27:15:27:19 | "foo" | false |
| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:38:5:38:7 | foo | barrier-guards.rb:37:17:37:19 | foo | true |
| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:45:9:45:11 | foo | barrier-guards.rb:43:4:43:6 | foo | true |
| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:45:9:45:11 | foo | barrier-guards.rb:43:11:43:15 | "foo" | true |
| barrier-guards.rb:70:4:70:21 | call to include? | barrier-guards.rb:71:5:71:7 | foo | barrier-guards.rb:70:18:70:20 | foo | true |
| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | barrier-guards.rb:82:4:82:18 | call to index | false |
| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | barrier-guards.rb:82:15:82:17 | foo | true |
| barrier-guards.rb:82:4:82:25 | ... != ... | barrier-guards.rb:83:5:83:7 | foo | barrier-guards.rb:82:23:82:25 | nil | false |
| barrier-guards.rb:207:4:207:15 | ... == ... | barrier-guards.rb:208:5:208:7 | foo | barrier-guards.rb:207:4:207:6 | foo | true |
| barrier-guards.rb:211:10:211:21 | ... == ... | barrier-guards.rb:212:5:212:7 | foo | barrier-guards.rb:211:10:211:12 | foo | true |
| barrier-guards.rb:215:16:215:27 | ... == ... | barrier-guards.rb:216:5:216:7 | foo | barrier-guards.rb:215:16:215:18 | foo | true |
Expand All @@ -18,7 +25,11 @@ oldStyleBarrierGuards
| barrier-guards.rb:219:21:219:32 | ... == ... | barrier-guards.rb:220:5:220:7 | foo | barrier-guards.rb:219:21:219:23 | foo | true |
| barrier-guards.rb:232:6:232:17 | ... == ... | barrier-guards.rb:233:5:233:7 | foo | barrier-guards.rb:232:6:232:8 | foo | true |
| barrier-guards.rb:237:6:237:17 | ... == ... | barrier-guards.rb:237:24:237:26 | foo | barrier-guards.rb:237:6:237:8 | foo | true |
| barrier-guards.rb:268:1:268:12 | ... == ... | barrier-guards.rb:268:17:268:19 | foo | barrier-guards.rb:268:1:268:3 | foo | true |
| barrier-guards.rb:259:4:259:16 | ... == ... | barrier-guards.rb:260:5:260:7 | foo | barrier-guards.rb:259:4:259:6 | foo | true |
| barrier-guards.rb:264:4:264:16 | ... == ... | barrier-guards.rb:265:5:265:7 | foo | barrier-guards.rb:264:4:264:6 | foo | true |
| barrier-guards.rb:272:1:272:12 | ... == ... | barrier-guards.rb:272:17:272:19 | foo | barrier-guards.rb:272:1:272:3 | foo | true |
| barrier-guards.rb:275:4:275:19 | call to include? | barrier-guards.rb:276:5:276:7 | foo | barrier-guards.rb:275:17:275:19 | foo | true |
| barrier-guards.rb:281:4:281:20 | call to include? | barrier-guards.rb:282:5:282:7 | foo | barrier-guards.rb:281:18:281:20 | foo | true |
newStyleBarrierGuards
| barrier-guards.rb:4:5:4:7 | foo |
| barrier-guards.rb:10:5:10:7 | foo |
Expand Down Expand Up @@ -49,7 +60,12 @@ newStyleBarrierGuards
| barrier-guards.rb:233:5:233:7 | foo |
| barrier-guards.rb:237:24:237:26 | foo |
| barrier-guards.rb:244:5:244:7 | foo |
| barrier-guards.rb:268:17:268:19 | foo |
| barrier-guards.rb:260:5:260:7 | foo |
| barrier-guards.rb:265:5:265:7 | foo |
| barrier-guards.rb:272:17:272:19 | foo |
| barrier-guards.rb:276:5:276:7 | foo |
| barrier-guards.rb:282:5:282:7 | foo |
| barrier-guards.rb:292:5:292:7 | foo |
controls
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true |
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false |
Expand Down Expand Up @@ -313,12 +329,35 @@ controls
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:260:5:260:7 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:264:1:266:3 | if ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:265:5:265:7 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:268:1:268:19 | ... && ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:268:17:268:19 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:269:1:269:19 | ... && ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:269:8:269:10 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:268:1:270:3 | if ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:269:5:269:7 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:272:1:272:19 | ... && ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:272:17:272:19 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:273:1:273:19 | ... && ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:273:8:273:10 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:275:1:277:3 | if ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:276:5:276:7 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:281:1:283:3 | if ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:282:5:282:7 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:286:1:288:3 | if ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:287:5:287:7 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:290:1:295:3 | case ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:291:1:292:19 | [match] when ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:291:1:292:19 | [no-match] when ... | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:292:5:292:7 | foo | match |
| barrier-guards.rb:250:4:250:8 | "foo" | barrier-guards.rb:294:5:294:7 | foo | match |
| barrier-guards.rb:254:4:254:28 | ... == ... | barrier-guards.rb:255:5:255:7 | foo | true |
| barrier-guards.rb:259:4:259:16 | ... == ... | barrier-guards.rb:260:5:260:7 | foo | true |
| barrier-guards.rb:264:4:264:16 | ... == ... | barrier-guards.rb:265:5:265:7 | foo | true |
| barrier-guards.rb:268:1:268:12 | ... == ... | barrier-guards.rb:268:17:268:19 | foo | true |
| barrier-guards.rb:269:1:269:3 | foo | barrier-guards.rb:269:8:269:10 | foo | true |
| barrier-guards.rb:268:4:268:30 | ... == ... | barrier-guards.rb:269:5:269:7 | foo | true |
| barrier-guards.rb:272:1:272:12 | ... == ... | barrier-guards.rb:272:17:272:19 | foo | true |
| barrier-guards.rb:273:1:273:3 | foo | barrier-guards.rb:273:8:273:10 | foo | true |
| barrier-guards.rb:275:4:275:19 | call to include? | barrier-guards.rb:276:5:276:7 | foo | true |
| barrier-guards.rb:281:4:281:20 | call to include? | barrier-guards.rb:282:5:282:7 | foo | true |
| barrier-guards.rb:286:4:286:20 | call to include? | barrier-guards.rb:287:5:287:7 | foo | true |
| barrier-guards.rb:291:1:292:19 | [match] when ... | barrier-guards.rb:292:5:292:7 | foo | match |
| barrier-guards.rb:291:1:292:19 | [no-match] when ... | barrier-guards.rb:294:5:294:7 | foo | no-match |
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:291:1:292:19 | [match] when ... | match |
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:291:1:292:19 | [no-match] when ... | no-match |
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:292:5:292:7 | foo | match |
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:294:5:294:7 | foo | no-match |
32 changes: 29 additions & 3 deletions ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,13 +257,39 @@

F = "foo"
if foo == "#{F}"
foo # $ MISSING: guarded
foo # $ guarded
end

f = "foo"
if foo == "#{f}"
foo # $ MISSING: guarded
foo # $ guarded
end

if foo == "#{f}#{unknown_var}"
foo
end

foo == "foo" && foo # $ guarded
foo && foo == "foo"
foo && foo == "foo"

if [f].include? foo
foo # $ guarded
end

g = "g"
foos = [f, g]
if foos.include? foo
foo # $ guarded
end

foos = [f, g, some_method_call]
if foos.include? foo
foo
end

case foo
when g
foo # $ guarded
else
foo
end