Skip to content

Commit ad40c4b

Browse files
erik-krogherik-krogh
committed
add a sanitizer guard for safe attribute string concatenations
1 parent a9bea63 commit ad40c4b

File tree

8 files changed

+94
-1
lines changed

8 files changed

+94
-1
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ module DomBasedXss {
2424
node instanceof Sanitizer
2525
}
2626

27+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
28+
guard instanceof SanitizerGuard
29+
}
30+
2731
override predicate isAdditionalLoadStoreStep(
2832
DataFlow::Node pred, DataFlow::Node succ, string predProp, string succProp
2933
) {

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,5 +22,9 @@ module ReflectedXss {
2222
super.isSanitizer(node) or
2323
node instanceof Sanitizer
2424
}
25+
26+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
27+
guard instanceof SanitizerGuard
28+
}
2529
}
2630
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ module StoredXss {
2222
super.isSanitizer(node) or
2323
node instanceof Sanitizer
2424
}
25+
26+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
27+
guard instanceof SanitizerGuard
28+
}
2529
}
2630

2731
/** A file name, considered as a flow source for stored XSS. */

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ module Shared {
2323
/** A sanitizer for XSS vulnerabilities. */
2424
abstract class Sanitizer extends DataFlow::Node { }
2525

26+
/** A sanitizer guard for XSS vulnerabilities. */
27+
abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode { }
28+
2629
/**
2730
* A regexp replacement involving an HTML meta-character, viewed as a sanitizer for
2831
* XSS vulnerabilities.
@@ -51,10 +54,30 @@ module Shared {
5154
)
5255
}
5356
}
57+
58+
private import semmle.javascript.security.dataflow.IncompleteHtmlAttributeSanitizationCustomizations::IncompleteHtmlAttributeSanitization as IncompleteHTML
59+
60+
/**
61+
* A guard that checks if a string can contain quotes, which is a guard for strings that are inside a HTML attribute.
62+
*/
63+
class QuoteGuard extends SanitizerGuard, StringOps::Includes {
64+
QuoteGuard() {
65+
this.getSubstring().mayHaveStringValue("\"") and
66+
this
67+
.getBaseString()
68+
.getALocalSource()
69+
.flowsTo(any(IncompleteHTML::HtmlAttributeConcatenation attributeConcat))
70+
}
71+
72+
override predicate sanitizes(boolean outcome, Expr e) {
73+
e = this.getBaseString().getEnclosingExpr() and outcome = this.getPolarity().booleanNot()
74+
}
75+
}
5476
}
5577

