Skip to content

Commit 3cfc1e5

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3560 from erik-krogh/OptionalSanitizer
Approved by asgerf
2 parents fd05314 + 319363f commit 3cfc1e5

File tree

9 files changed

+292
-0
lines changed

9 files changed

+292
-0
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636

3737
| **Query** | **Expected impact** | **Change** |
3838
|--------------------------------|------------------------------|---------------------------------------------------------------------------|
39+
| Client-side cross-site scripting (`js/xss`) | Fewer results | This query no longer flags optionally sanitized values. |
3940
| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Fewer results | This query now recognizes additional safe patterns of doing URL redirects. |
4041
| Client-side cross-site scripting (`js/xss`) | Fewer results | This query now recognizes additional safe strings based on URLs. |
4142
| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,11 @@ private class DefaultHtmlSanitizerCall extends HtmlSanitizerCall {
4848
or
4949
callee = LodashUnderscore::member("escape")
5050
or
51+
exists(DataFlow::PropRead read | read = callee |
52+
read.getPropertyName() = "sanitize" and
53+
read.getBase().asExpr().(VarAccess).getName() = "DOMPurify"
54+
)
55+
or
5156
exists(string name | name = "encode" or name = "encodeNonUTF" |
5257
callee =
5358
DataFlow::moduleMember("html-entities", _).getAnInstantiation().getAPropertyRead(name) or

‎javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -827,6 +827,28 @@ module TaintTracking {
827827
override predicate appliesTo(Configuration cfg) { any() }
828828
}
829829

830+
/**
831+
* A test of form `x.length === "0"`, preventing `x` from being tainted.
832+
*/
833+
class IsEmptyGuard extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
834+
override EqualityTest astNode;
835+
boolean polarity;
836+
Expr operand;
837+
838+
IsEmptyGuard() {
839+
astNode.getPolarity() = polarity and
840+
astNode.getAnOperand().(ConstantExpr).getIntValue() = 0 and
841+
exists(DataFlow::PropRead read | read.asExpr() = astNode.getAnOperand() |
842+
read.getBase().asExpr() = operand and
843+
read.getPropertyName() = "length"
844+
)
845+
}
846+
847+
override predicate sanitizes(boolean outcome, Expr e) { polarity = outcome and e = operand }
848+
849+
override predicate appliesTo(Configuration cfg) { any() }
850+
}
851+
830852
/** DEPRECATED. This class has been renamed to `InclusionSanitizer`. */
831853
deprecated class StringInclusionSanitizer = InclusionSanitizer;
832854

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ module DomBasedXss {
4747
prop = urlSuffixPseudoProperty()
4848
)
4949
}
50+
51+
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
52+
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
53+
}
5054
}
5155

5256
private string urlSuffixPseudoProperty() { result = "$UrlSuffix$" }

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,36 @@ module DomBasedXss {
302302
private class MetacharEscapeSanitizer extends Sanitizer, Shared::MetacharEscapeSanitizer { }
303303

304304
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
305+
306+
/**
307+
* Holds if there exists two dataflow edges to `succ`, where one edges is sanitized, and the other edge starts with `pred`.
308+
*/
309+
predicate isOptionallySanitizedEdge(DataFlow::Node pred, DataFlow::Node succ) {
310+
exists(HtmlSanitizerCall sanitizer |
311+
// sanitized = sanitize ? sanitizer(source) : source;
312+
exists(ConditionalExpr branch, Variable var, VarAccess access |
313+
branch = succ.asExpr() and access = var.getAnAccess()
314+
|
315+
branch.getABranch() = access and
316+
pred.getEnclosingExpr() = access and
317+
sanitizer = branch.getABranch().flow() and
318+
sanitizer.getAnArgument().getEnclosingExpr() = var.getAnAccess()
319+
)
320+
or
321+
// sanitized = source; if (sanitize) {sanitized = sanitizer(source)};
322+
exists(SsaPhiNode phi, SsaExplicitDefinition a, SsaDefinition b |
323+
a = phi.getAnInput().getDefinition() and
324+
b = phi.getAnInput().getDefinition() and
325+
count(phi.getAnInput()) = 2 and
326+
not a = b and
327+
sanitizer = DataFlow::valueNode(a.getDef().getSource()) and
328+
sanitizer.getAnArgument().asExpr().(VarAccess).getVariable() = b.getSourceVariable()
329+
|
330+
pred = DataFlow::ssaDefinitionNode(b) and
331+
succ = DataFlow::ssaDefinitionNode(phi)
332+
)
333+
)
334+
}
305335
}
306336

