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

[huntr-helper]

trafficstars

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 | https://github.com/418sec/cordova-serve/pull/2 GitHub Issue URL | https://github.com/apache/cordova-serve/issues/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

[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 :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.