[Fix] Patched RCE arising inside the "launchpad" library

[Mik317]

📊 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.

1. 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.
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 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.

3. 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) *

1. Download the JS library (launchpad)
2. 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(){});

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

🔥 Proof of Fix (PoF) *

1. Download the fixed version
2. Use the same POC previously indexed
3. 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

[toufik-airane]

I think that the dangerous function exec() should be avoided.

[Mik317]

Hi @toufik-airane :),
I've replaced the exec function with the execFile one in order to avoid problems. I removed also the safe function in the windows case because I saw certain systems won't recognize the string inside as a argument (edge cases covered anyway by execFile now introduced).

Regards,
Mik

[toufik-airane]

Have you run the tests and update the description?

[Mik317]

Yeah, I updated the description of the fixes applied and I retested the new code.
I saw I had missed a ) ... however now it should be fixed well and running the POC I previously used it doesn't lead to RCE or errors due to malformed syntax.

Regards,
Mik

[toufik-airane]

Thank you. 😊

[mufeedvh]

Good fix, just complete this one change request and it's all perfect! 👏🎉

[mufeedvh]

Can you please change this exec() to execFile()

[JamieSlome]

@Mik317 - once you have made the fix - we will get this merged - cheers! 🍰

[Mik317]

Ok, I just updated the fix as requested. In the meanwhile I also applied a minor fix because I had inserted certain characters as blacklisted that however could be part of a path (not only of a filename) :).

Regards,
Mik

[JamieSlome]

@Mik317 - awesome, thanks for making the changes!

[huntr-helper]

Congratulations Mik317 - your fix has been selected! 🎉

Thanks for being part of the community & helping secure the world's open source code.
If you have any questions, please respond in the comments section. Your bounty is on its way - keep hunting!