Skip to content

Port js/TaintedPath tests to post-process and don't expect 'Source' tag on same line #18643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
asgerf wants to merge 11 commits into from
Closed

Port js/TaintedPath tests to post-process and don't expect 'Source' tag on same line #18643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d20dd75
JS: Convert TaintedPath to post-processed inline test
asgerf Jan 31, 2025
2f5de5c
JS: Convert OK-style comments to $-style
asgerf Jan 31, 2025
928d962
JS: Raw update of test output
asgerf Jan 31, 2025
01e1fc4
JS: Move some Alerts to their actual location
asgerf Jan 31, 2025
0f33bf1
JS: Add missing Alert expectations
asgerf Jan 31, 2025
915aa3d
JS: Add Source expectations
asgerf Jan 31, 2025
9f01faa
JS: Update expected output, with the test failures gone
asgerf Jan 31, 2025
d349b51
Test: Don't expect 'Source' tag when source and alert are on same line
asgerf Jan 30, 2025
c5545f0
JS: Remove same-line Source expectations
asgerf Jan 31, 2025
4c645aa
JS: Update another test
asgerf Jan 31, 2025
ecd418b
Rust: update a Rust test case
asgerf Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file removed javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/Consistency.expected
View file
Open in desktop
Empty file.
9 changes: 0 additions & 9 deletions javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/Consistency.ql
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath-es6.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@ import { parse } from 'url';
import { join } from 'path';

var server = createServer(function(req, res) {
let path = parse(req.url, true).query.path;
let path = parse(req.url, true).query.path; // $ Source

// BAD: This could read any file on the file system
res.write(readFileSync(join("public", path)));
res.write(readFileSync(join("public", path))); // $ Alert - This could read any file on the file system
});
1,668 changes: 834 additions & 834 deletions javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected
View file
Open in desktop

Large diffs are not rendered by default.

171 changes: 80 additions & 91 deletions javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,73 +6,62 @@ var fs = require('fs'),
;

var server = http.createServer(function(req, res) {
let path = url.parse(req.url, true).query.path;
let path = url.parse(req.url, true).query.path; // $ Source

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

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

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

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

if (fs.existsSync(path))
res.write(fs.readFileSync(path)); // BAD: Insufficient sanitisation
res.write(fs.readFileSync(path)); // $ Alert - Insufficient sanitisation

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

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

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

path = sanitize(path);
res.write(fs.readFileSync(path)); // GOOD: Path is sanitized
res.write(fs.readFileSync(path)); // OK - Path is sanitized

path = url.parse(req.url, true).query.path;
// GOOD: basename is safe
path = url.parse(req.url, true).query.path; // $ Source
// OK - basename is safe
res.write(fs.readFileSync(pathModule.basename(path)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.dirname(path)));
// GOOD: extname is safe
res.write(fs.readFileSync(pathModule.dirname(path))); // $ Alert - taint is preserved
// OK - extname is safe
res.write(fs.readFileSync(pathModule.extname(path)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.join(path)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.join(x, y, path, z)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.normalize(path)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.relative(x, path)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.relative(path, x)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.resolve(path)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.resolve(x, y, path, z)));
// BAD: taint is preserved
res.write(fs.readFileSync(pathModule.toNamespacedPath(path)));
res.write(fs.readFileSync(pathModule.join(path))); // $ Alert - taint is preserved
res.write(fs.readFileSync(pathModule.join(x, y, path, z))); // $ Alert - taint is preserved
res.write(fs.readFileSync(pathModule.normalize(path))); // $ Alert - taint is preserved
res.write(fs.readFileSync(pathModule.relative(x, path))); // $ Alert - taint is preserved
res.write(fs.readFileSync(pathModule.relative(path, x))); // $ Alert - taint is preserved
res.write(fs.readFileSync(pathModule.resolve(path))); // $ Alert - taint is preserved
res.write(fs.readFileSync(pathModule.resolve(x, y, path, z))); // $ Alert - taint is preserved
res.write(fs.readFileSync(pathModule.toNamespacedPath(path))); // $ Alert - taint is preserved
});

var server = http.createServer(function(req, res) {
// tests for a few uri-libraries
res.write(fs.readFileSync(require("querystringify").parse(req.url).query)); // NOT OK
res.write(fs.readFileSync(require("query-string").parse(req.url).query)); // NOT OK
res.write(fs.readFileSync(require("querystring").parse(req.url).query)); // NOT OK
res.write(fs.readFileSync(require("querystringify").parse(req.url).query)); // $ Alert
res.write(fs.readFileSync(require("query-string").parse(req.url).query)); // $ Alert
res.write(fs.readFileSync(require("querystring").parse(req.url).query)); // $ Alert
});

