From ffbbb807f4dc3ea7b36f3d42fbca070e9c382673 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Fri, 12 Oct 2018 09:37:30 +0200 Subject: [PATCH] JS: avoid flagging early returns in js/user-controlled-bypass --- change-notes/1.19/analysis-javascript.md | 1 + .../src/Security/CWE-807/ConditionalBypass.ql | 28 +++++++++++++++-- .../CWE-807/ConditionalBypass.expected | 2 ++ .../test/query-tests/Security/CWE-807/tst.js | 31 +++++++++++++++++++ 4 files changed, 60 insertions(+), 2 deletions(-) diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index f00370ac34d1..68092aa26d05 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -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 diff --git a/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql b/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql index ca7188ce5eb2..e939bb61f8a1 100644 --- a/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql +++ b/javascript/ql/src/Security/CWE-807/ConditionalBypass.ql @@ -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 @@ -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) { + 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" diff --git a/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected b/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected index 36c6f163cf5f..0ee7194b924b 100644 --- a/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected +++ b/javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected @@ -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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-807/tst.js b/javascript/ql/test/query-tests/Security/CWE-807/tst.js index 84235fa0d3f1..aa00c47f1ebd 100644 --- a/javascript/ql/test/query-tests/Security/CWE-807/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-807/tst.js @@ -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(); + } + +});