-
Notifications
You must be signed in to change notification settings - Fork 28
Security Fix for Remote Code Execution - huntr.dev #124
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
Conversation
[Fix] Patched RCE arising inside the "launchpad" library
|
@daffl - let me know your thoughts! 🍰 |
|
@matthewp @justinbmeyer this should get merged i think it looks pritty good to me. |
|
After this PR, the browsers do not exit as expected during the test suite. Prior to the PR during the test battery browsers would exit. I don't know the source of the issue yet but am investigating. @JamieSlome and @Mik317 I would appreciate your help on this. |
|
I am seeing the above problem on MacOS 10.15.7 (my dev machine). Other platforms are as yet untested. |
|
Root cause: Because execFile does not create a subshell, quoting does not work the same way when constructing the command to terminate a process: Also not working: In each of these cases, the single quote at the beginning and end of the AppleScript snippet is passed to the script interpreter as a literal quote, and it is not accepted. (The error returned is Working: I have made a PR to address this. No further action is required. Thanks for the security fix! |
https://huntr.dev/app/users/Mik317 has fixed the Remote Code Execution vulnerability 🔨. Mik317 has been awarded $25 for fixing the vulnerability through the huntr bug bounty program 💵. Think you could fix a vulnerability like this?
Get involved at https://huntr.dev/
Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#1
GitHub Issue URL | #123
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/launchpad/1/README.md
User Comments:
📊 Metadata *
Please enter the direct URL for this bounty on huntr.dev. This is compulsory and will help us process your bounty submission quicker.
Bounty URL: https://www.huntr.dev/app/bounties/open/1-npm-launchpad
⚙️ Description *
The issue arised in multiple locations, so I to validate the type of data the functions were going to use (like the
paths) and since there were somemulti commandsI didn't use theexecFilefunction, which should have had stored many new variables because we wouldn't have been to concatenate and return results that would have been used in a second/third command correlated to the first one executed. In this case I simply made a functionality thatdeletes every quotefrom the variable, making impossible threat thevariables concatenatedas commands, but only asargumentsof the specific command.💻 Technical Description *
The fix has been applied in 3 different ways inside 3 different files, so I'll comment each one.
namevariable isconcatenatedinside the various commands without being sanitized. Since thenameis inside somesingle-quotesit would have been useless split the 3 different commands inside 3 differentexecFilethat would have used more resources to store the content of the singular commands that should be concatenated again ... instead I introduced thesafefunction which deletes thequotesfrom thenamein order to make it to be only an argument not escapable from quotes.Note I've used the
execFilefunction later in this file in the following lines: https://github.com/Mik317/launchpad/blob/master/lib/local/instance.js#L104 and https://github.com/Mik317/launchpad/blob/master/lib/local/instance.js#L110, in order to avoidcommandscould be executed in a dangerous context.2. The 2' issue arised inside the following line: https://github.com/bitovi/launchpad/blob/master/lib/local/version.js#L21
In this case it I used
execFilein order to avoid concatenation of other strings containing dangerous characters.Patched with:
In this case the first part of the
commandis taken ascommand to execute(surely a path since thecommandvariable is made through), while the second part are the
arguments.In this case the
browser path and filenameweren't checked completely, and even if the execution of malicious code would have been possible only if thedefault browserof the victim has a badly craftedfilename, I inserted a check to see if thepath+filenamepointing to the browser is a validpath.The issue has been fixed through this function:
🐛 Proof of Concept (PoC) *
poc.jsfile:node poc.jsHACKEDfile will we created🔥 Proof of Fix (PoF) *
HACKEDfile is NOT created anymore👍 User Acceptance Testing (UAT)
It doesn't introduce any error (at least using the module through the PoC I crafted)
Regards,
Mik