Skip to content

Commit 4963a8f

Browse files
codeql-cicodeql-ci
authored
Merge pull request #6024 from erik-krogh/serialize-javascript
Approved by asgerf
2 parents 3923acb + 0adc001 commit 4963a8f

8 files changed

Lines changed: 61 additions & 1 deletion

File tree

‎javascript/change-notes/2021-06-06-serialize-javascript.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* The dataflow libraries now model dataflow in the [`serialize-javascript`](https://npmjs.com/package/serialize-javascript) library.
3+
Affected packages are
4+
[serialize-javascript](https://npmjs.com/package/serialize-javascript)

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ private class PlainJsonParserCall extends JsonParserCall {
2929
callee = DataFlow::moduleImport("parse-json") or
3030
callee = DataFlow::moduleImport("json-parse-better-errors") or
3131
callee = DataFlow::moduleImport("json-safe-parse") or
32-
callee = AngularJS::angular().getAPropertyRead("fromJson")
32+
callee = AngularJS::angular().getAPropertyRead("fromJson") or
33+
callee = DataFlow::moduleImport("serialize-javascript")
3334
)
3435
}
3536

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,17 @@ module Shared {
5555
}
5656
}
5757

58+
/**
59+
* A call to `serialize-javascript`, which prevents XSS vulnerabilities unless
60+
* the `unsafe` option is set.t
61+
*/
62+
class SerializeJavascriptSanitizer extends Sanitizer, DataFlow::CallNode {
63+
SerializeJavascriptSanitizer() {
64+
this = DataFlow::moduleImport("serialize-javascript").getACall() and
65+
not this.getOptionArgument(1, "unsafe").mayHaveBooleanValue(true)
66+
}
67+
}
68+
5869
private import semmle.javascript.security.dataflow.IncompleteHtmlAttributeSanitizationCustomizations::IncompleteHtmlAttributeSanitization as IncompleteHTML
5970

