Merge pull request #1862 from asger-semmle/prototype-pollution-angular-merge

Approved by esben-semmle
2025-12-17 01:03:14 +01:00 · 2019-09-05 12:50:58 +01:00
parent e6bfe2bd5d 0e4c34bd81
commit fd2e8486e4
9 changed files with 81 additions and 22 deletions
--- a/change-notes/1.23/analysis-javascript.md
+++ b/change-notes/1.23/analysis-javascript.md
@@ -18,7 +18,7 @@
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Client-side cross-site scripting (`js/xss`) | More results | More potential vulnerabilities involving functions that manipulate DOM attributes are now recognized. |
-| Prototype pollution (`js/prototype-pollution`) | Same results | The results are now shown on LGTM by default. |
+| Prototype pollution (`js/prototype-pollution`) | More results | The query now highlights vulnerable uses of jQuery and Angular, and the results are shown on LGTM by default. |

 ## Changes to QL libraries

--- a/javascript/ql/src/Security/CWE-400/PrototypePollution.ql
+++ b/javascript/ql/src/Security/CWE-400/PrototypePollution.ql
@@ -17,12 +17,10 @@ import DataFlow::PathGraph
 import semmle.javascript.dependencies.Dependencies

 from
-  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Dependency dependency,
-  string dependencyId
+  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, string moduleName, Locatable dependencyLoc
 where
  cfg.hasFlowPath(source, sink) and
-  dependency = sink.getNode().(Sink).getDependency() and
-  dependency.info(dependencyId, _)
+  sink.getNode().(Sink).dependencyInfo(moduleName, dependencyLoc)
 select sink.getNode(), source, sink,
  "Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@.",
-  source, "here", dependency, dependencyId
+  source, "here", dependencyLoc, moduleName
--- a/javascript/ql/src/semmle/javascript/Extend.qll
+++ b/javascript/ql/src/semmle/javascript/Extend.qll
@@ -93,7 +93,8 @@ private class ExtendCallDeep extends ExtendCall {
      callee = DataFlow::moduleMember("smart-extend", "deep") or
      callee = LodashUnderscore::member("merge") or
      callee = LodashUnderscore::member("mergeWith") or
-      callee = LodashUnderscore::member("defaultsDeep")
+      callee = LodashUnderscore::member("defaultsDeep") or
+      callee = AngularJS::angular().getAPropertyRead("merge")
    )
  }

@@ -122,7 +123,8 @@ private class ExtendCallShallow extends ExtendCall {
      callee = DataFlow::moduleImport("util-extend") or
      callee = DataFlow::moduleImport("utils-merge") or
      callee = DataFlow::moduleImport("xtend/mutable") or
-      callee = LodashUnderscore::member("extend")
+      callee = LodashUnderscore::member("extend") or
+      callee = AngularJS::angular().getAPropertyRead("extend")
    )
  }

--- a/javascript/ql/src/semmle/javascript/JsonParsers.qll
+++ b/javascript/ql/src/semmle/javascript/JsonParsers.qll
@@ -28,7 +28,8 @@ private class PlainJsonParserCall extends JsonParserCall {
      callee = DataFlow::globalVarRef("JSON").getAPropertyRead("parse") or
      callee = DataFlow::moduleImport("parse-json") or
      callee = DataFlow::moduleImport("json-parse-better-errors") or
-      callee = DataFlow::moduleImport("json-safe-parse")
+      callee = DataFlow::moduleImport("json-safe-parse") or
+      callee = AngularJS::angular().getAPropertyRead("fromJson")
    )
  }

