Ruby: Resolve simple string interpolations #7334
Conversation
hmac
force-pushed
the
hmac/regexp-interpolations
branch
from
0f03e42 to
06f09e4
Compare
| // If last statement in the interpolation is a constant or local variable read, | ||
| // we attempt to look up its string value. | ||
| // If there's a result, we return that as the string value of the interpolation. | ||
| final override string getValueText() { |
There was a problem hiding this comment.
This (and the getValueText further down) should be implemented in CfgNodes.qll instead (i.e., on CFG nodes instead of AST nodes). In general, if getValueText is defined recursively, it should be done in CfgNodes for potential higher precision (in context of CFG splitting). Example
if (b)
x = "true"
else
x = "false"
if (b)
puts xIf (when) Ruby has Boolean CFG splitting, the access to x in the b = true case will have constant value "true", while it will have constant value "false" in the b = false case. However, the AST node x in puts x will not have a constant value.
There was a problem hiding this comment.
OK - I think I understand what you're saying here. I've updated this PR to push more calculation into the CFG. We still need a few changes at the AST level, I think, because regular string constants aren't CFG nodes. Let me know if I've got the right idea or if I'm still off.
hmac
force-pushed
the
hmac/regexp-interpolations
branch
from
06f09e4 to
a4a3922
Compare
|
I have added some suggested changes to this PR at #7440. |
hmac
force-pushed
the
hmac/regexp-interpolations
branch
from
acb45ff to
1db95eb
Compare
The superclass definition uses SSA, which doesn't track constants.
This change is split over several commits so it is easier to see. This change adds some extra lines, which will be populated in the next commit.
When calculating `StringlikeLiteral.getValueText`, include results from
interpolations where we can determine their string value. For example:
b = "b" # local variable
D = "d" # constant
"a#{b}c" # getValueText() = "abc"
"a#{b}c{D}" # getValueText() = "abcd"
/#a#{b}c{D}/ # getValueText() = "abcd"
This exercises the support for resolving string interpolations, and is based on a real vulnerability: GHSA-jxhc-q857-3j6g)
hmac
force-pushed
the
hmac/regexp-interpolations
branch
from
1db95eb to
ac9cac7
Compare
hmac
force-pushed
the
hmac/regexp-interpolations
branch
from
047dd38 to
c81b4ce
Compare
Create a set of classes for components of regex literals, separate from those of string literals. This allows us to special-case components of free-spacing regexes (ones with the /x flag) to not have a `getValueText()`. This in turn is useful because our regex parser can't handle free-spacing regexes, so excluding them ensures that we don't generate erroneous ReDoS alerts.
hmac
force-pushed
the
hmac/regexp-interpolations
branch
from
47a0b47 to
4f7f924
Compare
|
Performance is improved from the last DCA run. Still slower than
|
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Will merge if the latest DCA run looks good.
When calculating
StringlikeLiteral.getValueText, include results frominterpolations where we can determine their string value. For example:
Because this also applies to regexes, it increases the sensitivity of our ReDoS queries. With this change we find a true positive in GHSA-jxhc-q857-3j6g.
There's one shortcoming to this change: ReDoS alerts may not include the correct location of the problematic (sub)string if it is included via an interpolation. The alert itself, and the suggested problematic input, will still be correct.