1372112 - (CVE-2017-7795) XUL Injection in Inspector Image Tooltip

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Frederik Braun [:freddy]]

Attachment: proof of concept

ImageToolTipHelper.js uses innerHTML to create tooltips.
While doing this, it does not properly encode image URLs, which can lead to chrome privileged XSS.

STR:
1) Open attached file
2) right-click broken image and select "Inspect Element" to open DevTools
3) highlight the underlined image src attribute
4) the toolbox contains XUL markup supplied by the website (here: a harmless <button> element)

I suggest we fix this by using encodeURI(imageURL) instead of imageURL.

Comment 1

[Frederik Braun [:freddy]]

The vulnerability is in 
http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/devtools/client/shared/widgets/tooltip/ImageTooltipHelper.js#98

Let me quote:

> let html = `
>    <div style="flex: 1;
>                display: flex;
>                padding: ${IMAGE_PADDING}px;
>                align-items: center;
>                justify-content: center;
>                min-height: 1px;">
>      <img class="${imageClass}"
>           style="height: ${imgHeight}px; max-height: 100%;"
>           src="${imageUrl}"/>
>    </div>`;
...
>  div.innerHTML = html;

Causing the injection is the imageURL variable.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: bug1372112.patch

Comment 3

[Julian Descottes [:jdescottes]]

Simply following the suggestion from freddyb and calling encodeURI on the image source.

Comment 4

[Julian Descottes [:jdescottes]]

The code updated here comes from Bug 1266448 (https://hg.mozilla.org/mozilla-central/rev/a41f8a55229c) which landed in Firefox 49.

Comment 5

[Frederik Braun [:freddy]]

Comment on attachment 8876668 [details] [diff] [review]
bug1372112.patch

Review of attachment 8876668 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, I already tested this. Consider this patch reviewed from a security perspective.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8876668 [details] [diff] [review]
bug1372112.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Crafting a page exploiting this is easy (see attached example). Requires the user to use devtools to be impacted by the issue.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No test added/comment added to try and land this asap. I supposed we can open a follow up sec bug to add tests?

Which older supported branches are affected by this flaw?
All versions of firefox up to 49 are affected, so:
- central
- beta
- release
- esr-52

If not all supported branches, which bug introduced the flaw?
Bug 1266448

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backports are not ready yet, but it is a simple one-liner on code that hasn't changed much since ff49.

How likely is this patch to cause regressions; how much testing does it need?
This is a minor change, regressions are very unlikely.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8876668 [details] [diff] [review]
bug1372112.patch

Review of attachment 8876668 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Julian! Looks good to me.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for checkin on June 26, two weeks into the new cycle. We'll want patches nominated to go into Beta and ESR52 once it lands on trunk as well.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8876668 [details] [diff] [review]
bug1372112.patch

Approval Request Comment

[Feature/Bug causing the regression]: Bug 1266448 (fixed in FF49)

[User impact if declined]: security issue for devtools users

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: no

[Needs manual test from QE? If yes, steps to reproduce]: not sure what is the usual process here, but if QE verification is done on security bugs, you can follow the STRs:
- open the attachment 'proof of concept' in Firefox
- open the inspector
- in the markup view, hover with the mouse on the image-url
- check that the tooltip does not contain a button (it should only display a broken image preview)  

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: no

[Why is the change risky/not risky?]: one-liner javascript change, devtools only

[String changes made/needed]: none

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: bug1372112.esr52.patch

The existing patch does not apply cleanly on esr52. It's still the same fix, lines just moved between 52 and 55.

Freddy: Since this is a different attachment, should I flag it for sec-review?

Comment 11

[Frederik Braun [:freddy]]

No need, it's very close to being the same thing (and I just checked).

Comment 12

[Liz Henry (:lizzard)]

Comment on attachment 8876668 [details] [diff] [review]
bug1372112.patch

This has sec approval for 6/26 so we can check it in on trunk. 
This might make it to today's beta build but if not, it will be in beta 6 on Thursday.

Comment 13

[Liz Henry (:lizzard)]

Comment on attachment 8877517 [details] [diff] [review]
bug1372112.esr52.patch

Sec-critical, taking this for esr52.3.0.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c66c287c6180759c3bff16f7c5f5a679ae009834
https://hg.mozilla.org/releases/mozilla-beta/rev/aea0295302b4

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c66c287c6180

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/0c8359ac6718

Comment 18

[Matt Wobensmith [:mwobensmith][:matt:]]

Freddy, I was wondering:

a) Has this bug had been fixed as filed? If yes, you could mark verified. If not, let's reopen it and try to get some attention on it.

b) Did you just find a variation for Linux? Is it significant enough to merit its own bug? 

It appears that we are releasing an advisory for Fx 55. If there's more work here, let's make sure it's tracked.

Comment 19

[Frederik Braun [:freddy]]

The bug is fixed, sorry for the confusion.

Comment 21

[Daniel Veditz [:dveditz]]

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 22

[Frederik Braun [:freddy]]

I poked a bit again and I did not get further than <button>i</button> for various reasons
- the payload is in a URL
- the injection happens through an innerHTML assignment, so <script>..</script> won't work
- reading it from the DOM, JavaScript sees the URL with spaces encoded as %20, which limits the HTML payload significantly
- finding a way to execute scripts without <script> tags and with all spaces encoded as %20 limits us greatly
- an obvious avenue for attack would be things like <svg/onload=alert(1)>
- the current document is an XHTML (XUL) document, that parses way stricter than normal HTML
- despite the well-known XHTML strictness that demands proper quoting and ending tags, it also disallows the trick to replace the space that separates the tag name and the attribute with a forward slash

In summary: I'd be amazed to see if someone else gets any farther.

Comment 23

[Frederik Braun [:freddy]]

(In reply to Frederik Braun [:freddyb] from comment #22)
> - reading it from the DOM, JavaScript sees the URL with spaces encoded as
> %20, which limits the HTML payload significantly
> - finding a way to execute scripts without <script> tags and with all spaces
> encoded as %20 limits us greatly
> - an obvious avenue for attack would be things like <svg/onload=alert(1)>

I just also tested \r, \n, \t, and other things listed in https://en.wikipedia.org/wiki/Whitespace_character but no luck.

Comment 24

[Frederik Braun [:freddy]]

Attachment: proof of concept (alerts)

I couldn't let go and found a way to make this execute script with chrome privileges, which raises the severity back to critical.

(The bug is still fixed in the relevant branches, but I wanted to make sure we don't rate it improperly)