Skip to content

Commit 1c8547c

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#4774 from erik-krogh/forms
Approved by asgerf
2 parents 4bc287e + 47488f8 commit 1c8547c

File tree

5 files changed

+50
-6
lines changed

5 files changed

+50
-6
lines changed

‎javascript/ql/src/semmle/javascript/DOM.qll

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,23 @@ module DOM {
298298
)
299299
}
300300

301+
/**
302+
* A data flow node that might refer to some form.
303+
* Either by a read like `document.forms[0]`, or a property read from `document` with some constant property-name.
304+
* E.g. if `<form name="foobar">..</form>` exists, then `document.foobar` refers to that form.
305+
*/
306+
private DataFlow::SourceNode forms() {
307+
result = documentRef().getAPropertyRead("forms").getAPropertyRead()
308+
or
309+
exists(DataFlow::PropRead read |
310+
read = documentRef().getAPropertyRead() and
311+
result = read
312+
|
313+
read.mayHavePropertyName(_) and
314+
not read.mayHavePropertyName(getADomPropertyName())
315+
)
316+
}
317+
301318
private class DefaultRange extends Range {
302319
DefaultRange() {
303320
this.asExpr().(VarAccess).getVariable() instanceof DOMGlobalVariable
@@ -317,6 +334,14 @@ module DOM {
317334
or
318335
this = domElementCollection()
319336
or
337+
this = forms()
338+
or
339+
// reading property `foo` - where a child has `name="foo"` - resolves to that child.
340+
// We only look for such properties on forms/document, to avoid potential false positives.
341+
exists(DataFlow::SourceNode form | form = [forms(), documentRef()] |
342+
this = form.getAPropertyRead(any(string s | not s = getADomPropertyName()))
343+
)
344+
or
320345
exists(JQuery::MethodCall call | this = call and call.getMethodName() = "get" |
321346
call.getNumArgument() = 1 and
322347
forex(InferredType t | t = call.getArgument(0).analyze().getAType() | t = TTNumber())

‎javascript/ql/test/library-tests/DOM/Customizations.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ test_documentRef
44
test_locationRef
55
| customization.js:3:3:3:14 | doc.___location |
66
test_domValueRef
7+
| customization.js:4:3:4:20 | doc.getElementById |
78
| customization.js:4:3:4:28 | doc.get ... 'test') |
9+
| nameditems.js:1:1:1:23 | documen ... entById |
810
| nameditems.js:1:1:1:30 | documen ... ('foo') |
911
| nameditems.js:1:1:2:19 | documen ... em('x') |
1012
| tst.js:49:3:49:8 | window |

‎javascript/ql/test/library-tests/DOM/externs/externs.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,13 @@ function EventTarget() {}
88

99
/** @type {EventTarget} */
1010
var window;
11+
12+
/**
13+
* @see http://dev.w3.org/html5/workers/
14+
* @interface
15+
* @extends {EventTarget}
16+
*/
17+
function WorkerGlobalScope() {}
18+
19+
/** @type {WorkerLocation} */
20+
WorkerGlobalScope.prototype.___location;

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,11 @@ nodes
4444
| xss-through-dom.js:73:9:73:41 | selector |
4545
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name |
4646
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name |
47-
| xss-through-dom.js:77:7:77:14 | selector |
48-
| xss-through-dom.js:77:7:77:14 | selector |
47+
| xss-through-dom.js:77:4:77:11 | selector |
48+
| xss-through-dom.js:77:4:77:11 | selector |
49+
| xss-through-dom.js:79:4:79:34 | documen ... t.value |
50+
| xss-through-dom.js:79:4:79:34 | documen ... t.value |
51+
| xss-through-dom.js:79:4:79:34 | documen ... t.value |
4952
edges
5053
| xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() |
5154
| xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
@@ -61,10 +64,11 @@ edges
6164
| xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") |
6265
| xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() |
6366
| xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name | xss-through-dom.js:71:11:71:32 | $("inpu ... 0).name |
64-
| xss-through-dom.js:73:9:73:41 | selector | xss-through-dom.js:77:7:77:14 | selector |
65-
| xss-through-dom.js:73:9:73:41 | selector | xss-through-dom.js:77:7:77:14 | selector |
67+
| xss-through-dom.js:73:9:73:41 | selector | xss-through-dom.js:77:4:77:11 | selector |
68+
| xss-through-dom.js:73:9:73:41 | selector | xss-through-dom.js:77:4:77:11 | selector |
6669
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:73:9:73:41 | selector |
6770
| xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:73:9:73:41 | selector |
71+
| xss-through-dom.js:79:4:79:34 | documen ... t.value | xss-through-dom.js:79:4:79:34 | documen ... t.value |
6872
#select
6973
| xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:2:16:2:34 | $("textarea").val() | DOM text |
7074
| xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | DOM text |
@@ -80,4 +84,5 @@ edges
8084
| xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:61:30:61:69 | $(docum ... value") | DOM text |
8185
| xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:64:30:64:40 | valMethod() | DOM text |
8286
| 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 |
83-
| xss-through-dom.js:77:7:77:14 | selector | xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | xss-through-dom.js:77:7:77:14 | selector | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:73:20:73:41 | $("inpu ... 0).name | DOM text |
87+
| 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 |
88+
| 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 |

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,5 +74,7 @@
7474
if (something()) {
7575
selector = $("textarea").val || ''
7676
}
77-
$(selector); // NOT OK
77+
$(selector); // NOT OK
78+
79+
$(document.my_form.my_input.value); // NOT OK
7880
})();

0 commit comments

Comments
 (0)