javascript: URI is triggered when clicking 'view image' opening up old XSS vectors

VERIFIED FIXED in Firefox 61

Status

()

P1
normal
VERIFIED FIXED
10 months ago
8 months ago

People

(Reporter: qab, Assigned: jkt)

Tracking

(Blocks: 1 bug, 4 keywords)

61 Branch
mozilla62
csectype-sop, csectype-spoof, regression, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61+ verified, firefox62+ verified)

Details

(Whiteboard: [domsecurity-active])

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

10 months ago
Posted file imgXss.html
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.181 Safari/537.36

Steps to reproduce:

1. Open attached PoC on latest Nightly build
2. Right click on broken image
3. Click on 'view image'




Actual results:

javascript is executed.


Expected results:

I tested Firefox 60.0b11 and this does not work. This is a regression introducing old XSS vectors. Other context menu options may be affected as well.

See also: https://www.mozilla.org/en-US/security/advisories/mfsa2006-34/
This sounds possibly-bad.

INFO: Last good revision: 70ab5d0f6da37783913e39857edf488ccfc2ab7b
INFO: First bad revision: de33fb39fa4878714120d06ce6a0c72179016be9
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=70ab5d0f6da37783913e39857edf488ccfc2ab7b&tochange=de33fb39fa4878714120d06ce6a0c72179016be9

Christoph, can you please help find an assignee since jkt is on PTO?
Blocks: 1374741
Group: firefox-core-security → dom-core-security
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
status-firefox60: --- → unaffected
status-firefox61: --- → affected
status-firefox62: --- → affected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → unaffected
tracking-firefox61: --- → +
tracking-firefox62: --- → +
Component: Untriaged → DOM: Security
Ever confirmed: true
Flags: needinfo?(ckerschb)
Product: Firefox → Core
Keywords: regression
(Assignee)

Updated

10 months ago
Assignee: nobody → jkt
(Assignee)

Comment 2

10 months ago
Looking into this.

Interestingly I can't get the PoC to work in a container which is odd.
Flags: needinfo?(ckerschb)
I don't know why, but I can't reproduce this on Mac. I can on Windows, though.

The script runs in the privilege of the page, not any kind of elevated privilege. Might be useful if you found some kind of a site with XSS that allowed injecting <IMG> (while correctly blocking more direct XSS vectors) and for some reason didn't check for javascript: src urls.

** May be bypassing CSP inline script checks! **

