Skip to content

Commit 33da82d

Browse files
erik-krogherik-krogh
committed
Merge branch 'master' of https://github.com/github/codeql into pr/erik-krogh/3566
2 parents d05a61c + 3cfc1e5 commit 33da82d

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 patterns of constructing HTML. |
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
@@ -51,6 +51,10 @@ module DomBasedXss {
5151
prop = urlSuffixPseudoProperty()
5252
)
5353
}
54+
55+
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
56+
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
57+
}
5458
}
5559

5660
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
@@ -329,6 +329,36 @@ module DomBasedXss {
329329
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
330330

331331
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
332+
333+
/**
334+
* Holds if there exists two dataflow edges to `succ`, where one edges is sanitized, and the other edge starts with `pred`.
335+
*/
336+
predicate isOptionallySanitizedEdge(DataFlow::Node pred, DataFlow::Node succ) {
337+
exists(HtmlSanitizerCall sanitizer |
338+
// sanitized = sanitize ? sanitizer(source) : source;
339+
exists(ConditionalExpr branch, Variable var, VarAccess access |
340+
branch = succ.asExpr() and access = var.getAnAccess()
341+
|
342+
branch.getABranch() = access and
343+
pred.getEnclosingExpr() = access and
344+
sanitizer = branch.getABranch().flow() and
345+
sanitizer.getAnArgument().getEnclosingExpr() = var.getAnAccess()
346+
)
347+
or
348+
// sanitized = source; if (sanitize) {sanitized = sanitizer(source)};
349+
exists(SsaPhiNode phi, SsaExplicitDefinition a, SsaDefinition b |
350+
a = phi.getAnInput().getDefinition() and
351+
b = phi.getAnInput().getDefinition() and
352+
count(phi.getAnInput()) = 2 and
353+
not a = b and
354+
sanitizer = DataFlow::valueNode(a.getDef().getSource()) and
355+
sanitizer.getAnArgument().asExpr().(VarAccess).getVariable() = b.getSourceVariable()
356+
|
357+
pred = DataFlow::ssaDefinitionNode(b) and
358+
succ = DataFlow::ssaDefinitionNode(phi)
359+
)
360+
)
361+
}
332362
}
333363

