Skip to content

Commit d2f07e4

Browse files
jorgectfjorgectf
committed
Merge branch 'jorgectf/python/deserialization' of https://github.com/jorgectf/codeql into jorgectf/python/deserialization
2 parents 43fde35 + 99e14d1 commit d2f07e4

File tree

14 files changed

+199
-8
lines changed

14 files changed

+199
-8
lines changed

‎CODEOWNERS

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@
1313
/python/**/experimental/**/* @github/codeql-python @xcorail
1414
/ruby/**/experimental/**/* @github/codeql-ruby @xcorail
1515

16+
# ML-powered queries
17+
/javascript/ql/experimental/adaptivethreatmodeling/ @github/codeql-ml-powered-queries-reviewers
18+
1619
# Notify members of codeql-go about PRs to the shared data-flow library files
1720
/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll @github/codeql-java @github/codeql-go
1821
/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl2.qll @github/codeql-java @github/codeql-go
@@ -27,4 +30,4 @@
2730
/docs/query-*-style-guide.md @github/codeql-analysis-reviewers
2831

2932
# QL for QL reviewers
30-
/ql/ @github/codeql-ql-for-ql-reviewers
33+
/ql/ @github/codeql-ql-for-ql-reviewers

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/FunctionBodyFeatures.qll

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,18 @@ ASTNode getAnASTNodeWithAFeature(Function f) {
127127
result = getAnASTNodeToFeaturize(f)
128128
}
129129

