Remote Code Execution vulnerability

[vvanmol]

Hello,

I haven't seen any open issue about this so I'm creating this ticket, but Snyk reported a vulnerability about this package.
Please find more details about it here: https://snyk.io/vuln/SNYK-JS-LOCUTUS-575119

[kvz]

Is this the issue that needs fixing? #395

[vvanmol]

Seems to be related to the same function, but not the same issue. Here it's about sanitizing two single quotes characters as explained in this Blog post: https://reallinkers.github.io/CVE-2020-13619/

The following code seems to exploit the vulnerability (taken from the node sample in the blog post above):

var phpExec = require('locutus/php/exec');
var child = require('child_process');

var directory = "/home'; whoami;''"
var command = "ls -la " + phpExec.escapeshellarg(directory);
console.log(command);

child.exec(command, function (error,stdout,stderr) {
	console.log(stdout);
	console.log(stderr);
});

[divinity76]

here is how i would implement it personally,

function escapeshellarg(arg){
  if(arg.indexOf("\x00") !== -1) {
    throw new Error('escapeshellarg(): Argument #1 ($arg) must not contain any null bytes');
  }
  return "'"+arg.replace(/\'/g,'\'\\\'\'')+"'";
}

for one, it's much closer to how PHP's escapeshellarg() works, and second, it's not vulnerable to the above shell execution exploit, correctly generating the command

$ ls -la '/home'\''; whoami;'\'''\'''
ls: cannot access '/home'\''; whoami;'\'''\''': No such file or directory

$ ls -la ' starts the quote, /home' ends the quote, \' adds an escaped quote, ' starts the quote, ; whoami;' ends the quote, \' adds an escaped quote, ' starts the quote, ' ends the quote, \' adds an escaped quote, and ' starts the last quote required to make the always-appended last ' correctly end the quote :)

(it's technically possible to create a smaller command with the same data, \'\' and ''\'''\''' compiles to the exact same string, but given the security-sensitive aspect if we get the optimization wrong, and the fact that even the php core developers didn't try to optimize this part, we probably shouldn't try to do that either.)

[divinity76]

proposed a fix in #426

[divinity76]

uhm.. guys? can i get some feedback? (the PR has been completely silent for 10 days now~)

[vvanmol]

Hi @divinity76

Your solution seems to be good, and you confirmed it fixes the vulnerability. Not sure I can do anything from my side, but waiting for maintainers...

[kvz]

Thank you! Merged and released as locutus@2.0.13