hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-24 04:36:35 +01:00
Code Issues Packages Projects Releases Wiki Activity

add support for Object.hasOwn(obj, key)

Browse Source
This commit is contained in:
Erik Krogh Kristensen
2022-05-24 13:47:41 +02:00
parent 1717d17fb3
commit 2a97dd9f6f
11 changed files with 171 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
javascript/ql/lib/semmle/javascript/MembershipCandidates.qll
View File

@@ -234,6 +234,14 @@ module MembershipCandidate {
test = hasOwn and
hasOwn.calls(membersNode, "hasOwnProperty")
)
or
exists(DataFlow::CallNode hasOwn |
hasOwn = DataFlow::globalVarRef("Object").getAMemberCall("hasOwn")
|
hasOwn.getArgument(0).asExpr() = membersNode and
this = hasOwn.getArgument(1) and
test = hasOwn.asExpr()
)
}
override DataFlow::Node getTest() { result = test.flow() }

16
javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
View File

@@ -1027,18 +1027,16 @@ module TaintTracking {
class WhitelistContainmentCallSanitizer extends AdditionalSanitizerGuardNode,
DataFlow::MethodCallNode {
WhitelistContainmentCallSanitizer() {
exists(string name |
name = "contains" or
name = "has" or
name = "hasOwnProperty"
|
this.getMethodName() = name
)
this.getMethodName() = ["contains", "has", "hasOwnProperty", "hasOwn"]
}
override predicate sanitizes(boolean outcome, Expr e) {
outcome = true and
e = this.getArgument(0).asExpr()
exists(int propertyIndex |
if this.getMethodName() = "hasOwn" then propertyIndex = 1 else propertyIndex = 0
|
outcome = true and
e = this.getArgument(propertyIndex).asExpr()
)
}
override predicate appliesTo(Configuration cfg) { any() }

2
javascript/ql/src/Declarations/UnusedProperty.ql
View File

@@ -27,6 +27,8 @@ predicate hasUnknownPropertyRead(LocalObject obj) {
or
exists(obj.getAPropertyRead("hasOwnProperty"))
or
obj.flowsTo(DataFlow::globalVarRef("Object").getAMemberCall("hasOwn").getArgument(0))
or
exists(obj.getAPropertyRead("propertyIsEnumerable"))
}

14
javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.ql
View File

@@ -286,6 +286,7 @@ class PropNameTracking extends DataFlow::Configuration {
node instanceof DenyListEqualityGuard or
node instanceof AllowListEqualityGuard or
node instanceof HasOwnPropertyGuard or
node instanceof HasOwnGuard or
node instanceof InExprGuard or
node instanceof InstanceOfGuard or
node instanceof TypeofGuard or
@@ -355,6 +356,19 @@ class HasOwnPropertyGuard extends DataFlow::BarrierGuardNode, CallNode {
}
}
/** Sanitizer guard for calls to `Object.hasOwn(obj, prop)`. */
class HasOwnGuard extends DataFlow::BarrierGuardNode, CallNode {
HasOwnGuard() {
this = DataFlow::globalVarRef("Object").getAMemberCall("hasOwn") and
// same check as in HasOwnPropertyGuard
not arePropertiesEnumerated(getArgument(0).getALocalSource())
}
override predicate blocks(boolean outcome, Expr e) {
e = getArgument(1).asExpr() and outcome = true
}
}
/**
* A sanitizer guard for `key in dst`.
*

14
javascript/ql/test/library-tests/TaintBarriers/tests.expected
View File

@@ -43,6 +43,10 @@ isLabeledBarrier
| ExampleConfiguration | tst.js:361:14:361:14 | v | taint |
| ExampleConfiguration | tst.js:371:14:371:16 | o.p | taint |
| ExampleConfiguration | tst.js:378:14:378:17 | o[p] | taint |
| ExampleConfiguration | tst.js:392:14:392:14 | v | taint |
| ExampleConfiguration | tst.js:394:14:394:16 | v.p | taint |
| ExampleConfiguration | tst.js:396:14:396:18 | v.p.q | taint |
| ExampleConfiguration | tst.js:402:14:402:14 | v | taint |
isSanitizer
| ExampleConfiguration | tst.js:176:18:176:18 | v |
sanitizingGuard
@@ -122,6 +126,13 @@ sanitizingGuard
| tst.js:370:9:370:29 | o.p == ... listed" | tst.js:370:16:370:29 | "white-listed" | true |
| tst.js:377:11:377:32 | o[p] == ... listed" | tst.js:377:11:377:14 | o[p] | true |
| tst.js:377:11:377:32 | o[p] == ... listed" | tst.js:377:19:377:32 | "white-listed" | true |
| tst.js:391:9:391:27 | o.hasOwnProperty(v) | tst.js:391:26:391:26 | v | true |
| tst.js:393:16:393:36 | o.hasOw ... ty(v.p) | tst.js:393:33:393:35 | v.p | true |
| tst.js:395:16:395:38 | o.hasOw ... (v.p.q) | tst.js:395:33:395:37 | v.p.q | true |
| tst.js:397:16:397:36 | o.hasOw ... ty(v.p) | tst.js:397:33:397:35 | v.p | true |
| tst.js:399:16:399:41 | o.hasOw ... "p.q"]) | tst.js:399:33:399:40 | v["p.q"] | true |
| tst.js:401:16:401:34 | Object.hasOwn(o, v) | tst.js:401:30:401:30 | o | true |
| tst.js:401:16:401:34 | Object.hasOwn(o, v) | tst.js:401:33:401:33 | v | true |
taintedSink
| tst.js:2:13:2:20 | SOURCE() | tst.js:3:10:3:10 | v |
| tst.js:2:13:2:20 | SOURCE() | tst.js:8:14:8:14 | v |
@@ -186,3 +197,6 @@ taintedSink
| tst.js:367:13:367:20 | SOURCE() | tst.js:373:14:373:16 | o.p |
| tst.js:367:13:367:20 | SOURCE() | tst.js:380:14:380:17 | o[p] |
| tst.js:367:13:367:20 | SOURCE() | tst.js:382:14:382:17 | o[p] |
| tst.js:388:13:388:20 | SOURCE() | tst.js:389:10:389:14 | v.p.q |
| tst.js:388:13:388:20 | SOURCE() | tst.js:398:14:398:14 | v |
| tst.js:388:13:388:20 | SOURCE() | tst.js:400:14:400:18 | v.p.q |

19
javascript/ql/test/library-tests/TaintBarriers/tst.js
View File

@@ -383,3 +383,22 @@ function constantComparisonSanitizer2() {
}
}
}
function propertySanitization(o) {
var v = SOURCE();
SINK(v.p.q); // NOT OK
if (o.hasOwnProperty(v)) {
SINK(v); // OK
} else if (o.hasOwnProperty(v.p)) {
SINK(v.p); // OK
} else if (o.hasOwnProperty(v.p.q)) {
SINK(v.p.q); // OK
} else if (o.hasOwnProperty(v.p)) {
SINK(v); // NOT OK
} else if (o.hasOwnProperty(v["p.q"])) {
SINK(v.p.q); // NOT OK
} else if (Object.hasOwn(o, v)) {
SINK(v); // OK
}
}

8
javascript/ql/test/query-tests/Declarations/UnusedProperty/tst.js
View File

@@ -81,3 +81,11 @@
(function(){
({ unusedProp: 42 }, 42);
});
(function(){
var foo = {
unused: 42
};
foo.unused = 42;
Object.hasOwn(foo, blab);
});

15
javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected
View File

@@ -50,6 +50,13 @@ nodes
| express.js:146:16:146:24 | query.foo |
| express.js:146:16:146:24 | query.foo |
| express.js:146:16:146:24 | query.foo |
| express.js:150:7:150:34 | target |
| express.js:150:16:150:34 | req.param("target") |
| express.js:150:16:150:34 | req.param("target") |
| express.js:155:18:155:23 | target |
| express.js:155:18:155:23 | target |
| express.js:160:18:160:23 | target |
| express.js:160:18:160:23 | target |
| koa.js:6:6:6:27 | url |
| koa.js:6:12:6:27 | ctx.query.target |
| koa.js:6:12:6:27 | ctx.query.target |
@@ -140,6 +147,12 @@ edges
| express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user |
| express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo |
| express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo |
| express.js:150:7:150:34 | target | express.js:155:18:155:23 | target |
| express.js:150:7:150:34 | target | express.js:155:18:155:23 | target |
| express.js:150:7:150:34 | target | express.js:160:18:160:23 | target |
| express.js:150:7:150:34 | target | express.js:160:18:160:23 | target |
| express.js:150:16:150:34 | req.param("target") | express.js:150:7:150:34 | target |
| express.js:150:16:150:34 | req.param("target") | express.js:150:7:150:34 | target |
| koa.js:6:6:6:27 | url | koa.js:7:15:7:17 | url |
| koa.js:6:6:6:27 | url | koa.js:7:15:7:17 | url |
| koa.js:6:6:6:27 | url | koa.js:8:18:8:20 | url |
@@ -199,6 +212,8 @@ edges
| express.js:136:16:136:36 | 'u' + r ... ms.user | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user | Untrusted URL redirection due to $@. | express.js:136:22:136:36 | req.params.user | user-provided value |
| express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo | Untrusted URL redirection due to $@. | express.js:143:16:143:28 | req.query.foo | user-provided value |
| express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo | Untrusted URL redirection due to $@. | express.js:146:16:146:24 | query.foo | user-provided value |
| express.js:155:18:155:23 | target | express.js:150:16:150:34 | req.param("target") | express.js:155:18:155:23 | target | Untrusted URL redirection due to $@. | express.js:150:16:150:34 | req.param("target") | user-provided value |
| express.js:160:18:160:23 | target | express.js:150:16:150:34 | req.param("target") | express.js:160:18:160:23 | target | Untrusted URL redirection due to $@. | express.js:150:16:150:34 | req.param("target") | user-provided value |
| koa.js:7:15:7:17 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:7:15:7:17 | url | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |
| koa.js:8:15:8:26 | `${url}${x}` | koa.js:6:12:6:27 | ctx.query.target | koa.js:8:15:8:26 | `${url}${x}` | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |
| koa.js:14:16:14:18 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:14:16:14:18 | url | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |

16
javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js
View File

@@ -144,4 +144,18 @@ app.get("foo", (req, res) => {
});
app.get("bar", ({query}, res) => {
res.redirect(query.foo); // NOT OK
})
})
app.get('/some/path', function(req, res) {
let target = req.param("target");
if (SAFE_TARGETS.hasOwnProperty(target))
res.redirect(target); // OK: request parameter is checked against whitelist
else
res.redirect(target); // NOT OK
if (Object.hasOwn(SAFE_TARGETS, target))
res.redirect(target); // OK: request parameter is checked against whitelist
else
res.redirect(target); // NOT OK
});

57
javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingFunction/PrototypePollutingFunction.expected
View File

@@ -1478,6 +1478,31 @@ nodes
| tests.js:547:24:547:28 | value |
| tests.js:547:24:547:28 | value |
| tests.js:547:24:547:28 | value |
| tests.js:552:35:552:37 | src |
| tests.js:552:35:552:37 | src |
| tests.js:553:14:553:16 | key |
| tests.js:553:14:553:16 | key |
| tests.js:553:14:553:16 | key |
| tests.js:557:43:557:45 | src |
| tests.js:557:43:557:45 | src |
| tests.js:557:43:557:50 | src[key] |
| tests.js:557:43:557:50 | src[key] |
| tests.js:557:43:557:50 | src[key] |
| tests.js:557:43:557:50 | src[key] |
| tests.js:557:43:557:50 | src[key] |
| tests.js:559:17:559:19 | key |
| tests.js:559:17:559:19 | key |
| tests.js:559:17:559:19 | key |
| tests.js:559:24:559:26 | src |
| tests.js:559:24:559:26 | src |
| tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:31 | src[key] |
| tests.js:559:28:559:30 | key |
| tests.js:559:28:559:30 | key |
edges
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
@@ -3347,6 +3372,38 @@ edges
| tests.js:545:43:545:47 | value | tests.js:542:35:542:37 | src |
| tests.js:545:43:545:47 | value | tests.js:542:35:542:37 | src |
| tests.js:545:43:545:47 | value | tests.js:542:35:542:37 | src |
| tests.js:552:35:552:37 | src | tests.js:557:43:557:45 | src |
| tests.js:552:35:552:37 | src | tests.js:557:43:557:45 | src |
| tests.js:552:35:552:37 | src | tests.js:559:24:559:26 | src |
| tests.js:552:35:552:37 | src | tests.js:559:24:559:26 | src |
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
| tests.js:553:14:553:16 | key | tests.js:559:28:559:30 | key |
| tests.js:553:14:553:16 | key | tests.js:559:28:559:30 | key |
| tests.js:553:14:553:16 | key | tests.js:559:28:559:30 | key |
| tests.js:553:14:553:16 | key | tests.js:559:28:559:30 | key |
| tests.js:557:43:557:45 | src | tests.js:557:43:557:50 | src[key] |
| tests.js:557:43:557:45 | src | tests.js:557:43:557:50 | src[key] |
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
| tests.js:559:24:559:26 | src | tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:26 | src | tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:26 | src | tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:26 | src | tests.js:559:24:559:31 | src[key] |
| tests.js:559:24:559:31 | src[key] | tests.js:559:24:559:31 | src[key] |
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
#select
| examples/PrototypePollutingFunction.js:7:13:7:15 | dst | examples/PrototypePollutingFunction.js:2:14:2:16 | key | examples/PrototypePollutingFunction.js:7:13:7:15 | dst | Properties are copied from $@ to $@ without guarding against prototype pollution. | examples/PrototypePollutingFunction.js:2:21:2:23 | src | src | examples/PrototypePollutingFunction.js:7:13:7:15 | dst | dst |
| path-assignment.js:15:13:15:18 | target | path-assignment.js:8:19:8:25 | keys[i] | path-assignment.js:15:13:15:18 | target | The property chain $@ is recursively assigned to $@ without guarding against prototype pollution. | path-assignment.js:8:19:8:25 | keys[i] | here | path-assignment.js:15:13:15:18 | target | target |

12
javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingFunction/tests.js
View File

@@ -548,3 +548,15 @@ function mergeUsingCallback3(dst, src) {
}
});
}
function copyHasOwnProperty2(dst, src) {
for (let key in src) {
// Guarding the recursive case by dst.hasOwnProperty (or Object.hasOwn) is safe,
// since '__proto__' and 'constructor' are not own properties of the destination object.
if (Object.hasOwn(dst, key)) {
copyHasOwnProperty2(dst[key], src[key]);
} else {
dst[key] = src[key]; // OK
}
}
}