Closed Bug 1371586 (CVE-2017-7798) Opened 7 years ago Closed 7 years ago

XUL injection in StyleEditorUI.jsm

Categories

(DevTools :: Style Editor, defect)

defect
Not set
normal

Tracking

(firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 55+ fixed
firefox54 --- wontfix
firefox55 + fixed
firefox56 + fixed

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)

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.
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., &lt;s&gt;) 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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Needinfoing myself as a reminder to look at this.
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(ntim.bugs)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8876146 - Flags: review?(pbrosset)
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?
(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.
(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"
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+
comment 5 was just a drive-by comment. Al Billings will comment on the sec-approval flag sometime later today.
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.
Whiteboard: [checkin on 6/26]
Attachment #8876146 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
Please don't set checkin-needed on bugs that can't land for 2 weeks.
Keywords: checkin-needed
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?
The patch applies fine (with a slight fuzz) on ESR 52 and release.
Group: core-security → firefox-core-security
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+
Attached patch Fixed eslint failure (obsolete) — Splinter Review
Flags: needinfo?(ntim.bugs)
Attachment #8881524 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8876146 - Attachment is obsolete: true
Attachment #8876146 - Flags: approval-mozilla-release?
Comment on attachment 8881534 [details] [diff] [review]
Fixed eslint failure

see comment 11
Attachment #8881534 - Flags: approval-mozilla-release?
https://hg.mozilla.org/mozilla-central/rev/fd74a7d185ff
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment on attachment 8881534 [details] [diff] [review]
Fixed eslint failure

Per comment #9, Release54-.
Attachment #8881534 - Flags: approval-mozilla-release? → approval-mozilla-release-
Group: firefox-core-security → core-security-release
Blocks: 1029371
Keywords: regression
Whiteboard: [adv-main55+][adv-esr52.3+]
Alias: CVE-2017-7798
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]
yep
Status: RESOLVED → VERIFIED
Flags: needinfo?(fbraun)
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"
The above comment was for bug 1372112. For this bug see attachment 8882983 [details] (misplaced in bug 1372112).
Group: core-security-release
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.