Skip to content

Commit 2f5de5c

Browse files
asgerfasgerf
committed
JS: Convert OK-style comments to $-style
Simply converts the comments as they are, regardless of actual query output
1 parent d20dd75 commit 2f5de5c

17 files changed

+260
-278
lines changed

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath-es6.js‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,5 @@ import { join } from 'path';
66
var server = createServer(function(req, res) {
77
let path = parse(req.url, true).query.path;
88

9-
// BAD: This could read any file on the file system
10-
res.write(readFileSync(join("public", path)));
9+
res.write(readFileSync(join("public", path))); // $ Alert - This could read any file on the file system
1110
});

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

Lines changed: 68 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -8,63 +8,52 @@ var fs = require('fs'),
88
var server = http.createServer(function(req, res) {
99
let path = url.parse(req.url, true).query.path;
1010

11-
// BAD: This could read any file on the file system
12-
res.write(fs.readFileSync(path));
11+
res.write(fs.readFileSync(path)); // $ Alert - This could read any file on the file system
1312

14-
// BAD: This could still read any file on the file system
15-
res.write(fs.readFileSync("/home/user/" + path));
13+
res.write(fs.readFileSync("/home/user/" + path)); // $ Alert - This could still read any file on the file system
1614

1715
if (path.startsWith("/home/user/"))
18-
res.write(fs.readFileSync(path)); // BAD: Insufficient sanitisation
16+
res.write(fs.readFileSync(path)); // $ Alert - Insufficient sanitisation
1917

2018
if (path.indexOf("secret") == -1)
21-
res.write(fs.readFileSync(path)); // BAD: Insufficient sanitisation
19+
res.write(fs.readFileSync(path)); // $ Alert - Insufficient sanitisation
2220

2321
if (fs.existsSync(path))
24-
res.write(fs.readFileSync(path)); // BAD: Insufficient sanitisation
22+
res.write(fs.readFileSync(path)); // $ Alert - Insufficient sanitisation
2523

2624
if (path === 'foo.txt')
27-
res.write(fs.readFileSync(path)); // GOOD: Path is compared to white-list
25+
res.write(fs.readFileSync(path)); // OK - Path is compared to white-list
2826

2927
if (path === 'foo.txt' || path === 'bar.txt')
30-
res.write(fs.readFileSync(path)); // GOOD: Path is compared to white-list
28+
res.write(fs.readFileSync(path)); // OK - Path is compared to white-list
3129

3230
if (path === 'foo.txt' || path === 'bar.txt' || someOpaqueCondition())
33-
res.write(fs.readFileSync(path)); // BAD: Path is incompletely compared to white-list
31+
res.write(fs.readFileSync(path)); // $ Alert - Path is incompletely compared to white-list
3432

3533
path = sanitize(path);
36-
res.write(fs.readFileSync(path)); // GOOD: Path is sanitized
34+
res.write(fs.readFileSync(path)); // OK - Path is sanitized
3735

3836
path = url.parse(req.url, true).query.path;
39-
// GOOD: basename is safe
37+
// OK - basename is safe
4038
res.write(fs.readFileSync(pathModule.basename(path)));
41-
// BAD: taint is preserved
42-
res.write(fs.readFileSync(pathModule.dirname(path)));
43-
// GOOD: extname is safe
39+
res.write(fs.readFileSync(pathModule.dirname(path))); // $ Alert - taint is preserved
40+
// OK - extname is safe
4441
res.write(fs.readFileSync(pathModule.extname(path)));
45-
// BAD: taint is preserved
46-
res.write(fs.readFileSync(pathModule.join(path)));
47-
// BAD: taint is preserved
48-
res.write(fs.readFileSync(pathModule.join(x, y, path, z)));
49-
// BAD: taint is preserved
50-
res.write(fs.readFileSync(pathModule.normalize(path)));
51-
// BAD: taint is preserved
52-
res.write(fs.readFileSync(pathModule.relative(x, path)));
53-
// BAD: taint is preserved
54-
res.write(fs.readFileSync(pathModule.relative(path, x)));
55-
// BAD: taint is preserved
56-
res.write(fs.readFileSync(pathModule.resolve(path)));
57-
// BAD: taint is preserved
58-
res.write(fs.readFileSync(pathModule.resolve(x, y, path, z)));
59-
// BAD: taint is preserved
60-
res.write(fs.readFileSync(pathModule.toNamespacedPath(path)));
42+
res.write(fs.readFileSync(pathModule.join(path))); // $ Alert - taint is preserved
43+
res.write(fs.readFileSync(pathModule.join(x, y, path, z))); // $ Alert - taint is preserved
44+
res.write(fs.readFileSync(pathModule.normalize(path))); // $ Alert - taint is preserved
45+
res.write(fs.readFileSync(pathModule.relative(x, path))); // $ Alert - taint is preserved
46+
res.write(fs.readFileSync(pathModule.relative(path, x))); // $ Alert - taint is preserved
47+
res.write(fs.readFileSync(pathModule.resolve(path))); // $ Alert - taint is preserved
48+
res.write(fs.readFileSync(pathModule.resolve(x, y, path, z))); // $ Alert - taint is preserved
49+
res.write(fs.readFileSync(pathModule.toNamespacedPath(path))); // $ Alert - taint is preserved
6150
});
6251