(function(){

var express = require('express');
var application = express();

var views_local = (req, res) => res.render(req.params[0]);
var views_local = (req, res) => res.render(req.params[0]); // $ Alert
application.get('/views/*', views_local);

var views_imported = require("./views");
Expand All @@ -81,12 +70,12 @@ var server = http.createServer(function(req, res) {
})();

var server = http.createServer(function(req, res) {
let path = url.parse(req.url, true).query.path;
let path = url.parse(req.url, true).query.path; // $ Source

res.write(fs.readFileSync(fs.realpathSync(path)));
res.write(fs.readFileSync(fs.realpathSync(path))); // $ Alert
fs.realpath(path,
function(err, realpath){
res.write(fs.readFileSync(realpath));
res.write(fs.readFileSync(realpath)); // $ Alert
}
);

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

res.write(fs.readFileSync(path)); // OK. Is sanitized above.
res.write(fs.readFileSync(path)); // OK - Is sanitized above.
});

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

res.write(fs.readFileSync(path)); // OK. Is sanitized above.
res.write(fs.readFileSync(path)); // OK - Is sanitized above.
});

var server = http.createServer(function(req, res) {
let path = url.parse(req.url, true).query.path;
let path = url.parse(req.url, true).query.path; // $ Source

require('send')(req, path); // NOT OK
require('send')(req, path); // $ Alert
});

var server = http.createServer(function(req, res) {
let path = url.parse(req.url, true).query.path;
let path = url.parse(req.url, true).query.path; // $ Source

fs.readFileSync(path); // NOT OK
fs.readFileSync(path); // $ Alert

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

fs.readFileSync(split.join("/")); // NOT OK
fs.readFileSync(split.join("/")); // $ Alert

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

fs.readFileSync(split[x]) // NOT OK
fs.readFileSync(prefix + split[x]) // NOT OK
fs.readFileSync(split[x]) // $ Alert
fs.readFileSync(prefix + split[x]) // $ Alert

var concatted = prefix.concat(split);
fs.readFileSync(concatted.join("/")); // NOT OK
fs.readFileSync(concatted.join("/")); // $ Alert

var concatted2 = split.concat(prefix);
fs.readFileSync(concatted2.join("/")); // NOT OK
fs.readFileSync(concatted2.join("/")); // $ Alert

fs.readFileSync(split.pop()); // NOT OK
fs.readFileSync(split.pop()); // $ Alert

});

var server = http.createServer(function(req, res) {
let path = url.parse(req.url, true).query.path;
let path = url.parse(req.url, true).query.path; // $ Source

// Removal of forward-slash or dots.
res.write(fs.readFileSync(path.replace(/[\]\[*,;'"`<>\\?\/]/g, ''))); // OK.
res.write(fs.readFileSync(path.replace(/[abcd]/g, ''))); // NOT OK
res.write(fs.readFileSync(path.replace(/[./]/g, ''))); // OK
res.write(fs.readFileSync(path.replace(/[foobar/foobar]/g, ''))); // OK
res.write(fs.readFileSync(path.replace(/\//g, ''))); // OK
res.write(fs.readFileSync(path.replace(/\.|\//g, ''))); // OK

res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // NOT OK (can be absolute)
res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // NOT OK (can be absolute)
res.write(fs.readFileSync(path.replace(/\./g, ''))); // NOT OK (can be absolute)
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // NOT OK (can be absolute)
res.write(fs.readFileSync(path.replace(/[\]\[*,;'"`<>\\?\/]/g, '')));
res.write(fs.readFileSync(path.replace(/[abcd]/g, ''))); // $ Alert
res.write(fs.readFileSync(path.replace(/[./]/g, '')));
res.write(fs.readFileSync(path.replace(/[foobar/foobar]/g, '')));
res.write(fs.readFileSync(path.replace(/\//g, '')));
res.write(fs.readFileSync(path.replace(/\.|\//g, '')));

res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // $ Alert - can be absolute
res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // $ Alert - can be absolute
res.write(fs.readFileSync(path.replace(/\./g, ''))); // $ Alert - can be absolute
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // $ Alert - can be absolute

if (!pathModule.isAbsolute(path)) {
res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // OK
res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // OK
res.write(fs.readFileSync(path.replace(/\./g, ''))); // OK
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // OK
res.write(fs.readFileSync(path.replace(/[.]/g, '')));
res.write(fs.readFileSync(path.replace(/[..]/g, '')));
res.write(fs.readFileSync(path.replace(/\./g, '')));
res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, '')));
}

// removing of "../" from prefix.
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/^(\.\.[\/\\])+/, ''))); // OK
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.[\/\\])+/, ''))); // OK
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)+/, ''))); // OK
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)*/, ''))); // OK
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/^(\.\.[\/\\])+/, '')));
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.[\/\\])+/, '')));
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)+/, '')));
res.write(fs.readFileSync("prefix" + pathModule.normalize(path).replace(/(\.\.\/)*/, '')));

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