--- a/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutionCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutionCustomizations.qll
@@ -52,9 +52,17 @@ module PrototypePollution {
    abstract DataFlow::FlowLabel getAFlowLabel();

    /**
-     * Gets the dependency that defines this sink.
+     * DEPRECATED. Override `dependencyInfo` instead.
     */
-    abstract Dependency getDependency();
+    deprecated Dependency getDependency() { none() }
+
+    /**
+     * Holds if `moduleName` is the name of the module that defines this sink,
+     * and `location` is the declaration of that dependency.
+     *
+     * If no meaningful `location` exists, it should be bound to the sink itself.
+     */
+    abstract predicate dependencyInfo(string moduleName, Locatable location);
  }

  /**
@@ -80,12 +88,21 @@ module PrototypePollution {

  class DeepExtendSink extends Sink {
    ExtendCall call;
-
-    Dependency dependency;
+    string moduleName;
+    Locatable location;

    DeepExtendSink() {
      this = call.getASourceOperand() and
-      isVulnerableDeepExtendCall(call, dependency)
+      (
+        exists(Dependency dep |
+          isVulnerableVersionOfDeepExtendCall(call, dep) and
+          dep = location and
+          dep.info(moduleName, _)
+        )
+        or
+        isVulnerableDeepExtendCallAllVersions(call, moduleName) and
+        location = call.asExpr()
+      )
    }

    override DataFlow::FlowLabel getAFlowLabel() {
@@ -94,13 +111,21 @@ module PrototypePollution {
      result = TaintedObjectWrapper::label()
    }

-    override Dependency getDependency() { result = dependency }
+    override predicate dependencyInfo(string moduleName_, Locatable loc) {
+      moduleName = moduleName_ and
+      location = loc
+    }
  }

+  /**
+   * DEPRECATED. Use `isVulnerableVersionOfDeepExtendCall` or `isVulnerableDeepExtendCallAllVersions` instead.
+   */
+  deprecated predicate isVulnerableDeepExtendCall = isVulnerableVersionOfDeepExtendCall/2;
+
  /**
   * Holds if `call` is vulnerable to prototype pollution because the callee is defined by `dep`.
   */
-  predicate isVulnerableDeepExtendCall(ExtendCall call, Dependency dep) {
+  predicate isVulnerableVersionOfDeepExtendCall(ExtendCall call, Dependency dep) {
    call.isDeep() and
    (
      call = DataFlow::dependencyModuleImport(dep).getAMemberCall(_) or
@@ -110,8 +135,6 @@ module PrototypePollution {
      id = "assign-deep" and
      version.maybeBefore("0.4.7")
      or
-      id = "deep"
-      or
      id = "deep-extend" and
      version.maybeBefore("0.5.1")
      or
@@ -121,13 +144,12 @@ module PrototypePollution {
      id = "extend" and
      (version.maybeBefore("2.0.2") or version.maybeBetween("3.0.0", "3.0.2"))
      or
-      id = "extend2"
-      or
-      id = "js-extend"
-      or
      id = "just-extend" and
      version.maybeBefore("4.0.1")
      or
+      id = "jquery" and
+      version.maybeBefore("3.4.0")
+      or
      id = "lodash" + any(string s) and
      version.maybeBefore("4.17.12")
      or
@@ -142,8 +164,31 @@ module PrototypePollution {
      or
      id = "node.extend" and
      (version.maybeBefore("1.1.7") or version.maybeBetween("2.0.0", "2.0.1"))
+    )
+  }
+
+  /**
+   * Holds if `call` comes from a package named `id` and is vulnerable to prototype pollution
+   * in every version of that package.
+   */
+  predicate isVulnerableDeepExtendCallAllVersions(ExtendCall call, string id) {
+    call.isDeep() and
+    (
+      call = DataFlow::moduleImport(id).getACall() or
+      call = DataFlow::moduleImport(id).getAMemberCall(_)
+    ) and
+    (
+      id = "deep"
+      or
+      id = "extend2"
+      or
+      id = "js-extend"
      or
      id = "smart-extend"
    )
+    or
+    call.isDeep() and
+    call = AngularJS::angular().getAMemberCall("merge") and
+    id = "angular"
  }
 }
--- a/javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.expected
+++ b/javascript/ql/test/library-tests/JsonParsers/JsonParserCalls.expected
@@ -1,3 +1,4 @@
+| angular.js:1:1:1:34 | checkJS ... input)) | OK |
 | tst.js:11:1:11:28 | checkJS ... input)) | OK |
 | tst.js:12:1:12:39 | checkJS ... input)) | OK |
 | tst.js:13:1:13:53 | checkJS ... input)) | OK |
