Skip to content

Commit e9f1e99

Browse files
asgerfasgerf
authored
Merge pull request github#12887 from asgerf/js/unsafe-yaml-deserialization
JS: Update model of js-yaml
2 parents 1bba525 + 5f011a2 commit e9f1e99

File tree

7 files changed

+89
-36
lines changed

7 files changed

+89
-36
lines changed

‎javascript/ql/lib/semmle/javascript/ApiGraphs.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,7 @@ module API {
491491
* In other words, the value of a use of `that` may flow into the right-hand side of a
492492
* definition of this node.
493493
*/
494+
pragma[inline]
494495
predicate refersTo(Node that) { this.asSink() = that.getAValueReachableFromSource() }
495496

496497
/**

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeDeserializationCustomizations.qll

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,25 +25,36 @@ module UnsafeDeserialization {
2525
/** A source of remote user input, considered as a flow source for unsafe deserialization. */
2626
class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource { }
2727

28+
private API::Node unsafeYamlSchema() {
29+
result = API::moduleImport("js-yaml").getMember("DEFAULT_FULL_SCHEMA") // from older versions
30+
or
31+
result = API::moduleImport("js-yaml-js-types").getMember(["all", "function"])
32+
or
33+
result = unsafeYamlSchema().getMember("extend").getReturn()
34+
or
35+
exists(API::CallNode call |
36+
call.getAParameter().refersTo(unsafeYamlSchema()) and
37+
call.getCalleeName() = "extend" and
38+
result = call.getReturn()
39+
)
40+
}
41+
2842
/**
2943
* An expression passed to one of the unsafe load functions of the `js-yaml` package.
44+
*
45+
* `js-yaml` since v4 defaults to being safe, but is unsafe when invoked with a schema
46+
* that permits unsafe values.
3047
*/
3148
class JsYamlUnsafeLoad extends Sink {
3249
JsYamlUnsafeLoad() {
33-
exists(DataFlow::ModuleImportNode mi | mi.getPath() = "js-yaml" |
34-
// the first argument to a call to `load` or `loadAll`
35-
exists(string n | n = "load" or n = "loadAll" | this = mi.getAMemberCall(n).getArgument(0))
36-
or
37-
// the first argument to a call to `safeLoad` or `safeLoadAll` where
38-
// the schema is specified to be `DEFAULT_FULL_SCHEMA`
39-
exists(string n, DataFlow::CallNode c, DataFlow::Node fullSchema |
40-
n = "safeLoad" or n = "safeLoadAll"
41-
|
42-
c = mi.getAMemberCall(n) and
43-
this = c.getArgument(0) and
44-
fullSchema = c.getOptionArgument(c.getNumArgument() - 1, "schema") and
45-
mi.getAPropertyRead("DEFAULT_FULL_SCHEMA").flowsTo(fullSchema)
46-
)
50+
exists(API::CallNode call |
51+
// Note: we include the old 'safeLoad' and 'safeLoadAll' functon because they were also unsafe when invoked with an unsafe schema.
52+
call =
53+
API::moduleImport("js-yaml")
54+
.getMember(["load", "loadAll", "safeLoad", "safeLoadAll"])
55+
.getACall() and
56+
call.getAParameter().getMember("schema").refersTo(unsafeYamlSchema()) and
57+
this = call.getArgument(0)
4758
)
4859
}
4960
}

‎javascript/ql/src/change-notes/2023-04-26-unsafe-yaml-deserialization.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `js/unsafe-deserialization` query no longer flags deserialization through the `js-yaml` library, except
5+
when it is used with an unsafe schema.

‎javascript/ql/test/query-tests/Security/CWE-502/Consistency.expected

Whitespace-only changes.

‎javascript/ql/test/query-tests/Security/CWE-502/Consistency.ql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
import javascript
2+
import semmle.javascript.security.dataflow.UnsafeDeserializationQuery
3+
import testUtilities.ConsistencyChecking

‎javascript/ql/test/query-tests/Security/CWE-502/UnsafeDeserialization.expected

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,43 @@
11
nodes
2-
| tst.js:7:22:7:36 | req.params.data |
3-
| tst.js:7:22:7:36 | req.params.data |
4-
| tst.js:7:22:7:36 | req.params.data |
5-
| tst.js:8:25:8:39 | req.params.data |
6-
| tst.js:8:25:8:39 | req.params.data |
7-
| tst.js:8:25:8:39 | req.params.data |
8-
| tst.js:12:26:12:40 | req.params.data |
9-
| tst.js:12:26:12:40 | req.params.data |
10-
| tst.js:12:26:12:40 | req.params.data |
11-
| tst.js:13:29:13:43 | req.params.data |
12-
| tst.js:13:29:13:43 | req.params.data |
13-
| tst.js:13:29:13:43 | req.params.data |
2+
| tst.js:13:22:13:36 | req.params.data |
3+
| tst.js:13:22:13:36 | req.params.data |
4+
| tst.js:13:22:13:36 | req.params.data |
5+
| tst.js:14:25:14:39 | req.params.data |
6+
| tst.js:14:25:14:39 | req.params.data |
7+
| tst.js:14:25:14:39 | req.params.data |
8+
| tst.js:15:26:15:40 | req.params.data |
9+
| tst.js:15:26:15:40 | req.params.data |
10+
| tst.js:15:26:15:40 | req.params.data |
11+
| tst.js:16:29:16:43 | req.params.data |
12+
| tst.js:16:29:16:43 | req.params.data |
13+
| tst.js:16:29:16:43 | req.params.data |
14+
| tst.js:20:22:20:36 | req.params.data |
15+
| tst.js:20:22:20:36 | req.params.data |
16+
| tst.js:20:22:20:36 | req.params.data |
17+
| tst.js:21:22:21:36 | req.params.data |
18+
| tst.js:21:22:21:36 | req.params.data |
19+
| tst.js:21:22:21:36 | req.params.data |
20+
| tst.js:24:22:24:36 | req.params.data |
21+
| tst.js:24:22:24:36 | req.params.data |
22+
| tst.js:24:22:24:36 | req.params.data |
23+
| tst.js:25:22:25:36 | req.params.data |
24+
| tst.js:25:22:25:36 | req.params.data |
25+
| tst.js:25:22:25:36 | req.params.data |
1426
edges
15-
| tst.js:7:22:7:36 | req.params.data | tst.js:7:22:7:36 | req.params.data |
16-
| tst.js:8:25:8:39 | req.params.data | tst.js:8:25:8:39 | req.params.data |
17-
| tst.js:12:26:12:40 | req.params.data | tst.js:12:26:12:40 | req.params.data |
18-
| tst.js:13:29:13:43 | req.params.data | tst.js:13:29:13:43 | req.params.data |
27+
| tst.js:13:22:13:36 | req.params.data | tst.js:13:22:13:36 | req.params.data |
28+
| tst.js:14:25:14:39 | req.params.data | tst.js:14:25:14:39 | req.params.data |
29+
| tst.js:15:26:15:40 | req.params.data | tst.js:15:26:15:40 | req.params.data |
30+
| tst.js:16:29:16:43 | req.params.data | tst.js:16:29:16:43 | req.params.data |
31+
| tst.js:20:22:20:36 | req.params.data | tst.js:20:22:20:36 | req.params.data |
32+
| tst.js:21:22:21:36 | req.params.data | tst.js:21:22:21:36 | req.params.data |
33+
| tst.js:24:22:24:36 | req.params.data | tst.js:24:22:24:36 | req.params.data |
34+
| tst.js:25:22:25:36 | req.params.data | tst.js:25:22:25:36 | req.params.data |
1935
#select
20-
| tst.js:7:22:7:36 | req.params.data | tst.js:7:22:7:36 | req.params.data | tst.js:7:22:7:36 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:7:22:7:36 | req.params.data | user-provided value |
21-
| tst.js:8:25:8:39 | req.params.data | tst.js:8:25:8:39 | req.params.data | tst.js:8:25:8:39 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:8:25:8:39 | req.params.data | user-provided value |
22-
| tst.js:12:26:12:40 | req.params.data | tst.js:12:26:12:40 | req.params.data | tst.js:12:26:12:40 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:12:26:12:40 | req.params.data | user-provided value |
23-
| tst.js:13:29:13:43 | req.params.data | tst.js:13:29:13:43 | req.params.data | tst.js:13:29:13:43 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:13:29:13:43 | req.params.data | user-provided value |
36+
| tst.js:13:22:13:36 | req.params.data | tst.js:13:22:13:36 | req.params.data | tst.js:13:22:13:36 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:13:22:13:36 | req.params.data | user-provided value |
37+
| tst.js:14:25:14:39 | req.params.data | tst.js:14:25:14:39 | req.params.data | tst.js:14:25:14:39 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:14:25:14:39 | req.params.data | user-provided value |
38+
| tst.js:15:26:15:40 | req.params.data | tst.js:15:26:15:40 | req.params.data | tst.js:15:26:15:40 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:15:26:15:40 | req.params.data | user-provided value |
39+
| tst.js:16:29:16:43 | req.params.data | tst.js:16:29:16:43 | req.params.data | tst.js:16:29:16:43 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:16:29:16:43 | req.params.data | user-provided value |
40+
| tst.js:20:22:20:36 | req.params.data | tst.js:20:22:20:36 | req.params.data | tst.js:20:22:20:36 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:20:22:20:36 | req.params.data | user-provided value |
41+
| tst.js:21:22:21:36 | req.params.data | tst.js:21:22:21:36 | req.params.data | tst.js:21:22:21:36 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:21:22:21:36 | req.params.data | user-provided value |
42+
| tst.js:24:22:24:36 | req.params.data | tst.js:24:22:24:36 | req.params.data | tst.js:24:22:24:36 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:24:22:24:36 | req.params.data | user-provided value |
43+
| tst.js:25:22:25:36 | req.params.data | tst.js:25:22:25:36 | req.params.data | tst.js:25:22:25:36 | req.params.data | Unsafe deserialization depends on a $@. | tst.js:25:22:25:36 | req.params.data | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-502/tst.js

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,24 @@ var express = require('express');
44
var app = express();
55
app.post('/store/:id', function(req, res) {
66
let data;
7-
data = jsyaml.load(req.params.data); // NOT OK
8-
data = jsyaml.loadAll(req.params.data); // NOT OK
7+
data = jsyaml.load(req.params.data); // OK
8+
data = jsyaml.loadAll(req.params.data); // OK
99
data = jsyaml.safeLoad(req.params.data); // OK
1010
data = jsyaml.safeLoadAll(req.params.data); // OK
11+
1112
let unsafeConfig = { schema: jsyaml.DEFAULT_FULL_SCHEMA };
13+
data = jsyaml.load(req.params.data, unsafeConfig); // NOT OK
14+
data = jsyaml.loadAll(req.params.data, unsafeConfig); // NOT OK
1215
data = jsyaml.safeLoad(req.params.data, unsafeConfig); // NOT OK
1316
data = jsyaml.safeLoadAll(req.params.data, unsafeConfig); // NOT OK
17+
18+
data = jsyaml.load(req.params.data, { schema: jsyaml.DEFAULT_SCHEMA }); // OK
19+
20+
data = jsyaml.load(req.params.data, { schema: jsyaml.DEFAULT_SCHEMA.extend(require('js-yaml-js-types').all) }); // NOT OK
21+
data = jsyaml.load(req.params.data, { schema: jsyaml.DEFAULT_SCHEMA.extend(require('js-yaml-js-types').function) }); // NOT OK
22+
data = jsyaml.load(req.params.data, { schema: jsyaml.DEFAULT_SCHEMA.extend(require('js-yaml-js-types').undefined) }); // OK
23+
24+
data = jsyaml.load(req.params.data, { schema: require('js-yaml-js-types').all.extend(jsyaml.DEFAULT_SCHEMA) }); // NOT OK
25+
data = jsyaml.load(req.params.data, { schema: require('js-yaml-js-types').function.extend(jsyaml.DEFAULT_SCHEMA) }); // NOT OK
26+
data = jsyaml.load(req.params.data, { schema: require('js-yaml-js-types').undefined.extend(jsyaml.DEFAULT_SCHEMA) }); // OK
1427
});

0 commit comments

Comments
 (0)