fix(helper): sanitize uuid in log_to_file_queue to prevent path traversal (CWE-22) - #179
fix(helper): sanitize uuid in log_to_file_queue to prevent path traversal (CWE-22)#179sebastionoss wants to merge 1 commit into
Conversation
…rsal (CWE-22)
log_to_file_queue() built the log file path as
path.join('logs', uuid + '_log.txt')
using the raw uuid argument. Several callers pass req.body.uuid directly
(e.g. /cancel in app.js, fast-scan.js, slow-scan.js, special-scan.js,
string-analysis.js), so a request with a uuid such as '../foo' would
cause fs.appendFile to write outside the logs/ directory.
Apply the same allow-list sanitizer that the sibling get_log_file()
already uses so both helpers always resolve to the same on-disk file
for a given uuid.
|
if you will take a look at this current version of app.js this is in markdown this actually works to fix that problem I have included the entire text of the code for this version of app.js I recently edited and fixed it is waiting on approval but this does work. // ------------------------------------------------------------- import yargs from 'yargs' if (argv.output !== 'json') { import semver from 'semver' if (semver.satisfies(process.version, '>=14')) { import express from 'express' const pe = new PrettyError() if (!fs.existsSync('logs')) { import helper from './modules/helper.js' const app = express() app.post('/get_logs', async function (req, res, next) { app.get('/get_settings', async function (req, res, next) { })) temp_list = temp_list.filter(item => item !== undefined) app.post('/save_settings', async function (req, res, next) { if (helper.proxy !== '') { res.json('Done') app.get('/generate', async function (req, res, next) { app.post('/cancel', async function (req, res, next) { app.post('/analyze_string', async function (req, res, next) { let stats_default = { if (req.body.string === 'test_user_2021_2022_') { } app.use((err, req, res, next) => { app.use((req, res, next) => { process.on('uncaughtException', function (err) { process.on('unhandledRejection', function (err) { function delete_keys (object, temp_keys) { function clean_up_item (object, temp_keys_str) { function search_and_change (site, _dict) { async function check_user_cli (argv) { await helper.websites_entries.forEach(async function (value, i) { if (argv.websites === 'all') { } else { if (req.body.string.includes(',')) { if (req.body.group) { if (typeof ret === 'undefined' || ret === undefined || ret.length === 0) { } async function list_all_websites () { console.log('[Listing] Available websites\n' + temp_arr.join('\n')) let server_host = 'localhost' if (argv.grid !== '') { |
Summary
log_to_file_queue()inmodules/helper.jsbuilds a log-file path by concatenating the caller-supplieduuidstraight intopath.join('logs', uuid + '_log.txt'). Several Express routes — most notablyPOST /cancel— passreq.body.uuidto this helper without sanitizing it first, so a request withuuid = "../foo"causesfs.appendFileto write/append to a file outside thelogs/directory.modules/helper.js→log_to_file_queue(uuid, msg, …)fs.appendFile(slash(path.join('logs', uuid + '_log.txt')), …)(helper.js line ~106 before the fix)POST /cancel(app.js:226-235) passesreq.body.uuiddirectly; other routes such as/analyze_stringhappen to sanitizeuuidin-place earlier, but the helper itself made no guarantees.app.jsonly registersexpress.json(),express.urlencoded(), andexpress.static('public'); there is no auth middleware. Under--dockermode (docker-compose.ymlentrypoint) the server binds0.0.0.0.Why the path is reachable
get_log_file()already applies a[^a-zA-Z0-9-]+allow-list touuidbefore constructing a path.log_to_file_queue()did not, even though both helpers are supposed to resolve to the same file for a given uuid.Looking at the callers in
app.js:temp_uuidis sanitized for theglobal_lockcheck, but the rawreq.body.uuidis what the helper sees.path.join('logs', '../foo' + '_log.txt')normalizes tofoo_log.txtin the parent oflogs/, andfs.appendFilehappily creates/appends it.Fix
Apply the same allow-list inside
log_to_file_queue()so the helper is safe regardless of caller. This mirrors whatget_log_file()already does, so both helpers continue to agree on the on-disk filename for a givenuuid:The diff is +8/-1 in a single file (
modules/helper.js); no public API or filename semantics change for any valid uuid (uuids generated by the app are already in[a-zA-Z0-9-]).Proof of Concept
Reproduces against
npm start -- --docker(or any invocation that exposes port 9005). Run from the repo root afternpm install:Before the fix,
pwn_pre_log.txtappears next toapp.js, not underlogs/. After the fix, the same payload writes tologs/pwn_log.txt(the..,/, and other disallowed characters are stripped, matchingget_log_file()'s behaviour).You can verify the sanitizer directly without the server:
Testing
pwn_pre_log.txtinto the parent oflogs/viaPOST /cancel.logs/pwn_log.txt(file stays insidelogs/).1f3c8a9d-…) round-trip identically through the new allow-list, so existing log filenames are unchanged.get_log_file()andlog_to_file_queue()now resolve to the same path for any given input uuid (this was already the intent — they previously diverged only on adversarial input).modules/helper.jsand does not touch any route, signature, or exported symbol.Impact
An unauthenticated network attacker reaching the Express server (the default Docker deployment binds
0.0.0.0:9005) can:*_log.txtfiles in directories the Node process can reach (the path is still anchored atlogs/viapath.join, but..segments collapse out of it). This is enough to:.txtartifacts in adjacent directories,[Canceling] task: <uuid>), so an attacker also influences the file contents via the uuid string itself.It does not give arbitrary-extension write (the
_log.txtsuffix is always appended), and it does not give read access — so the severity is bounded, but the fix is small and clearly correct, and it removes a footgun that any future caller oflog_to_file_queue()would otherwise inherit.Adversarial review
Before submitting we tried to disprove this. Specifically: (a) is there an upstream sanitizer we missed?
app.jsonly mountsexpress.json/urlencoded/staticand there is no router-levelapp.usedoing input scrubbing —grepfor auth/middleware markers returns nothing relevant, and/cancel's own sanitizer is applied totemp_uuidbut not to the value passed into the helper. (b) Doespath.joinitself block traversal? It doesn't —path.join('logs', '../foo_log.txt')normalizes tofoo_log.txt, which is exactly the bug. (c) Is the/cancelroute accidentally unreachable or gated by something else? No — it's a plainapp.postwith no auth, and the only condition (option === 'on' && uuid !== '') is attacker-controlled. (d) Is fixing the helper redundant if every current caller already sanitizes? No —/canceldoes not sanitize the value it passes in, and even if it did, putting the allow-list at the sink prevents the next contributor from re-introducing the bug.Discovered by the Sebastion AI GitHub App.