5678
/** Provides classes and predicates for the DOM-based XSS query. */
5779
module DomBasedXss {
80+
// StringReplaceCallSequence
5881
/** A data flow source for DOM-based XSS vulnerabilities. */
5982
abstract class Source extends Shared::Source { }
6083

@@ -64,6 +87,9 @@ module DomBasedXss {
6487
/** A sanitizer for DOM-based XSS vulnerabilities. */
6588
abstract class Sanitizer extends Shared::Sanitizer { }
6689

90+
/** A sanitizer guard for DOM-based XSS vulnerabilities. */
91+
abstract class SanitizerGuard extends Shared::SanitizerGuard { }
92+
6793
/**
6894
* An expression whose value is interpreted as HTML
6995
* and may be inserted into the DOM through a library.
@@ -302,6 +328,8 @@ module DomBasedXss {
302328
private class MetacharEscapeSanitizer extends Sanitizer, Shared::MetacharEscapeSanitizer { }
303329

304330
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
331+
332+
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
305333
}
306334

307335
/** Provides classes and predicates for the reflected XSS query. */
@@ -315,6 +343,9 @@ module ReflectedXss {
315343
/** A sanitizer for reflected XSS vulnerabilities. */
316344
abstract class Sanitizer extends Shared::Sanitizer { }
317345

346+
/** A sanitizer guard for reflected XSS vulnerabilities. */
347+
abstract class SanitizerGuard extends Shared::SanitizerGuard { }
348+
318349
/**
319350
* An expression that is sent as part of an HTTP response, considered as an XSS sink.
320351
*
@@ -401,6 +432,8 @@ module ReflectedXss {
401432
private class MetacharEscapeSanitizer extends Sanitizer, Shared::MetacharEscapeSanitizer { }
402433

403434
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
435+
436+
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
404437
}
405438

406439
/** Provides classes and predicates for the stored XSS query. */
@@ -414,6 +447,9 @@ module StoredXss {
414447
/** A sanitizer for stored XSS vulnerabilities. */
415448
abstract class Sanitizer extends Shared::Sanitizer { }
416449

450+
/** A sanitizer guard for stored XSS vulnerabilities. */
451+
abstract class SanitizerGuard extends Shared::SanitizerGuard { }
452+
417453
/** An arbitrary XSS sink, considered as a flow sink for stored XSS. */
418454
private class AnySink extends Sink {
419455
AnySink() { this instanceof Shared::Sink }
@@ -429,6 +465,8 @@ module StoredXss {
429465
private class MetacharEscapeSanitizer extends Sanitizer, Shared::MetacharEscapeSanitizer { }
430466

431467
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
468+
469+
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
432470
}
433471

434472
/** Provides classes and predicates for the XSS through DOM query. */

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ module XssThroughDom {
3131

3232
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
3333
guard instanceof TypeTestGuard or
34-
guard instanceof UnsafeJQuery::PropertyPresenceSanitizer
34+
guard instanceof UnsafeJQuery::PropertyPresenceSanitizer or
35+
guard instanceof DomBasedXss::SanitizerGuard
3536
}
3637
}
3738

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,11 @@ nodes
5353
| stored-xss.js:5:20:5:52 | session ... ssion') |
5454
| stored-xss.js:8:20:8:48 | localSt ... local') |
5555
| stored-xss.js:8:20:8:48 | localSt ... local') |
56+
| stored-xss.js:10:9:10:44 | href |
57+
| stored-xss.js:10:16:10:44 | localSt ... local') |
58+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
59+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
60+
| stored-xss.js:12:35:12:38 | href |
5661
| string-manipulations.js:3:16:3:32 | document.location |
5762
| string-manipulations.js:3:16:3:32 | document.location |
5863
| string-manipulations.js:3:16:3:32 | document.location |
@@ -431,6 +436,11 @@ edges
431436
| stored-xss.js:3:35:3:51 | document.location | stored-xss.js:3:35:3:58 | documen ... .search |
432437
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
433438
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
439+
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:10:16:10:44 | localSt ... local') |
440+
| stored-xss.js:10:9:10:44 | href | stored-xss.js:12:35:12:38 | href |
441+
| stored-xss.js:10:16:10:44 | localSt ... local') | stored-xss.js:10:9:10:44 | href |
442+
| stored-xss.js:12:35:12:38 | href | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
443+
| stored-xss.js:12:35:12:38 | href | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
434444
| string-manipulations.js:3:16:3:32 | document.location | string-manipulations.js:3:16:3:32 | document.location |
435445
| string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |
436446
| string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |
@@ -732,6 +742,7 @@ edges
732742
| react-native.js:9:27:9:33 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:9:27:9:33 | tainted | Cross-site scripting vulnerability due to $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value |
733743
| stored-xss.js:5:20:5:52 | session ... ssion') | stored-xss.js:2:39:2:55 | document.location | stored-xss.js:5:20:5:52 | session ... ssion') | Cross-site scripting vulnerability due to $@. | stored-xss.js:2:39:2:55 | document.location | user-provided value |
734744
| stored-xss.js:8:20:8:48 | localSt ... local') | stored-xss.js:3:35:3:51 | document.location | stored-xss.js:8:20:8:48 | localSt ... local') | Cross-site scripting vulnerability due to $@. | stored-xss.js:3:35:3:51 | document.location | user-provided value |
745+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" | stored-xss.js:3:35:3:51 | document.location | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" | Cross-site scripting vulnerability due to $@. | stored-xss.js:3:35:3:51 | document.location | user-provided value |
735746
| string-manipulations.js:3:16:3:32 | document.location | string-manipulations.js:3:16:3:32 | document.location | string-manipulations.js:3:16:3:32 | document.location | Cross-site scripting vulnerability due to $@. | string-manipulations.js:3:16:3:32 | document.location | user-provided value |
736747
| string-manipulations.js:4:16:4:37 | documen ... on.href | string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href | Cross-site scripting vulnerability due to $@. | string-manipulations.js:4:16:4:32 | document.location | user-provided value |
737748
| string-manipulations.js:5:16:5:47 | documen ... lueOf() | string-manipulations.js:5:16:5:32 | document.location | string-manipulations.js:5:16:5:47 | documen ... lueOf() | Cross-site scripting vulnerability due to $@. | string-manipulations.js:5:16:5:32 | document.location | user-provided value |

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,11 @@ nodes
5353
| stored-xss.js:5:20:5:52 | session ... ssion') |
5454
| stored-xss.js:8:20:8:48 | localSt ... local') |
5555
| stored-xss.js:8:20:8:48 | localSt ... local') |
56+
| stored-xss.js:10:9:10:44 | href |
57+
| stored-xss.js:10:16:10:44 | localSt ... local') |
58+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
59+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
60+
| stored-xss.js:12:35:12:38 | href |
5661
| string-manipulations.js:3:16:3:32 | document.location |
5762
| string-manipulations.js:3:16:3:32 | document.location |
5863
| string-manipulations.js:3:16:3:32 | document.location |
@@ -435,6 +440,11 @@ edges
435440
| stored-xss.js:3:35:3:51 | document.location | stored-xss.js:3:35:3:58 | documen ... .search |
436441
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
437442
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
443+
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:10:16:10:44 | localSt ... local') |
444+
| stored-xss.js:10:9:10:44 | href | stored-xss.js:12:35:12:38 | href |
445+
| stored-xss.js:10:16:10:44 | localSt ... local') | stored-xss.js:10:9:10:44 | href |
446+
| stored-xss.js:12:35:12:38 | href | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
447+
| stored-xss.js:12:35:12:38 | href | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
438448
| string-manipulations.js:3:16:3:32 | document.location | string-manipulations.js:3:16:3:32 | document.location |
439449
| string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |
440450
| string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |

‎javascript/ql/test/query-tests/Security/CWE-079/stored-xss.js

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,25 @@
66
$('myId').html(localStorage.getItem('session')); // OK
77
$('myId').html(sessionStorage.getItem('local')); // OK
88
$('myId').html(localStorage.getItem('local')); // NOT OK
9+
10+
var href = localStorage.getItem('local');
11+
12+
$('myId').html("<a href=\"" + href + ">foobar</a>"); // NOT OK
13+
14+
if (href.indexOf("\"") !== -1) {
15+
return;
16+
}
17+
$('myId').html("<a href=\"" + href + "/>"); // OK
18+
19+
var href2 = localStorage.getItem('local');
20+
if (href2.indexOf("\"") !== -1) {
21+
return;
22+
}
23+
$('myId').html("\n<a href=\"" + href2 + ">foobar</a>"); // OK
24+
25+
var href3 = localStorage.getItem('local');
26+
if (href3.indexOf("\"") !== -1) {
27+
return;
28+
}
29+
$('myId').html('\r\n<a href="/' + href3 + '">' + "something" + '</a>'); // OK
930
});

0 commit comments

Comments
 (0)