hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-30 11:15:13 +02:00
Code Issues Packages Projects Releases Wiki Activity

JS: Fix FP in js/type-confusion-through-parameter-tampering

Browse Source
This commit is contained in:
Asger F
2022-05-25 09:41:17 +02:00
parent efda248bea
commit 877a9d8bcc
3 changed files with 80 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll
View File

@@ -27,4 +27,32 @@ class Configuration extends DataFlow::Configuration {
}
override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
guard instanceof TypeOfTestBarrier or
guard instanceof IsArrayBarrier
}
}
private class TypeOfTestBarrier extends DataFlow::BarrierGuardNode, DataFlow::ValueNode {
override EqualityTest astNode;
private Expr operand;
TypeOfTestBarrier() { astNode.getAnOperand().(TypeofExpr).getOperand() = operand }
override predicate blocks(boolean outcome, Expr e) {
e = operand and
if astNode.getAnOperand().getStringValue() = ["string", "object"]
then outcome = [true, false] // separation between string/array removes type confusion in both branches
else outcome = astNode.getPolarity() // block flow to branch where value is neither string nor array
}
}
private class IsArrayBarrier extends DataFlow::BarrierGuardNode, DataFlow::CallNode {
IsArrayBarrier() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray").getACall() }
override predicate blocks(boolean outcome, Expr e) {
e = getArgument(0).asExpr() and
outcome = [true, false] // separation between string/array removes type confusion in both branches
}
}

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

@@ -32,6 +32,18 @@ nodes
| tst.js:81:9:81:9 | p |
| tst.js:82:9:82:9 | p |
| tst.js:82:9:82:9 | p |
| tst.js:90:5:90:12 | data.foo |
| tst.js:90:5:90:12 | data.foo |
| tst.js:90:5:90:12 | data.foo |
| tst.js:92:9:92:16 | data.foo |
| tst.js:92:9:92:16 | data.foo |
| tst.js:92:9:92:16 | data.foo |
| tst.js:95:9:95:16 | data.foo |
| tst.js:95:9:95:16 | data.foo |
| tst.js:95:9:95:16 | data.foo |
| tst.js:98:9:98:16 | data.foo |
| tst.js:98:9:98:16 | data.foo |
| tst.js:98:9:98:16 | data.foo |
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 |
@@ -63,6 +75,10 @@ edges
| 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 |
| tst.js:90:5:90:12 | data.foo | tst.js:90:5:90:12 | data.foo |
| tst.js:92:9:92:16 | data.foo | tst.js:92:9:92:16 | data.foo |
| tst.js:95:9:95:16 | data.foo | tst.js:95:9:95:16 | data.foo |
| tst.js:98:9:98:16 | data.foo | tst.js:98:9:98:16 | data.foo |
#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 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 |
@@ -75,3 +91,7 @@ edges
| 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 |
| tst.js:90:5:90:12 | data.foo | tst.js:90:5:90:12 | data.foo | tst.js:90:5:90:12 | data.foo | Potential type confusion as $@ may be either an array or a string. | tst.js:90:5:90:12 | data.foo | this HTTP request parameter |
| tst.js:92:9:92:16 | data.foo | tst.js:92:9:92:16 | data.foo | tst.js:92:9:92:16 | data.foo | Potential type confusion as $@ may be either an array or a string. | tst.js:92:9:92:16 | data.foo | this HTTP request parameter |
| tst.js:95:9:95:16 | data.foo | tst.js:95:9:95:16 | data.foo | tst.js:95:9:95:16 | data.foo | Potential type confusion as $@ may be either an array or a string. | tst.js:95:9:95:16 | data.foo | this HTTP request parameter |
| tst.js:98:9:98:16 | data.foo | tst.js:98:9:98:16 | data.foo | tst.js:98:9:98:16 | data.foo | Potential type confusion as $@ may be either an array or a string. | tst.js:98:9:98:16 | data.foo | this HTTP request parameter |

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

@@ -1,7 +1,7 @@
var express = require('express');
var Koa = require('koa');
express().get('/some/path', function(req, res) {
express().get('/some/path', function (req, res) {
var foo = req.query.foo;
foo.indexOf(); // NOT OK
@@ -41,38 +41,38 @@ express().get('/some/path', function(req, res) {
foo.length; // NOT OK
});
new Koa().use(function handler(ctx){
new Koa().use(function handler(ctx) {
var foo = ctx.request.query.foo;
foo.indexOf(); // NOT OK
});
express().get('/some/path/:foo', function(req, res) {
express().get('/some/path/:foo', function (req, res) {
var foo = req.params.foo;
foo.indexOf(); // OK
});
express().get('/some/path/:foo', function(req, res) {
if (req.query.path.length) {} // OK
express().get('/some/path/:foo', function (req, res) {
if (req.query.path.length) { } // OK
req.query.path.length == 0; // OK
!req.query.path.length; // OK
req.query.path.length > 0; // OK
});
express().get('/some/path/:foo', function(req, res) {
express().get('/some/path/:foo', function (req, res) {
let p = req.query.path;
if (typeof p !== 'string') {
return;
return;
}
while (p.length) { // OK
p = p.substr(1);
p = p.substr(1);
}
p.length < 1; // OK
});
express().get('/some/path/:foo', function(req, res) {
express().get('/some/path/:foo', function (req, res) {
let someObject = {};
safeGet(someObject, req.query.path).bar = 'baz'; // prototype pollution here - but flagged in `safeGet`
});
@@ -84,3 +84,26 @@ function safeGet(obj, p) {
}
return obj[p];
}
express().get('/foo', function (req, res) {
let data = req.query;
data.foo.indexOf(); // NOT OK
if (typeof data.foo !== 'undefined') {
data.foo.indexOf(); // NOT OK
}
if (typeof data.foo !== 'string') {
data.foo.indexOf(); // OK
}
if (typeof data.foo !== 'undefined') {
data.foo.indexOf(); // NOT OK
}
});
express().get('/foo', function (req, res) {
let data = req.query;
if (Array.isArray(data)) {
data.indexOf(); // OK
} else {
data.indexOf(); // OK
}
});