--- a/javascript/ql/test/library-tests/JsonParsers/angular.js
+++ b/javascript/ql/test/library-tests/JsonParsers/angular.js
@@ -0,0 +1 @@
+checkJSON(angular.fromJson(input));
--- a/javascript/ql/test/query-tests/Security/CWE-400/PrototypePollution.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-400/PrototypePollution.expected
@@ -1,4 +1,8 @@
 nodes
+| angularmerge.js:1:30:1:34 | event |
+| angularmerge.js:2:21:2:42 | JSON.pa ... t.data) |
+| angularmerge.js:2:32:2:36 | event |
+| angularmerge.js:2:32:2:41 | event.data |
 | src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo |
 | src-vulnerable-lodash/tst.js:10:17:12:5 | {\\n      ... K\\n    } |
 | src-vulnerable-lodash/tst.js:11:16:11:30 | req.query.value |
@@ -6,10 +10,14 @@ nodes
 | src-vulnerable-lodash/tst.js:17:17:19:5 | {\\n      ... K\\n    } |
 | src-vulnerable-lodash/tst.js:18:16:18:25 | opts.thing |
 edges
+| angularmerge.js:1:30:1:34 | event | angularmerge.js:2:32:2:36 | event |
+| angularmerge.js:2:32:2:36 | event | angularmerge.js:2:32:2:41 | event.data |
+| angularmerge.js:2:32:2:41 | event.data | angularmerge.js:2:21:2:42 | JSON.pa ... t.data) |
 | src-vulnerable-lodash/tst.js:11:16:11:30 | req.query.value | src-vulnerable-lodash/tst.js:10:17:12:5 | {\\n      ... K\\n    } |
 | src-vulnerable-lodash/tst.js:15:14:15:28 | req.query.value | src-vulnerable-lodash/tst.js:18:16:18:25 | opts.thing |
 | src-vulnerable-lodash/tst.js:18:16:18:25 | opts.thing | src-vulnerable-lodash/tst.js:17:17:19:5 | {\\n      ... K\\n    } |
 #select
+| angularmerge.js:2:21:2:42 | JSON.pa ... t.data) | angularmerge.js:1:30:1:34 | event | angularmerge.js:2:21:2:42 | JSON.pa ... t.data) | Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@. | angularmerge.js:1:30:1:34 | event | here | angularmerge.js:2:3:2:43 | angular ... .data)) | angular |
 | src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo | src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo | src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo | Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@. | src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo | here | src-vulnerable-lodash/package.json:3:19:3:26 | "4.17.4" | lodash |
 | src-vulnerable-lodash/tst.js:10:17:12:5 | {\\n      ... K\\n    } | src-vulnerable-lodash/tst.js:11:16:11:30 | req.query.value | src-vulnerable-lodash/tst.js:10:17:12:5 | {\\n      ... K\\n    } | Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@. | src-vulnerable-lodash/tst.js:11:16:11:30 | req.query.value | here | src-vulnerable-lodash/package.json:3:19:3:26 | "4.17.4" | lodash |
 | src-vulnerable-lodash/tst.js:17:17:19:5 | {\\n      ... K\\n    } | src-vulnerable-lodash/tst.js:15:14:15:28 | req.query.value | src-vulnerable-lodash/tst.js:17:17:19:5 | {\\n      ... K\\n    } | Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@. | src-vulnerable-lodash/tst.js:15:14:15:28 | req.query.value | here | src-vulnerable-lodash/package.json:3:19:3:26 | "4.17.4" | lodash |
--- a/javascript/ql/test/query-tests/Security/CWE-400/angularmerge.js
+++ b/javascript/ql/test/query-tests/Security/CWE-400/angularmerge.js
@@ -0,0 +1,3 @@
+addEventListener("message", (event) => {
+  angular.merge({}, JSON.parse(event.data)); // NOT OK
+});