Skip to content

Commit 674c184

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3566 from erik-krogh/XssAttributeSanitizer
Approved by asgerf
2 parents 3cfc1e5 + 33da82d commit 674c184

File tree

10 files changed

+99
-4
lines changed

10 files changed

+99
-4
lines changed

‎change-notes/1.25/analysis-javascript.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
|--------------------------------|------------------------------|---------------------------------------------------------------------------|
3939
| Client-side cross-site scripting (`js/xss`) | Fewer results | This query no longer flags optionally sanitized values. |
4040
| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Fewer results | This query now recognizes additional safe patterns of doing URL redirects. |
41-
| Client-side cross-site scripting (`js/xss`) | Fewer results | This query now recognizes additional safe strings based on URLs. |
41+
| Client-side cross-site scripting (`js/xss`) | Fewer results | This query now recognizes additional safe patterns of constructing HTML. |
4242
| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
4343
| Expression has no effect (`js/useless-expression`) | Fewer results | This query no longer flags an expression when that expression is the only content of the containing file. |
4444
| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |

‎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/IncompleteHtmlAttributeSanitizationCustomizations.qll

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,11 @@ module IncompleteHtmlAttributeSanitization {
5151
string lhs;
5252

5353
HtmlAttributeConcatenation() {
54-
lhs = this.getPreviousLeaf().getStringValue().regexpCapture("(.*)=\"[^\"]*", 1) and
55-
this.getNextLeaf().getStringValue().regexpMatch(".*\".*")
54+
lhs = this.getPreviousLeaf().getStringValue().regexpCapture("(?s)(.*)=\"[^\"]*", 1) and
55+
(
56+
this.getNextLeaf().getStringValue().regexpMatch(".*\".*") or
57+
this instanceof StringOps::HtmlConcatenationLeaf
58+
)
5659
}
5760

5861
/**

‎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: 37 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,6 +54,25 @@ 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. */
@@ -64,6 +86,9 @@ module DomBasedXss {
6486
/** A sanitizer for DOM-based XSS vulnerabilities. */
6587
abstract class Sanitizer extends Shared::Sanitizer { }
6688

89+
/** A sanitizer guard for DOM-based XSS vulnerabilities. */
90+
abstract class SanitizerGuard extends Shared::SanitizerGuard { }
91+
6792
/**
6893
* An expression whose value is interpreted as HTML
6994
* and may be inserted into the DOM through a library.
@@ -303,6 +328,8 @@ module DomBasedXss {
303328

304329
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
305330

331+
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
332+
306333
/**
307334
* Holds if there exists two dataflow edges to `succ`, where one edges is sanitized, and the other edge starts with `pred`.
308335
*/
@@ -345,6 +372,9 @@ module ReflectedXss {
345372
/** A sanitizer for reflected XSS vulnerabilities. */
346373
abstract class Sanitizer extends Shared::Sanitizer { }
347374

375+
/** A sanitizer guard for reflected XSS vulnerabilities. */
376+
abstract class SanitizerGuard extends Shared::SanitizerGuard { }
377+
348378
/**
349379
* An expression that is sent as part of an HTTP response, considered as an XSS sink.
350380
*
@@ -431,6 +461,8 @@ module ReflectedXss {
431461
private class MetacharEscapeSanitizer extends Sanitizer, Shared::MetacharEscapeSanitizer { }
432462

433463
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
464+
465+
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
434466
}
435467

436468
/** Provides classes and predicates for the stored XSS query. */
@@ -444,6 +476,9 @@ module StoredXss {
444476
/** A sanitizer for stored XSS vulnerabilities. */
445477
abstract class Sanitizer extends Shared::Sanitizer { }
446478

479+
/** A sanitizer guard for stored XSS vulnerabilities. */
480+
abstract class SanitizerGuard extends Shared::SanitizerGuard { }
481+
447482
/** An arbitrary XSS sink, considered as a flow sink for stored XSS. */
448483
private class AnySink extends Sink {
449484
AnySink() { this instanceof Shared::Sink }
@@ -459,6 +494,8 @@ module StoredXss {
459494
private class MetacharEscapeSanitizer extends Sanitizer, Shared::MetacharEscapeSanitizer { }
460495

461496
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
497+
498+
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
462499
}
463500

464501
/** 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
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,11 @@ nodes
9494
| stored-xss.js:5:20:5:52 | session ... ssion') |
9595
| stored-xss.js:8:20:8:48 | localSt ... local') |
9696
| stored-xss.js:8:20:8:48 | localSt ... local') |
97+
| stored-xss.js:10:9:10:44 | href |
98+
| stored-xss.js:10:16:10:44 | localSt ... local') |
99+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
100+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
101+
| stored-xss.js:12:35:12:38 | href |
97102
| string-manipulations.js:3:16:3:32 | document.location |
98103
| string-manipulations.js:3:16:3:32 | document.location |
99104
| string-manipulations.js:3:16:3:32 | document.location |
@@ -517,6 +522,11 @@ edges
517522
| stored-xss.js:3:35:3:51 | document.location | stored-xss.js:3:35:3:58 | documen ... .search |
518523
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
519524
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
525+
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:10:16:10:44 | localSt ... local') |
526+
| stored-xss.js:10:9:10:44 | href | stored-xss.js:12:35:12:38 | href |
527+
| stored-xss.js:10:16:10:44 | localSt ... local') | stored-xss.js:10:9:10:44 | href |
528+
| stored-xss.js:12:35:12:38 | href | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
529+
| stored-xss.js:12:35:12:38 | href | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
520530
| string-manipulations.js:3:16:3:32 | document.location | string-manipulations.js:3:16:3:32 | document.location |
521531
| string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |
522532
| string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |
@@ -826,6 +836,7 @@ edges
826836
| 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 |
827837
| 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 |
828838
| 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 |
839+
| 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 |
829840
| 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 |
830841
| 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 |
831842
| 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
@@ -94,6 +94,11 @@ nodes
9494
| stored-xss.js:5:20:5:52 | session ... ssion') |
9595
| stored-xss.js:8:20:8:48 | localSt ... local') |
9696
| stored-xss.js:8:20:8:48 | localSt ... local') |
97+
| stored-xss.js:10:9:10:44 | href |
98+
| stored-xss.js:10:16:10:44 | localSt ... local') |
99+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
100+
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
101+
| stored-xss.js:12:35:12:38 | href |
97102
| string-manipulations.js:3:16:3:32 | document.location |
98103
| string-manipulations.js:3:16:3:32 | document.location |
99104
| string-manipulations.js:3:16:3:32 | document.location |
@@ -521,6 +526,11 @@ edges
521526
| stored-xss.js:3:35:3:51 | document.location | stored-xss.js:3:35:3:58 | documen ... .search |
522527
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
523528
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
529+
| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:10:16:10:44 | localSt ... local') |
530+
| stored-xss.js:10:9:10:44 | href | stored-xss.js:12:35:12:38 | href |
531+
| stored-xss.js:10:16:10:44 | localSt ... local') | stored-xss.js:10:9:10:44 | href |
532+
| stored-xss.js:12:35:12:38 | href | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
533+
| stored-xss.js:12:35:12:38 | href | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" |
524534
| string-manipulations.js:3:16:3:32 | document.location | string-manipulations.js:3:16:3:32 | document.location |
525535
| string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |
526536
| 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)