Repository navigation
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hello,
I am writing to report a potential OS Command Injection vulnerability in the following file:
app.jsThe
/syncupload endpoint derives a file path from the uploaded file name and concatenates it into a command that is executed withchild_process.execSync(...):The upload file name is attacker-controlled (the endpoint has no authentication and
app.listen(HTTP_PORT)binds all interfaces). Although the path is wrapped in double quotes, POSIX shells still perform command substitution inside double quotes, so a file name such asx$(calc &)is executed as a separate command on thelinuxanddarwinbranches.The same unvalidated name is also used to build a filesystem path, which allows path traversal in both
/synchandlers (arbitrary file write on upload, arbitrary file read on download).Root Cause
The issue is caused by unsafe shell command construction in the following handler:
_file['originalname']comes straight from the multipart upload. The resulting path is interpolated into a shell command string, so shell metacharacters in the file name are interpreted as command syntax.Note that double quotes do not protect against command substitution in POSIX shells:
`...`and$(...)are still evaluated inside them. On Windows the same command line is safe, becausecmd.exeperforms no command substitution and"is not a legal character in a Windows file name.The task ID is also used for a filesystem path without confinement:
Reproduction Material
A minimal reproduction script is provided in:
poc_app.jspoc_app.js
This Proof of Concept (PoC) is intended to demonstrate that external input can reach dangerous command execution logic through the vulnerable code path. For observability, the PoC hooks
child_process.execSync()to log the intercepted command before it is executed.What the PoC Does
The PoC performs a minimal end-to-end trigger of the vulnerable code path:
express,multerandbody-parserto capture the/syncroute handler without starting a server.child_process.execSyncto log the command produced by the application.app.js.x$(calc &)WIDGET_FILEbecomes<scripts>/x$(calc &).jsand thelinuxbranch buildssh -c code "<scripts>/x$(calc &).js".calcbefore executing the rest of the command.Because the injection only expands on the POSIX branches while the PoC is executed on Windows, the PoC selects the
linuxbranch and hands the exact command string produced by the application to a real POSIX shell (Git Bash). The command execution itself is not simulated.In the provided example, the injected payload is crafted so that successful command execution launches the Calculator application. This serves as a visible indicator that external input can reach the OS command execution sink without proper sanitization.
Example Payload
Uploaded file name:
which produces the shell command:
sh -c code "<scripts>/x$(calc &).js"On a Windows or POSIX host the same handler also allows path traversal, for example the uploaded name
..\..\evil(Windows) or../../evil.How to Run
Run from the folder containing the files:
On Windows, Git Bash must be installed (the PoC uses it as the POSIX shell).
Expected Output
When the PoC runs, you should see output similar to:
In the vulnerable version, when the crafted request is processed, the shell interprets
$(...)and executes the injectedcalccommand, launching the Calculator application as a benign demonstration effect.This shows that an attacker-controlled upload file name can influence command execution behavior through the vulnerable
/synchandler.Patch Explanation
This branch also includes a patched version of
app.js(new_app.js) intended to mitigate the command injection risk described above.What the patch changes
path.basename(_file['originalname'])inPOST /sync;path.basename(String(name || ''))inGET /sync.What this means for the rest of the flow
path.basenameis the identity for ordinary widget names, so normal uploads and downloads behave exactly as before.linuxanddarwinbranches still runcode <path>through the same shell with the same argument; only quoting changes, so the editor still opens the same file.win32branch is left untouched:"is not a valid character in a Windows file name andcmd.exehas no command substitution, so the doubled-quoted path cannot be broken out of.Validation introduced by the patch
Rather than filtering out "dangerous" characters (a block-list, which is inherently incomplete), the patch makes the shell treat the path as data:
$(), backticks,;,&,|and every other metacharacter;'\'', so any legal file name round-trips correctly;path.basenameguarantees the value cannot address a directory outsideScripts/.With the patch, the file name
x$(calc &)producessh -c code '<scripts>/x$(calc &).js', which the shell treats as a literal path.