diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index d190d16dc147..3255eb5a9945 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. | +| 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/config/suites/javascript/security b/javascript/config/suites/javascript/security index 20b62e4b3bd0..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 diff --git a/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp new file mode 100644 index 000000000000..c655b2b78814 --- /dev/null +++ b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.qhelp @@ -0,0 +1,53 @@ + + + + +

+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. +

+
+ + +

+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. +

+
+ + +

+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 or Map. It is also good +practice to prevent invocation of inherited methods like toString and valueOf. +

+ + + +
+ + +
  • +OWASP: +Code Injection. +
  • +
  • +MDN: Global functions. +
  • +
  • +MDN: Function constructor. +
  • +
    +
    diff --git a/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.ql b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.ql new file mode 100644 index 000000000000..b0715177412c --- /dev/null +++ b/javascript/ql/src/Security/CWE-094/UnsafeDynamicMethodAccess.ql @@ -0,0 +1,17 @@ +/** + * @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/unsafe-dynamic-method-access + * @tags security + * external/cwe/cwe-094 + */ +import javascript +import semmle.javascript.security.dataflow.UnsafeDynamicMethodAccess::UnsafeDynamicMethodAccess +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/UnsafeDynamicMethodAccess.js b/javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccess.js new file mode 100644 index 000000000000..ec4a3a8d9d71 --- /dev/null +++ b/javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccess.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); +}); diff --git a/javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccessGood.js b/javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccessGood.js new file mode 100644 index 000000000000..1636507ba3bd --- /dev/null +++ b/javascript/ql/src/Security/CWE-094/examples/UnsafeDynamicMethodAccessGood.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); +}); 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..b211e49cc802 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll @@ -0,0 +1,39 @@ +/** + * Provides predicates for reasoning about flow of user-controlled values that are used + * as property names. + */ +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 + } + } + + /** + * 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 + // document.write can be accessed + isDocument(node.asExpr()) + 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 1b2b20ad8e1e..72a540465b34 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 } } @@ -76,9 +78,12 @@ module RemotePropertyInjection { */ 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 + + // Omit sinks covered by the UnsafeDynamicMethodAccess query + not PropertyInjection::hasUnsafeMethods(pr.getBase().getALocalSource()) + ) } override string getMessage() { @@ -105,18 +110,4 @@ module RemotePropertyInjection { result = " a header name." } } - - /** - * A binary expression that sanitzes 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; - - ConcatSanitizer() { - astNode.getAnOperand() instanceof ConstantString - } - } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll new file mode 100644 index 000000000000..2ef6dd71c51e --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll @@ -0,0 +1,129 @@ +/** + * Provides a taint-tracking configuration for reasoning about method invocations + * with a user-controlled method name on objects with unsafe methods. + */ + +import javascript +import semmle.javascript.frameworks.Express +import PropertyInjectionShared + +module UnsafeDynamicMethodAccess { + private import DataFlow::FlowLabel + + /** + * A data flow source for unsafe dynamic method access. + */ + abstract class Source extends DataFlow::Node { + /** + * Gets the flow label relevant for this source. + */ + DataFlow::FlowLabel getFlowLabel() { + result = data() + } + } + + /** + * A data flow sink for unsafe dynamic method access. + */ + abstract class Sink extends DataFlow::Node { + /** + * Gets the flow label relevant for this sink + */ + abstract DataFlow::FlowLabel getFlowLabel(); + } + + /** + * A sanitizer for unsafe dynamic method access. + */ + 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 unsafe dynamic method access. + */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "UnsafeDynamicMethodAccess" } + + override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { + source.(Source).getFlowLabel() = label + } + + 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 or + node instanceof PropertyInjection::Sanitizer + } + + /** + * Holds if a property of the given object is an unsafe function. + */ + predicate hasUnsafeMethods(DataFlow::SourceNode node) { + PropertyInjection::hasUnsafeMethods(node) // Redefined here so custom queries can override it + } + + /** + * 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 | + hasUnsafeMethods(read.getBase().getALocalSource()) and + src = read.getPropertyNameExpr().flow() and + dst = read 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 + exists (PropertyProjection proj | + not isPrototypeLessObject(proj.getObject().getALocalSource()) and + src = proj.getASelector() and + dst = proj and + (srclabel = data() or srclabel = taint()) and + dstlabel = unsafeFunction()) + } + } + + /** + * 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 unsafe dynamic method access. + */ + class DocumentUrlAsSource extends Source { + DocumentUrlAsSource() { isDocumentURL(asExpr()) } + } + + /** + * A function invocation of an unsafe function, as a sink for remote unsafe dynamic method access. + */ + class CalleeAsSink extends Sink { + CalleeAsSink() { + this = any(DataFlow::InvokeNode node).getCalleeNode() + } + + override DataFlow::FlowLabel getFlowLabel() { + result = unsafeFunction() + } + } +} 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 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 new file mode 100644 index 000000000000..e9de22bffb86 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected @@ -0,0 +1,54 @@ +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) | +| 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: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 | +| 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: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 | +| 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: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/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/UnsafeDynamicMethodAccess/example.js b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/example.js new file mode 100644 index 000000000000..8ffd5a8addda --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/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 +}); 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 new file mode 100644 index 000000000000..21931585eee7 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js @@ -0,0 +1,16 @@ +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 + 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 + + window[ev](ev); // NOT OK +}); 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) {