Security Fix for Command Injection & Arbitrary File Read - huntr.dev

[huntr-helper]

https://huntr.dev/app/users/mufeedvh has fixed the Command Injection vulnerability 🔨. mufeedvh 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#2
GitHub Issue URL | #68
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/curlrequest/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-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

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.

[AdamGold]

Hi, this was not a command injection in the first place -- When running the attached POC (which, btw, is a PoC made by the Snyk Security Team) in a production environment, you will notice that the whoami command isn't being executed. It only appears this way when debugging. Anyway, there IS actually a file arbitrary vulnerability in this package: https://snyk.io/vuln/SNYK-JS-CURLREQUEST-568274

[JamieSlome]

Hey @AdamGold, thanks for that! 👍

If you would like to help the open-source community, why not disclose and fix this vulnerability through us on https://huntr.dev & get rewarded! 🥇

I look forward to reviewing your contributions!

[JamieSlome]

Hey @AdamGold - just a heads up. We have now opened a bounty for the vulnerability (Arbitrary File Read - https://www.huntr.dev/app/bounties/open/2-npm-curlrequest).

We have also opened a GitHub Issue (#70) to track a code fix coming soon.

Cheers! 🍰

[huntr-helper]

https://huntr.dev/app/users/Hbkhan has fixed the Arbitrary File Read vulnerability 🔨. @Hbkhan 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#4
GitHub Issue URL | #70
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/curlrequest/2/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/2-npm-curlrequest

⚙️ Description *

Affected versions of this package are vulnerable to Arbitrary File Read. It is possible to read any file by populating the file parameter with user input. The proposed fix will only allow the user to read files inside the current directory

💻 Technical Description *

According to the documentation file is defined as:

file - default: false
Open a file and process it like a request response, useful if using temporary files.

In the provided PoC the vulnerability exists in L239.

The vulnerability allows users to read files outside of the current directory. To fix that I set root directory which will always be the current directory in which the program exists and then join it with any file name requested by the user. In case if the root directory doesn't match the program will exit.

+  var rootDirectory = path.resolve(process.cwd(), './');
+  var filename = path.join(rootDirectory, options.file);
+  if (filename.indexOf(rootDirectory) !== 0) {
+	// trying to sneak out of the root directory?
+	return
+    }

So now if the filename contains an absolute path and doesn't contain .. sequences anymore - path.join takes care of that. To make sure that user input doesn't contain ( ../ ) or any other character to break out of the current directory / read any system files, I added a check to confirm the filename starts with rootDirectory and reject any if it's outside of current directory

What about /etc/passwd/?
It will join that with the current directory (instead of actually calling /etc/passwd)
e.g if your current directory is /tmp/ it will append that to the current (rootDirectory) and it will become /tmp/etc/passwd

🐛 Proof of Concept (PoC) *

// poc.js
var curl = require("curlrequest");

let userPayload = "/etc/passwd";
curl.request({ file: userPayload }, function (err, stdout, meta) {
    console.log("%s %s", meta.cmd, meta.args.join(" "));
});

// poc2.js
var curl = require("curlrequest");

let userPayload = "../etc/passwd";
curl.request({ file: userPayload }, function (err, stdout, meta) {
    console.log("%s %s", meta.cmd, meta.args.join(" "));
});

🔥 Proof of Fix (PoF) *

👍 User Acceptance Testing (UAT)

../
..\
..\/
%2e%2e%2f
%252e%252e%252f
%c0%ae%c0%ae%c0%af
%uff0e%uff0e%u2215
%uff0e%uff0e%u2216
/etc/issue
/etc/passwd

[JamieSlome]

@AdamGold - please see the fix to the mentioned vulnerability previously added to this pull request.

@weisjohn - would love to get your thoughts! 🍰

[weisjohn]

@JamieSlome LGTM, but I don't have contrib on this repo? I last used this project in 2013. I think you want @chriso?

[JamieSlome]

@chriso - any thoughts on this? Happy to answer any questions!

Cheers! 🍰