Use CSSOM to insert rules in SelectParentHelper.
Categories
(Core :: Layout: Form Controls, enhancement, P3)
Tracking
()
People
(Reporter: emilio, Assigned: freddy, Mentored)
References
Details
(Keywords: csectype-priv-escalation, csectype-sandbox-escape, sec-high, Whiteboard: [post-critsmash-triage][adv-main70+])
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
It would avoid CSS surprises in IPC messages from a potentially compromised content process.
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Turns out there's another flow from child->parent for the insertRule
call in populateChildren
(currently at line 435).
Assignee | ||
Comment 3•5 years ago
|
||
checkin-needed assuming this is going to be green like its predecessors :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cddf2c44ba44ece99e8e241d511c5b1fecfb289
Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b54dc70e042
Restrict styles to known selectors and single rules per message r=emilio
Comment 5•5 years ago
|
||
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
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?
Assignee | ||
Comment 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
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!
Reporter | ||
Comment 8•5 years ago
|
||
(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 withinlinear-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.
Assignee | ||
Updated•5 years ago
|
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15163b51d8f1
Restrict styles to known selectors and single rules per message r=emilio
Comment 10•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 12•5 years ago
|
||
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•5 years ago
|
||
uplift |
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 14•2 years ago
|
||
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•2 years ago
|
||
Also, because we know this was a sandbox escape, I'm retroactively bumping it to sec-high
Updated•2 years ago
|
Comment 16•2 years ago
|
||
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.
Updated•2 years ago
|
Description
•