hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-04 21:25:44 +02:00
Code Issues Packages Projects Releases Wiki Activity

JS: Add type confusion sink for prototype pollution checks

Browse Source
This commit is contained in:
Asger Feldthaus
2020-12-02 11:38:21 +00:00
parent e10a22ec26
commit f132b4a279
5 changed files with 55 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
javascript/change-notes/2020-11-25-prototype-pollution.md
View File

@@ -6,5 +6,6 @@ lgtm,codescanning
This highlights indirect modification of `Object.prototype` via an unsafe `merge` call taking a user-controlled object as argument.
* The query previously named "Prototype pollution in utility function" (`js/prototype-pollution-utility`) has been renamed to "Prototype-polluting function".
This query highlights the implementation of an unsafe `merge` function, to ensure a robust API is exposed downstream.
* The prototype pollution queries have been moved to the Security/CWE-915 folder,
and tagged with CWE-079, CWE-094, CWE-400, and CWE-915.
* The above queries have been moved to the Security/CWE-915 folder, and tagged with CWE-079, CWE-094, CWE-400, and CWE-915.
* The query "Type confusion through parameter tampering" (`js/type-confusion-through-parameter-tampering`) now highlights
ineffective prototype pollution checks that can be bypassed by type confusion.

4
javascript/ql/src/Security/CWE-843/TypeConfusionThroughParameterTampering.ql
View File

@@ -15,5 +15,5 @@ import DataFlow::PathGraph
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "Potential type confusion for $@.", source.getNode(),
"HTTP request parameter"
select sink.getNode(), source, sink, "Potential type confusion as $@ may be either an array or a string.", source.getNode(),
"this HTTP request parameter"

13
javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingCustomizations.qll
View File

@@ -92,4 +92,17 @@ module TypeConfusionThroughParameterTampering {
)
}
}
/**
* A value compared to the string `__proto__` or `constructor`, which may be bypassed by wrapping
* the payload in an array.
*/
private class ProtoStringComparison extends Sink {
ProtoStringComparison() {
exists(EqualityTest test |
test.hasOperands(this.asExpr(), any(Expr e | e.getStringValue() = ["__proto__", "constructor"])) and
test.isStrict()
)
}
}
}

33
javascript/ql/test/query-tests/Security/CWE-843/TypeConfusionThroughParameterTampering.expected
View File

@@ -25,6 +25,13 @@ nodes
| tst.js:45:15:45:35 | ctx.req ... ery.foo |
| tst.js:46:5:46:7 | foo |
| tst.js:46:5:46:7 | foo |
| tst.js:77:25:77:38 | req.query.path |
| tst.js:77:25:77:38 | req.query.path |
| tst.js:80:23:80:23 | p |
| tst.js:81:9:81:9 | p |
| tst.js:81:9:81:9 | p |
| tst.js:82:9:82:9 | p |
| tst.js:82:9:82:9 | p |
edges
| tst.js:5:9:5:27 | foo | tst.js:6:5:6:7 | foo |
| tst.js:5:9:5:27 | foo | tst.js:6:5:6:7 | foo |
@@ -50,13 +57,21 @@ edges
| tst.js:45:9:45:35 | foo | tst.js:46:5:46:7 | foo |
| tst.js:45:15:45:35 | ctx.req ... ery.foo | tst.js:45:9:45:35 | foo |
| tst.js:45:15:45:35 | ctx.req ... ery.foo | tst.js:45:9:45:35 | foo |
| tst.js:77:25:77:38 | req.query.path | tst.js:80:23:80:23 | p |
| tst.js:77:25:77:38 | req.query.path | tst.js:80:23:80:23 | p |
| tst.js:80:23:80:23 | p | tst.js:81:9:81:9 | p |
| tst.js:80:23:80:23 | p | tst.js:81:9:81:9 | p |
| tst.js:80:23:80:23 | p | tst.js:82:9:82:9 | p |
| tst.js:80:23:80:23 | p | tst.js:82:9:82:9 | p |
#select
| tst.js:6:5:6:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:6:5:6:7 | foo | Potential type confusion for $@. | tst.js:5:15:5:27 | req.query.foo | HTTP request parameter |
| tst.js:8:5:8:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:8:5:8:7 | foo | Potential type confusion for $@. | tst.js:5:15:5:27 | req.query.foo | HTTP request parameter |
| tst.js:11:9:11:11 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:11:9:11:11 | foo | Potential type confusion for $@. | tst.js:5:15:5:27 | req.query.foo | HTTP request parameter |
| tst.js:15:9:15:11 | bar | tst.js:5:15:5:27 | req.query.foo | tst.js:15:9:15:11 | bar | Potential type confusion for $@. | tst.js:5:15:5:27 | req.query.foo | HTTP request parameter |
| tst.js:27:5:27:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:27:5:27:7 | foo | Potential type confusion for $@. | tst.js:5:15:5:27 | req.query.foo | HTTP request parameter |
| tst.js:28:5:28:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:28:5:28:7 | foo | Potential type confusion for $@. | tst.js:5:15:5:27 | req.query.foo | HTTP request parameter |
| tst.js:36:9:36:11 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:36:9:36:11 | foo | Potential type confusion for $@. | tst.js:5:15:5:27 | req.query.foo | HTTP request parameter |
| tst.js:41:5:41:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:41:5:41:7 | foo | Potential type confusion for $@. | tst.js:5:15:5:27 | req.query.foo | HTTP request parameter |
| tst.js:46:5:46:7 | foo | tst.js:45:15:45:35 | ctx.req ... ery.foo | tst.js:46:5:46:7 | foo | Potential type confusion for $@. | tst.js:45:15:45:35 | ctx.req ... ery.foo | HTTP request parameter |
| tst.js:6:5:6:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:6:5:6:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
| tst.js:8:5:8:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:8:5:8:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
| tst.js:11:9:11:11 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:11:9:11:11 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
| tst.js:15:9:15:11 | bar | tst.js:5:15:5:27 | req.query.foo | tst.js:15:9:15:11 | bar | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
| tst.js:27:5:27:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:27:5:27:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
| tst.js:28:5:28:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:28:5:28:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
| tst.js:36:9:36:11 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:36:9:36:11 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
| tst.js:41:5:41:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:41:5:41:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
| tst.js:46:5:46:7 | foo | tst.js:45:15:45:35 | ctx.req ... ery.foo | tst.js:46:5:46:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:45:15:45:35 | ctx.req ... ery.foo | this HTTP request parameter |
| tst.js:81:9:81:9 | p | tst.js:77:25:77:38 | req.query.path | tst.js:81:9:81:9 | p | Potential type confusion as $@ may be either an array or a string. | tst.js:77:25:77:38 | req.query.path | this HTTP request parameter |
| tst.js:82:9:82:9 | p | tst.js:77:25:77:38 | req.query.path | tst.js:82:9:82:9 | p | Potential type confusion as $@ may be either an array or a string. | tst.js:77:25:77:38 | req.query.path | this HTTP request parameter |

13
javascript/ql/test/query-tests/Security/CWE-843/tst.js
View File

@@ -71,3 +71,16 @@ express().get('/some/path/:foo', function(req, res) {
p.length < 1; // OK
});
express().get('/some/path/:foo', function(req, res) {
let someObject = {};
safeGet(someObject, req.query.path).bar = 'baz'; // prototype pollution here - but flagged in `safeGet`
});
function safeGet(obj, p) {
if (p === '__proto__' || // NOT OK - could be singleton array
p === 'constructor') { // NOT OK - could be singleton array
return null;
}
return obj[p];
}