Merge branch 'main' into dragAndDrop

Author: erik-krogh

cpp/ql/src/experimental/Security/CWE/CWE-020/NoCheckBeforeUnsafePutUser.ql

@@ -21,7 +21,7 @@ class WriteAccessCheckMacro extends Macro {
   VariableAccess va;
 
   WriteAccessCheckMacro() {
-    this.getName() = ["user_write_access_begin", "user_access_begin"] and
+    this.getName() = ["user_write_access_begin", "user_access_begin", "access_ok"] and
     va.getEnclosingElement() = this.getAnInvocation().getAnExpandedElement()
   }
 
@@ -37,7 +37,8 @@ class UnSafePutUserMacro extends Macro {
   }
 
   Expr getUserModePtr() {
-    result = writeUserPtr.getOperand().(AddressOfExpr).getOperand().(FieldAccess).getQualifier()
+    result = writeUserPtr.getOperand().(AddressOfExpr).getOperand().(FieldAccess).getQualifier() or
+    result = writeUserPtr.getOperand()
   }
 }
 
@@ -46,11 +47,13 @@ class ExploitableUserModePtrParam extends Parameter {
     not exists(WriteAccessCheckMacro writeAccessCheck |
       DataFlow::localFlow(DataFlow::parameterNode(this),
         DataFlow::exprNode(writeAccessCheck.getArgument()))
+    ) and
+    exists(UnSafePutUserMacro unsafePutUser |
+      DataFlow::localFlow(DataFlow::parameterNode(this),
+        DataFlow::exprNode(unsafePutUser.getUserModePtr()))
     )
   }
 }
 
-from ExploitableUserModePtrParam p, UnSafePutUserMacro unsafePutUser
-where
-  DataFlow::localFlow(DataFlow::parameterNode(p), DataFlow::exprNode(unsafePutUser.getUserModePtr()))
+from ExploitableUserModePtrParam p
 select p, "unsafe_put_user write user-mode pointer $@ without check.", p, p.toString()

javascript/ql/lib/semmle/javascript/Arrays.qll

@@ -45,9 +45,20 @@ module ArrayTaintTracking {
     )
     or
     // `array.reduce` with tainted value in callback
+    // The callback parameters are: (previousValue, currentValue, currentIndex, array)
     call.(DataFlow::MethodCallNode).getMethodName() = "reduce" and
-    pred = call.getArgument(0).(DataFlow::FunctionNode).getAReturn() and // Require the argument to be a closure to avoid spurious call/return flow
-    succ = call
+    exists(DataFlow::FunctionNode callback |
+      callback = call.getArgument(0) // Require the argument to be a closure to avoid spurious call/return flow
+    |
+      pred = callback.getAReturn() and
+      succ = call
+      or
+      pred = call.getReceiver() and
+      succ = callback.getParameter([1, 3]) // into currentValue or array
+      or
+      pred = [call.getArgument(1), callback.getAReturn()] and
+      succ = callback.getParameter(0) // into previousValue
+    )
     or
     // `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`.
     pred = call.getAnArgument() and

javascript/ql/lib/semmle/javascript/DOM.qll

@@ -430,6 +430,12 @@ module DOM {
     result.hasUnderlyingType("Element")
     or
     result.hasUnderlyingType(any(string s | s.matches("HTML%Element")))
+    or
+    exists(DataFlow::ClassNode cls |
+      cls.getASuperClassNode().getALocalSource() =
+        DataFlow::globalVarRef(any(string s | s.matches("HTML%Element"))) and
+      result = cls.getAnInstanceReference()
+    )
   }
 
   module LocationSource {

javascript/ql/src/change-notes/2022-04-07-missing-flow-fixes.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Improved handling of custom DOM elements, potentially leading to more alerts for the XSS queries.
+* Improved taint tracking through calls to the `Array.prototype.reduce` function.

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -16,6 +16,7 @@ typeInferenceMismatch
 | arrays.js:2:15:2:22 | source() | arrays.js:8:10:8:22 | arrayIfy(foo) |
 | arrays.js:2:15:2:22 | source() | arrays.js:11:10:11:28 | union(["bla"], foo) |
 | arrays.js:2:15:2:22 | source() | arrays.js:14:10:14:18 | flat(foo) |
+| arrays.js:2:15:2:22 | source() | arrays.js:19:10:19:12 | res |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:4:8:4:8 | x |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:13:10:13:10 | x |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:19:10:19:10 | x |

javascript/ql/test/library-tests/TaintTracking/arrays.js

@@ -12,4 +12,9 @@ function test() {
 
     const flat = require("arr-flatten");
     sink(flat(foo)); // NOT OK
-}
+
+    let res = foo.reduce((prev, current) => {
+        return prev + '<b>' + current + '</b>';
+    }, '');
+    sink(res); // NOT OK
+}

javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected

@@ -152,6 +152,10 @@ nodes
 | clipboard.ts:73:29:73:39 | droppedHtml |
 | clipboard.ts:73:29:73:39 | droppedHtml |
 | clipboard.ts:73:29:73:39 | droppedHtml |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
 | d3.js:4:12:4:22 | window.name |
 | d3.js:4:12:4:22 | window.name |
 | d3.js:4:12:4:22 | window.name |