6071
/**
@@ -359,6 +370,9 @@ module DomBasedXss {
359370

360371
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
361372

373+
private class SerializeJavascriptSanitizer extends Sanitizer, Shared::SerializeJavascriptSanitizer {
374+
}
375+
362376
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
363377

364378
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
@@ -497,6 +511,9 @@ module ReflectedXss {
497511

498512
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
499513

514+
private class SerializeJavascriptSanitizer extends Sanitizer, Shared::SerializeJavascriptSanitizer {
515+
}
516+
500517
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
501518

502519
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }
@@ -534,6 +551,9 @@ module StoredXss {
534551

535552
private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
536553

554+
private class SerializeJavascriptSanitizer extends Sanitizer, Shared::SerializeJavascriptSanitizer {
555+
}
556+
537557
private class IsEscapedInSwitchSanitizer extends Sanitizer, Shared::IsEscapedInSwitchSanitizer { }
538558

539559
private class QuoteGuard extends SanitizerGuard, Shared::QuoteGuard { }

‎javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,7 @@ typeInferenceMismatch
166166
| tst.js:2:13:2:20 | source() | tst.js:45:10:45:24 | x.map(x2 => x2) |
167167
| tst.js:2:13:2:20 | source() | tst.js:47:10:47:30 | Buffer. ... 'hex') |
168168
| tst.js:2:13:2:20 | source() | tst.js:48:10:48:22 | new Buffer(x) |
169+
| tst.js:2:13:2:20 | source() | tst.js:51:10:51:31 | seriali ... ript(x) |
169170
| xml.js:5:18:5:25 | source() | xml.js:8:14:8:17 | text |
170171
| xml.js:12:17:12:24 | source() | xml.js:13:14:13:19 | result |
171172
| xml.js:23:18:23:25 | source() | xml.js:20:14:20:17 | attr |

‎javascript/ql/test/library-tests/TaintTracking/tst.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,4 +46,7 @@ function test() {
4646

4747
sink(Buffer.from(x, 'hex')); // NOT OK
4848
sink(new Buffer(x)); // NOT OK
49+
50+
const serializeJavaScript = require("serialize-javascript");
51+
sink(serializeJavaScript(x)) // NOT OK
4952
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,14 @@ nodes
182182
| tst2.js:36:12:36:12 | p |
183183
| tst2.js:37:12:37:18 | other.p |
184184
| tst2.js:37:12:37:18 | other.p |
185+
| tst2.js:43:7:43:24 | p |
186+
| tst2.js:43:9:43:9 | p |
187+
| tst2.js:43:9:43:9 | p |
188+
| tst2.js:49:7:49:53 | unsafe |
189+
| tst2.js:49:16:49:53 | seriali ... true}) |
190+
| tst2.js:49:36:49:36 | p |
191+
| tst2.js:51:12:51:17 | unsafe |
192+
| tst2.js:51:12:51:17 | unsafe |
185193
| tst3.js:5:7:5:24 | p |
186194
| tst3.js:5:9:5:9 | p |
187195
| tst3.js:5:9:5:9 | p |
@@ -338,6 +346,13 @@ edges
338346
| tst2.js:30:9:30:9 | p | tst2.js:30:7:30:24 | p |
339347
| tst2.js:33:11:33:11 | p | tst2.js:37:12:37:18 | other.p |
340348
| tst2.js:33:11:33:11 | p | tst2.js:37:12:37:18 | other.p |
349+
| tst2.js:43:7:43:24 | p | tst2.js:49:36:49:36 | p |
350+
| tst2.js:43:9:43:9 | p | tst2.js:43:7:43:24 | p |
351+
| tst2.js:43:9:43:9 | p | tst2.js:43:7:43:24 | p |
352+
| tst2.js:49:7:49:53 | unsafe | tst2.js:51:12:51:17 | unsafe |
353+
| tst2.js:49:7:49:53 | unsafe | tst2.js:51:12:51:17 | unsafe |
354+
| tst2.js:49:16:49:53 | seriali ... true}) | tst2.js:49:7:49:53 | unsafe |
355+
| tst2.js:49:36:49:36 | p | tst2.js:49:16:49:53 | seriali ... true}) |
341356
| tst3.js:5:7:5:24 | p | tst3.js:6:12:6:12 | p |
342357
| tst3.js:5:7:5:24 | p | tst3.js:6:12:6:12 | p |
343358
| tst3.js:5:9:5:9 | p | tst3.js:5:7:5:24 | p |
@@ -385,4 +400,5 @@ edges
385400
| tst2.js:21:14:21:14 | p | tst2.js:14:9:14:9 | p | tst2.js:21:14:21:14 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |
386401
| tst2.js:36:12:36:12 | p | tst2.js:30:9:30:9 | p | tst2.js:36:12:36:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
387402
| tst2.js:37:12:37:18 | other.p | tst2.js:30:9:30:9 | p | tst2.js:37:12:37:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
403+
| tst2.js:51:12:51:17 | unsafe | tst2.js:43:9:43:9 | p | tst2.js:51:12:51:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:43:9:43:9 | p | user-provided value |
388404
| tst3.js:6:12:6:12 | p | tst3.js:5:9:5:9 | p | tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to $@. | tst3.js:5:9:5:9 | p | user-provided value |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,4 +39,5 @@
3939
| tst2.js:21:14:21:14 | p | Cross-site scripting vulnerability due to $@. | tst2.js:14:9:14:9 | p | user-provided value |
4040
| tst2.js:36:12:36:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
4141
| tst2.js:37:12:37:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:30:9:30:9 | p | user-provided value |
42+
| tst2.js:51:12:51:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:43:9:43:9 | p | user-provided value |
4243
| tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to $@. | tst3.js:5:9:5:9 | p | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,4 +35,18 @@ app.get('/baz', function(req, res) {
3535

3636
res.send(p); // NOT OK
3737
res.send(other.p); // NOT OK
38+
});
39+
40+
const serializeJavaScript = require('serialize-javascript');
41+
42+
app.get('/baz', function(req, res) {
43+
let { p } = req.params;
44+
45+
var serialized = serializeJavaScript(p);
46+
47+
res.send(serialized); // OK
48+
49+
var unsafe = serializeJavaScript(p, {unsafe: true});
50+
51+
res.send(unsafe); // NOT OK
3852
});

0 commit comments

Comments
 (0)