Skip to content

JS: avoid flagging early returns in js/user-controlled-bypass #315

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 1 commit into from Oct 17, 2018
Merged

JS: avoid flagging early returns in js/user-controlled-bypass #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions change-notes/1.19/analysis-javascript.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 guard early returns. The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. |
| Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |

## Changes to QL libraries
Expand Down
28 changes: 26 additions & 2 deletions javascript/ql/src/Security/CWE-807/ConditionalBypass.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* @description Conditions that the user controls are not suited for making security-related decisions.
* @kind problem
* @problem.severity error
* @precision high
* @precision medium
* @id js/user-controlled-bypass
* @tags security
* external/cwe/cwe-807
Expand Down Expand Up @@ -83,8 +83,32 @@ predicate isTaintedGuardForSensitiveAction(Sink sink, DataFlow::Node source, Sen
)
}

/**
* Holds if `e` effectively guards access to `action` by returning or throwing early.
*
* Example: `if (e) return; action(x)`.
*/
predicate isEarlyAbortGuard(Sink e, SensitiveAction action) {
Copy link

Choose a reason for hiding this comment

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

This is a very syntactic check. Could we perhaps generalise this a bit using dominance and condition guard nodes?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

exists (IfStmt guard |
// `e` is in the condition of an if-statement ...
e.asExpr().getParentExpr*() = guard.getCondition() and
// ... where the then-branch always throws or returns
exists (Stmt abort |
abort instanceof ThrowStmt or
abort instanceof ReturnStmt |
abort.nestedIn(guard) and
abort.getBasicBlock().(ReachableBasicBlock).postDominates(guard.getThen().getBasicBlock() )
) and
// ... and the else-branch does not exist
not exists (guard.getElse()) |
// ... and `action` is outside the if-statement
not action.asExpr().getEnclosingStmt().nestedIn(guard)
)
}

from DataFlow::Node source, DataFlow::Node sink, SensitiveAction action
where isTaintedGuardForSensitiveAction(sink, source, action)
where isTaintedGuardForSensitiveAction(sink, source, action) and
not isEarlyAbortGuard(sink, action)
select sink, "This condition guards a sensitive $@, but $@ controls it.",
action, "action",
source, "a user-provided value"
2 changes: 2 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@
| tst.js:76:9:76:10 | v1 | This condition guards a sensitive $@, but $@ controls it. | tst.js:78:9:78:22 | process.exit() | action | tst.js:75:14:75:24 | req.cookies | a user-provided value |
| tst.js:76:9:76:10 | v1 | This condition guards a sensitive $@, but $@ controls it. | tst.js:78:9:78:22 | process.exit() | action | tst.js:75:39:75:58 | req.params.requestId | a user-provided value |
| tst.js:90:9:90:41 | req.coo ... secret" | This condition guards a sensitive $@, but $@ controls it. | tst.js:92:9:92:22 | process.exit() | action | tst.js:90:9:90:19 | req.cookies | a user-provided value |
| tst.js:111:13:111:32 | req.query.vulnerable | This condition guards a sensitive $@, but $@ controls it. | tst.js:114:9:114:16 | verify() | action | tst.js:111:13:111:32 | req.query.vulnerable | a user-provided value |
| tst.js:118:13:118:32 | req.query.vulnerable | This condition guards a sensitive $@, but $@ controls it. | tst.js:121:13:121:20 | verify() | action | tst.js:118:13:118:32 | req.query.vulnerable | a user-provided value |
31 changes: 31 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-807/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,34 @@ app.get('/user/:id', function(req, res) {
console.log(commit.author().toString());
}
});

app.get('/user/:id', function(req, res) {
if (!req.body || !username || !password || riskAssessnment == null) { // OK: early return below
res.status(400).send({ error: '...', id: '...' });
return
}
customerLogin.customerLogin(username, password, riskAssessment, clientIpAddress);

while (!verified) {
if (req.query.vulnerable) { // NOT OK
break;
}
verify();
}

while (!verified) {
if (req.query.vulnerable) { // NOT OK
break;
} else {
verify();
}
}

while (!verified) {
if (req.query.vulnerable) { // OK: early return
return;
}
verify();
}

});