Skip to content
This repository has been archived by the owner on Dec 19, 2023. It is now read-only.

Fixed Command Injection #2

Merged
merged 3 commits into from
Jun 17, 2020
Merged

Fixed Command Injection #2

merged 3 commits into from
Jun 17, 2020

Conversation

mufeedvh
Copy link

@mufeedvh mufeedvh commented May 22, 2020

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

⚙️ Description *

curlrequest module suffers from a Command Injection vulnerability caused by the lack of sanitizing the input arguments before executing it.

💻 Technical Description *

curlrequest is a curl utility library/module for Node projects. It covers a variety of features but it works with the command utility after processing the input where every input is turned into a curl command and blindly executed by the spawn() function.

The spawn() function is required here but the cause of this issue is actually because of the lack of sanitization of user input.

To fix the issue, I used a module shell-escape which converts arguments into shell-friendly and safe escaped strings. As the curl command can contain a lot of special characters from URLs, it's not a problem faced with the usage of this module as the suggested example from the documentation showcases the use of the curl command as arguments.

var shellescape = require('shell-escape');
 
var args = ['curl', '-v', '-H', 'Location;', '-H', 'User-Agent: dave#10', 'http://www.daveeddy.com/?name=dave&age=24'];
 
var escaped = shellescape(args);

🐛 Proof of Concept (PoC) *

Place this file under the root directory of the project (poc.js)

var curl = require("./index.js");

let userPayload = ";whoami#";
curl.request({ url: userPayload, pretend: true }, function(err, stdout, meta) {
  console.log("%s %s", meta.cmd, meta.args.join(" "));
});

🔥 Proof of Fix (PoF) *

$ cd curlrequest/
$ npm install
$ node poc.js

curlrequest-fix

As you can see from the above screenshot, the payload didn't get executed.

👍 User Acceptance Testing (UAT)

The POC demonstrates an all-around test of the code. The test also showed that this project uses Buffer() which is deprecated due to security and usability issues. As it poses a security risk, I fixed the issue by moving to Buffer.alloc() which is a safe method to use.

The node console warning:
DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

So the only change pushed to the code is:

  • Buffer() to Buffer.alloc() which also fixes another security issue.
  • Usage of the shell-escape module to sanitize the command arguments to mitigate the Command Injection.

@JamieSlome JamieSlome requested a review from toufik-airane June 9, 2020 14:57
Copy link

@toufik-airane toufik-airane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @mufeedvh,

Some changes in the index.js and package.json seem not related to fixing the security vulnerability.

I think an allow list is a better solution to fix this issue regarding the predictable URL type we are waiting in user input and rely on the package shell-escape to introduce more dependency.

@mufeedvh
Copy link
Author

Hello @toufik-airane,

The other change in the index.js is the replacement of Buffer() with Buffer.alloc() which actually is to prevent a vulnerability as mentioned in the original PR message.

I think an allow list is a better solution to fix this issue regarding the predictable URL type we are waiting in user input and rely on the package shell-escape to introduce more dependency.

The module used here shell-escape doesn't have any dependencies and the code is really efficient and simple.

An allow list would be to only accept the URL if it meets certain criteria but with escaping the input, it can finish the process whatever the input URL looks like and if it's malformed, it would throw the default error.

Copy link

@toufik-airane toufik-airane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huntr

@JamieSlome JamieSlome merged commit 23a8114 into 418sec:master Jun 17, 2020
@huntr-helper
Copy link
Member

Congratulations mufeedvh - 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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants