Fix for Suspected Vulnerability - External Code Execution - huntr.dev

[huntr-helper]

https://huntr.dev/app/users/Hbkhan has fixed the suspected External Code Execution 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#2
GitHub Issue URL | #36
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/cordova-serve/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-cordova-serve

⚙️ Description *

The cordova-serve module suffers from remote code execution caused by the lack of validating dataDir & URL input arguments before executing the command in #L89. The proposed fix will add a check for both inputs.

💻 Technical Description *

According to cordova-serve documentation dataDir & URL are described as:

dataDir - a data dir to provide to Chrome (can be used to force it to open in a new window)
url - the url to open in the browser

Types of the parameters

@param url: string, dataDir: string

For the dataDir input I added a regex check which will make sure that the input doesn't contain any illegal character (which can result in RCE). If the validation fails it will print an error message

For the URL input I simply used encodeURI function which will encode the input as URL and give it to the chrome. If any extra command were added it will still be considered as a part of URL

🐛 Proof of Concept (PoC) *

node poc.js

// poc.js
const cordovaServe = require('./src/main.js');

var server = cordovaServe();

cordovaServe.launchBrowser({target: "chrome", url: "http://localhost", dataDir: "; touch hbkhan"}).then(
  stdout => {
    console.log(`Browser was launched successfully: ${stdout}`);
  },
  error => {
    console.log(`An error occurred: ${error}`);
  }
);

node poc2.js

// poc2.js
const cordovaServe = require('./src/main.js');
var server = cordovaServe();
cordovaServe.launchBrowser({target: "chrome", url: "http://localhost; touch hbkhan", dataDir: ""}).then(
  stdout => {
    console.log(`Browser was launched successfully: ${stdout}`);
  },
  error => {
    console.log(`An error occurred: ${error}`);
  }
);

🔥 Proof of Fix (PoF) *

node poc.js

// poc.js
const cordovaServe = require('./src/main.js');

var server = cordovaServe();

cordovaServe.launchBrowser({target: "chrome", url: "http://localhost", dataDir: "; touch hbkhan"}).then(
  stdout => {
    console.log(`Browser was launched successfully: ${stdout}`);
  },
  error => {
    console.log(`An error occurred: ${error}`);
  }
);

// poc2.js
const cordovaServe = require('./src/main.js');
var server = cordovaServe();
cordovaServe.launchBrowser({target: "chrome", url: "http://localhost; touch hbkhan", dataDir: ""}).then(
  stdout => {
    console.log(`Browser was launched successfully: ${stdout}`);
  },
  error => {
    console.log(`An error occurred: ${error}`);
  }
);

👍 User Acceptance Testing (UAT)

w'h'o'am'i
w\ho\am\i
echo test >> test
whoami

[breautek]

Couple of notes...

Firstly, it really frustrates me to see security vulnerabilities (however serious) to be publicly reported without even attempting to go the secure channels that Apache has in place. All vulnerabilities should be disclosed through https://www.apache.org/security/

Secondly, I'm not so sure if this is actually a security issue. cordova-serve is a development tool, so any process that access to this (or an attacker that can gain access to this) would already be able to execute commands without through to cordova-serve. See #36 (comment)

Nonetheless, I still think it's a good idea to close behaviour that is obviously not intended. I've tested this fork locally and tests are passing.

[brody4hire]

This change should not be called a security fix, because we do not consider this to be a security vulnerability.

The name of the contributing developer or organization should not appear in the title.

The name of the actual human author, preferably with valid email address, should appear as the author in any commits that we would consider merging.

The proposal seems to be in multiple commits. Considering the relatively small size, I think we should consider merging in a single squash commit.

I think a title such as "avoid external code execution" would be much better.

[JamieSlome]

@brodybits - thanks for your notes! We will make sure in future that our fixers and community members properly sign their commits. Unfortunately, we are not able to disclose their e-mail address for GDPR reasons, but I hope with my signature on the commit, this will suitably meet the expectations for contribution.

We have reviewed your contribution guidelines and we believe that we are inline. Please let us know if we are out of line and we will look to rectify this.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 27.60%. Comparing base (bbe740c) to head (edc0af5).
Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
src/browser.js	20.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
- Coverage   28.11%   27.60%   -0.51%     
==========================================
  Files           6        6              
  Lines         217      221       +4     
==========================================
  Hits           61       61              
- Misses        156      160       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.