130+
/** Returns the number of source-code characters in a function. */
131+
int getNumCharsInFunction(Function f) {
132+
result =
133+
strictsum(ASTNode node | node = getAnASTNodeWithAFeature(f) | getTokenizedAstNode(node).length())
134+
}
135+
136+
/**
137+
* The maximum number of characters a feature can be.
138+
* The evaluator string limit is 5395415 characters. We choose a limit lower than this.
139+
*/
140+
private int getMaxChars() { result = 1000000 }
141+
130142
/**
131143
* Returns a featurized representation of the function that can be used to populate the
132144
* `enclosingFunctionBody` feature for an endpoint.
@@ -141,6 +153,9 @@ string getBodyTokensFeature(Function function) {
141153
node = getAnASTNodeToFeaturize(function) and
142154
exists(getTokenizedAstNode(node))
143155
) <= 256 and
156+
// Performance optimization: If a function has more than getMaxChars() characters in its body subtokens,
157+
// then featurize it as absent.
158+
getNumCharsInFunction(function) <= getMaxChars() and
144159
result =
145160
strictconcat(Location l, string token |
146161
// The use of a nested exists here allows us to avoid duplicates due to two AST nodes in the

‎javascript/ql/lib/change-notes/2022-02-04-jszip

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added sources from the [`jszip`](https://www.npmjs.com/package/jszip) library to the `js/zipslip` query.

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,5 +58,12 @@ class Configuration extends TaintTracking::Configuration {
5858
// avoid overlapping results with unsafe dynamic method access query
5959
not PropertyInjection::hasUnsafeMethods(read.getBase().getALocalSource())
6060
)
61+
or
62+
exists(DataFlow::SourceNode base, DataFlow::CallNode get | get = base.getAMethodCall("get") |
63+
src = get.getArgument(0) and
64+
dst = get
65+
) and
66+
srclabel.isTaint() and
67+
dstlabel instanceof MaybeNonFunction
6168
}
6269
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,29 @@ module ZipSlip {
9797
}
9898
}
9999

100+
private import semmle.javascript.DynamicPropertyAccess as DynamicPropertyAccess
101+
102+
/** A object key in the JSZip files object */
103+
class JSZipFilesSource extends Source instanceof DynamicPropertyAccess::EnumeratedPropName {
104+
JSZipFilesSource() {
105+
super.getSourceObject() =
106+
API::moduleImport("jszip").getInstance().getMember("files").getAnImmediateUse()
107+
}
108+
}
109+
110+
/** A relative path from iterating the files in the JSZip object */
111+
class JSZipFileSource extends Source {
112+
JSZipFileSource() {
113+
this =
114+
API::moduleImport("jszip")
115+
.getInstance()
116+
.getMember(["forEach", "filter"])
117+
.getParameter(0)
118+
.getParameter(0)
119+
.getAnImmediateUse()
120+
}
121+
}
122+
100123
/** A call to `fs.createWriteStream`, as a sink for unsafe archive extraction. */
101124
class CreateWriteStreamSink extends Sink {
102125
CreateWriteStreamSink() {

‎javascript/ql/src/Security/CWE-754/examples/UnvalidatedDynamicMethodCallGood.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ actions.put("pause", function pause(data) {
1111

1212
app.get('/perform/:action/:payload', function(req, res) {
1313
if (actions.has(req.params.action)) {
14-
let action = actions.get(req.params.action);
14+
if (typeof actions.get(req.params.action) === 'function'){
15+
let action = actions.get(req.params.action);
16+
}
1517
// GOOD: `action` is either the `play` or the `pause` function from above
1618
res.end(action(req.params.payload));
1719
} else {

‎javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,18 @@ nodes
4545
| ZipSlipBad.js:23:28:23:35 | fileName |
4646
| ZipSlipBad.js:23:28:23:35 | fileName |
4747
| ZipSlipBad.js:23:28:23:35 | fileName |
48+
| ZipSlipBad.js:29:14:29:17 | name |
49+
| ZipSlipBad.js:29:14:29:17 | name |
50+
| ZipSlipBad.js:29:14:29:17 | name |
51+
| ZipSlipBad.js:30:26:30:29 | name |
52+
| ZipSlipBad.js:30:26:30:29 | name |
53+
| ZipSlipBad.js:30:26:30:29 | name |
54+
| ZipSlipBad.js:33:16:33:19 | name |
55+
| ZipSlipBad.js:33:16:33:19 | name |
56+
| ZipSlipBad.js:33:16:33:19 | name |
57+
| ZipSlipBad.js:34:26:34:29 | name |
58+
| ZipSlipBad.js:34:26:34:29 | name |
59+
| ZipSlipBad.js:34:26:34:29 | name |
4860
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
4961
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
5062
| ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
@@ -91,6 +103,20 @@ edges
91103
| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
92104
| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
93105
| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
106+
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
107+
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
108+
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
109+
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
110+
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
111+
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
112+
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
113+
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
114+
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
115+
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
116+
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
117+
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
118+
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
119+
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
94120
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
95121
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
96122
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
@@ -107,4 +133,6 @@ edges
107133
| ZipSlipBad.js:8:37:8:44 | fileName | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:7:22:7:31 | entry.path | item path |
108134
| ZipSlipBad.js:16:30:16:37 | fileName | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:16:30:16:37 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:15:22:15:31 | entry.path | item path |
109135
| ZipSlipBad.js:23:28:23:35 | fileName | ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:23:28:23:35 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:22:22:22:31 | entry.path | item path |
136+
| ZipSlipBad.js:30:26:30:29 | name | ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:29:14:29:17 | name | item path |
137+
| ZipSlipBad.js:34:26:34:29 | name | ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:33:16:33:19 | name | item path |
110138
| ZipSlipBadUnzipper.js:8:37:8:44 | fileName | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | item path |

‎javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,16 @@ fs.createReadStream('archive.zip')
2121
.on('entry', entry => {
2222
const fileName = entry.path;
2323
var file = fs.openSync(fileName, "w");
24-
});
24+
});
25+
26+
const JSZip = require('jszip');
27+
const zip = new JSZip();
28+
function doZipSlip() {
29+
for (const name in zip.files) {
30+
fs.createWriteStream(name);
31+
}
32+
33+
zip.forEach((name, file) => {
34+
fs.createWriteStream(name);
35+
});
36+
}

‎javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCall.expected

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,12 @@ nodes
1010
| UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] |
1111
| UnsafeDynamicMethodAccess.js:15:9:15:15 | message |
1212
| UnsafeDynamicMethodAccess.js:15:9:15:20 | message.name |
13+
| UnvalidatedDynamicMethodCall2.js:13:9:13:47 | action |
14+
| UnvalidatedDynamicMethodCall2.js:13:18:13:47 | actions ... action) |
15+
| UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action |
16+
| UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action |
17+
| UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action |
18+
| UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action |
1319
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action |
1420
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action |
1521
| UnvalidatedDynamicMethodCall.js:14:16:14:41 | actions ... action] |
@@ -19,6 +25,12 @@ nodes
1925
| UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
2026
| UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
2127
| UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
28+
| UnvalidatedDynamicMethodCallGood4.js:14:13:14:51 | action |
29+
| UnvalidatedDynamicMethodCallGood4.js:14:22:14:51 | actions ... action) |
30+
| UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action |
31+
| UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action |
32+
| UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action |
33+
| UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action |
2234
| tst.js:6:39:6:40 | ev |
2335
| tst.js:6:39:6:40 | ev |
2436
| tst.js:7:9:7:39 | name |
@@ -91,6 +103,11 @@ edges
91103
| UnsafeDynamicMethodAccess.js:15:9:15:20 | message.name | UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] |
92104
| UnsafeDynamicMethodAccess.js:15:9:15:20 | message.name | UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] |
93105
| UnsafeDynamicMethodAccess.js:15:9:15:20 | message.name | UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] |
106+
| UnvalidatedDynamicMethodCall2.js:13:9:13:47 | action | UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action |
107+
| UnvalidatedDynamicMethodCall2.js:13:9:13:47 | action | UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action |
108+
| UnvalidatedDynamicMethodCall2.js:13:18:13:47 | actions ... action) | UnvalidatedDynamicMethodCall2.js:13:9:13:47 | action |
109+
| UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action | UnvalidatedDynamicMethodCall2.js:13:18:13:47 | actions ... action) |
110+
| UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action | UnvalidatedDynamicMethodCall2.js:13:18:13:47 | actions ... action) |
94111
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action | UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
95112
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action | UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
96113
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action | UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
@@ -101,6 +118,11 @@ edges
101118
| UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | UnvalidatedDynamicMethodCall.js:14:16:14:41 | actions ... action] |
102119
| UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | UnvalidatedDynamicMethodCall.js:14:16:14:41 | actions ... action] |
103120
| UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | UnvalidatedDynamicMethodCall.js:14:16:14:41 | actions ... action] |
121+
| UnvalidatedDynamicMethodCallGood4.js:14:13:14:51 | action | UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action |
122+
| UnvalidatedDynamicMethodCallGood4.js:14:13:14:51 | action | UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action |
123+
| UnvalidatedDynamicMethodCallGood4.js:14:22:14:51 | actions ... action) | UnvalidatedDynamicMethodCallGood4.js:14:13:14:51 | action |
124+
| UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action | UnvalidatedDynamicMethodCallGood4.js:14:22:14:51 | actions ... action) |
125+
| UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action | UnvalidatedDynamicMethodCallGood4.js:14:22:14:51 | actions ... action) |
104126
| tst.js:6:39:6:40 | ev | tst.js:7:27:7:28 | ev |
105127
| tst.js:6:39:6:40 | ev | tst.js:7:27:7:28 | ev |
106128
| tst.js:6:39:6:40 | ev | tst.js:9:9:9:10 | ev |
@@ -164,7 +186,9 @@ edges
164186
| tst.js:49:19:49:22 | name | tst.js:49:14:49:23 | obj2[name] |
165187
#select
166188
| UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] | UnsafeDynamicMethodAccess.js:5:37:5:38 | ev | UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | UnsafeDynamicMethodAccess.js:5:37:5:38 | ev | user-controlled |
189+
| UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action | UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action | UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action | user-controlled |
167190
| UnvalidatedDynamicMethodCall.js:15:11:15:16 | action | UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | UnvalidatedDynamicMethodCall.js:15:11:15:16 | action | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | user-controlled |
191+
| UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action | UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action | UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action | user-controlled |
168192
| tst.js:9:5:9:16 | obj[ev.data] | tst.js:6:39:6:40 | ev | tst.js:9:5:9:16 | obj[ev.data] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
169193
| tst.js:11:5:11:13 | obj[name] | tst.js:6:39:6:40 | ev | tst.js:11:5:11:13 | obj[name] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
170194
| tst.js:18:5:18:6 | fn | tst.js:6:39:6:40 | ev | tst.js:18:5:18:6 | fn | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |

‎javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCall2.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
var express = require('express');
2+
var app = express();
3+
4+
var actions = new Map();
5+
actions.put("play", function play(data) {
6+
// ...
7+
});
8+
actions.put("pause", function pause(data) {
9+
// ...
10+
});
11+
12+
app.get('/perform/:action/:payload', function(req, res) {
13+
let action = actions.get(req.params.action);
14+
res.end(action(req.params.payload)); // NOT OK
15+
});

0 commit comments

Comments
 (0)