Skip to content

Commit cb6ee54

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5379 from asgerf/js/d3
Approved by erik-krogh
2 parents a8b84e4 + a2d1e88 commit cb6ee54

File tree

10 files changed

+235
-7
lines changed

10 files changed

+235
-7
lines changed

‎javascript/change-notes/2021-03-10-d3.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
lgtm,codescanning
2+
* Support for `d3` has improved. The XSS queries now recognize HTML injection sinks
3+
from the `d3` API.
4+
Affected packages are
5+
[d3](https://npmjs.com/package/d3),
6+
[d3-selection](https://npmjs.com/package/d3-selection).

‎javascript/ql/src/javascript.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ import semmle.javascript.frameworks.ClosureLibrary
8080
import semmle.javascript.frameworks.CookieLibraries
8181
import semmle.javascript.frameworks.Credentials
8282
import semmle.javascript.frameworks.CryptoLibraries
83+
import semmle.javascript.frameworks.D3
8384
import semmle.javascript.frameworks.DateFunctions
8485
import semmle.javascript.frameworks.DigitalOcean
8586
import semmle.javascript.frameworks.Electron

‎javascript/ql/src/semmle/javascript/frameworks/D3.qll

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
/** Provides classes and predicates modelling aspects of the `d3` library. */
2+
3+
private import javascript
4+
private import semmle.javascript.security.dataflow.Xss
5+
6+
/** Provides classes and predicates modelling aspects of the `d3` library. */
7+
module D3 {
8+
/** The global variable `d3` as an entry point for API graphs. */
9+
private class D3GlobalEntry extends API::EntryPoint {
10+
D3GlobalEntry() { this = "D3GlobalEntry" }
11+
12+
override DataFlow::SourceNode getAUse() { result = DataFlow::globalVarRef("d3") }
13+
14+
override DataFlow::Node getARhs() { none() }
15+
}
16+
17+
/** Gets an API node referring to the `d3` module. */
18+
API::Node d3() {
19+
result = API::moduleImport("d3")
20+
or
21+
// recognize copies of d3 in a scope
22+
result = API::moduleImport(any(string s | s.regexpMatch("@.*/d3(-\\w+)?")))
23+
or
24+
result = API::moduleImport("d3-node").getInstance().getMember("d3")
25+
or
26+
result = API::root().getASuccessor(any(D3GlobalEntry i))
27+
}
28+
29+
/**
30+
* Gets an API node referring to the given module, or to `d3`.
31+
*
32+
* Useful for accessing modules that are re-exported by `d3`.
33+
*/
34+
bindingset[name]
35+
API::Node d3Module(string name) {
36+
result = d3() // all d3 modules are re-exported as part of 'd3'
37+
or
38+
result = API::moduleImport(name)
39+
}
40+
41+
/** Gets an API node referring to a `d3` selection object, such as `d3.selection()`. */
42+
API::Node d3Selection() {
43+
result = d3Module("d3-selection").getMember(["selection", "select", "selectAll"]).getReturn()
44+
or
45+
exists(API::CallNode call, string name |
46+
call = d3Selection().getMember(name).getACall() and
47+
result = call.getReturn()
48+
|
49+
name =
50+
[
51+
"select", "selectAll", "filter", "merge", "selectChild", "selectChildren", "selection",
52+
"insert", "remove", "clone", "sort", "order", "raise", "lower", "append", "data", "join",
53+
"enter", "exit", "call"
54+
]
55+
or
56+
name = ["text", "html", "datum"] and
57+
call.getNumArgument() > 0 // exclude 0-argument version, which returns the current value
58+
or
59+
name = ["attr", "classed", "style", "property", "on"] and
60+
call.getNumArgument() > 1 // exclude 1-argument version, which returns the current value
61+
or
62+
// Setting multiple things at once
63+
name = ["attr", "classed", "style", "property", "on"] and
64+
call.getArgument(0).getALocalSource() instanceof DataFlow::ObjectLiteralNode
65+
)
66+
or
67+
result = d3Selection().getMember("call").getParameter(0).getParameter(0)
68+
}
69+
70+
private class D3XssSink extends DomBasedXss::Sink {
71+
D3XssSink() {
72+
exists(API::Node htmlArg |
73+
htmlArg = d3Selection().getMember("html").getParameter(0) and
74+
this = [htmlArg, htmlArg.getReturn()].getARhs()
75+
)
76+
}
77+
}
78+
79+
private class D3DomValueSource extends DOM::DomValueSource::Range {
80+
D3DomValueSource() {
81+
this = d3Selection().getMember("each").getReceiver().getAnImmediateUse()
82+
or
83+
this = d3Selection().getMember("node").getReturn().getAnImmediateUse()
84+
or
85+
this = d3Selection().getMember("nodes").getReturn().getUnknownMember().getAnImmediateUse()
86+
}
87+
}
88+
89+
private class D3AttributeDefinition extends DOM::AttributeDefinition {
90+
DataFlow::CallNode call;
91+
92+
D3AttributeDefinition() {
93+
call = d3Selection().getMember("attr").getACall() and
94+
call.getNumArgument() > 1 and
95+
this = call.asExpr()
96+
}
97+
98+
override string getName() { result = call.getArgument(0).getStringValue() }
99+
100+
override DataFlow::Node getValueNode() { result = call.getArgument(1) }
101+
}
102+
}

‎javascript/ql/src/semmle/javascript/frameworks/jQuery.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -479,8 +479,8 @@ module JQuery {
479479
// either a reference to a global variable `$` or `jQuery`
480480
this = DataFlow::globalVarRef(any(string jq | jq = "$" or jq = "jQuery"))
481481
or
482-
// or imported from a module named `jquery`
483-
this = DataFlow::moduleImport("jquery")
482+
// or imported from a module named `jquery` or `zepto`
483+
this = DataFlow::moduleImport(["jquery", "zepto"])
484484
or
485485
this.hasUnderlyingType("JQueryStatic")
486486
}

‎javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,11 @@ module XssThroughDom {
2727
result = ["name", "value", "title", "alt"]
2828
}
2929

30+
/**
31+
* Gets a DOM property name that could store user-controlled data.
32+
*/
33+
string unsafeDomPropertyName() { result = ["innerText", "textContent", "value", "name"] }
34+
3035
/**
3136
* A source for text from the DOM from a JQuery method call.
3237
*/
@@ -35,10 +40,16 @@ module XssThroughDom {
3540
(
3641
this.getMethodName() = ["text", "val"] and this.getNumArgument() = 0
3742
or
38-
this.getMethodName() = "attr" and
39-
this.getNumArgument() = 1 and
40-
forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
41-
this.getArgument(0).mayHaveStringValue(unsafeAttributeName())
43+
exists(string methodName, string value |
44+
this.getMethodName() = methodName and
45+
this.getNumArgument() = 1 and
46+
forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
47+
this.getArgument(0).mayHaveStringValue(value)
48+
|
49+
methodName = "attr" and value = unsafeAttributeName()
50+
or
51+
methodName = "prop" and value = unsafeDomPropertyName()
52+
)
4253
) and
4354
// looks like a $("<p>" + ... ) source, which is benign for this query.
4455
not exists(DataFlow::Node prefix |
@@ -51,14 +62,37 @@ module XssThroughDom {
5162
}
5263
}
5364

65+
/**
66+
* A source for text from the DOM from a `d3` method call.
67+
*/
68+
class D3TextSource extends Source {
69+
D3TextSource() {
70+
exists(DataFlow::MethodCallNode call, string methodName |
71+
this = call and
72+
call = D3::d3Selection().getMember(methodName).getACall()
73+
|
74+
methodName = "attr" and
75+
call.getNumArgument() = 1 and
76+
call.getArgument(0).mayHaveStringValue(unsafeAttributeName())
77+
or
78+
methodName = "property" and
79+
call.getNumArgument() = 1 and
80+
call.getArgument(0).mayHaveStringValue(unsafeDomPropertyName())
81+
or
82+
methodName = "text" and
83+
call.getNumArgument() = 0
84+
)
85+
}
86+
}
87+
5488
/**
5589
* A source for text from the DOM from a DOM property read or call to `getAttribute()`.
5690
*/
5791
class DOMTextSource extends Source {
5892
DOMTextSource() {
5993
exists(DataFlow::PropRead read | read = this |
6094
read.getBase().getALocalSource() = DOM::domValueRef() and
61-
read.mayHavePropertyName(["innerText", "textContent", "value", "name"])
95+
read.mayHavePropertyName(unsafeDomPropertyName())
6296
)
6397
or
6498
exists(DataFlow::MethodCallNode mcn | mcn = this |

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,16 @@ nodes
9292
| classnames.js:15:47:15:63 | clsx(window.name) |
9393
| classnames.js:15:52:15:62 | window.name |
9494
| classnames.js:15:52:15:62 | window.name |
95+
| d3.js:4:12:4:22 | window.name |
96+
| d3.js:4:12:4:22 | window.name |
97+
| d3.js:11:15:11:24 | getTaint() |
98+
| d3.js:11:15:11:24 | getTaint() |
99+
| d3.js:12:20:12:29 | getTaint() |
100+
| d3.js:12:20:12:29 | getTaint() |
101+
| d3.js:14:20:14:29 | getTaint() |
102+
| d3.js:14:20:14:29 | getTaint() |
103+
| d3.js:21:15:21:24 | getTaint() |
104+
| d3.js:21:15:21:24 | getTaint() |
95105
| dates.js:9:9:9:69 | taint |
96106
| dates.js:9:17:9:69 | decodeU ... ing(1)) |
97107
| dates.js:9:36:9:50 | window.location |
@@ -772,6 +782,22 @@ edges
772782
| classnames.js:15:47:15:63 | clsx(window.name) | classnames.js:15:31:15:78 | `<span ... <span>` |
773783
| classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) |
774784
| classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) |
785+
| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
786+
| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
787+
| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
788+
| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
789+
| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() |
790+
| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() |
791+
| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() |
792+
| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() |
793+
| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() |
794+
| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() |
795+
| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() |
796+
| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() |
797+
| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() |
798+
| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() |
799+
| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() |
800+
| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() |
775801
| dates.js:9:9:9:69 | taint | dates.js:11:63:11:67 | taint |
776802
| dates.js:9:9:9:69 | taint | dates.js:12:66:12:70 | taint |
777803
| dates.js:9:9:9:69 | taint | dates.js:13:59:13:63 | taint |
@@ -1338,6 +1364,10 @@ edges
13381364
| classnames.js:11:31:11:79 | `<span ... <span>` | classnames.js:10:45:10:55 | window.name | classnames.js:11:31:11:79 | `<span ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:10:45:10:55 | window.name | user-provided value |
13391365
| classnames.js:13:31:13:83 | `<span ... <span>` | classnames.js:13:57:13:67 | window.name | classnames.js:13:31:13:83 | `<span ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:13:57:13:67 | window.name | user-provided value |
13401366
| classnames.js:15:31:15:78 | `<span ... <span>` | classnames.js:15:52:15:62 | window.name | classnames.js:15:31:15:78 | `<span ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:15:52:15:62 | window.name | user-provided value |
1367+
| 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 |
1368+
| 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 |
1369+
| 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 |
1370+
| 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 |
13411371
| 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 |
13421372
| 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 |
13431373
| 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 |

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,16 @@ nodes
9292
| classnames.js:15:47:15:63 | clsx(window.name) |
9393
| classnames.js:15:52:15:62 | window.name |
9494
| classnames.js:15:52:15:62 | window.name |
95+
| d3.js:4:12:4:22 | window.name |
96+
| d3.js:4:12:4:22 | window.name |
97+
| d3.js:11:15:11:24 | getTaint() |
98+
| d3.js:11:15:11:24 | getTaint() |
99+
| d3.js:12:20:12:29 | getTaint() |
100+
| d3.js:12:20:12:29 | getTaint() |
101+
| d3.js:14:20:14:29 | getTaint() |
102+
| d3.js:14:20:14:29 | getTaint() |
103+
| d3.js:21:15:21:24 | getTaint() |
104+
| d3.js:21:15:21:24 | getTaint() |
95105
| dates.js:9:9:9:69 | taint |
96106
| dates.js:9:17:9:69 | decodeU ... ing(1)) |
97107
| dates.js:9:36:9:50 | window.location |
@@ -783,6 +793,22 @@ edges
783793
| classnames.js:15:47:15:63 | clsx(window.name) | classnames.js:15:31:15:78 | `<span ... <span>` |
784794
| classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) |
785795
| classnames.js:15:52:15:62 | window.name | classnames.js:15:47:15:63 | clsx(window.name) |
796+
| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
797+
| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
798+
| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
799+
| d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
800+
| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() |
801+
| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() |
802+
| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() |
803+
| d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() |
804+
| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() |
805+
| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() |
806+
| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() |
807+
| d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() |
808+
| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() |
809+
| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() |
810+
| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() |
811+
| d3.js:4:12:4:22 | window.name | d3.js:21:15:21:24 | getTaint() |
786812
| dates.js:9:9:9:69 | taint | dates.js:11:63:11:67 | taint |
787813
| dates.js:9:9:9:69 | taint | dates.js:12:66:12:70 | taint |
788814
| dates.js:9:9:9:69 | taint | dates.js:13:59:13:63 | taint |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/d3.js

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
const d3 = require('d3');
2+
3+
function getTaint() {
4+
return window.name;
5+
}
6+
7+
function doSomething() {
8+
d3.select('#main')
9+
.attr('width', 100)
10+
.style('color', 'red')
11+
.html(getTaint()) // NOT OK
12+
.html(d => getTaint()) // NOT OK
13+
.call(otherFunction)
14+
.html(d => getTaint()); // NOT OK
15+
}
16+
17+
18+
function otherFunction(selection) {
19+
selection
20+
.attr('foo', 'bar')
21+
.html(getTaint()); // NOT OK
22+
}

‎javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/XssThroughDom.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,9 @@ nodes
100100
| xss-through-dom.js:79:4:79:34 | documen ... t.value |
101101
| xss-through-dom.js:79:4:79:34 | documen ... t.value |
102102
| xss-through-dom.js:79:4:79:34 | documen ... t.value |
103+
| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') |
104+
| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') |
105+
| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') |
103106
edges
104107
| forms.js:8:23:8:28 | values | forms.js:9:31:9:36 | values |
105108
| forms.js:8:23:8:28 | values | forms.js:9:31:9:36 | values |
@@ -157,6 +160,7 @@ edges
157160
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:73:9:73:41 | selector |
158161
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:73:9:73:41 | selector |
159162
| xss-through-dom.js:79:4:79:34 | documen ... t.value | xss-through-dom.js:79:4:79:34 | documen ... t.value |
163+
| xss-through-dom.js:81:17:81:43 | $('#foo ... rText') | xss-through-dom.js:81:17:81:43 | $('#foo ... rText') |
160164
#select
161165
| 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 |
162166
| 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
185189
| 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 |
186190
| 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 |
187191
| 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 |
192+
| 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 |

‎javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,6 @@
7777
$(selector); // NOT OK
7878

7979
$(document.my_form.my_input.value); // NOT OK
80+
81+
$("#id").html( $('#foo').prop('innerText') ); // NOT OK
8082
})();

0 commit comments

Comments
 (0)