6352
var server = http.createServer(function(req, res) {
6453
// tests for a few uri-libraries
65-
res.write(fs.readFileSync(require("querystringify").parse(req.url).query)); // NOT OK
66-
res.write(fs.readFileSync(require("query-string").parse(req.url).query)); // NOT OK
67-
res.write(fs.readFileSync(require("querystring").parse(req.url).query)); // NOT OK
54+
res.write(fs.readFileSync(require("querystringify").parse(req.url).query)); // $ Alert
55+
res.write(fs.readFileSync(require("query-string").parse(req.url).query)); // $ Alert
56+
res.write(fs.readFileSync(require("querystring").parse(req.url).query)); // $ Alert
6857
});
6958

7059
(function(){
@@ -100,7 +89,7 @@ var server = http.createServer(function(req, res) {
10089
path = path.replace(/\.\./g, ''); // remove all ".."
10190
}
10291

103-
res.write(fs.readFileSync(path)); // OK. Is sanitized above.
92+
res.write(fs.readFileSync(path)); // OK - Is sanitized above.
10493
});
10594

10695
var server = http.createServer(function(req, res) {
@@ -113,109 +102,109 @@ var server = http.createServer(function(req, res) {
113102
path = path.replace(/\.\./g, ''); // remove all ".."
114103
}
115104

116-
res.write(fs.readFileSync(path)); // OK. Is sanitized above.
105+
res.write(fs.readFileSync(path)); // OK - Is sanitized above.
117106
});
118107

119108
var server = http.createServer(function(req, res) {
120109
let path = url.parse(req.url, true).query.path;
121110

122-
require('send')(req, path); // NOT OK
111+
require('send')(req, path); // $ Alert
123112
});
124113

125114
var server = http.createServer(function(req, res) {
126115
let path = url.parse(req.url, true).query.path;
127116

128-
fs.readFileSync(path); // NOT OK
117+
fs.readFileSync(path); // $ Alert
129118

130119
var split = path.split("/");
131120

132-
fs.readFileSync(split.join("/")); // NOT OK
121+
fs.readFileSync(split.join("/")); // $ Alert
133122

134-
fs.readFileSync(prefix + split[split.length - 1]) // OK
123+
fs.readFileSync(prefix + split[split.length - 1])
135124

136-
fs.readFileSync(split[x]) // NOT OK
137-
fs.readFileSync(prefix + split[x]) // NOT OK
125+
fs.readFileSync(split[x]) // $ Alert
126+
fs.readFileSync(prefix + split[x]) // $ Alert
138127

139128
var concatted = prefix.concat(split);
140-
fs.readFileSync(concatted.join("/")); // NOT OK
129+
fs.readFileSync(concatted.join("/")); // $ Alert
141130

142131
var concatted2 = split.concat(prefix);
143-
fs.readFileSync(concatted2.join("/")); // NOT OK
132+
fs.readFileSync(concatted2.join("/")); // $ Alert
144133

145-
fs.readFileSync(split.pop()); // NOT OK
134+
fs.readFileSync(split.pop()); // $ Alert
146135

147136
});
148137

149138
var server = http.createServer(function(req, res) {
150139
let path = url.parse(req.url, true).query.path;
151140

152141
// Removal of forward-slash or dots.
153-
res.write(fs.readFileSync(path.replace(/[\]\[*,;'"`<>\\?\/]/g, ''))); // OK.
154-
res.write(fs.readFileSync(path.replace(/[abcd]/g, ''))); // NOT OK
155-
res.write(fs.readFileSync(path.replace(/[./]/g, ''))); // OK
156-
res.write(fs.readFileSync(path.replace(/[foobar/foobar]/g, ''))); // OK
157-
res.write(fs.readFileSync(path.replace(/\//g, ''))); // OK
158-
res.write(fs.readFileSync(path.replace(/\.|\//g, ''))); // OK
159-
160-
res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // NOT OK (can be absolute)
161-
res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // NOT OK (can be absolute)
162-
res.write(fs.readFileSync(path.replace(/\./g, ''))); // NOT OK (can be absolute)
163-
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // NOT OK (can be absolute)
142+
res.write(fs.readFileSync(path.replace(/[\]\[*,;'"`<>\\?\/]/g, '')));
143+
res.write(fs.readFileSync(path.replace(/[abcd]/g, ''))); // $ Alert
144+
res.write(fs.readFileSync(path.replace(/[./]/g, '')));
145+
res.write(fs.readFileSync(path.replace(/[foobar/foobar]/g, '')));
146+
res.write(fs.readFileSync(path.replace(/\//g, '')));
147+
res.write(fs.readFileSync(path.replace(/\.|\//g, '')));
148+
149+
res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // $ Alert - can be absolute
150+
res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // $ Alert - can be absolute
151+
res.write(fs.readFileSync(path.replace(/\./g, ''))); // $ Alert - can be absolute
152+
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // $ Alert - can be absolute
164153

165154
if (!pathModule.isAbsolute(path)) {
166-
res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // OK
167-
res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // OK
168-
res.write(fs.readFileSync(path.replace(/\./g, ''))); // OK
169-
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // OK
155+
res.write(fs.readFileSync(path.replace(/[.]/g, '')));
156+
res.write(fs.readFileSync(path.replace(/[..]/g, '')));
157+
res.write(fs.readFileSync(path.replace(/\./g, '')));
158+
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, '')));
170159
}
171160

172161
// removing of "../" from prefix.
173-
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/^(\.\.[\/\\])+/, ''))); // OK
174-
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.[\/\\])+/, ''))); // OK
175-
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)+/, ''))); // OK
176-
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)*/, ''))); // OK
162+
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/^(\.\.[\/\\])+/, '')));
163+
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.[\/\\])+/, '')));
164+
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)+/, '')));
165+
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)*/, '')));
177166