307337
/** Provides classes and predicates for the reflected XSS query. */

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ module XssThroughDom {
3333
guard instanceof TypeTestGuard or
3434
guard instanceof UnsafeJQuery::PropertyPresenceSanitizer
3535
}
36+
37+
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
38+
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
39+
}
3640
}
3741

3842
/**

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

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,47 @@ nodes
3636
| nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` |
3737
| nodemailer.js:13:50:13:66 | req.query.message |
3838
| nodemailer.js:13:50:13:66 | req.query.message |
39+
| optionalSanitizer.js:2:7:2:39 | target |
40+
| optionalSanitizer.js:2:16:2:32 | document.location |
41+
| optionalSanitizer.js:2:16:2:32 | document.location |
42+
| optionalSanitizer.js:2:16:2:39 | documen ... .search |
43+
| optionalSanitizer.js:6:18:6:23 | target |
44+
| optionalSanitizer.js:6:18:6:23 | target |
45+
| optionalSanitizer.js:8:7:8:22 | tainted |
46+
| optionalSanitizer.js:8:17:8:22 | target |
47+
| optionalSanitizer.js:9:18:9:24 | tainted |
48+
| optionalSanitizer.js:9:18:9:24 | tainted |
49+
| optionalSanitizer.js:15:9:15:14 | target |
50+
| optionalSanitizer.js:16:18:16:18 | x |
51+
| optionalSanitizer.js:17:20:17:20 | x |
52+
| optionalSanitizer.js:17:20:17:20 | x |
53+
| optionalSanitizer.js:26:7:26:39 | target |
54+
| optionalSanitizer.js:26:16:26:32 | document.location |
55+
| optionalSanitizer.js:26:16:26:32 | document.location |
56+
| optionalSanitizer.js:26:16:26:39 | documen ... .search |
57+
| optionalSanitizer.js:31:7:31:23 | tainted2 |
58+
| optionalSanitizer.js:31:18:31:23 | target |
59+
| optionalSanitizer.js:32:18:32:25 | tainted2 |
60+
| optionalSanitizer.js:32:18:32:25 | tainted2 |
61+
| optionalSanitizer.js:34:5:34:36 | tainted2 |
62+
| optionalSanitizer.js:34:16:34:36 | sanitiz ... inted2) |
63+
| optionalSanitizer.js:34:28:34:35 | tainted2 |
64+
| optionalSanitizer.js:36:18:36:25 | tainted2 |
65+
| optionalSanitizer.js:36:18:36:25 | tainted2 |
66+
| optionalSanitizer.js:38:7:38:23 | tainted3 |
67+
| optionalSanitizer.js:38:18:38:23 | target |
68+
| optionalSanitizer.js:39:18:39:25 | tainted3 |
69+
| optionalSanitizer.js:39:18:39:25 | tainted3 |
70+
| optionalSanitizer.js:41:5:41:36 | tainted3 |
71+
| optionalSanitizer.js:41:16:41:36 | sanitiz ... inted3) |
72+
| optionalSanitizer.js:41:28:41:35 | tainted3 |
73+
| optionalSanitizer.js:43:18:43:25 | tainted3 |
74+
| optionalSanitizer.js:43:18:43:25 | tainted3 |
75+
| optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
76+
| optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
77+
| optionalSanitizer.js:45:29:45:47 | sanitizeBad(target) |
78+
| optionalSanitizer.js:45:41:45:46 | target |
79+
| optionalSanitizer.js:45:51:45:56 | target |
3980
| react-native.js:7:7:7:33 | tainted |
4081
| react-native.js:7:17:7:33 | req.param("code") |
4182
| react-native.js:7:17:7:33 | req.param("code") |
@@ -417,6 +458,51 @@ edges
417458
| nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` |
418459
| nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` |
419460
| nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` |
461+
| optionalSanitizer.js:2:7:2:39 | target | optionalSanitizer.js:6:18:6:23 | target |
462+
| optionalSanitizer.js:2:7:2:39 | target | optionalSanitizer.js:6:18:6:23 | target |
463+
| optionalSanitizer.js:2:7:2:39 | target | optionalSanitizer.js:8:17:8:22 | target |
464+
| optionalSanitizer.js:2:7:2:39 | target | optionalSanitizer.js:15:9:15:14 | target |
465+
| optionalSanitizer.js:2:16:2:32 | document.location | optionalSanitizer.js:2:16:2:39 | documen ... .search |
466+
| optionalSanitizer.js:2:16:2:32 | document.location | optionalSanitizer.js:2:16:2:39 | documen ... .search |
467+
| optionalSanitizer.js:2:16:2:39 | documen ... .search | optionalSanitizer.js:2:7:2:39 | target |
468+
| optionalSanitizer.js:8:7:8:22 | tainted | optionalSanitizer.js:9:18:9:24 | tainted |
469+
| optionalSanitizer.js:8:7:8:22 | tainted | optionalSanitizer.js:9:18:9:24 | tainted |
470+
| optionalSanitizer.js:8:17:8:22 | target | optionalSanitizer.js:8:7:8:22 | tainted |
471+
| optionalSanitizer.js:15:9:15:14 | target | optionalSanitizer.js:16:18:16:18 | x |
472+
| optionalSanitizer.js:16:18:16:18 | x | optionalSanitizer.js:17:20:17:20 | x |
473+
| optionalSanitizer.js:16:18:16:18 | x | optionalSanitizer.js:17:20:17:20 | x |
474+
| optionalSanitizer.js:26:7:26:39 | target | optionalSanitizer.js:31:18:31:23 | target |
475+
| optionalSanitizer.js:26:7:26:39 | target | optionalSanitizer.js:38:18:38:23 | target |
476+
| optionalSanitizer.js:26:7:26:39 | target | optionalSanitizer.js:45:41:45:46 | target |
477+
| optionalSanitizer.js:26:7:26:39 | target | optionalSanitizer.js:45:51:45:56 | target |
478+
| optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:26:16:26:39 | documen ... .search |
479+
| optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:26:16:26:39 | documen ... .search |
480+
| optionalSanitizer.js:26:16:26:39 | documen ... .search | optionalSanitizer.js:26:7:26:39 | target |
481+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:32:18:32:25 | tainted2 |
482+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:32:18:32:25 | tainted2 |
483+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:34:28:34:35 | tainted2 |
484+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:36:18:36:25 | tainted2 |
485+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:36:18:36:25 | tainted2 |
486+
| optionalSanitizer.js:31:18:31:23 | target | optionalSanitizer.js:31:7:31:23 | tainted2 |
487+
| optionalSanitizer.js:34:5:34:36 | tainted2 | optionalSanitizer.js:36:18:36:25 | tainted2 |
488+
| optionalSanitizer.js:34:5:34:36 | tainted2 | optionalSanitizer.js:36:18:36:25 | tainted2 |
489+
| optionalSanitizer.js:34:16:34:36 | sanitiz ... inted2) | optionalSanitizer.js:34:5:34:36 | tainted2 |
490+
| optionalSanitizer.js:34:28:34:35 | tainted2 | optionalSanitizer.js:34:16:34:36 | sanitiz ... inted2) |
491+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:39:18:39:25 | tainted3 |
492+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:39:18:39:25 | tainted3 |
493+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:41:28:41:35 | tainted3 |
494+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:43:18:43:25 | tainted3 |
495+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:43:18:43:25 | tainted3 |
496+
| optionalSanitizer.js:38:18:38:23 | target | optionalSanitizer.js:38:7:38:23 | tainted3 |
497+
| optionalSanitizer.js:41:5:41:36 | tainted3 | optionalSanitizer.js:43:18:43:25 | tainted3 |
498+
| optionalSanitizer.js:41:5:41:36 | tainted3 | optionalSanitizer.js:43:18:43:25 | tainted3 |
499+
| optionalSanitizer.js:41:16:41:36 | sanitiz ... inted3) | optionalSanitizer.js:41:5:41:36 | tainted3 |
500+
| optionalSanitizer.js:41:28:41:35 | tainted3 | optionalSanitizer.js:41:16:41:36 | sanitiz ... inted3) |
501+
| optionalSanitizer.js:45:29:45:47 | sanitizeBad(target) | optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
502+
| optionalSanitizer.js:45:29:45:47 | sanitizeBad(target) | optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
503+
| optionalSanitizer.js:45:41:45:46 | target | optionalSanitizer.js:45:29:45:47 | sanitizeBad(target) |
504+
| optionalSanitizer.js:45:51:45:56 | target | optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
505+
| optionalSanitizer.js:45:51:45:56 | target | optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
420506
| react-native.js:7:7:7:33 | tainted | react-native.js:8:18:8:24 | tainted |
421507
| react-native.js:7:7:7:33 | tainted | react-native.js:8:18:8:24 | tainted |
422508
| react-native.js:7:7:7:33 | tainted | react-native.js:9:27:9:33 | tainted |
@@ -728,6 +814,14 @@ edges
728814
| jquery.js:7:5:7:34 | "<div i ... + "\\">" | jquery.js:2:17:2:33 | document.location | jquery.js:7:5:7:34 | "<div i ... + "\\">" | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value |
729815
| jquery.js:8:18:8:34 | "XSS: " + tainted | jquery.js:2:17:2:33 | document.location | jquery.js:8:18:8:34 | "XSS: " + tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value |
730816
| nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` | nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` | HTML injection vulnerability due to $@. | nodemailer.js:13:50:13:66 | req.query.message | user-provided value |
817+
| optionalSanitizer.js:6:18:6:23 | target | optionalSanitizer.js:2:16:2:32 | document.location | optionalSanitizer.js:6:18:6:23 | target | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:2:16:2:32 | document.location | user-provided value |
818+
| optionalSanitizer.js:9:18:9:24 | tainted | optionalSanitizer.js:2:16:2:32 | document.location | optionalSanitizer.js:9:18:9:24 | tainted | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:2:16:2:32 | document.location | user-provided value |
819+
| optionalSanitizer.js:17:20:17:20 | x | optionalSanitizer.js:2:16:2:32 | document.location | optionalSanitizer.js:17:20:17:20 | x | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:2:16:2:32 | document.location | user-provided value |
820+
| optionalSanitizer.js:32:18:32:25 | tainted2 | optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:32:18:32:25 | tainted2 | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:26:16:26:32 | document.location | user-provided value |
821+
| optionalSanitizer.js:36:18:36:25 | tainted2 | optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:36:18:36:25 | tainted2 | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:26:16:26:32 | document.location | user-provided value |
822+
| optionalSanitizer.js:39:18:39:25 | tainted3 | optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:39:18:39:25 | tainted3 | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:26:16:26:32 | document.location | user-provided value |
823+
| optionalSanitizer.js:43:18:43:25 | tainted3 | optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:43:18:43:25 | tainted3 | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:26:16:26:32 | document.location | user-provided value |
824+
| optionalSanitizer.js:45:18:45:56 | sanitiz ... target | optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:45:18:45:56 | sanitiz ... target | Cross-site scripting vulnerability due to $@. | optionalSanitizer.js:26:16:26:32 | document.location | user-provided value |
731825
| react-native.js:8:18:8:24 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:18:8:24 | tainted | Cross-site scripting vulnerability due to $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value |
732826
| 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 |
733827
| 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 |

0 commit comments

Comments
 (0)