JS: avoid flagging early returns in js/user-controlled-bypass #315
Conversation
ghost
added
ghost
self-requested a review
as a 
| * | ||
| * Example: `if (e) return; action(x)`. | ||
| */ | ||
| predicate isEarlyAbortGuard(Sink e, SensitiveAction action) { |
There was a problem hiding this comment.
This is a very syntactic check. Could we perhaps generalise this a bit using dominance and condition guard nodes?
There was a problem hiding this comment.
The syntactic check is quite deliberate as it is intended to whitelist a specific programming pattern.
The use of guard nodes and dominance is part of the Sink-character:
https://github.com/Semmle/ql/blob/16b29b2d08ea9232cad3ff0ab2d31724f66cfe23/javascript/ql/src/semmle/javascript/security/dataflow/ConditionalBypass.qll#L85-L90
There was a problem hiding this comment.
It seems to me that this check goes against the intention of the query. You are essentially saying that if it's possible to take a branch that doesn't reach action, then we whitelist that. But isn't the whole point of the query to flag cases where user input can cause action not to be executed?
Perhaps what we really want to exclude here are cases where avoiding action causes an abnormal termination of some sort. The proposed approach achieves that in the given example, but surely not in general?
I'm thinking of something like this:
while (!verified) {
if (user-controlled) {
break;
}
verify();
}Here, we would consider verify() to be a sensitive action, and the syntactic pattern looks much like in the example we want to whitelist, except that in this case we wouldn't want to whitelist.
As I am writing this, though, I am becoming less and less convinced that I actually understand what this query is meant to flag.
There was a problem hiding this comment.
Good point about break. I have reformulated the pattern to match return and throw only, see tests.
As discussed elsewhere, I have also downgraded the precision of this query to medium.
|
Performance is unchanged. A single FP in test code has disappeared. |
| @@ -33,6 +33,7 @@ | |||
| | Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. | | |||
| | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. | | |||
| | Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. | | |||
| | User-controlled bypass of security check | Fewer results | This rule no longer flags conditions that guards early returns. The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. | | |||
|
Fixed and squashed. |
Let's discuss during the meeting. |
Add extension receiver type to function signature in trap file names
Fixes https://jira.semmle.com/browse/LGTM-2801 by whitelisting the problematic programming pattern.
I have considered lowering the precision of
js/user-controlled-bypasstomediumseveral times, so I am not too worried about losing TPs. Tagging as WIP while waiting for a sanity check of the performance.