Closed
Bug 1371586
(CVE-2017-7798)
Opened 7 years ago
Closed 7 years ago
XUL injection in StyleEditorUI.jsm
Categories
(DevTools :: Style Editor, defect)
DevTools
Style Editor
Tracking
(firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)
VERIFIED
FIXED
Firefox 56
People
(Reporter: freddy, Assigned: ntim)
References
()
Details
(Keywords: csectype-priv-escalation, regression, sec-critical, Whiteboard: [adv-main55+][adv-esr52.3+][post-critsmash-triage])
Attachments
(1 file, 2 obsolete files)
3.97 KB,
patch
|
gchang
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
To be clear, I am not sure whether this is an actualy security bug. I'm filing this as a bug to facilitate closed access and an asynchronous discussion. In <http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/devtools/client/styleeditor/StyleEditorUI.jsm#908>, the code is as follows: > const minMaxPattern = /(min\-|max\-)(width|height):\s\d+(px)/ig; > const replacement = > "<a href='#' class='media-responsive-mode-toggle'>$&</a>"; > cond.innerHTML = cond.textContent.replace(minMaxPattern, replacement); What I do not understand exactly, is where the variable 'cond' comes from. Is this indeed just text or a raw string from the web pages' CSS rules? I was testing all of this in a separate tab like > data:text/html,<style>@media screen, scr<b>een { body { max-width: 150px; min-width: 9g9px<s>lol; } }</style><h1>hello folks long text but it sounds to me that you'd be the real experts here and guessing on my part is probably not super promising. I've assigned you, ntim, because you are the original author of the code but feel free to change that accordingly if someone else is a better person to answer my question.
Reporter | ||
Comment 1•7 years ago
|
||
str |
Some notes from our fruitful discussion on IRC > cond.innerHTML = cond.textContent.replace() This is not a sanitizer. This turns inactive, escaped markup (e.g., <s>) into active markup. This enables a XUL injection. STR: 1) Visit this URL > data:text/html,<style>@media a, bzzzzbb\<button\>bb\<\/button\>zb, c { body { color: #000; }}</style><h1>hello folks long text 2) Open Devtools, switch to Style Editor 3) Notice how the right-hand media query tab contains media query for a, c and a XUL button in between.
Summary: Investigate potential XSS risk in StyleEditorUI.jsm → XUL injection in StyleEditorUI.jsm
Reporter | ||
Updated•7 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 2•7 years ago
|
||
Needinfoing myself as a reminder to look at this.
Flags: needinfo?(ntim.bugs)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8876146 -
Flags: review?(pbrosset)
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8876146 [details] [diff] [review] Patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not sure. Which older supported branches are affected by this flaw? All supported versions since Firefox 46: ESR 52 and Firefox 53. If not all supported branches, which bug introduced the flaw? Bug 1029371 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? How likely is this patch to cause regressions; how much testing does it need?
Attachment #8876146 -
Flags: sec-approval?
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #4) > Comment on attachment 8876146 [details] [diff] [review] > Patch > > [Security approval request comment] > How easily could an exploit be constructed based on the patch? > Hard. > > Do comments in the patch, the check-in comment, or tests included in the > patch paint a bulls-eye on the security problem? > Not sure. Sorry, to be annoying here. I think building an exploit isn't going to be hard. It's just an XSS after all. The patch, by the way, *does* point a bulls-eye in two ways 1) you replace something innerHTML with something textContent 2) you introduce a feature that talks about safe(r) templating.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Frederik Braun [:freddyb] from comment #5) > (In reply to Tim Nguyen :ntim from comment #4) > > Comment on attachment 8876146 [details] [diff] [review] > > Patch > > > > [Security approval request comment] > > How easily could an exploit be constructed based on the patch? > > Hard. > > > > Do comments in the patch, the check-in comment, or tests included in the > > patch paint a bulls-eye on the security problem? > > Not sure. > > > Sorry, to be annoying here. > I think building an exploit isn't going to be hard. It's just an XSS after > all. Sorry, I understood the question as "How easily could an exploit be constructed after the patch ?" rather than "before the patch"
Reporter | ||
Updated•7 years ago
|
Keywords: sec-high → sec-critical
Comment 7•7 years ago
|
||
Comment on attachment 8876146 [details] [diff] [review] Patch Review of attachment 8876146 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC last week, the code changes here look good to me. I'll let you finalize the discussion related to comment 4, comment 5 and comment 6 with :freddyb before landing, but on my end we're good.
Attachment #8876146 -
Flags: review?(pbrosset) → review+
Reporter | ||
Comment 8•7 years ago
|
||
comment 5 was just a drive-by comment. Al Billings will comment on the sec-approval flag sometime later today.
Comment 9•7 years ago
|
||
This can land on June 26, two weeks into the new cycle, on trunk. We'll want it on Beta and ESR52 once it is on trunk as well.
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox55:
--- → +
tracking-firefox56:
--- → +
tracking-firefox-esr52:
--- → 55+
Updated•7 years ago
|
Whiteboard: [checkin on 6/26]
Updated•7 years ago
|
Attachment #8876146 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Please don't set checkin-needed on bugs that can't land for 2 weeks.
Keywords: checkin-needed
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8876146 [details] [diff] [review] Patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1029371 [User impact if declined]: security risk when opening style editor [Has the fix been verified in Nightly?]: no, it can't land yet [Needs manual test from QE? If yes, steps to reproduce]: No, but I guess extra testing doesn't hurt. [List of other uplifts needed for the feature/fix]: none. [Is the change risky?]: Low [Why is the change risky/not risky?]: the feature itself is covered by automated tests. [String changes made/needed]: nope
Attachment #8876146 -
Flags: approval-mozilla-release?
Attachment #8876146 -
Flags: approval-mozilla-esr52?
Attachment #8876146 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 12•7 years ago
|
||
The patch applies fine (with a slight fuzz) on ESR 52 and release.
Updated•7 years ago
|
Group: core-security → firefox-core-security
Comment 13•7 years ago
|
||
Comment on attachment 8876146 [details] [diff] [review] Patch Let's get this into beta 5 or beta 6 this week. Also OK for ESR. If anyone feels strongly that this should drive a dot release (or be included in one for 54) please let me, jcristau, or gchang know.
Attachment #8876146 -
Flags: approval-mozilla-esr52?
Attachment #8876146 -
Flags: approval-mozilla-esr52+
Attachment #8876146 -
Flags: approval-mozilla-beta?
Attachment #8876146 -
Flags: approval-mozilla-beta+
Comment hidden (obsolete) |
Comment 15•7 years ago
|
||
backout |
Backed out for ESLint no-shadow failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/f98184b0cb93 https://hg.mozilla.org/releases/mozilla-beta/rev/af94af58d1f3 https://treeherder.mozilla.org/logviewer.html#?job_id=109940364&repo=mozilla-beta
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 16•7 years ago
|
||
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 17•7 years ago
|
||
Attachment #8881524 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Attachment #8876146 -
Attachment is obsolete: true
Attachment #8876146 -
Flags: approval-mozilla-release?
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd74a7d185ffe0d5a92403896d4aeb00f8cce358
Keywords: checkin-needed
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8881534 [details] [diff] [review] Fixed eslint failure see comment 11
Attachment #8881534 -
Flags: approval-mozilla-release?
Comment 20•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/dd98933b89b4 https://hg.mozilla.org/releases/mozilla-esr52/rev/d0c92199b9ed
Comment 21•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd74a7d185ff
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 22•7 years ago
|
||
Comment on attachment 8881534 [details] [diff] [review] Fixed eslint failure Per comment #9, Release54-.
Attachment #8881534 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Updated•7 years ago
|
Group: firefox-core-security → core-security-release
Updated•7 years ago
|
Blocks: 1029371
Keywords: regression
Updated•7 years ago
|
Whiteboard: [adv-main55+][adv-esr52.3+]
Updated•7 years ago
|
Alias: CVE-2017-7798
Comment 23•7 years ago
|
||
Hi Freddy, are you able to verify this bug as fixed?
Flags: needinfo?(fbraun)
Whiteboard: [adv-main55+][adv-esr52.3+] → [adv-main55+][adv-esr52.3+][post-critsmash-triage]
Updated•7 years ago
|
Comment 25•7 years ago
|
||
Lowering severity to sec-moderate; it's not critical unless it allows script injection and I keep getting "Unable to run script because scripts are blocked internally"
Comment 26•7 years ago
|
||
The above comment was for bug 1372112. For this bug see attachment 8882983 [details] (misplaced in bug 1372112).
Updated•6 years ago
|
Group: core-security-release
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•