Skip to content

Commit 4750327

Browse files
codeql-cicodeql-ci
authored
Merge pull request #6311 from asgerf/js/dom-element-methods
Approved by erik-krogh
2 parents 6a09a56 + 1b67b43 commit 4750327

File tree

6 files changed

+88
-2
lines changed

6 files changed

+88
-2
lines changed

‎javascript/change-notes/2021-07-16-dom-element-methods.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
lgtm,codescanning
2+
* Some methods from the DOM API are now modeled more precisely, potentially
3+
leading to more `js/xss` results.

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

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,10 +291,33 @@ module DOM {
291291
*/
292292
abstract class Range extends DataFlow::Node { }
293293

294+
private predicate isDomElementType(ExternalType type) { isDomRootType(type.getASupertype*()) }
295+
294296
private string getADomPropertyName() {
295297
exists(ExternalInstanceMemberDecl decl |
296298
result = decl.getName() and
297-
isDomRootType(decl.getDeclaringType().getASupertype*())
299+
isDomElementType(decl.getDeclaringType())
300+
)
301+
}
302+
303+
private predicate isDomElementTypeName(string name) {
304+
exists(ExternalType type |
305+
isDomElementType(type) and
306+
name = type.getName()
307+
)
308+
}
309+
310+
/** Gets a method name which, if invoked on a DOM element (possibly of a specific subtype), returns a DOM element. */
311+
private string getAMethodProducingDomElements() {
312+
exists(ExternalInstanceMemberDecl decl |
313+
result = decl.getName() and
314+
isDomElementType(decl.getDeclaringType()) and
315+
isDomElementTypeName(decl.getDocumentation()
316+
.getATagByTitle("return")
317+
.getType()
318+
.getAnUnderlyingType()
319+
.(JSDocNamedTypeExpr)
320+
.getName())
298321
)
299322
}
300323

@@ -339,6 +362,8 @@ module DOM {
339362
or
340363
this = domElementCollection()
341364
or
365+
this = domValueRef().getAMethodCall(getAMethodProducingDomElements())
366+
or
342367
this = forms()
343368
or
344369
// reading property `foo` - where a child has `name="foo"` - resolves to that child.

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -699,6 +699,15 @@ nodes
699699
| tst.js:456:18:456:42 | ansiToH ... source) |
700700
| tst.js:456:18:456:42 | ansiToH ... source) |
701701
| tst.js:456:36:456:41 | source |
702+
| tst.js:460:6:460:38 | source |
703+
| tst.js:460:15:460:38 | documen ... .search |
704+
| tst.js:460:15:460:38 | documen ... .search |
705+
| tst.js:463:21:463:26 | source |
706+
| tst.js:463:21:463:26 | source |
707+
| tst.js:465:19:465:24 | source |
708+
| tst.js:465:19:465:24 | source |
709+
| tst.js:467:20:467:25 | source |
710+
| tst.js:467:20:467:25 | source |
702711
| typeahead.js:20:13:20:45 | target |
703712
| typeahead.js:20:22:20:45 | documen ... .search |
704713
| typeahead.js:20:22:20:45 | documen ... .search |
@@ -1369,6 +1378,14 @@ edges
13691378
| tst.js:453:16:453:39 | documen ... .search | tst.js:453:7:453:39 | source |
13701379
| tst.js:456:36:456:41 | source | tst.js:456:18:456:42 | ansiToH ... source) |
13711380
| tst.js:456:36:456:41 | source | tst.js:456:18:456:42 | ansiToH ... source) |
1381+
| tst.js:460:6:460:38 | source | tst.js:463:21:463:26 | source |
1382+
| tst.js:460:6:460:38 | source | tst.js:463:21:463:26 | source |
1383+
| tst.js:460:6:460:38 | source | tst.js:465:19:465:24 | source |
1384+
| tst.js:460:6:460:38 | source | tst.js:465:19:465:24 | source |
1385+
| tst.js:460:6:460:38 | source | tst.js:467:20:467:25 | source |
1386+
| tst.js:460:6:460:38 | source | tst.js:467:20:467:25 | source |
1387+
| tst.js:460:15:460:38 | documen ... .search | tst.js:460:6:460:38 | source |
1388+
| tst.js:460:15:460:38 | documen ... .search | tst.js:460:6:460:38 | source |
13721389
| typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target |
13731390
| typeahead.js:20:22:20:45 | documen ... .search | typeahead.js:20:13:20:45 | target |
13741391
| typeahead.js:20:22:20:45 | documen ... .search | typeahead.js:20:13:20:45 | target |
@@ -1598,6 +1615,9 @@ edges
15981615
| tst.js:445:32:445:37 | source | tst.js:436:15:436:38 | documen ... .search | tst.js:445:32:445:37 | source | Cross-site scripting vulnerability due to $@. | tst.js:436:15:436:38 | documen ... .search | user-provided value |
15991616
| tst.js:455:18:455:23 | source | tst.js:453:16:453:39 | documen ... .search | tst.js:455:18:455:23 | source | Cross-site scripting vulnerability due to $@. | tst.js:453:16:453:39 | documen ... .search | user-provided value |
16001617
| tst.js:456:18:456:42 | ansiToH ... source) | tst.js:453:16:453:39 | documen ... .search | tst.js:456:18:456:42 | ansiToH ... source) | Cross-site scripting vulnerability due to $@. | tst.js:453:16:453:39 | documen ... .search | user-provided value |
1618+
| tst.js:463:21:463:26 | source | tst.js:460:15:460:38 | documen ... .search | tst.js:463:21:463:26 | source | Cross-site scripting vulnerability due to $@. | tst.js:460:15:460:38 | documen ... .search | user-provided value |
1619+
| tst.js:465:19:465:24 | source | tst.js:460:15:460:38 | documen ... .search | tst.js:465:19:465:24 | source | Cross-site scripting vulnerability due to $@. | tst.js:460:15:460:38 | documen ... .search | user-provided value |
1620+
| tst.js:467:20:467:25 | source | tst.js:460:15:460:38 | documen ... .search | tst.js:467:20:467:25 | source | Cross-site scripting vulnerability due to $@. | tst.js:460:15:460:38 | documen ... .search | user-provided value |
16011621
| typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:45 | documen ... .search | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:45 | documen ... .search | user-provided value |
16021622
| v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.location | user-provided value |
16031623
| various-concat-obfuscations.js:4:4:4:31 | "<div>" ... </div>" | various-concat-obfuscations.js:2:16:2:39 | documen ... .search | various-concat-obfuscations.js:4:4:4:31 | "<div>" ... </div>" | Cross-site scripting vulnerability due to $@. | various-concat-obfuscations.js:2:16:2:39 | documen ... .search | user-provided value |

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -706,6 +706,15 @@ nodes
706706
| tst.js:456:18:456:42 | ansiToH ... source) |
707707
| tst.js:456:18:456:42 | ansiToH ... source) |
708708
| tst.js:456:36:456:41 | source |
709+
| tst.js:460:6:460:38 | source |
710+
| tst.js:460:15:460:38 | documen ... .search |
711+
| tst.js:460:15:460:38 | documen ... .search |
712+
| tst.js:463:21:463:26 | source |
713+
| tst.js:463:21:463:26 | source |
714+
| tst.js:465:19:465:24 | source |
715+
| tst.js:465:19:465:24 | source |
716+
| tst.js:467:20:467:25 | source |
717+
| tst.js:467:20:467:25 | source |
709718
| typeahead.js:9:28:9:30 | loc |
710719
| typeahead.js:9:28:9:30 | loc |
711720
| typeahead.js:10:16:10:18 | loc |
@@ -1393,6 +1402,14 @@ edges
13931402
| tst.js:453:16:453:39 | documen ... .search | tst.js:453:7:453:39 | source |
13941403
| tst.js:456:36:456:41 | source | tst.js:456:18:456:42 | ansiToH ... source) |
13951404
| tst.js:456:36:456:41 | source | tst.js:456:18:456:42 | ansiToH ... source) |
1405+
| tst.js:460:6:460:38 | source | tst.js:463:21:463:26 | source |
1406+
| tst.js:460:6:460:38 | source | tst.js:463:21:463:26 | source |
1407+
| tst.js:460:6:460:38 | source | tst.js:465:19:465:24 | source |
1408+
| tst.js:460:6:460:38 | source | tst.js:465:19:465:24 | source |
1409+
| tst.js:460:6:460:38 | source | tst.js:467:20:467:25 | source |
1410+
| tst.js:460:6:460:38 | source | tst.js:467:20:467:25 | source |
1411+
| tst.js:460:15:460:38 | documen ... .search | tst.js:460:6:460:38 | source |
1412+
| tst.js:460:15:460:38 | documen ... .search | tst.js:460:6:460:38 | source |
13961413
| typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
13971414
| typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
13981415
| typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,3 +57,13 @@ function Node() {}
5757
* @type {Node}
5858
*/
5959
Node.prototype.parentNode;
60+
61+
/**
62+
* @return {DomObjectStub}
63+
*/
64+
DomObjectStub.prototype.insertRow = function() {};
65+
66+
/**
67+
* @return {DomObjectStub}
68+
*/
69+
DomObjectStub.prototype.insertCell = function() {};

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -454,4 +454,15 @@ function ansiToHTML() {
454454

455455
$("#foo").html(source); // NOT OK
456456
$("#foo").html(ansiToHtml.toHtml(source)); // NOT OK
457-
}
457+
}
458+
459+
function domMethods() {
460+
var source = document.location.search;
461+
462+
let table = document.getElementById('mytable');
463+
table.innerHTML = source; // NOT OK
464+
let row = table.insertRow(-1);
465+
row.innerHTML = source; // NOT OK
466+
let cell = row.insertCell();
467+
cell.innerHTML = source; // NOT OK
468+
}

0 commit comments

Comments
 (0)