import normalizeUrl from 'normalize-url';

var server = http.createServer(function(req, res) {
// tests for a few more uri-libraries
const qs = require("qs");
res.write(fs.readFileSync(qs.parse(req.url).foo)); // NOT OK
res.write(fs.readFileSync(qs.parse(normalizeUrl(req.url)).foo)); // NOT OK
res.write(fs.readFileSync(qs.parse(req.url).foo)); // $ Alert
res.write(fs.readFileSync(qs.parse(normalizeUrl(req.url)).foo)); // $ Alert
const parseqs = require("parseqs");
res.write(fs.readFileSync(parseqs.decode(req.url).foo)); // NOT OK
res.write(fs.readFileSync(parseqs.decode(req.url).foo)); // $ Alert
});

const cp = require("child_process");
var server = http.createServer(function(req, res) {
let path = url.parse(req.url, true).query.path;
cp.execSync("foobar", {cwd: path}); // NOT OK
cp.execFileSync("foobar", ["args"], {cwd: path}); // NOT OK
cp.execFileSync("foobar", {cwd: path}); // NOT OK
let path = url.parse(req.url, true).query.path; // $ Source
cp.execSync("foobar", {cwd: path}); // $ Alert
cp.execFileSync("foobar", ["args"], {cwd: path}); // $ Alert
cp.execFileSync("foobar", {cwd: path}); // $ Alert
});

var server = http.createServer(function(req, res) {
let path = url.parse(req.url, true).query.path;
let path = url.parse(req.url, true).query.path; // $ Source

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

var server = http.createServer(function(req, res) {
let path = url.parse(req.url, true).query.path;
let path = url.parse(req.url, true).query.path; // $ Source

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

if (!pathModule.isAbsolute(path)) {
res.write(fs.readFileSync(path.replace(new RegExp("[.]", ''), ''))); // NOT OK
res.write(fs.readFileSync(path.replace(new RegExp("[.]", 'g'), ''))); // OK
res.write(fs.readFileSync(path.replace(new RegExp("[.]", unknownFlags()), ''))); // OK
res.write(fs.readFileSync(path.replace(new RegExp("[.]", ''), ''))); // $ Alert
res.write(fs.readFileSync(path.replace(new RegExp("[.]", 'g'), '')));
res.write(fs.readFileSync(path.replace(new RegExp("[.]", unknownFlags()), '')));
}
});

3 changes: 2 additions & 1 deletion javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
Security/CWE-022/TaintedPath.ql
query: Security/CWE-022/TaintedPath.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
5 changes: 2 additions & 3 deletions javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/examples/TaintedPath.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ const fs = require('fs'),
const ROOT = "/var/www/";

var server = http.createServer(function(req, res) {
let filePath = url.parse(req.url, true).query.path;
let filePath = url.parse(req.url, true).query.path; // $ Source

// BAD: This function uses unsanitized input that can read any file on the file system.
res.write(fs.readFileSync(ROOT + filePath, 'utf8'));
res.write(fs.readFileSync(ROOT + filePath, 'utf8')); // $ Alert - This function uses unsanitized input that can read any file on the file system.
});
2 changes: 1 addition & 1 deletion javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/examples/TaintedPathGood.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const ROOT = "/var/www/";
var server = http.createServer(function(req, res) {
let filePath = url.parse(req.url, true).query.path;

// GOOD: Verify that the file path is under the root directory
// OK - Verify that the file path is under the root directory
filePath = fs.realpathSync(path.resolve(ROOT, filePath));
if (!filePath.startsWith(ROOT)) {
res.statusCode = 403;
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/express.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ let app = express();
app.use(fileUpload());

app.get("/some/path", function (req, res) {
req.files.foo.mv(req.query.bar);
req.files.foo.mv(req.query.bar); // $ Alert
});
14 changes: 7 additions & 7 deletions javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/handlebars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ const data = {};

function init() {
hb.registerHelper("catFile", function catFile(filePath) {
return fs.readFileSync(filePath); // SINK (reads file)
return fs.readFileSync(filePath); // $ Alert
});
hb.registerHelper("prependToLines", function prependToLines(prefix, filePath) {
return fs
.readFileSync(filePath)
.readFileSync(filePath) // $ Alert
.split("\n")
.map((line) => prefix + line)
.join("\n");
Expand All @@ -26,27 +26,27 @@ function init() {
init();

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

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

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

app.get('/some/path4', function (req, res) {
res.send(data.compiledMixed({
prefix: ">>> ",
path: req.params.path // NOT ALLOWED (template uses vulnerable helper)
path: req.params.path // $ Source - template uses vulnerable helper
}));
});

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