JS: Fix TypeofCheck

asgerf · asgerf · commit 254ac7f96344 · 2020-12-07T10:46:00.000Z
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignment.qll b/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignment.qll
@@ -163,15 +163,18 @@ module PrototypePollutingAssignment {
     string value;
 
     TypeofCheck() {
-      astNode.getLeftOperand().(TypeofExpr).getOperand() = operand and
-      astNode.getRightOperand().getStringValue() = value
+      exists(TypeofExpr typeof, Expr str |
+        astNode.hasOperands(typeof, str) and
+        typeof.getOperand() = operand and
+        str.getStringValue() = value
+      )
     }
 
     override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
       (
-        value = "object" and outcome = false
+        value = "object" and outcome = astNode.getPolarity().booleanNot()
         or
-        value != "object" and outcome = true
+        value != "object" and outcome = astNode.getPolarity()
       ) and
       e = operand and
       label instanceof ObjectPrototype
diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected
@@ -17,8 +17,12 @@ nodes
 | tst.js:33:23:33:25 | obj |
 | tst.js:34:5:34:7 | obj |
 | tst.js:34:5:34:7 | obj |
-| tst.js:42:9:42:11 | obj |
-| tst.js:42:9:42:11 | obj |
+| tst.js:39:9:39:11 | obj |
+| tst.js:39:9:39:11 | obj |
+| tst.js:45:9:45:11 | obj |
+| tst.js:45:9:45:11 | obj |
+| tst.js:48:9:48:11 | obj |
+| tst.js:48:9:48:11 | obj |
 edges
 | tst.js:5:9:5:38 | taint | tst.js:8:12:8:16 | taint |
 | tst.js:5:9:5:38 | taint | tst.js:9:12:9:16 | taint |
@@ -37,11 +41,17 @@ edges
 | tst.js:14:27:14:31 | taint | tst.js:14:5:14:32 | unsafeG ...  taint) |
 | tst.js:33:23:33:25 | obj | tst.js:34:5:34:7 | obj |
 | tst.js:33:23:33:25 | obj | tst.js:34:5:34:7 | obj |
-| tst.js:33:23:33:25 | obj | tst.js:42:9:42:11 | obj |
-| tst.js:33:23:33:25 | obj | tst.js:42:9:42:11 | obj |
+| tst.js:33:23:33:25 | obj | tst.js:39:9:39:11 | obj |
+| tst.js:33:23:33:25 | obj | tst.js:39:9:39:11 | obj |
+| tst.js:33:23:33:25 | obj | tst.js:45:9:45:11 | obj |
+| tst.js:33:23:33:25 | obj | tst.js:45:9:45:11 | obj |
+| tst.js:33:23:33:25 | obj | tst.js:48:9:48:11 | obj |
+| tst.js:33:23:33:25 | obj | tst.js:48:9:48:11 | obj |
 #select
 | tst.js:8:5:8:17 | object[taint] | tst.js:5:24:5:37 | req.query.data | tst.js:8:5:8:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
 | tst.js:9:5:9:17 | object[taint] | tst.js:5:24:5:37 | req.query.data | tst.js:9:5:9:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
 | tst.js:14:5:14:32 | unsafeG ...  taint) | tst.js:5:24:5:37 | req.query.data | tst.js:14:5:14:32 | unsafeG ...  taint) | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
 | tst.js:34:5:34:7 | obj | tst.js:5:24:5:37 | req.query.data | tst.js:34:5:34:7 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
-| tst.js:42:9:42:11 | obj | tst.js:5:24:5:37 | req.query.data | tst.js:42:9:42:11 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
+| tst.js:39:9:39:11 | obj | tst.js:5:24:5:37 | req.query.data | tst.js:39:9:39:11 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
+| tst.js:45:9:45:11 | obj | tst.js:5:24:5:37 | req.query.data | tst.js:45:9:45:11 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
+| tst.js:48:9:48:11 | obj | tst.js:5:24:5:37 | req.query.data | tst.js:48:9:48:11 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js
@@ -35,12 +35,21 @@ function mutateObject(obj, x) {
     if (obj instanceof Object) {
         obj.foo = x; // OK
     }
+    if (obj != null) {
+        obj.foo = x; // NOT OK
+    }
     if (typeof obj === 'function') {
         obj.foo = x; // OK
     }
-    if (obj != null) {
+    if (typeof obj !== 'function') {
         obj.foo = x; // NOT OK
     }
+    if (typeof obj === 'object') {
+        obj.foo = x; // NOT OK
+    }
+    if (typeof obj !== 'object') {
+        obj.foo = x; // OK
+    }
 }
 
 function unsafeGetProp(obj, prop) {

Original file line number	Diff line number	Diff line change
`@@ -35,12 +35,21 @@ function mutateObject(obj, x) {`
`35`	`35`	`if (obj instanceof Object) {`
`36`	`36`	`obj.foo = x; // OK`
`37`	`37`	`}`
	`38`	`+ if (obj != null) {`
	`39`	`+ obj.foo = x; // NOT OK`
	`40`	`+ }`
`38`	`41`	`if (typeof obj === 'function') {`
`39`	`42`	`obj.foo = x; // OK`
`40`	`43`	`}`
`41`		`- if (obj != null) {`
	`44`	`+ if (typeof obj !== 'function') {`
`42`	`45`	`obj.foo = x; // NOT OK`
`43`	`46`	`}`
	`47`	`+ if (typeof obj === 'object') {`
	`48`	`+ obj.foo = x; // NOT OK`
	`49`	`+ }`
	`50`	`+ if (typeof obj !== 'object') {`
	`51`	`+ obj.foo = x; // OK`
	`52`	`+ }`
`44`	`53`	`}`
`45`	`54`
`46`	`55`	`function unsafeGetProp(obj, prop) {`