From bc3b983768e9800cd9b5521f5fe76e492213800e Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 14:24:37 +0000 Subject: [PATCH 01/17] JS: move CodeInjection tests into subfolder --- .../Security/CWE-094/{ => CodeInjection}/CodeInjection.expected | 0 .../Security/CWE-094/{ => CodeInjection}/CodeInjection.qlref | 0 .../query-tests/Security/CWE-094/{ => CodeInjection}/angularjs.js | 0 .../Security/CWE-094/{ => CodeInjection}/eslint-escope-build.js | 0 .../query-tests/Security/CWE-094/{ => CodeInjection}/express.js | 0 .../query-tests/Security/CWE-094/{ => CodeInjection}/externs.js | 0 .../Security/CWE-094/{ => CodeInjection}/react-native.js | 0 .../test/query-tests/Security/CWE-094/{ => CodeInjection}/tst.js | 0 8 files changed, 0 insertions(+), 0 deletions(-) rename javascript/ql/test/query-tests/Security/CWE-094/{ => CodeInjection}/CodeInjection.expected (100%) rename javascript/ql/test/query-tests/Security/CWE-094/{ => CodeInjection}/CodeInjection.qlref (100%) rename javascript/ql/test/query-tests/Security/CWE-094/{ => CodeInjection}/angularjs.js (100%) rename javascript/ql/test/query-tests/Security/CWE-094/{ => CodeInjection}/eslint-escope-build.js (100%) rename javascript/ql/test/query-tests/Security/CWE-094/{ => CodeInjection}/express.js (100%) rename javascript/ql/test/query-tests/Security/CWE-094/{ => CodeInjection}/externs.js (100%) rename javascript/ql/test/query-tests/Security/CWE-094/{ => CodeInjection}/react-native.js (100%) rename javascript/ql/test/query-tests/Security/CWE-094/{ => CodeInjection}/tst.js (100%) diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/CodeInjection.expected rename to javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection.qlref b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.qlref similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/CodeInjection.qlref rename to javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.qlref diff --git a/javascript/ql/test/query-tests/Security/CWE-094/angularjs.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/angularjs.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/angularjs.js rename to javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/angularjs.js diff --git a/javascript/ql/test/query-tests/Security/CWE-094/eslint-escope-build.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/eslint-escope-build.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/eslint-escope-build.js rename to javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/eslint-escope-build.js diff --git a/javascript/ql/test/query-tests/Security/CWE-094/express.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/express.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/express.js rename to javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/express.js diff --git a/javascript/ql/test/query-tests/Security/CWE-094/externs.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/externs.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/externs.js rename to javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/externs.js diff --git a/javascript/ql/test/query-tests/Security/CWE-094/react-native.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/react-native.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/react-native.js rename to javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/react-native.js diff --git a/javascript/ql/test/query-tests/Security/CWE-094/tst.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/tst.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/tst.js rename to javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/tst.js From 2239f863f745eba9550e51c9e9bcd1a4db0475c3 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 15:40:37 +0000 Subject: [PATCH 02/17] JS: add query MethodNameInjection --- change-notes/1.19/analysis-javascript.md | 1 + .../CWE-094/MethodNameInjection.qhelp | 43 +++++ .../Security/CWE-094/MethodNameInjection.ql | 17 ++ .../CWE-094/examples/MethodNameInjection.js | 4 + .../security/dataflow/MethodNameInjection.qll | 148 ++++++++++++++++++ .../MethodNameInjection.expected | 33 ++++ .../MethodNameInjection.qlref | 1 + .../CWE-094/MethodNameInjection/tst.js | 13 ++ 8 files changed, 260 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp create mode 100644 javascript/ql/src/Security/CWE-094/MethodNameInjection.ql create mode 100644 javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.qlref create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index d190d16dc147..011a6aa579ae 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -24,6 +24,7 @@ | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. | | File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. | | Host header poisoning in email generation | security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default. | +| Method name injection (`js/method-name-injection` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. | | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. | diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp b/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp new file mode 100644 index 000000000000..bee559f73972 --- /dev/null +++ b/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp @@ -0,0 +1,43 @@ + + + + +

+Invoking a user-controlled method on certain objects can lead to invocation of unsafe functions, +such as eval or the Function constructor. In particular, the global object +contains the eval function, and any function object contains the Function constructor +in its constructor property. +

+ + + +

+Avoid invoking user-controlled methods on the global object or on any function object. +

+ + + +

+In the following example, a user-controlled string is used as the method name on a cross-window message handler. +In this case, a malicious website can embed the page in an iframe, and execute arbitary code in the page by +sending a message to it with name set to eval. +

+ + + + + +
  • +OWASP: +Code Injection. +
    • +
  • +MDN: Global functions. +
    • +
  • +MDN: Function constructor. +
    • +     +     diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql b/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql new file mode 100644 index 000000000000..482f238886e0 --- /dev/null +++ b/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql @@ -0,0 +1,17 @@ +/** + * @name Method name injection + * @description Invoking user-controlled methods on a arbitrary objects can lead to remote code execution. + * @kind path-problem + * @problem.severity warning + * @precision high + * @id js/method-name-injection + * @tags security + * external/cwe/cwe-094 + */ +import javascript +import semmle.javascript.security.dataflow.MethodNameInjection::MethodNameInjection +import DataFlow::PathGraph + +from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink, source, sink, "Invocation of method derived from $@ may lead to remote code execution.", source.getNode(), "user-controlled value" diff --git a/javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js b/javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js new file mode 100644 index 000000000000..dc00dae290ba --- /dev/null +++ b/javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js @@ -0,0 +1,4 @@ +window.addEventListener("message", (ev) => { + let message = JSON.parse(ev.data); + window[message.name](message.payload); +}); diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll new file mode 100644 index 000000000000..26f1b8f448f2 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll @@ -0,0 +1,148 @@ +/** + * Provides a taint tracking configuration for reasoning about method invocations + * with a user-controlled method name. + */ + +import javascript +import semmle.javascript.frameworks.Express + +module MethodNameInjection { + private import DataFlow::FlowLabel + + /** + * A data flow source for method name injection. + */ + abstract class Source extends DataFlow::Node { + /** + * Gets the flow label relevant for this source. + */ + DataFlow::FlowLabel getFlowLabel() { + result = data() + } + } + + /** + * A data flow sink for method name injection. + */ + abstract class Sink extends DataFlow::Node { + /** + * Gets the flow label relevant for this sink + */ + abstract DataFlow::FlowLabel getFlowLabel(); + } + + /** + * A sanitizer for method name injection. + */ + abstract class Sanitizer extends DataFlow::Node { } + + /** + * Gets the flow label describing values that may refer to an unsafe + * function as a result of an attacker-controlled property name. + */ + UnsafeFunction unsafeFunction() { any() } + private class UnsafeFunction extends DataFlow::FlowLabel { + UnsafeFunction() { this = "UnsafeFunction" } + } + + /** + * A taint-tracking configuration for reasoning about method name injection. + */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "RemotePropertyInjection" } + + override predicate isSource(DataFlow::Node source) { + source instanceof Source + } + + override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { + sink.(Sink).getFlowLabel() = label + } + + override predicate isSanitizer(DataFlow::Node node) { + super.isSanitizer(node) or + node instanceof Sanitizer + } + + /** + * Holds if a property of the given object is an unsafe function. + */ + predicate isUnsafeBaseObject(DataFlow::SourceNode node) { + // eval an friends can be accessed from the global object. + node = DataFlow::globalObjectRef() + or + // 'constructor' property leads to the Function constructor. + node.analyze().getAValue() instanceof AbstractCallable + or + // Assume that a value that is invoked can refer to a function. + exists (node.getAnInvocation()) + } + + /** + * Holds if the `node` is of form `Object.create(null)` and so it has no prototype. + */ + predicate isPrototypeLessObject(DataFlow::MethodCallNode node) { + node = DataFlow::globalVarRef("Object").getAMethodCall("create") and + node.getArgument(0).asExpr() instanceof NullLiteral + } + + override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel, DataFlow::FlowLabel dstlabel) { + // Reading a property of the global object or of a function + exists (DataFlow::PropRead read | + isUnsafeBaseObject(read.getBase().getALocalSource()) and + src = read.getPropertyNameExpr().flow() and + dst = read and + srclabel = taint() and + dstlabel = unsafeFunction()) + or + // Reading a chain of properties from any object with a prototype can lead to Function + exists (PropertyProjection proj | + not isPrototypeLessObject(proj.getObject().getALocalSource()) and + src = proj.getASelector() and + dst = proj and + srclabel = taint() and + dstlabel = unsafeFunction()) + } + } + + /** + * A source of remote user input, considered as a source for method name injection. + */ + class RemoteFlowSourceAsSource extends Source { + RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource } + } + + /** + * The page URL considered as a flow source for method name injection. + */ + class DocumentUrlAsSource extends Source { + DocumentUrlAsSource() { isDocumentURL(asExpr()) } + } + + /** + * A function invocation of an unsafe function, as a sink for remote method name injection. + */ + class CalleeAsSink extends Sink { + CalleeAsSink() { + this = any(DataFlow::InvokeNode node).getCalleeNode() + } + + override DataFlow::FlowLabel getFlowLabel() { + result = unsafeFunction() + } + } + + /** + * A binary expression that sanitzes a value for method name injection. That + * is, if a string is prepended or appended to the remote input, an attacker + * cannot access arbitrary properties. + */ + class ConcatSanitizer extends Sanitizer, DataFlow::ValueNode { + + override BinaryExpr astNode; + + ConcatSanitizer() { + astNode.getAnOperand() instanceof ConstantString + } + } +} diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected new file mode 100644 index 000000000000..536af36eac86 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected @@ -0,0 +1,33 @@ +nodes +| tst.js:3:37:3:38 | ev | +| tst.js:4:9:4:37 | message | +| tst.js:4:19:4:37 | JSON.parse(ev.data) | +| tst.js:4:30:4:31 | ev | +| tst.js:4:30:4:36 | ev.data | +| tst.js:5:5:5:24 | window[message.name] | +| tst.js:5:12:5:18 | message | +| tst.js:5:12:5:23 | message.name | +| tst.js:6:9:6:28 | window[message.name] | +| tst.js:6:16:6:22 | message | +| tst.js:6:16:6:27 | message.name | +| tst.js:10:5:10:19 | f[message.name] | +| tst.js:10:7:10:13 | message | +| tst.js:10:7:10:18 | message.name | +edges +| tst.js:3:37:3:38 | ev | tst.js:4:30:4:31 | ev | +| tst.js:4:9:4:37 | message | tst.js:5:12:5:18 | message | +| tst.js:4:9:4:37 | message | tst.js:6:16:6:22 | message | +| tst.js:4:9:4:37 | message | tst.js:10:7:10:13 | message | +| tst.js:4:19:4:37 | JSON.parse(ev.data) | tst.js:4:9:4:37 | message | +| tst.js:4:30:4:31 | ev | tst.js:4:30:4:36 | ev.data | +| tst.js:4:30:4:36 | ev.data | tst.js:4:19:4:37 | JSON.parse(ev.data) | +| tst.js:5:12:5:18 | message | tst.js:5:12:5:23 | message.name | +| tst.js:5:12:5:23 | message.name | tst.js:5:5:5:24 | window[message.name] | +| tst.js:6:16:6:22 | message | tst.js:6:16:6:27 | message.name | +| tst.js:6:16:6:27 | message.name | tst.js:6:9:6:28 | window[message.name] | +| tst.js:10:7:10:13 | message | tst.js:10:7:10:18 | message.name | +| tst.js:10:7:10:18 | message.name | tst.js:10:5:10:19 | f[message.name] | +#select +| tst.js:5:5:5:24 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:5:5:5:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | +| tst.js:6:9:6:28 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:6:9:6:28 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | +| tst.js:10:5:10:19 | f[message.name] | tst.js:3:37:3:38 | ev | tst.js:10:5:10:19 | f[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.qlref b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.qlref new file mode 100644 index 000000000000..02e1d5737979 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.qlref @@ -0,0 +1 @@ +Security/CWE-094/MethodNameInjection.ql diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js new file mode 100644 index 000000000000..b50b1411e293 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js @@ -0,0 +1,13 @@ +let obj = {}; + +window.addEventListener('message', (ev) => { + let message = JSON.parse(ev.data); + window[message.name](message.payload); // NOT OK - may invoke eval + new window[message.name](message.payload); // NOT OK - may invoke jQuery $ function or similar + window["HTMLElement" + message.name](message.payload); // OK - concatenation restricts choice of methods + + function f() {} + f[message.name](message.payload)(); // NOT OK - may acccess Function constructor + + obj[message.name](message.payload); // OK - may crash, but no code execution involved +}); From a2e5003c09b80f93efcc90bb74c4f3debad53bc4 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 15:44:26 +0000 Subject: [PATCH 03/17] JS: add to security suite --- javascript/config/suites/javascript/security | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/config/suites/javascript/security b/javascript/config/suites/javascript/security index 20b62e4b3bd0..1481d5e28f75 100644 --- a/javascript/config/suites/javascript/security +++ b/javascript/config/suites/javascript/security @@ -16,6 +16,7 @@ + semmlecode-javascript-queries/Security/CWE-327/BrokenCryptoAlgorithm.ql: /Security/CWE/CWE-327 + semmlecode-javascript-queries/Security/CWE-338/InsecureRandomness.ql: /Security/CWE/CWE-338 + semmlecode-javascript-queries/Security/CWE-346/CorsMisconfigurationForCredentials.ql: /Security/CWE/CWE-346 ++ semmlecode-javascript-queries/Security/CWE-352/MethodNameInjection.ql: /Security/CWE/CWE-094 + semmlecode-javascript-queries/Security/CWE-352/MissingCsrfMiddleware.ql: /Security/CWE/CWE-352 + semmlecode-javascript-queries/Security/CWE-400/RemotePropertyInjection.ql: /Security/CWE/CWE-400 + semmlecode-javascript-queries/Security/CWE-502/UnsafeDeserialization.ql: /Security/CWE/CWE-502 From 8aff66616b23028b8493dc4d631823cb76d21293 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 15:49:24 +0000 Subject: [PATCH 04/17] JS: suppress similar alerts from RemotePropertyInjection --- .../dataflow/RemotePropertyInjection.qll | 16 +++++++++++++--- .../ql/test/query-tests/Security/CWE-400/tst.js | 3 ++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll index 1b2b20ad8e1e..362cd485fa5d 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll @@ -70,15 +70,25 @@ module RemotePropertyInjection { result = " a property name to write to." } } + + /** + * Holds if the method name injection on the given base object is handled by another query. + */ + private predicate isCoveredByMethodNameInjection(DataFlow::SourceNode node) { + node = DataFlow::globalObjectRef() + or + node.analyze().getAValue() instanceof AbstractCallable + } /** * A sink for method calls using dynamically computed method names. */ class MethodCallSink extends Sink, DataFlow::ValueNode { MethodCallSink() { - exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() | - exists (pr.getAnInvocation()) - ) + exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() | + exists (pr.getAnInvocation()) and + not isCoveredByMethodNameInjection(pr.getBase().getALocalSource()) + ) } override string getMessage() { diff --git a/javascript/ql/test/query-tests/Security/CWE-400/tst.js b/javascript/ql/test/query-tests/Security/CWE-400/tst.js index 644a5318bb68..d3fdbdffd82a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-400/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-400/tst.js @@ -14,7 +14,8 @@ app.get('/user/:id', function(req, res) { Object.defineProperty(myObj, prop, {value: 24}); // NOT OK var headers = {}; headers[prop] = 42; // NOT OK - res.set(headers); + res.set(headers); + myCoolLocalFct[req.query.x](); // OK - flagged by method name injection }); function myCoolLocalFct(x) { From 1c06f45046af9fddf32a057b49e7a3bdbc9a87ec Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 18:11:46 +0000 Subject: [PATCH 05/17] JS: address some comments --- .../ql/src/Security/CWE-094/MethodNameInjection.qhelp | 1 + javascript/ql/src/Security/CWE-094/MethodNameInjection.ql | 4 ++-- .../javascript/security/dataflow/MethodNameInjection.qll | 8 ++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp b/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp index bee559f73972..1dc3c41081a5 100644 --- a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp +++ b/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp @@ -15,6 +15,7 @@ in its constructor property.

    Avoid invoking user-controlled methods on the global object or on any function object. +Whitelist the permitted method names or change the type of object the methods are stored on.

    diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql b/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql index 482f238886e0..bd44c2e6f55c 100644 --- a/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql +++ b/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql @@ -1,8 +1,8 @@ /** * @name Method name injection - * @description Invoking user-controlled methods on a arbitrary objects can lead to remote code execution. + * @description Invoking user-controlled methods on arbitrary objects can lead to remote code execution. * @kind path-problem - * @problem.severity warning + * @problem.severity error * @precision high * @id js/method-name-injection * @tags security diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll index 26f1b8f448f2..9e70cb60ae50 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll @@ -1,5 +1,5 @@ /** - * Provides a taint tracking configuration for reasoning about method invocations + * Provides a taint-tracking configuration for reasoning about method invocations * with a user-controlled method name. */ @@ -51,8 +51,8 @@ module MethodNameInjection { class Configuration extends TaintTracking::Configuration { Configuration() { this = "RemotePropertyInjection" } - override predicate isSource(DataFlow::Node source) { - source instanceof Source + override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { + source.(Source).getFlowLabel() = label } override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { @@ -68,7 +68,7 @@ module MethodNameInjection { * Holds if a property of the given object is an unsafe function. */ predicate isUnsafeBaseObject(DataFlow::SourceNode node) { - // eval an friends can be accessed from the global object. + // eval and friends can be accessed from the global object. node = DataFlow::globalObjectRef() or // 'constructor' property leads to the Function constructor. From 49cd2876c9c202beef7d5f3b7964394b21e6e9aa Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 18:12:07 +0000 Subject: [PATCH 06/17] JS: use StringConcatenation library in ConcatSanitizer --- .../security/dataflow/MethodNameInjection.qll | 7 ++----- .../security/dataflow/RemotePropertyInjection.qll | 9 +++------ .../MethodNameInjection.expected | 14 +++++++------- .../Security/CWE-094/MethodNameInjection/tst.js | 1 + 4 files changed, 13 insertions(+), 18 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll index 9e70cb60ae50..214b1766a8d9 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll @@ -133,16 +133,13 @@ module MethodNameInjection { } /** - * A binary expression that sanitzes a value for method name injection. That + * An expression that sanitizes a value for method name injection. That * is, if a string is prepended or appended to the remote input, an attacker * cannot access arbitrary properties. */ class ConcatSanitizer extends Sanitizer, DataFlow::ValueNode { - - override BinaryExpr astNode; - ConcatSanitizer() { - astNode.getAnOperand() instanceof ConstantString + StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString } } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll index 362cd485fa5d..b1c848c7e01c 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll @@ -117,16 +117,13 @@ module RemotePropertyInjection { } /** - * A binary expression that sanitzes a value for remote property injection. That + * An expression that sanitizes a value for remote property injection. That * is, if a string is prepended or appended to the remote input, an attacker * cannot access arbitrary properties. */ - class ConcatSanitizer extends Sanitizer, DataFlow::ValueNode { - - override BinaryExpr astNode; - + class ConcatSanitizer extends Sanitizer { ConcatSanitizer() { - astNode.getAnOperand() instanceof ConstantString + StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString } } } diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected index 536af36eac86..894c900cb0dc 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected @@ -10,14 +10,14 @@ nodes | tst.js:6:9:6:28 | window[message.name] | | tst.js:6:16:6:22 | message | | tst.js:6:16:6:27 | message.name | -| tst.js:10:5:10:19 | f[message.name] | -| tst.js:10:7:10:13 | message | -| tst.js:10:7:10:18 | message.name | +| tst.js:11:5:11:19 | f[message.name] | +| tst.js:11:7:11:13 | message | +| tst.js:11:7:11:18 | message.name | edges | tst.js:3:37:3:38 | ev | tst.js:4:30:4:31 | ev | | tst.js:4:9:4:37 | message | tst.js:5:12:5:18 | message | | tst.js:4:9:4:37 | message | tst.js:6:16:6:22 | message | -| tst.js:4:9:4:37 | message | tst.js:10:7:10:13 | message | +| tst.js:4:9:4:37 | message | tst.js:11:7:11:13 | message | | tst.js:4:19:4:37 | JSON.parse(ev.data) | tst.js:4:9:4:37 | message | | tst.js:4:30:4:31 | ev | tst.js:4:30:4:36 | ev.data | | tst.js:4:30:4:36 | ev.data | tst.js:4:19:4:37 | JSON.parse(ev.data) | @@ -25,9 +25,9 @@ edges | tst.js:5:12:5:23 | message.name | tst.js:5:5:5:24 | window[message.name] | | tst.js:6:16:6:22 | message | tst.js:6:16:6:27 | message.name | | tst.js:6:16:6:27 | message.name | tst.js:6:9:6:28 | window[message.name] | -| tst.js:10:7:10:13 | message | tst.js:10:7:10:18 | message.name | -| tst.js:10:7:10:18 | message.name | tst.js:10:5:10:19 | f[message.name] | +| tst.js:11:7:11:13 | message | tst.js:11:7:11:18 | message.name | +| tst.js:11:7:11:18 | message.name | tst.js:11:5:11:19 | f[message.name] | #select | tst.js:5:5:5:24 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:5:5:5:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | | tst.js:6:9:6:28 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:6:9:6:28 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | -| tst.js:10:5:10:19 | f[message.name] | tst.js:3:37:3:38 | ev | tst.js:10:5:10:19 | f[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | +| tst.js:11:5:11:19 | f[message.name] | tst.js:3:37:3:38 | ev | tst.js:11:5:11:19 | f[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js index b50b1411e293..1d1a8f72e6ce 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js @@ -5,6 +5,7 @@ window.addEventListener('message', (ev) => { window[message.name](message.payload); // NOT OK - may invoke eval new window[message.name](message.payload); // NOT OK - may invoke jQuery $ function or similar window["HTMLElement" + message.name](message.payload); // OK - concatenation restricts choice of methods + window[`HTMLElement${message.name}`](message.payload); // OK - concatenation restricts choice of methods function f() {} f[message.name](message.payload)(); // NOT OK - may acccess Function constructor From b16072a7bed2c6d314b2199b8c52429ac0a2a44d Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 18:19:15 +0000 Subject: [PATCH 07/17] JS: share ConcatSanitizer in common module --- .../security/dataflow/MethodNameInjection.qll | 15 +++------------ .../dataflow/PropertyInjectionShared.qll | 18 ++++++++++++++++++ .../dataflow/RemotePropertyInjection.qll | 15 +++------------ 3 files changed, 24 insertions(+), 24 deletions(-) create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll index 214b1766a8d9..f1414c2b15c0 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll @@ -5,6 +5,7 @@ import javascript import semmle.javascript.frameworks.Express +import PropertyInjectionShared module MethodNameInjection { private import DataFlow::FlowLabel @@ -61,7 +62,8 @@ module MethodNameInjection { override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or - node instanceof Sanitizer + node instanceof Sanitizer or + node instanceof PropertyInjection::Sanitizer } /** @@ -131,15 +133,4 @@ module MethodNameInjection { result = unsafeFunction() } } - - /** - * An expression that sanitizes a value for method name injection. That - * is, if a string is prepended or appended to the remote input, an attacker - * cannot access arbitrary properties. - */ - class ConcatSanitizer extends Sanitizer, DataFlow::ValueNode { - ConcatSanitizer() { - StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString - } - } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll new file mode 100644 index 000000000000..8957f01c9f85 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll @@ -0,0 +1,18 @@ +import javascript + +module PropertyInjection { + /** + * A data-flow node that sanitizes user-controlled property names that flow through it. + */ + abstract class Sanitizer extends DataFlow::Node { + } + + /** + * Concatenation with a constant, acting as a sanitizer. + */ + private class ConcatSanitizer extends Sanitizer { + ConcatSanitizer() { + StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString + } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll index b1c848c7e01c..5f94a31fea53 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll @@ -6,6 +6,7 @@ import javascript import semmle.javascript.frameworks.Express +import PropertyInjectionShared module RemotePropertyInjection { /** @@ -45,7 +46,8 @@ module RemotePropertyInjection { override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) or - node instanceof Sanitizer + node instanceof Sanitizer or + node instanceof PropertyInjection::Sanitizer } } @@ -115,15 +117,4 @@ module RemotePropertyInjection { result = " a header name." } } - - /** - * An expression that sanitizes a value for remote property injection. That - * is, if a string is prepended or appended to the remote input, an attacker - * cannot access arbitrary properties. - */ - class ConcatSanitizer extends Sanitizer { - ConcatSanitizer() { - StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString - } - } } From 3902f752d0020d8bff616018b978c363bd2cbb1b Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 18:26:20 +0000 Subject: [PATCH 08/17] JS: share detection of objects with unsafe methods --- .../security/dataflow/MethodNameInjection.qll | 13 +++---------- .../security/dataflow/PropertyInjectionShared.qll | 14 ++++++++++++++ .../security/dataflow/RemotePropertyInjection.qll | 13 +++---------- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll index f1414c2b15c0..6e3b14ae15f9 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll @@ -69,15 +69,8 @@ module MethodNameInjection { /** * Holds if a property of the given object is an unsafe function. */ - predicate isUnsafeBaseObject(DataFlow::SourceNode node) { - // eval and friends can be accessed from the global object. - node = DataFlow::globalObjectRef() - or - // 'constructor' property leads to the Function constructor. - node.analyze().getAValue() instanceof AbstractCallable - or - // Assume that a value that is invoked can refer to a function. - exists (node.getAnInvocation()) + predicate hasUnsafeMethods(DataFlow::SourceNode node) { + PropertyInjection::hasUnsafeMethods(node) // Redefined here so custom queries can override it } /** @@ -91,7 +84,7 @@ module MethodNameInjection { override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel, DataFlow::FlowLabel dstlabel) { // Reading a property of the global object or of a function exists (DataFlow::PropRead read | - isUnsafeBaseObject(read.getBase().getALocalSource()) and + hasUnsafeMethods(read.getBase().getALocalSource()) and src = read.getPropertyNameExpr().flow() and dst = read and srclabel = taint() and diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll index 8957f01c9f85..f284e7119de5 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll @@ -15,4 +15,18 @@ module PropertyInjection { StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString } } + + /** + * Holds if the methods of the given value are unsafe, such as `eval`. + */ + predicate hasUnsafeMethods(DataFlow::SourceNode node) { + // eval and friends can be accessed from the global object. + node = DataFlow::globalObjectRef() + or + // 'constructor' property leads to the Function constructor. + node.analyze().getAValue() instanceof AbstractCallable + or + // Assume that a value that is invoked can refer to a function. + exists (node.getAnInvocation()) + } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll index 5f94a31fea53..dcc8de6aec02 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll @@ -72,15 +72,6 @@ module RemotePropertyInjection { result = " a property name to write to." } } - - /** - * Holds if the method name injection on the given base object is handled by another query. - */ - private predicate isCoveredByMethodNameInjection(DataFlow::SourceNode node) { - node = DataFlow::globalObjectRef() - or - node.analyze().getAValue() instanceof AbstractCallable - } /** * A sink for method calls using dynamically computed method names. @@ -89,7 +80,9 @@ module RemotePropertyInjection { MethodCallSink() { exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() | exists (pr.getAnInvocation()) and - not isCoveredByMethodNameInjection(pr.getBase().getALocalSource()) + + // Omit sinks covered by the MethodNameInjection query + not PropertyInjection::hasUnsafeMethods(pr.getBase().getALocalSource()) ) } From 260ae36cf8f1ec5240ce69f615ab66c29a6c991e Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 18:27:02 +0000 Subject: [PATCH 09/17] JS: document the shared module --- .../javascript/security/dataflow/PropertyInjectionShared.qll | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll index f284e7119de5..451e9d783c31 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll @@ -1,3 +1,7 @@ +/** + * Provides predicates for reasoning about flow of user-controlled values that are used + * as property names. + */ import javascript module PropertyInjection { From 4138f814d86f26d32defe8f4820a8d0a87739c59 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 18:42:49 +0000 Subject: [PATCH 10/17] JS: expand example --- .../CWE-094/MethodNameInjection.qhelp | 15 ++++++++++++--- .../CWE-094/examples/MethodNameInjection.js | 10 ++++++++++ .../examples/MethodNameInjectionGood.js | 19 +++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 javascript/ql/src/Security/CWE-094/examples/MethodNameInjectionGood.js diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp b/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp index 1dc3c41081a5..cb5e366f3d02 100644 --- a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp +++ b/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp @@ -21,12 +21,21 @@ Whitelist the permitted method names or change the type of object the methods ar

    -In the following example, a user-controlled string is used as the method name on a cross-window message handler. -In this case, a malicious website can embed the page in an iframe, and execute arbitary code in the page by -sending a message to it with name set to eval. +In the following example, a message from the document's parent frame can invoke the play +or pause method. However, it can also invoke eval. +A malicious website could embed the page in an iframe and execute arbitrary code by sending a message +with the name eval.

    + +

    +Instead of storing the API methods in the global scope, put them in an API object. It is also good +practice to prevent invocation of inherited methods like toString and valueOf. +

    + + +     diff --git a/javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js b/javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js index dc00dae290ba..ec4a3a8d9d71 100644 --- a/javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js +++ b/javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js @@ -1,4 +1,14 @@ +// API methods +function play(data) { + // ... +} +function pause(data) { + // ... +} + window.addEventListener("message", (ev) => { let message = JSON.parse(ev.data); + + // Let the parent frame call the 'play' or 'pause' function window[message.name](message.payload); }); diff --git a/javascript/ql/src/Security/CWE-094/examples/MethodNameInjectionGood.js b/javascript/ql/src/Security/CWE-094/examples/MethodNameInjectionGood.js new file mode 100644 index 000000000000..1636507ba3bd --- /dev/null +++ b/javascript/ql/src/Security/CWE-094/examples/MethodNameInjectionGood.js @@ -0,0 +1,19 @@ +// API methods +let api = { + play: function(data) { + // ... + }, + pause: function(data) { + // ... + } +}; + +window.addEventListener("message", (ev) => { + let message = JSON.parse(ev.data); + + // Let the parent frame call the 'play' or 'pause' function + if (!api.hasOwnProperty(message.name)) { + return; + } + api[message.name](message.payload); +}); From 7d8084783205cb6a6d7c50b81066a33ecdd05f88 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 20 Nov 2018 18:44:18 +0000 Subject: [PATCH 11/17] JS: add qhelp example to test suite --- .../MethodNameInjection.expected | 16 ++++++++++++++++ .../CWE-094/MethodNameInjection/example.js | 14 ++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/example.js diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected index 894c900cb0dc..12f23486c2bd 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected @@ -1,4 +1,12 @@ nodes +| example.js:9:37:9:38 | ev | +| example.js:10:9:10:37 | message | +| example.js:10:19:10:37 | JSON.parse(ev.data) | +| example.js:10:30:10:31 | ev | +| example.js:10:30:10:36 | ev.data | +| example.js:13:5:13:24 | window[message.name] | +| example.js:13:12:13:18 | message | +| example.js:13:12:13:23 | message.name | | tst.js:3:37:3:38 | ev | | tst.js:4:9:4:37 | message | | tst.js:4:19:4:37 | JSON.parse(ev.data) | @@ -14,6 +22,13 @@ nodes | tst.js:11:7:11:13 | message | | tst.js:11:7:11:18 | message.name | edges +| example.js:9:37:9:38 | ev | example.js:10:30:10:31 | ev | +| example.js:10:9:10:37 | message | example.js:13:12:13:18 | message | +| example.js:10:19:10:37 | JSON.parse(ev.data) | example.js:10:9:10:37 | message | +| example.js:10:30:10:31 | ev | example.js:10:30:10:36 | ev.data | +| example.js:10:30:10:36 | ev.data | example.js:10:19:10:37 | JSON.parse(ev.data) | +| example.js:13:12:13:18 | message | example.js:13:12:13:23 | message.name | +| example.js:13:12:13:23 | message.name | example.js:13:5:13:24 | window[message.name] | | tst.js:3:37:3:38 | ev | tst.js:4:30:4:31 | ev | | tst.js:4:9:4:37 | message | tst.js:5:12:5:18 | message | | tst.js:4:9:4:37 | message | tst.js:6:16:6:22 | message | @@ -28,6 +43,7 @@ edges | tst.js:11:7:11:13 | message | tst.js:11:7:11:18 | message.name | | tst.js:11:7:11:18 | message.name | tst.js:11:5:11:19 | f[message.name] | #select +| example.js:13:5:13:24 | window[message.name] | example.js:9:37:9:38 | ev | example.js:13:5:13:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | example.js:9:37:9:38 | ev | user-controlled value | | tst.js:5:5:5:24 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:5:5:5:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | | tst.js:6:9:6:28 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:6:9:6:28 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | | tst.js:11:5:11:19 | f[message.name] | tst.js:3:37:3:38 | ev | tst.js:11:5:11:19 | f[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/example.js b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/example.js new file mode 100644 index 000000000000..8ffd5a8addda --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/example.js @@ -0,0 +1,14 @@ +// API methods +function play(data) { + // ... +} +function pause(data) { + // ... +} + +window.addEventListener("message", (ev) => { + let message = JSON.parse(ev.data); + + // Let the parent frame call the 'play' or 'pause' function + window[message.name](message.payload); // NOT OK +}); From fa761c07bdd1157ac851de2be44f450a4afe59b6 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 21 Nov 2018 10:55:38 +0000 Subject: [PATCH 12/17] Update javascript/ql/src/Security/CWE-094/MethodNameInjection.ql Co-Authored-By: asger-semmle <42069257+asger-semmle@users.noreply.github.com> --- javascript/ql/src/Security/CWE-094/MethodNameInjection.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql b/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql index bd44c2e6f55c..8da30bcb3606 100644 --- a/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql +++ b/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql @@ -1,6 +1,6 @@ /** * @name Method name injection - * @description Invoking user-controlled methods on arbitrary objects can lead to remote code execution. + * @description Invoking user-controlled methods on certain objects can lead to remote code execution. * @kind path-problem * @problem.severity error * @precision high From 84d642612e4c3ae7465292681545b00eb1ee8c7c Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 21 Nov 2018 11:14:13 +0000 Subject: [PATCH 13/17] JS: more comments --- javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp | 2 +- .../javascript/security/dataflow/PropertyInjectionShared.qll | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp b/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp index cb5e366f3d02..c5a2c9341ffe 100644 --- a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp +++ b/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp @@ -30,7 +30,7 @@ with the name eval.

    -Instead of storing the API methods in the global scope, put them in an API object. It is also good +Instead of storing the API methods in the global scope, put them in an API object or Map. It is also good practice to prevent invocation of inherited methods like toString and valueOf.

    diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll index 451e9d783c31..b211e49cc802 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll @@ -27,6 +27,9 @@ module PropertyInjection { // eval and friends can be accessed from the global object. node = DataFlow::globalObjectRef() or + // document.write can be accessed + isDocument(node.asExpr()) + or // 'constructor' property leads to the Function constructor. node.analyze().getAValue() instanceof AbstractCallable or From 4ae2493798d2214f0214735684b17cdd26d7210b Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 21 Nov 2018 12:34:18 +0000 Subject: [PATCH 14/17] JS: rename query to Unsafe Dynamic Method Access --- change-notes/1.19/analysis-javascript.md | 2 +- javascript/config/suites/javascript/security | 2 +- ...n.qhelp => UnsafeDynamicMethodAccess.qhelp} | 4 ++-- ...jection.ql => UnsafeDynamicMethodAccess.ql} | 6 +++--- ...jection.js => UnsafeDynamicMethodAccess.js} | 0 ...ood.js => UnsafeDynamicMethodAccessGood.js} | 0 .../dataflow/RemotePropertyInjection.qll | 2 +- ...ction.qll => UnsafeDynamicMethodAccess.qll} | 18 +++++++++--------- .../MethodNameInjection.qlref | 1 - .../UnsafeDynamicMethodAccess.expected} | 0 .../UnsafeDynamicMethodAccess.qlref | 1 + .../example.js | 0 .../tst.js | 0 13 files changed, 18 insertions(+), 18 deletions(-) rename javascript/ql/src/Security/CWE-094/{MethodNameInjection.qhelp => UnsafeDynamicMethodAccess.qhelp} (93%) rename javascript/ql/src/Security/CWE-094/{MethodNameInjection.ql => UnsafeDynamicMethodAccess.ql} (75%) rename javascript/ql/src/Security/CWE-094/examples/{MethodNameInjection.js => UnsafeDynamicMethodAccess.js} (100%) rename javascript/ql/src/Security/CWE-094/examples/{MethodNameInjectionGood.js => UnsafeDynamicMethodAccessGood.js} (100%) rename javascript/ql/src/semmle/javascript/security/dataflow/{MethodNameInjection.qll => UnsafeDynamicMethodAccess.qll} (86%) delete mode 100644 javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.qlref rename javascript/ql/test/query-tests/Security/CWE-094/{MethodNameInjection/MethodNameInjection.expected => UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected} (100%) create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.qlref rename javascript/ql/test/query-tests/Security/CWE-094/{MethodNameInjection => UnsafeDynamicMethodAccess}/example.js (100%) rename javascript/ql/test/query-tests/Security/CWE-094/{MethodNameInjection => UnsafeDynamicMethodAccess}/tst.js (100%) diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 011a6aa579ae..26c74e4ed85e 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -24,7 +24,7 @@ | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. | | File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. | | Host header poisoning in email generation | security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default. | -| Method name injection (`js/method-name-injection` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. | +| Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. | | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. | diff --git a/javascript/config/suites/javascript/security b/javascript/config/suites/javascript/security index 1481d5e28f75..ee9e07b578bf 100644 --- a/javascript/config/suites/javascript/security +++ b/javascript/config/suites/javascript/security @@ -7,6 +7,7 @@ + semmlecode-javascript-queries/Security/CWE-079/Xss.ql: /Security/CWE/CWE-079 + semmlecode-javascript-queries/Security/CWE-089/SqlInjection.ql: /Security/CWE/CWE-089 + semmlecode-javascript-queries/Security/CWE-094/CodeInjection.ql: /Security/CWE/CWE-094 ++ semmlecode-javascript-queries/Security/CWE-094/UnsafeDynamicMethodAccess.ql: /Security/CWE/CWE-094 + semmlecode-javascript-queries/Security/CWE-116/IncompleteSanitization.ql: /Security/CWE/CWE-116 + semmlecode-javascript-queries/Security/CWE-134/TaintedFormatString.ql: /Security/CWE/CWE-134 + semmlecode-javascript-queries/Security/CWE-209/StackTraceExposure.ql: /Security/CWE/CWE-209 @@ -16,7 +17,6 @@ + semmlecode-javascript-queries/Security/CWE-327/BrokenCryptoAlgorithm.ql: /Security/CWE/CWE-327 + semmlecode-javascript-queries/Security/CWE-338/InsecureRandomness.ql: /Security/CWE/CWE-338 + semmlecode-javascript-queries/Security/CWE-346/CorsMisconfigurationForCredentials.ql: /Security/CWE/CWE-346 -+ semmlecode-javascript-queries/Security/CWE-352/MethodNameInjection.ql: /Security/CWE/CWE-094 + semmlecode-javascript-queries/Security/CWE-352/MissingCsrfMiddleware.ql: /Security/CWE/CWE-352 + semmlecode-javascript-queries/Security/CWE-400/RemotePropertyInjection.ql: /Security/CWE/CWE-400 + semmlecode-javascript-queries/Security/CWE-502/UnsafeDeserialization.ql: /Security/CWE/CWE-502 diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp similarity index 93% rename from javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp rename to javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp index c5a2c9341ffe..87a07e43cd5e 100644 --- a/javascript/ql/src/Security/CWE-094/MethodNameInjection.qhelp +++ b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp @@ -27,14 +27,14 @@ A malicious website could embed the page in an iframe and execute arbitrary code with the name eval.

    - +

    Instead of storing the API methods in the global scope, put them in an API object or Map. It is also good practice to prevent invocation of inherited methods like toString and valueOf.

    - + diff --git a/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.ql similarity index 75% rename from javascript/ql/src/Security/CWE-094/MethodNameInjection.ql rename to javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.ql index 8da30bcb3606..b0715177412c 100644 --- a/javascript/ql/src/Security/CWE-094/MethodNameInjection.ql +++ b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.ql @@ -1,15 +1,15 @@ /** - * @name Method name injection + * @name Unsafe dynamic method access * @description Invoking user-controlled methods on certain objects can lead to remote code execution. * @kind path-problem * @problem.severity error * @precision high - * @id js/method-name-injection + * @id js/unsafe-dynamic-method-access * @tags security * external/cwe/cwe-094 */ import javascript -import semmle.javascript.security.dataflow.MethodNameInjection::MethodNameInjection +import semmle.javascript.security.dataflow.UnsafeDynamicMethodAccess::UnsafeDynamicMethodAccess import DataFlow::PathGraph from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink diff --git a/javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js b/javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccess.js similarity index 100% rename from javascript/ql/src/Security/CWE-094/examples/MethodNameInjection.js rename to javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccess.js diff --git a/javascript/ql/src/Security/CWE-094/examples/MethodNameInjectionGood.js b/javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccessGood.js similarity index 100% rename from javascript/ql/src/Security/CWE-094/examples/MethodNameInjectionGood.js rename to javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccessGood.js diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll index dcc8de6aec02..72a540465b34 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll @@ -81,7 +81,7 @@ module RemotePropertyInjection { exists (DataFlow::PropRead pr | astNode = pr.getPropertyNameExpr() | exists (pr.getAnInvocation()) and - // Omit sinks covered by the MethodNameInjection query + // Omit sinks covered by the UnsafeDynamicMethodAccess query not PropertyInjection::hasUnsafeMethods(pr.getBase().getALocalSource()) ) } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll similarity index 86% rename from javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll rename to javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll index 6e3b14ae15f9..8b2630faad88 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll @@ -1,17 +1,17 @@ /** * Provides a taint-tracking configuration for reasoning about method invocations - * with a user-controlled method name. + * with a user-controlled method name on objects with unsafe methods. */ import javascript import semmle.javascript.frameworks.Express import PropertyInjectionShared -module MethodNameInjection { +module UnsafeDynamicMethodAccess { private import DataFlow::FlowLabel /** - * A data flow source for method name injection. + * A data flow source for unsafe dynamic method access. */ abstract class Source extends DataFlow::Node { /** @@ -23,7 +23,7 @@ module MethodNameInjection { } /** - * A data flow sink for method name injection. + * A data flow sink for unsafe dynamic method access. */ abstract class Sink extends DataFlow::Node { /** @@ -33,7 +33,7 @@ module MethodNameInjection { } /** - * A sanitizer for method name injection. + * A sanitizer for unsafe dynamic method access. */ abstract class Sanitizer extends DataFlow::Node { } @@ -47,7 +47,7 @@ module MethodNameInjection { } /** - * A taint-tracking configuration for reasoning about method name injection. + * A taint-tracking configuration for reasoning about unsafe dynamic method access. */ class Configuration extends TaintTracking::Configuration { Configuration() { this = "RemotePropertyInjection" } @@ -101,21 +101,21 @@ module MethodNameInjection { } /** - * A source of remote user input, considered as a source for method name injection. + * A source of remote user input, considered as a source for unsafe dynamic method access. */ class RemoteFlowSourceAsSource extends Source { RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource } } /** - * The page URL considered as a flow source for method name injection. + * The page URL considered as a flow source for unsafe dynamic method access. */ class DocumentUrlAsSource extends Source { DocumentUrlAsSource() { isDocumentURL(asExpr()) } } /** - * A function invocation of an unsafe function, as a sink for remote method name injection. + * A function invocation of an unsafe function, as a sink for remote unsafe dynamic method access. */ class CalleeAsSink extends Sink { CalleeAsSink() { diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.qlref b/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.qlref deleted file mode 100644 index 02e1d5737979..000000000000 --- a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE-094/MethodNameInjection.ql diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/MethodNameInjection.expected rename to javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected diff --git a/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.qlref b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.qlref new file mode 100644 index 000000000000..5c4a993df5a7 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.qlref @@ -0,0 +1 @@ +Security/CWE-094/UnsafeDynamicMethodAccess.ql diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/example.js b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/example.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/example.js rename to javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/example.js diff --git a/javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js similarity index 100% rename from javascript/ql/test/query-tests/Security/CWE-094/MethodNameInjection/tst.js rename to javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js From 8c7e19567beb4ef26b89b033ceb241e9177fc2b4 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 21 Nov 2018 12:35:35 +0000 Subject: [PATCH 15/17] JS: fix string value of taint configuration --- .../javascript/security/dataflow/UnsafeDynamicMethodAccess.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll index 8b2630faad88..f06f7dc2e89f 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll @@ -50,7 +50,7 @@ module UnsafeDynamicMethodAccess { * A taint-tracking configuration for reasoning about unsafe dynamic method access. */ class Configuration extends TaintTracking::Configuration { - Configuration() { this = "RemotePropertyInjection" } + Configuration() { this = "UnsafeDynamicMethodAccess" } override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { source.(Source).getFlowLabel() = label From 27c9326e7063104114221edb786bd4ccbfa61204 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 21 Nov 2018 14:19:14 +0000 Subject: [PATCH 16/17] JS: address doc review --- change-notes/1.19/analysis-javascript.md | 2 +- .../ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 26c74e4ed85e..3255eb5a9945 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -24,7 +24,7 @@ | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. | | File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. | | Host header poisoning in email generation | security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default. | -| Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. | +| Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. Results are shown on LGTM by default. | | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. | diff --git a/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp index 87a07e43cd5e..c655b2b78814 100644 --- a/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp +++ b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp @@ -5,7 +5,7 @@

    -Invoking a user-controlled method on certain objects can lead to invocation of unsafe functions, +Calling a user-controlled method on certain objects can lead to invocation of unsafe functions, such as eval or the Function constructor. In particular, the global object contains the eval function, and any function object contains the Function constructor in its constructor property. From 61ef6552c3ab15a505f139a937a18b3505a7c3b9 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 22 Nov 2018 09:59:31 +0000 Subject: [PATCH 17/17] JS: handle both data() and taint() source labels --- .../security/dataflow/UnsafeDynamicMethodAccess.qll | 4 ++-- .../UnsafeDynamicMethodAccess.expected | 5 +++++ .../Security/CWE-094/UnsafeDynamicMethodAccess/tst.js | 2 ++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll index f06f7dc2e89f..2ef6dd71c51e 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll @@ -87,7 +87,7 @@ module UnsafeDynamicMethodAccess { hasUnsafeMethods(read.getBase().getALocalSource()) and src = read.getPropertyNameExpr().flow() and dst = read and - srclabel = taint() and + (srclabel = data() or srclabel = taint()) and dstlabel = unsafeFunction()) or // Reading a chain of properties from any object with a prototype can lead to Function @@ -95,7 +95,7 @@ module UnsafeDynamicMethodAccess { not isPrototypeLessObject(proj.getObject().getALocalSource()) and src = proj.getASelector() and dst = proj and - srclabel = taint() and + (srclabel = data() or srclabel = taint()) and dstlabel = unsafeFunction()) } } diff --git a/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected index 12f23486c2bd..e9de22bffb86 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected @@ -21,6 +21,8 @@ nodes | tst.js:11:5:11:19 | f[message.name] | | tst.js:11:7:11:13 | message | | tst.js:11:7:11:18 | message.name | +| tst.js:15:5:15:14 | window[ev] | +| tst.js:15:12:15:13 | ev | edges | example.js:9:37:9:38 | ev | example.js:10:30:10:31 | ev | | example.js:10:9:10:37 | message | example.js:13:12:13:18 | message | @@ -30,6 +32,7 @@ edges | example.js:13:12:13:18 | message | example.js:13:12:13:23 | message.name | | example.js:13:12:13:23 | message.name | example.js:13:5:13:24 | window[message.name] | | tst.js:3:37:3:38 | ev | tst.js:4:30:4:31 | ev | +| tst.js:3:37:3:38 | ev | tst.js:15:12:15:13 | ev | | tst.js:4:9:4:37 | message | tst.js:5:12:5:18 | message | | tst.js:4:9:4:37 | message | tst.js:6:16:6:22 | message | | tst.js:4:9:4:37 | message | tst.js:11:7:11:13 | message | @@ -42,8 +45,10 @@ edges | tst.js:6:16:6:27 | message.name | tst.js:6:9:6:28 | window[message.name] | | tst.js:11:7:11:13 | message | tst.js:11:7:11:18 | message.name | | tst.js:11:7:11:18 | message.name | tst.js:11:5:11:19 | f[message.name] | +| tst.js:15:12:15:13 | ev | tst.js:15:5:15:14 | window[ev] | #select | example.js:13:5:13:24 | window[message.name] | example.js:9:37:9:38 | ev | example.js:13:5:13:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | example.js:9:37:9:38 | ev | user-controlled value | | tst.js:5:5:5:24 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:5:5:5:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | | tst.js:6:9:6:28 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:6:9:6:28 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | | tst.js:11:5:11:19 | f[message.name] | tst.js:3:37:3:38 | ev | tst.js:11:5:11:19 | f[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | +| tst.js:15:5:15:14 | window[ev] | tst.js:3:37:3:38 | ev | tst.js:15:5:15:14 | window[ev] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js index 1d1a8f72e6ce..21931585eee7 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js @@ -11,4 +11,6 @@ window.addEventListener('message', (ev) => { f[message.name](message.payload)(); // NOT OK - may acccess Function constructor obj[message.name](message.payload); // OK - may crash, but no code execution involved + + window[ev](ev); // NOT OK });