Security Fix for Remote Code Execution - huntr.dev

huntr-helper commented 4 years ago

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/

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 some multi commands I didn't use the execFile function, 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 that deletes every quote from the variable, making impossible threat the variables concatenated as commands, but only as arguments of the specific command.

💻 Technical Description *

The fix has been applied in 3 different ways inside 3 different files, so I'll comment each one.

The issue arises firstly here: https://github.com/bitovi/launchpad/blob/master/lib/local/instance.js#L12 because of the fact the name variable is concatenated inside the various commands without being sanitized. Since the name is inside some single-quotes it would have been useless split the 3 different commands inside 3 different execFile that would have used more resources to store the content of the singular commands that should be concatenated again ... instead I introduced the safe function which deletes the quotes from the name in order to make it to be only an argument not escapable from quotes.

var safe = function (str) {
   // Avoid quotes makes impossible escape the `multi command` scenario
   return str.replace(/['"]+/g, '');
}

Note I've used the execFile function 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 avoid commands could be executed in a dangerous context.

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 execFile in order to avoid concatenation of other strings containing dangerous characters. Patched with:

    exec(command.split(' ')[0], command.split(' ').slice(1), function(error, stdout) {

In this case the first part of the command is taken as command to execute (surely a path since the command variable is made through

    var command = path.join('"' + __dirname, '..', '..', 'resources', 'ShowVer.exe" "' + browser.command + '"');

), while the second part are the arguments.

The last issue arised here: https://github.com/bitovi/launchpad/blob/master/lib/local/version.js#L50 In this case the browser path and filename weren't checked completely, and even if the execution of malicious code would have been possible only if the default browser of the victim has a badly crafted filename, I inserted a check to see if the path+filename pointing to the browser is a valid path. The issue has been fixed through this function:

// Validate paths supplied by the user in order to avoid "arbitrary command execution"
var validPath = function (filename){
  var filter = /[`!@#$%^&*()_+\-=\[\]{};':"\\|,<>\/?~]/;
  if (filter.test(filename)){
    console.log('\nInvalid characters inside the path to the browser\n');
    return
  }
  return filename;
}

🐛 Proof of Concept (PoC) *

Download the JS library (launchpad)
Go inside the path you downloaded it and make the following poc.js file:

// poc.js
var launchpad = require("./launchpad/lib/local/instance");
var tst = new launchpad.Instance('t', {}, {}, {process:"s'; touch HACKED; # "});
tst.getPid(function(){});

Execute through node poc.js
The HACKED file will we created

🔥 Proof of Fix (PoF) *

Download the fixed version
Use the same POC previously indexed
The HACKED file 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

JamieSlome commented 4 years ago

@daffl - let me know your thoughts! 🍰

frank-dspeed commented 3 years ago

@matthewp @justinbmeyer this should get merged i think it looks pritty good to me.

bmomberger-bitovi commented 2 years ago

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.

bmomberger-bitovi commented 2 years ago

I am seeing the above problem on MacOS 10.15.7 (my dev machine). Other platforms are as yet untested.

bmomberger-bitovi commented 2 years ago

Root cause: Because execFile does not create a subshell, quoting does not work the same way when constructing the command to terminate a process: Not working: execFile('osascript', ['-e', '\'tell', 'application', '"Opera"', 'to', 'quit\'']); // this is the result of splitting the original line on spaces

Also not working: `execFile('osascript', ['-e', '\'tell application' "Opera"' to quit\'']);

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 Syntax Error: A unknown token can’t go here)

Working: execFile('osascript', ['-e', 'tell application "Opera" to quit']

I have made a PR to address this. No further action is required. Thanks for the security fix!

bitovi / launchpad