334364
/** 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
@@ -34,6 +34,10 @@ module XssThroughDom {
3434
guard instanceof UnsafeJQuery::PropertyPresenceSanitizer or
3535
guard instanceof DomBasedXss::SanitizerGuard
3636
}
37+
38+
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
39+
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
40+
}
3741
}
3842

3943
/**

‎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") |
@@ -422,6 +463,51 @@ edges
422463
| nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` |
423464
| nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` |
424465
| nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` |
466+
| optionalSanitizer.js:2:7:2:39 | target | optionalSanitizer.js:6:18:6:23 | target |
467+
| optionalSanitizer.js:2:7:2:39 | target | optionalSanitizer.js:6:18:6:23 | target |
468+
| optionalSanitizer.js:2:7:2:39 | target | optionalSanitizer.js:8:17:8:22 | target |
469+
| optionalSanitizer.js:2:7:2:39 | target | optionalSanitizer.js:15:9:15:14 | target |
470+
| optionalSanitizer.js:2:16:2:32 | document.location | optionalSanitizer.js:2:16:2:39 | documen ... .search |
471+
| optionalSanitizer.js:2:16:2:32 | document.location | optionalSanitizer.js:2:16:2:39 | documen ... .search |
472+
| optionalSanitizer.js:2:16:2:39 | documen ... .search | optionalSanitizer.js:2:7:2:39 | target |
473+
| optionalSanitizer.js:8:7:8:22 | tainted | optionalSanitizer.js:9:18:9:24 | tainted |
474+
| optionalSanitizer.js:8:7:8:22 | tainted | optionalSanitizer.js:9:18:9:24 | tainted |
475+
| optionalSanitizer.js:8:17:8:22 | target | optionalSanitizer.js:8:7:8:22 | tainted |
476+
| optionalSanitizer.js:15:9:15:14 | target | optionalSanitizer.js:16:18:16:18 | x |
477+
| optionalSanitizer.js:16:18:16:18 | x | optionalSanitizer.js:17:20:17:20 | x |
478+
| optionalSanitizer.js:16:18:16:18 | x | optionalSanitizer.js:17:20:17:20 | x |
479+
| optionalSanitizer.js:26:7:26:39 | target | optionalSanitizer.js:31:18:31:23 | target |
480+
| optionalSanitizer.js:26:7:26:39 | target | optionalSanitizer.js:38:18:38:23 | target |
481+
| optionalSanitizer.js:26:7:26:39 | target | optionalSanitizer.js:45:41:45:46 | target |
482+
| optionalSanitizer.js:26:7:26:39 | target | optionalSanitizer.js:45:51:45:56 | target |
483+
| optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:26:16:26:39 | documen ... .search |
484+
| optionalSanitizer.js:26:16:26:32 | document.location | optionalSanitizer.js:26:16:26:39 | documen ... .search |
485+
| optionalSanitizer.js:26:16:26:39 | documen ... .search | optionalSanitizer.js:26:7:26:39 | target |
486+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:32:18:32:25 | tainted2 |
487+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:32:18:32:25 | tainted2 |
488+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:34:28:34:35 | tainted2 |
489+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:36:18:36:25 | tainted2 |
490+
| optionalSanitizer.js:31:7:31:23 | tainted2 | optionalSanitizer.js:36:18:36:25 | tainted2 |
491+
| optionalSanitizer.js:31:18:31:23 | target | optionalSanitizer.js:31:7:31:23 | tainted2 |
492+
| optionalSanitizer.js:34:5:34:36 | tainted2 | optionalSanitizer.js:36:18:36:25 | tainted2 |
493+
| optionalSanitizer.js:34:5:34:36 | tainted2 | optionalSanitizer.js:36:18:36:25 | tainted2 |
494+
| optionalSanitizer.js:34:16:34:36 | sanitiz ... inted2) | optionalSanitizer.js:34:5:34:36 | tainted2 |
495+
| optionalSanitizer.js:34:28:34:35 | tainted2 | optionalSanitizer.js:34:16:34:36 | sanitiz ... inted2) |
496+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:39:18:39:25 | tainted3 |
497+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:39:18:39:25 | tainted3 |
498+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:41:28:41:35 | tainted3 |
499+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:43:18:43:25 | tainted3 |
500+
| optionalSanitizer.js:38:7:38:23 | tainted3 | optionalSanitizer.js:43:18:43:25 | tainted3 |
501+
| optionalSanitizer.js:38:18:38:23 | target | optionalSanitizer.js:38:7:38:23 | tainted3 |
502+
| optionalSanitizer.js:41:5:41:36 | tainted3 | optionalSanitizer.js:43:18:43:25 | tainted3 |
503+
| optionalSanitizer.js:41:5:41:36 | tainted3 | optionalSanitizer.js:43:18:43:25 | tainted3 |
504+
| optionalSanitizer.js:41:16:41:36 | sanitiz ... inted3) | optionalSanitizer.js:41:5:41:36 | tainted3 |
505+
| optionalSanitizer.js:41:28:41:35 | tainted3 | optionalSanitizer.js:41:16:41:36 | sanitiz ... inted3) |
506+
| optionalSanitizer.js:45:29:45:47 | sanitizeBad(target) | optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
507+
| optionalSanitizer.js:45:29:45:47 | sanitizeBad(target) | optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
508+
| optionalSanitizer.js:45:41:45:46 | target | optionalSanitizer.js:45:29:45:47 | sanitizeBad(target) |
509+
| optionalSanitizer.js:45:51:45:56 | target | optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
510+
| optionalSanitizer.js:45:51:45:56 | target | optionalSanitizer.js:45:18:45:56 | sanitiz ... target |
425511
| react-native.js:7:7:7:33 | tainted | react-native.js:8:18:8:24 | tainted |
426512
| react-native.js:7:7:7:33 | tainted | react-native.js:8:18:8:24 | tainted |
427513
| react-native.js:7:7:7:33 | tainted | react-native.js:9:27:9:33 | tainted |
@@ -738,6 +824,14 @@ edges
738824
| 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 |
739825
| 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 |
740826
| 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 |
827+
| 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 |
828+
| 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 |
829+
| 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 |
830+
| 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 |
831+
| 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 |
832+
| 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 |
833+
| 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 |
834+
| 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 |
741835
| 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 |
742836
| 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 |
743837
| 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)