1530709 - (CVE-2019-25136) Use CSSOM to insert rules in SelectParentHelper.

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

It would avoid CSS surprises in IPC messages from a potentially compromised content process.

Comment 1

[Frederik Braun [:freddy]]

Attachment: Bug 1530709 - Restrict styles to known selectors and single rules per message r=emilio

Comment 2

[Frederik Braun [:freddy]]

Turns out there's another flow from child->parent for the insertRule call in populateChildren (currently at line 435).

Comment 3

[Frederik Braun [:freddy]]

checkin-needed assuming this is going to be green like its predecessors :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cddf2c44ba44ece99e8e241d511c5b1fecfb289

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b54dc70e042
Restrict styles to known selectors and single rules per message r=emilio

Comment 5

[Arthur Iakab [arthur_iakab]]

Backed out changeset 1b54dc70e042 (bug 1530709) for causing browser-chrome failures on browser_selectpopup_colors.js CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/86eb37e35690fc467ec6a583e7b9681418a14260

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=264770622&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=1b54dc70e04270f601ab01861a15e8836cc57926&searchStr=bc

Failure logs https://treeherder.mozilla.org/logviewer.html#?job_id=264770622&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=264778744&repo=autoland

Frederik can you please take a look?

Comment 6

[Frederik Braun [:freddy]]

One of the failures is probably due to background-image being left out of SUPPORTED_PROPERTIES, when it might in fact be required due to the manual override in https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/toolkit/actors/SelectParent.jsm#104

However, this can't be fixed by just allowing background-image, as the color value might break out of the linear-gradient() function and start a new url() thing. I'll have to think some more on how to fix this.

Comment 7

[Frederik Braun [:freddy]]

OK, so I think this needs to go through a parser to be actually safe from escaping. I tried writing into cssRules[0]["background-color"] and then reading out of it again, to get some meaningful color back, but that actually just returns whatever has been written in. Nothing we can use safely within linear-gradient().

If I had a dummy HTML element, I could assign to dummy.style.color and read out of it again. - That does work.
I fear it could be too costly to do so, so maybe you can think of a smarter thing emilio? I also see you have a lot of pending needinfos. Feel free to redirect me!

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Frederik Braun [:freddyb] from comment #7)

OK, so I think this needs to go through a parser to be actually safe from escaping. I tried writing into cssRules[0]["background-color"] and then reading out of it again, to get some meaningful color back, but that actually just returns whatever has been written in. Nothing we can use safely within linear-gradient().

That is an oversight. cssRules[0]["background-color"] is not the right thing to set. That's just setting a random property in the object, which is why it's not doing the right thing.

You need to use cssRules[0].style["background-color"], which will do what you want.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15163b51d8f1
Restrict styles to known selectors and single rules per message r=emilio

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/15163b51d8f1

Comment 11

[Frederik Braun [:freddy]]

Comment on attachment 9089019 [details]
Bug 1530709 - Restrict styles to known selectors and single rules per message r=emilio

Beta/Release Uplift Approval Request

• User impact if declined: risk of styling bugs in the browser chrome (parent process)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): fix uses a different (safer) API. Functionality & logic is the same. The tests confirm it's working.
• String changes made/needed: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9089019 [details]
Bug 1530709 - Restrict styles to known selectors and single rules per message r=emilio

Fixes a CSS bug which can potentially lead to incorrect styling in the parent process. Approved for 70.0b8.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c97e152648be55d4daef8239bafa288aeb797f0e

Comment 14

[Tom Ritter [:tjr]]

Turns out this was an exploitable vulnerability that was used in the wild. Details are available here: https://blog.google/threat-analysis-group/new-details-on-commercial-spyware-vendor-variston/

Comment 15

[Tom Ritter [:tjr]]

Also, because we know this was a sandbox escape, I'm retroactively bumping it to sec-high

Comment 16

[Daniel Veditz [:dveditz]]

We never established a "regressor" because this was more of a hardening effort than a vulnerability fix when it started. Without knowing the regressor, marking old versions as "unaffected" was premature. If we had known then what we know now we would have pushed to take it on the ESR, but at the time it was not filed as a severe vulnerability and ESR patching is conservative.

According to analysis from the Google TAG (see the "Heliconia Files" section) this was exploited in versions Fx64-Fx68 at least.