178-
res.write(fs.readFileSync("prefix" + path.replace(/^(\.\.[\/\\])+/, ''))); // NOT OK - not normalized
179-
res.write(fs.readFileSync(pathModule.normalize(path).replace(/^(\.\.[\/\\])+/, ''))); // NOT OK (can be absolute)
167+
res.write(fs.readFileSync("prefix" + path.replace(/^(\.\.[\/\\])+/, ''))); // $ Alert - not normalized
168+
res.write(fs.readFileSync(pathModule.normalize(path).replace(/^(\.\.[\/\\])+/, ''))); // $ Alert - can be absolute
180169
});
181170

182171
import normalizeUrl from 'normalize-url';
183172

184173
var server = http.createServer(function(req, res) {
185174
// tests for a few more uri-libraries
186175
const qs = require("qs");
187-
res.write(fs.readFileSync(qs.parse(req.url).foo)); // NOT OK
188-
res.write(fs.readFileSync(qs.parse(normalizeUrl(req.url)).foo)); // NOT OK
176+
res.write(fs.readFileSync(qs.parse(req.url).foo)); // $ Alert
177+
res.write(fs.readFileSync(qs.parse(normalizeUrl(req.url)).foo)); // $ Alert
189178
const parseqs = require("parseqs");
190-
res.write(fs.readFileSync(parseqs.decode(req.url).foo)); // NOT OK
179+
res.write(fs.readFileSync(parseqs.decode(req.url).foo)); // $ Alert
191180
});
192181

193182
const cp = require("child_process");
194183
var server = http.createServer(function(req, res) {
195184
let path = url.parse(req.url, true).query.path;
196-
cp.execSync("foobar", {cwd: path}); // NOT OK
197-
cp.execFileSync("foobar", ["args"], {cwd: path}); // NOT OK
198-
cp.execFileSync("foobar", {cwd: path}); // NOT OK
185+
cp.execSync("foobar", {cwd: path}); // $ Alert
186+
cp.execFileSync("foobar", ["args"], {cwd: path}); // $ Alert
187+
cp.execFileSync("foobar", {cwd: path}); // $ Alert
199188
});
200189