need to do a better test, but on the bugzilla POC I didn't see a warning from the report-only policy.
Assignee: jkt → nobody
Keywords: csectype-spoof, sec-low
Assignee: nobody → jkt
(In reply to Jonathan Kingston [:jkt] from comment #2)
> Interestingly I can't get the PoC to work in a container which is odd.

Ah -- that's why I couldn't reproduce on Mac. I can reproduce in a default-container tab on Mac.
(Assignee)

Comment 5

10 months ago
Using a null principal instead of this.browser.contentPrincipal appears to work in browser/base/content/nsContextMenu.js viewMedia function.

It's a concerning to me that:
- We don't have tests for this feature at all
- JS executes with the contentPrincipal but not system principal?

It looks to me as if this is working fine with Null here but just making some checks.
(Assignee)

Comment 6

10 months ago
Posted patch bug-1465160.patch (obsolete) — Splinter Review
Attachment #8981680 - Flags: review?(ckerschb)
(Assignee)

Comment 7

10 months ago
Actually that patch isn't enough...

background-image: url("javascript:alert(1)");

Also allows for "View background image" to do the same.
(Assignee)

Comment 8

10 months ago
Posted patch bug-1465160.patch (obsolete) — Splinter Review
Attachment #8981680 - Attachment is obsolete: true
Attachment #8981680 - Flags: review?(ckerschb)
Attachment #8981692 - Flags: review?(ckerschb)
Attachment #8981692 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

10 months ago
Comment on attachment 8981692 [details] [diff] [review]
bug-1465160.patch

Issue still exists on this patch.
Attachment #8981692 - Attachment is obsolete: true
Attachment #8981692 - Flags: review?(ckerschb)
Attachment #8981692 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

10 months ago
Oh and <img src="..." longdesc="javascript:alert(1)" >
(Assignee)

Comment 11

10 months ago
Posted patch bug-1465160.patch (obsolete) — Splinter Review
Attachment #8981742 - Flags: review?(ckerschb)
Attachment #8981742 - Flags: review?(bzbarsky)
(Reporter)

Comment 12

10 months ago
Posted file imgXss.html
Attached is a new PoC for uXSS using this

1. Open new tab or your favorite site
2. Navigate to attached PoC (so that history.back() takes you back to your fav site/about:newtab
3. Right click anywhere
4. Once redirected, click on 'view background image'

This shouldn't work on privileged pages since they don't support javascript: uri. Please let me know if this is a new bug that should be filed seperately.

(In reply to Daniel Veditz [:dveditz] from comment #3)
> I don't know why, but I can't reproduce this on Mac. I can on Windows,
> though.
> 
> Might be useful if you found some kind of a site with XSS that
> allowed injecting <IMG> (while correctly blocking more direct XSS vectors)
> and for some reason didn't check for javascript: src urls.
> 


With comment 7 I think its safe to say this introduces new vectors that most likely wont get caught by current protections. 

An example of a new vector: <style>*{background-image:url('javascript:alert(1)')}</style> 

Also, this does seem to bypass CSP.
(Assignee)

Updated

10 months ago
Blocks: 1465303
Once upon a time, we used to do checkLoadURI with the DISALLOW_INHERIT_PRINCIPAL flag for this sort of stuff.  Did we stop doing that at some point?
(Assignee)

Comment 14

10 months ago
This bug https://bugzilla.mozilla.org/attachment.cgi?id=8454035&action=diff appears to have exempted javascript: urls because they didn't load a tab. I would rather we throw in this function with this combination tbh.
(Assignee)

Comment 15

10 months ago
I'm not sure the PoC in Comment 12 increases the risk at all. Ok the background is a bigger surface area but it doesn't look to be history stuffing or anything like that.
(Reporter)

Comment 16

10 months ago
Posted file xssSetup.html
(In reply to Jonathan Kingston [:jkt] from comment #15)
> I'm not sure the PoC in Comment 12 increases the risk at all. Ok the
> background is a bigger surface area but it doesn't look to be history
> stuffing or anything like that.

uXSS is definitely worse than original report. If you meant the XSS vector then its not that it results in a more severe bug but that it is harder to detect as an XSS vector. My one example is that Chromes XSS auditor blocks <img src=javascript:alert()> but does not block the vector using style tag.

I have attached a helper for comment 12, just open it and follow instructions. This makes the whole uXSS more automated.

Did you file bug 1465303 to cover the uXSS? May I get access to it?
Flags: needinfo?(jkt)
(Assignee)

Comment 17

10 months ago
Sorry I see now, It was super late and I was trying to squash all cases I could find.

I don't think we need another bug here tbh; I'm going to upgrade this to a sec-high as I think it meets the criteria.
Flags: needinfo?(jkt)
Keywords: sec-low → sec-high
Priority: -- → P1
Whiteboard: [domsecurity-active]
Comment on attachment 8981742 [details] [diff] [review]
bug-1465160.patch

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

As Boris pointed out within comment 13, we used to perform explicit security checks which e.g. permitted script execution explicitly. It seems those checks were removed within Bug 1374741, in particular here [1]. Probably it's wise to bring those checks back.

And as you mentioned, we need an automated test for it!

[1] https://hg.mozilla.org/mozilla-central/diff/3182593eb59a/browser/base/content/nsContextMenu.js#l1.61
Attachment #8981742 - Flags: review?(ckerschb)
(Assignee)

Comment 19

10 months ago
Posted patch bug-1465160.patch (obsolete) — Splinter Review
Removing Null principals which also fixed the bug here however may cause breakage. Adding back in these manual checks seems the safest thing to do here. 

In the follow up Bug
1465303 I will add tests and clean up this code which I think should be done inside openUILink methods as an opt in/out param. I will also double check if we can use Null here instead too.

Something like:
openUILink(url, e, {allowScripts: true});
Attachment #8981742 - Attachment is obsolete: true
Attachment #8981742 - Flags: review?(bzbarsky)
Attachment #8982032 - Flags: review?(bzbarsky)
Comment on attachment 8982032 [details] [diff] [review]
bug-1465160.patch

This really needs review from someone familiar with this code.  For example, I can't tell with any degree of assurance what

>+                     this.principal,

is in practice.

That said, I would recomment putting the url/principal in local variables so we ensure that what we pass to urlSecurityCheck stays in sync with what we pass to other functions.  And more context would have helped some of these chunks....

I was going to redirect to Gijs, but he's not accepting reviews.  Dão, can you review this?
Attachment #8982032 - Flags: review?(bzbarsky) → review?(dao+bmo)
(Reporter)

Comment 21

10 months ago
One more that should be taken into account.
Right click->View video


<video controls="">
  <source src="javascript:console.dir(this)" type="video/mp4">
</video>
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #20)
> Comment on attachment 8982032 [details] [diff] [review]
> bug-1465160.patch
> 
> This really needs review from someone familiar with this code.  For example,
> I can't tell with any degree of assurance what
> 
> >+                     this.principal,
> 
> is in practice.

Here is where it comes from:

https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/browser/modules/ContextMenu.jsm#593

> That said, I would recomment putting the url/principal in local variables so
> we ensure that what we pass to urlSecurityCheck stays in sync with what we
> pass to other functions.

It shouldn't be possible for this.principal or this.browser.contentPrincipal to change between the urlSecurityCheck and openUILink calls, but either way it sounds like bug 1465303 (which I cannot access) would address your concern.
Comment on attachment 8982032 [details] [diff] [review]
bug-1465160.patch

>   reloadImage() {
>+    urlSecurityCheck(this.mediaURL,
>+                     this.principal,
>+                     Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
>+
>     this.browser.messageManager.sendAsyncMessage("ContextMenu:ReloadImage",
>                                                  null, { target: this.target });
>   },

What is this good for in the reload image case?
Attachment #8982032 - Flags: review?(dao+bmo) → review+
> It shouldn't be possible for this.principal or this.browser.contentPrincipal to change between the
> urlSecurityCheck and openUILink calls

Sure.  I meant that we should make it harder to accidentally make code changes that just pass different values to the two function calls (via one callsite but not the other being edited).
(Assignee)

Comment 25

10 months ago
> I meant that we should make it harder to accidentally make code changes that just pass different values to the two function calls (via one callsite but not the other being edited).

Yeah the follow up bug can handle this to simplify this code such that this becomes much harder to get wrong in future, I think we should be doing this check as part of openLinkIn etc.

> What is this good for in the reload image case?

I just added back in the original code before the change, I will review if it's needed still in the follow up.
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
As a sec-high affecting more than just trunk, this patch needs sec-approval before it can land.
Flags: needinfo?(jkt)
Keywords: checkin-needed
(Assignee)

Comment 27

10 months ago
Comment on attachment 8982032 [details] [diff] [review]
bug-1465160.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Fairly easy, however I haven't checked the uXSS flaw for if it applies to that window context. If it doesn't then it's just a spoofing risk.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No however the code is somewhat clear about the issue.

Which older supported branches are affected by this flaw?
61-62

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply cleanly however I haven't checked, they shouldn't be risky at all though.

How likely is this patch to cause regressions; how much testing does it need?
Should be near zero chance given the previous code is getting reapplied.
Flags: needinfo?(jkt)
Attachment #8982032 - Flags: sec-approval?
(In reply to Jonathan Kingston [:jkt] from comment #27)
> Comment on attachment 8982032 [details] [diff] [review]
> bug-1465160.patch
> 
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Fairly easy, however I haven't checked the uXSS flaw for if it applies to
> that window context. If it doesn't then it's just a spoofing risk.


That seems relevant to the security rating. Can you check this or is there someone who could?

In either case, sec-approval given since this only affects beta and trunk.
Attachment #8982032 - Flags: sec-approval? → sec-approval+
(Reporter)

Comment 29

10 months ago
(In reply to Al Billings [:abillings] from comment #28)
> 
> That seems relevant to the security rating. Can you check this or is there
> someone who could?
> 


The PoC in comment 16 and comment 12 result in executing 'alert(location)' on cross-origin website 'addons.mozilla.org'.
(Assignee)

Comment 30

10 months ago
> That seems relevant to the security rating. Can you check this or is there someone who could?

As mentioned the location was readable. I verified that the DOM can also be manipulated and read from using this redirection technique.
I think as mentioned elsewhere in the bug, we should perhaps have yet another bug to investigate if we can lock down this uXSS too, such that code from one site wont run across different principals.
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 31

10 months ago
Ugggh. 

FWIW, given the openUILinkIn calls (almost all - why not all?) use disallowInheritPrincipal anyway, I don't understand why we're making the urlSecurityCheck calls use DISALLOW_SCRIPT instead of DISALLOW_INHERIT_PRINCIPAL (which is strictly broader, AIUI).

That said, it's also... surprising... that we have a disallowInheritPrincipal option for openUILink that doesn't actually do that for JS, and we should probably audit for that (and then maybe fix it).

Finally, the changes in bug 1374741 also removed the urlSecurityCheck in pocket code. We should probably add one back inside openTabWithUrl(), even if that isn't exploitable today without compromising pocket content or convincing the user to pocket compromised JS URLs or soemthing like that.
(Assignee)

Comment 32

10 months ago
Removing flag to add back in the check for Pocket.
Keywords: checkin-needed
> it's also... surprising... that we have a disallowInheritPrincipal option for openUILink
> that doesn't actually do that for JS

Er... that's odd, yes.  I just took a quick look at the code, and this should lead to us running the JS with a nullprincipal, assuming it turns into the LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL flag to docshell.

Can we please check whether that flag is failing to get passed or whether it's not working right?
Flags: needinfo?(jkt)
Ah, bug 1466801 tracks that.
Flags: needinfo?(jkt)
(Assignee)

Comment 35

10 months ago
Posted patch bug-1465160.patch (obsolete) — Splinter Review
Attachment #8982032 - Attachment is obsolete: true
Attachment #8983392 - Flags: superreview+
Attachment #8983392 - Flags: review?(gijskruitbosch+bugs)

Comment 36

10 months ago
Comment on attachment 8983392 [details] [diff] [review]
bug-1465160.patch

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

r=me with the frame case fixed to use the content principal.

We may want a non-sec-sensitive followup to give the context (ie this.) principal here a better name.

::: browser/base/content/nsContextMenu.js
@@ +869,5 @@
>      let referrer = gContextMenuContentData.referrer;
>      openWebLinkIn(gContextMenuContentData.docLocation, "current", {
>        disallowInheritPrincipal: true,
>        referrerURI: referrer ? makeURI(referrer) : null,
> +      triggeringPrincipal: this.principal,

This is the only case out of these, AFAICT, where `this.browser.contentPrincipal` is more correct than `this.principal`. `this.principal` comes off context.principal, which comes from ContextMenu.jsm, which runs in the content, and gets it like this:

https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/browser/modules/ContextMenu.jsm#768

    context.principal = context.target.ownerDocument.nodePrincipal;

so in this case, that'll be the node principal of the framed document. Of course that will be allowed to execute the load (unless it's a null principal, maybe...), but really what we should check against is whether the load is allowed from the frame's parent chain - ie was the containing page allowed to frame the contained page - not whether the contained page was allowed to load itself, which is a bit meaningless. We don't have the full chain of principals here, but the toplevel principal (ie the browser's content principal) is our best guess.

I don't think there are many reasonable situations where the distinction would matter in practice (ie how can this node exist, ie be loaded, if it's from principal B and the frame was created by principal A, given that "can-trigger-load-of" is normally transitive (so if A can load B and B can load C, then A must be able to load C anyway)), but we should probably try to do the right thing for the edgecases that do exist (web content isn't the only thing that can create frames and load content in them...).
Attachment #8983392 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 37

10 months ago
Attachment #8983392 - Attachment is obsolete: true
Attachment #8983424 - Flags: review+
(Assignee)

Comment 38

10 months ago
Sec approval should be fine from Al's previous comment 28.
Keywords: checkin-needed
(Reporter)

Comment 40

10 months ago
Please change tag to 'csectype-sop' since I am 100% sure this is a universal XSS and not strictly a spoof. 

As far as a second filing of a bug; I'm trying to figure out what to file exactly? The behavior I am abusing to achieve uXSS can be seen to operate as intended, in that, when the document navigates back to a different origin website from history, the already-open context menu options start applying to the current document (we just history.back'd to) rather than the document the context menu originated from (where we pressed the right mousekey and opened the context menu). 

For example, if you navigate to a.com then navigate to b.com then open the context menu and finally go back to a.com from history. You will have ended up in a.com with a context menu opened from b.com. If you now click on 'View source' you will find that 'view-source:a.com' opened, which is the current location you are in. 

Is that a bug? Are we saying that it should have opened the view source of b.com? I can't see how it can be abused if the view-image xss is fixed. 

One interesting behavior is with anchor tags. If you right click on an anchor tag then after navigation clicked on 'Open Link in New Tab', the link will be opened with the principal of the original document that contained the anchor tag before navigation. This is probably the desired behavior for 'view image'.

So should I file a bug that asks for context menus to be canceled on navigation? Or that context menu actions perform those actions (view source or view image) using the originating (context-menu-opener) windows principal instead of current focused window?
(Assignee)

Updated

10 months ago
Blocks: 1466961
(Assignee)

Comment 41

10 months ago
> Please change tag to 'csectype-sop' since I am 100% sure this is a universal XSS and not strictly a spoof.

I honestly never use these tags. :abillings is this ok? I agree that it fits into this category.


> So should I file a bug that asks for context menus to be canceled on navigation? Or that context menu actions perform those actions (view source or view image) using the originating (context-menu-opener) windows principal instead of current focused window?

I personally think we should hide these context menus as a belt and braces protection against this type of uXSS attack also I just raised Bug 1466961 for this.

> Are we saying that it should have opened the view source of b.com?

Yes exactly that and/or ignore the history.back as part of a prevention against this attack. Even ignoring showing the context menu would be fine as a solution here also given that it's unlikely the user would want to see it.
No longer blocks: 1466961
https://hg.mozilla.org/mozilla-central/rev/21a2d648e367

Please request Beta approval on this when you get a chance.
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: affected → fixed
Flags: needinfo?(jkt)
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(Assignee)

Comment 43

10 months ago
Comment on attachment 8983424 [details] [diff] [review]
bug-1465160.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1374741
[User impact if declined]: a uXSS via javascript: image tags and similar attrs on user interaction.
[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]: POC attached to the bug would verify if the exploit has gone (the POC needs to be downloaded and the URLs modified as bugzilla CSP prevents it from loading. Using file:// paths work here). The POC will alert the URL of the redirected page, if the patch applies correctly then the alert will never fire on using the context menu.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: As this is using the previous code prior to the exploit was caused this should have limited impact on functionality. These functions aren't often changed either.
[String changes made/needed]: N/A
Flags: needinfo?(jkt)
Attachment #8983424 - Flags: approval-mozilla-beta?

Comment 44

10 months ago
(In reply to Jonathan Kingston [:jkt] from comment #41)
> I personally think we should hide these context menus as a belt and braces
> protection against this type of uXSS attack also I just raised Bug 1466961
> for this.

Can you move this bug to a frontend component + sec group and CC me so I can see it? :-)
Comment on attachment 8983424 [details] [diff] [review]
bug-1465160.patch

Fixes a uXSS introduced during the Fx61 cycle. Approved for 61.0b12.
Attachment #8983424 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 47

9 months ago
Hi, i retested this issue in version 61.0b11 and i was able to reproduce it after which i tested version 61.0b12 as well as nightly 62.0a1 (2018-06-08) and it no longer reproduces, i will mark this issue accordingly as verified.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
status-firefox62: fixed → verified
Flags: qe-verify+
Flags: sec-bounty?
This is on the low end of sec-high because of the user-interaction required (would you click "view image" if a page suddenly navigated on you and you didn't ask for it?) but still worth a bounty. The bounty includes (is primarily for) the effects enhanced by the context menu issue that was spun off into bug 1466961
Flags: sec-bounty? → sec-bounty+
Keywords: csectype-sop
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.