hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-01 03:35:13 +02:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
(cherry picked from commit ffbbb807f4)
This commit is contained in:
Esben Sparre Andreasen
2018-10-12 09:37:30 +02:00
committed by Max Schaefer
parent f9634040b0
commit 2e49cd117a
3 changed files with 59 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
javascript/ql/src/Security/CWE-807/ConditionalBypass.ql
View File

@@ -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"

2
javascript/ql/test/query-tests/Security/CWE-807/ConditionalBypass.expected
View File

@@ -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
javascript/ql/test/query-tests/Security/CWE-807/tst.js
View File

@@ -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();
}
});