diff --git a/javascript/change-notes/2021-03-10-d3.md b/javascript/change-notes/2021-03-10-d3.md new file mode 100644 index 000000000000..22e1220110fb --- /dev/null +++ b/javascript/change-notes/2021-03-10-d3.md @@ -0,0 +1,6 @@ +lgtm,codescanning +* Support for `d3` has improved. The XSS queries now recognize HTML injection sinks + from the `d3` API. + Affected packages are + [d3](https://npmjs.com/package/d3), + [d3-selection](https://npmjs.com/package/d3-selection). diff --git a/javascript/ql/src/javascript.qll b/javascript/ql/src/javascript.qll index 8c9493eab06e..b3664fce00ea 100644 --- a/javascript/ql/src/javascript.qll +++ b/javascript/ql/src/javascript.qll @@ -80,6 +80,7 @@ import semmle.javascript.frameworks.ClosureLibrary import semmle.javascript.frameworks.CookieLibraries import semmle.javascript.frameworks.Credentials import semmle.javascript.frameworks.CryptoLibraries +import semmle.javascript.frameworks.D3 import semmle.javascript.frameworks.DateFunctions import semmle.javascript.frameworks.DigitalOcean import semmle.javascript.frameworks.Electron diff --git a/javascript/ql/src/semmle/javascript/frameworks/D3.qll b/javascript/ql/src/semmle/javascript/frameworks/D3.qll new file mode 100644 index 000000000000..49ec68e24241 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/frameworks/D3.qll @@ -0,0 +1,102 @@ +/** Provides classes and predicates modelling aspects of the `d3` library. */ + +private import javascript +private import semmle.javascript.security.dataflow.Xss + +/** Provides classes and predicates modelling aspects of the `d3` library. */ +module D3 { + /** The global variable `d3` as an entry point for API graphs. */ + private class D3GlobalEntry extends API::EntryPoint { + D3GlobalEntry() { this = "D3GlobalEntry" } + + override DataFlow::SourceNode getAUse() { result = DataFlow::globalVarRef("d3") } + + override DataFlow::Node getARhs() { none() } + } + + /** Gets an API node referring to the `d3` module. */ + API::Node d3() { + result = API::moduleImport("d3") + or + // recognize copies of d3 in a scope + result = API::moduleImport(any(string s | s.regexpMatch("@.*/d3(-\\w+)?"))) + or + result = API::moduleImport("d3-node").getInstance().getMember("d3") + or + result = API::root().getASuccessor(any(D3GlobalEntry i)) + } + + /** + * Gets an API node referring to the given module, or to `d3`. + * + * Useful for accessing modules that are re-exported by `d3`. + */ + bindingset[name] + API::Node d3Module(string name) { + result = d3() // all d3 modules are re-exported as part of 'd3' + or + result = API::moduleImport(name) + } + + /** Gets an API node referring to a `d3` selection object, such as `d3.selection()`. */ + API::Node d3Selection() { + result = d3Module("d3-selection").getMember(["selection", "select", "selectAll"]).getReturn() + or + exists(API::CallNode call, string name | + call = d3Selection().getMember(name).getACall() and + result = call.getReturn() + | + name = + [ + "select", "selectAll", "filter", "merge", "selectChild", "selectChildren", "selection", + "insert", "remove", "clone", "sort", "order", "raise", "lower", "append", "data", "join", + "enter", "exit", "call" + ] + or + name = ["text", "html", "datum"] and + call.getNumArgument() > 0 // exclude 0-argument version, which returns the current value + or + name = ["attr", "classed", "style", "property", "on"] and + call.getNumArgument() > 1 // exclude 1-argument version, which returns the current value + or + // Setting multiple things at once + name = ["attr", "classed", "style", "property", "on"] and + call.getArgument(0).getALocalSource() instanceof DataFlow::ObjectLiteralNode + ) + or + result = d3Selection().getMember("call").getParameter(0).getParameter(0) + } + + private class D3XssSink extends DomBasedXss::Sink { + D3XssSink() { + exists(API::Node htmlArg | + htmlArg = d3Selection().getMember("html").getParameter(0) and + this = [htmlArg, htmlArg.getReturn()].getARhs() + ) + } + } + + private class D3DomValueSource extends DOM::DomValueSource::Range { + D3DomValueSource() { + this = d3Selection().getMember("each").getReceiver().getAnImmediateUse() + or + this = d3Selection().getMember("node").getReturn().getAnImmediateUse() + or + this = d3Selection().getMember("nodes").getReturn().getUnknownMember().getAnImmediateUse() + } + } + + private class D3AttributeDefinition extends DOM::AttributeDefinition { + DataFlow::CallNode call; + + D3AttributeDefinition() { + call = d3Selection().getMember("attr").getACall() and + call.getNumArgument() > 1 and + this = call.asExpr() + } + + override string getName() { result = call.getArgument(0).getStringValue() } + + override DataFlow::Node getValueNode() { result = call.getArgument(1) } + } +} diff --git a/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll b/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll index 1716917282f5..38401464b4a3 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll @@ -479,8 +479,8 @@ module JQuery { // either a reference to a global variable `$` or `jQuery` this = DataFlow::globalVarRef(any(string jq | jq = "$" or jq = "jQuery")) or - // or imported from a module named `jquery` - this = DataFlow::moduleImport("jquery") + // or imported from a module named `jquery` or `zepto` + this = DataFlow::moduleImport(["jquery", "zepto"]) or this.hasUnderlyingType("JQueryStatic") } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll index 2176d5f83c67..efedee1477d9 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll @@ -27,6 +27,11 @@ module XssThroughDom { result = ["name", "value", "title", "alt"] } + /** + * Gets a DOM property name that could store user-controlled data. + */ + string unsafeDomPropertyName() { result = ["innerText", "textContent", "value", "name"] } + /** * A source for text from the DOM from a JQuery method call. */ @@ -35,10 +40,16 @@ module XssThroughDom { ( this.getMethodName() = ["text", "val"] and this.getNumArgument() = 0 or - this.getMethodName() = "attr" and - this.getNumArgument() = 1 and - forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and - this.getArgument(0).mayHaveStringValue(unsafeAttributeName()) + exists(string methodName, string value | + this.getMethodName() = methodName and + this.getNumArgument() = 1 and + forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and + this.getArgument(0).mayHaveStringValue(value) + | + methodName = "attr" and value = unsafeAttributeName() + or + methodName = "prop" and value = unsafeDomPropertyName() + ) ) and // looks like a $("

" + ... ) source, which is benign for this query. not exists(DataFlow::Node prefix | @@ -51,6 +62,29 @@ module XssThroughDom { } } + /** + * A source for text from the DOM from a `d3` method call. + */ + class D3TextSource extends Source { + D3TextSource() { + exists(DataFlow::MethodCallNode call, string methodName | + this = call and + call = D3::d3Selection().getMember(methodName).getACall() + | + methodName = "attr" and + call.getNumArgument() = 1 and + call.getArgument(0).mayHaveStringValue(unsafeAttributeName()) + or + methodName = "property" and + call.getNumArgument() = 1 and + call.getArgument(0).mayHaveStringValue(unsafeDomPropertyName()) + or + methodName = "text" and + call.getNumArgument() = 0 + ) + } + } + /** * A source for text from the DOM from a DOM property read or call to `getAttribute()`. */ @@ -58,7 +92,7 @@ module XssThroughDom { DOMTextSource() { exists(DataFlow::PropRead read | read = this | read.getBase().getALocalSource() = DOM::domValueRef() and - read.mayHavePropertyName(["innerText", "textContent", "value", "name"]) + read.mayHavePropertyName(unsafeDomPropertyName()) ) or exists(DataFlow::MethodCallNode mcn | mcn = this | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected index d6f2f2a86570..b0d66661e229 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected @@ -92,6 +92,16 @@ nodes | classnames.js:15:47:15:63 | clsx(window.name) | | classnames.js:15:52:15:62 | window.name | | classnames.js:15:52:15:62 | window.name | +| d3.js:4:12:4:22 | window.name | +| d3.js:4:12:4:22 | window.name | +| d3.js:11:15:11:24 | getTaint() | +| d3.js:11:15:11:24 | getTaint() | +| d3.js:12:20:12:29 | getTaint() | +| d3.js:12:20:12:29 | getTaint() | +| d3.js:14:20:14:29 | getTaint() | +| d3.js:14:20:14:29 | getTaint() | +| d3.js:21:15:21:24 | getTaint() | +| d3.js:21:15:21:24 | getTaint() | | dates.js:9:9:9:69 | taint | | dates.js:9:17:9:69 | decodeU ... ing(1)) | | dates.js:9:36:9:50 | window.location | @@ -772,6 +782,22 @@ edges | classnames.js:15:47:15:63 | clsx(window.name) | classnames.js:15:31:15:78 | `` | | classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) | | classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) | +| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | | dates.js:9:9:9:69 | taint | dates.js:11:63:11:67 | taint | | dates.js:9:9:9:69 | taint | dates.js:12:66:12:70 | taint | | dates.js:9:9:9:69 | taint | dates.js:13:59:13:63 | taint | @@ -1338,6 +1364,10 @@ edges | classnames.js:11:31:11:79 | `` | classnames.js:10:45:10:55 | window.name | classnames.js:11:31:11:79 | `` | Cross-site scripting vulnerability due to $@. | classnames.js:10:45:10:55 | window.name | user-provided value | | classnames.js:13:31:13:83 | `` | classnames.js:13:57:13:67 | window.name | classnames.js:13:31:13:83 | `` | Cross-site scripting vulnerability due to $@. | classnames.js:13:57:13:67 | window.name | user-provided value | | classnames.js:15:31:15:78 | `` | classnames.js:15:52:15:62 | window.name | classnames.js:15:31:15:78 | `` | Cross-site scripting vulnerability due to $@. | classnames.js:15:52:15:62 | window.name | user-provided value | +| d3.js:11:15:11:24 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value | +| d3.js:12:20:12:29 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value | +| d3.js:14:20:14:29 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value | +| d3.js:21:15:21:24 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value | | dates.js:11:31:11:70 | `Time i ... aint)}` | dates.js:9:36:9:50 | window.location | dates.js:11:31:11:70 | `Time i ... aint)}` | Cross-site scripting vulnerability due to $@. | dates.js:9:36:9:50 | window.location | user-provided value | | dates.js:12:31:12:73 | `Time i ... aint)}` | dates.js:9:36:9:50 | window.location | dates.js:12:31:12:73 | `Time i ... aint)}` | Cross-site scripting vulnerability due to $@. | dates.js:9:36:9:50 | window.location | user-provided value | | dates.js:13:31:13:72 | `Time i ... time)}` | dates.js:9:36:9:50 | window.location | dates.js:13:31:13:72 | `Time i ... time)}` | Cross-site scripting vulnerability due to $@. | dates.js:9:36:9:50 | window.location | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected index ae2abf7566d5..ebe2d816f97f 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected @@ -92,6 +92,16 @@ nodes | classnames.js:15:47:15:63 | clsx(window.name) | | classnames.js:15:52:15:62 | window.name | | classnames.js:15:52:15:62 | window.name | +| d3.js:4:12:4:22 | window.name | +| d3.js:4:12:4:22 | window.name | +| d3.js:11:15:11:24 | getTaint() | +| d3.js:11:15:11:24 | getTaint() | +| d3.js:12:20:12:29 | getTaint() | +| d3.js:12:20:12:29 | getTaint() | +| d3.js:14:20:14:29 | getTaint() | +| d3.js:14:20:14:29 | getTaint() | +| d3.js:21:15:21:24 | getTaint() | +| d3.js:21:15:21:24 | getTaint() | | dates.js:9:9:9:69 | taint | | dates.js:9:17:9:69 | decodeU ... ing(1)) | | dates.js:9:36:9:50 | window.location | @@ -783,6 +793,22 @@ edges | classnames.js:15:47:15:63 | clsx(window.name) | classnames.js:15:31:15:78 | `` | | classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) | | classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) | +| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | +| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() | | dates.js:9:9:9:69 | taint | dates.js:11:63:11:67 | taint | | dates.js:9:9:9:69 | taint | dates.js:12:66:12:70 | taint | | dates.js:9:9:9:69 | taint | dates.js:13:59:13:63 | taint | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/d3.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/d3.js new file mode 100644 index 000000000000..1bb64b48b214 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/d3.js @@ -0,0 +1,22 @@ +const d3 = require('d3'); + +function getTaint() { + return window.name; +} + +function doSomething() { + d3.select('#main') + .attr('width', 100) + .style('color', 'red') + .html(getTaint()) // NOT OK + .html(d => getTaint()) // NOT OK + .call(otherFunction) + .html(d => getTaint()); // NOT OK +} + + +function otherFunction(selection) { + selection + .attr('foo', 'bar') + .html(getTaint()); // NOT OK +} diff --git a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/XssThroughDom.expected b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/XssThroughDom.expected index cfa5ba990707..b88c3eae8ef3 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/XssThroughDom.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/XssThroughDom.expected @@ -100,6 +100,9 @@ nodes | xss-through-dom.js:79:4:79:34 | documen ... t.value | | xss-through-dom.js:79:4:79:34 | documen ... t.value | | xss-through-dom.js:79:4:79:34 | documen ... t.value | +| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | +| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | +| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | edges | forms.js:8:23:8:28 | values | forms.js:9:31:9:36 | values | | forms.js:8:23:8:28 | values | forms.js:9:31:9:36 | values | @@ -157,6 +160,7 @@ edges | xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:73:9:73:41 | selector | | xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:73:9:73:41 | selector | | xss-through-dom.js:79:4:79:34 | documen ... t.value | xss-through-dom.js:79:4:79:34 | documen ... t.value | +| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | #select | forms.js:9:31:9:40 | values.foo | forms.js:8:23:8:28 | values | forms.js:9:31:9:40 | values.foo | $@ is reinterpreted as HTML without escaping meta-characters. | forms.js:8:23:8:28 | values | DOM text | | forms.js:12:31:12:40 | values.bar | forms.js:11:24:11:29 | values | forms.js:12:31:12:40 | values.bar | $@ is reinterpreted as HTML without escaping meta-characters. | forms.js:11:24:11:29 | values | DOM text | @@ -185,3 +189,4 @@ edges | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | DOM text | | xss-through-dom.js:77:4:77:11 | selector | xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:77:4:77:11 | selector | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | DOM text | | xss-through-dom.js:79:4:79:34 | documen ... t.value | xss-through-dom.js:79:4:79:34 | documen ... t.value | xss-through-dom.js:79:4:79:34 | documen ... t.value | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:79:4:79:34 | documen ... t.value | DOM text | +| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | DOM text | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js index 72694a9e02e9..9f85627ee7c7 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js @@ -77,4 +77,6 @@ $(selector); // NOT OK $(document.my_form.my_input.value); // NOT OK + + $("#id").html( $('#foo').prop('innerText') ); // NOT OK })(); \ No newline at end of file