telephone / LookingGlass

A user-friendly PHP Looking Glass
MIT License
1.25k stars 345 forks source link

Improve character escaping #37

Closed devicenull closed 8 years ago

devicenull commented 8 years ago

This method of escaping host IPs is pretty scary:

    // sanitize + remove single quotes
    $host = str_replace('\'', '', filter_var($host, FILTER_SANITIZE_URL));
    // execute command
    $process = proc_open("{$cmd} '{$host}'", $spec, $pipes, null);

Per the PHP docs:

FILTER_SANITIZEURL -> Remove all characters except letters, digits and $-.+!*'(),{}|\^~[]`<>#%";/?:@&=.

The majority of those characters have no business being in a domain name/IP address. I'd suggest checking the host against FILTER_VALIDATE_IP instead... if that doesn't validate, pass it to gethostbyname and use whatever that returns.

Using escapeshellarg instead of just '{$host}' would also be a good move.

I don't see an obvious exploit for this, but that code really stands out to me as suspicious.

telephone commented 8 years ago

If you look at LookingGlass::validate() and it's subsidiary calls, you'll notice it's not that scary. The input is always validated against an IP or URL. The part that you've quoted (FILTER_SANITIZE_URL) is there as a backup.

Using escapeshellarg() isn't necessary, as I've already ensured single quotes are removed (in the section you've quoted). Instead of escaping single quotes via escapeshellarg(), I'm simply removing them.