201190
var server = http.createServer(function(req, res) {
202191
let path = url.parse(req.url, true).query.path;
203192

204193
// Removal of forward-slash or dots.
205-
res.write(fs.readFileSync(path.replace(new RegExp("[\\]\\[*,;'\"`<>\\?/]", 'g'), ''))); // OK
206-
res.write(fs.readFileSync(path.replace(new RegExp("[\\]\\[*,;'\"`<>\\?/]", ''), ''))); // NOT OK.
207-
res.write(fs.readFileSync(path.replace(new RegExp("[\\]\\[*,;'\"`<>\\?/]", unknownFlags()), ''))); // OK -- Might be okay depending on what unknownFlags evaluates to.
194+
res.write(fs.readFileSync(path.replace(new RegExp("[\\]\\[*,;'\"`<>\\?/]", 'g'), '')));
195+
res.write(fs.readFileSync(path.replace(new RegExp("[\\]\\[*,;'\"`<>\\?/]", ''), ''))); // $ Alert
196+
res.write(fs.readFileSync(path.replace(new RegExp("[\\]\\[*,;'\"`<>\\?/]", unknownFlags()), ''))); // OK - Might be okay depending on what unknownFlags evaluates to.
208197
});
209198

210199
var server = http.createServer(function(req, res) {
211200
let path = url.parse(req.url, true).query.path;
212201

213-
res.write(fs.readFileSync(path.replace(new RegExp("[.]", 'g'), ''))); // NOT OK (can be absolute)
202+
res.write(fs.readFileSync(path.replace(new RegExp("[.]", 'g'), ''))); // $ Alert - can be absolute
214203

215204
if (!pathModule.isAbsolute(path)) {
216-
res.write(fs.readFileSync(path.replace(new RegExp("[.]", ''), ''))); // NOT OK
217-
res.write(fs.readFileSync(path.replace(new RegExp("[.]", 'g'), ''))); // OK
218-
res.write(fs.readFileSync(path.replace(new RegExp("[.]", unknownFlags()), ''))); // OK
205+
res.write(fs.readFileSync(path.replace(new RegExp("[.]", ''), ''))); // $ Alert
206+
res.write(fs.readFileSync(path.replace(new RegExp("[.]", 'g'), '')));
207+
res.write(fs.readFileSync(path.replace(new RegExp("[.]", unknownFlags()), '')));
219208
}
220209
});
221210

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/examples/TaintedPath.js‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,5 @@ const ROOT = "/var/www/";
77
var server = http.createServer(function(req, res) {
88
let filePath = url.parse(req.url, true).query.path;
99

10-
// BAD: This function uses unsanitized input that can read any file on the file system.
11-
res.write(fs.readFileSync(ROOT + filePath, 'utf8'));
10+
res.write(fs.readFileSync(ROOT + filePath, 'utf8')); // $ Alert - This function uses unsanitized input that can read any file on the file system.
1211
});

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/examples/TaintedPathGood.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ const ROOT = "/var/www/";
88
var server = http.createServer(function(req, res) {
99
let filePath = url.parse(req.url, true).query.path;
1010

11-
// GOOD: Verify that the file path is under the root directory
11+
// OK - Verify that the file path is under the root directory
1212
filePath = fs.realpathSync(path.resolve(ROOT, filePath));
1313
if (!filePath.startsWith(ROOT)) {
1414
res.statusCode = 403;

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,27 +26,27 @@ function init() {
2626
init();
2727

2828
app.get('/some/path1', function (req, res) {
29-
res.send(data.compiledFileAccess({ path: req.params.path })); // NOT ALLOWED (template uses vulnerable catFile)
29+
res.send(data.compiledFileAccess({ path: req.params.path })); // $ Alert - template uses vulnerable catFile
3030
});
3131

3232
app.get('/some/path2', function (req, res) {
33-
res.send(data.compiledBenign({ name: req.params.name })); // ALLOWED (this template does not use catFile)
33+
res.send(data.compiledBenign({ name: req.params.name })); // OK - this template does not use catFile
3434
});
3535

3636
app.get('/some/path3', function (req, res) {
37-
res.send(data.compiledUnknown({ name: req.params.name })); // ALLOWED (could be using a vulnerable helper, but we'll assume it's ok)
37+
res.send(data.compiledUnknown({ name: req.params.name })); // OK - could be using a vulnerable helper, but we'll assume it's ok
3838
});
3939

4040
app.get('/some/path4', function (req, res) {
4141
res.send(data.compiledMixed({
4242
prefix: ">>> ",
43-
path: req.params.path // NOT ALLOWED (template uses vulnerable helper)
43+
path: req.params.path // $ Alert - template uses vulnerable helper
4444
}));
4545
});
4646

4747
app.get('/some/path5', function (req, res) {
4848
res.send(data.compiledMixed({
49-
prefix: req.params.prefix, // ALLOWED (this parameter is safe)
49+
prefix: req.params.prefix, // OK - this parameter is safe
5050
path: "data/path-5.txt"
5151
}));
5252
});

0 commit comments

Comments
 (0)