@@ -1198,6 +1202,7 @@ edges
 | clipboard.ts:71:27:71:62 | e.clipb ... /html') | clipboard.ts:71:13:71:62 | droppedHtml |
 | clipboard.ts:71:27:71:62 | e.clipb ... /html') | clipboard.ts:71:13:71:62 | droppedHtml |
 | clipboard.ts:71:27:71:62 | e.clipb ... /html') | clipboard.ts:71:13:71:62 | droppedHtml |
+| custom-element.js:5:26:5:36 | window.name | custom-element.js:5:26:5:36 | window.name |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
@@ -2159,6 +2164,7 @@ edges
 | clipboard.ts:33:19:33:68 | e.origi ... /html') | clipboard.ts:33:19:33:68 | e.origi ... /html') | clipboard.ts:33:19:33:68 | e.origi ... /html') | Cross-site scripting vulnerability due to $@. | clipboard.ts:33:19:33:68 | e.origi ... /html') | user-provided value |
 | clipboard.ts:50:29:50:32 | html | clipboard.ts:43:22:43:55 | clipboa ... /html') | clipboard.ts:50:29:50:32 | html | Cross-site scripting vulnerability due to $@. | clipboard.ts:43:22:43:55 | clipboa ... /html') | user-provided value |
 | clipboard.ts:73:29:73:39 | droppedHtml | clipboard.ts:71:27:71:62 | e.clipb ... /html') | clipboard.ts:73:29:73:39 | droppedHtml | Cross-site scripting vulnerability due to $@. | clipboard.ts:71:27:71:62 | e.clipb ... /html') | user-provided value |
+| custom-element.js:5:26:5:36 | window.name | custom-element.js:5:26:5:36 | window.name | custom-element.js:5:26:5:36 | window.name | Cross-site scripting vulnerability due to $@. | custom-element.js:5:26:5:36 | window.name | user-provided value |
 | d3.js:11:15:11:24 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value |
 | d3.js:12:20:12:29 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value |
 | d3.js:14:20:14:29 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected

@@ -152,6 +152,10 @@ nodes
 | clipboard.ts:73:29:73:39 | droppedHtml |
 | clipboard.ts:73:29:73:39 | droppedHtml |
 | clipboard.ts:73:29:73:39 | droppedHtml |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
 | d3.js:4:12:4:22 | window.name |
 | d3.js:4:12:4:22 | window.name |
 | d3.js:4:12:4:22 | window.name |
@@ -1248,6 +1252,7 @@ edges
 | clipboard.ts:71:27:71:62 | e.clipb ... /html') | clipboard.ts:71:13:71:62 | droppedHtml |
 | clipboard.ts:71:27:71:62 | e.clipb ... /html') | clipboard.ts:71:13:71:62 | droppedHtml |
 | clipboard.ts:71:27:71:62 | e.clipb ... /html') | clipboard.ts:71:13:71:62 | droppedHtml |
+| custom-element.js:5:26:5:36 | window.name | custom-element.js:5:26:5:36 | window.name |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |

javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/custom-element.js

@@ -0,0 +1,7 @@
+import * as dummy from 'dummy';
+
+class CustomElm extends HTMLElement {
+    test() {
+        this.innerHTML = window.name; // NOT OK
+    }
+}

javascript/ql/test/query-tests/Security/CWE-312/BuildArtifactLeak.expected

@@ -37,12 +37,14 @@ edges
 | build-leaks.js:15:24:15:34 | process.env | build-leaks.js:14:18:14:20 | env |
 | build-leaks.js:15:24:15:34 | process.env | build-leaks.js:14:18:14:20 | env |
 | build-leaks.js:16:20:16:22 | env | build-leaks.js:13:17:19:10 | Object. ...      }) |
+| build-leaks.js:16:20:16:22 | env | build-leaks.js:14:18:14:20 | env |
 | build-leaks.js:21:11:26:5 | stringifed | build-leaks.js:30:22:30:31 | stringifed |
 | build-leaks.js:21:24:26:5 | {\\n      ... )\\n    } | build-leaks.js:21:11:26:5 | stringifed |
 | build-leaks.js:22:24:25:14 | Object. ...  }, {}) | build-leaks.js:21:24:26:5 | {\\n      ... )\\n    } |
 | build-leaks.js:22:49:22:51 | env | build-leaks.js:24:20:24:22 | env |
 | build-leaks.js:23:39:23:41 | raw | build-leaks.js:22:49:22:51 | env |
 | build-leaks.js:24:20:24:22 | env | build-leaks.js:22:24:25:14 | Object. ...  }, {}) |
+| build-leaks.js:24:20:24:22 | env | build-leaks.js:22:49:22:51 | env |
 | build-leaks.js:30:22:30:31 | stringifed | build-leaks.js:34:26:34:57 | getEnv( ... ngified |
 | build-leaks.js:30:22:30:31 | stringifed | build-leaks.js:34:26:34:57 | getEnv( ... ngified |
 | build-leaks.js:40:9:40:60 | pw | build-leaks.js:41:82:41:83 | pw |