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

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Abdulrahman Alqabandi]

Attachment: 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/

Comment 1

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 2

[Jonathan Kingston [:jkt] he/him]

Looking into this.

Interestingly I can't get the PoC to work in a container which is odd.

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Daniel Veditz [:dveditz]]

(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.

Comment 5

[Jonathan Kingston [:jkt] he/him]

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.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1465160.patch

Comment 7

[Jonathan Kingston [:jkt] he/him]

Actually that patch isn't enough...

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

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

Comment 8

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1465160.patch

Comment 9

[Jonathan Kingston [:jkt] he/him]

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

Issue still exists on this patch.

Comment 10

[Jonathan Kingston [:jkt] he/him]

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

Comment 11

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1465160.patch

Comment 12

[Abdulrahman Alqabandi]

Attachment: 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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Jonathan Kingston [:jkt] he/him]

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.

Comment 15

[Jonathan Kingston [:jkt] he/him]

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.

Comment 16

[Abdulrahman Alqabandi]

Attachment: 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?

Comment 17

[Jonathan Kingston [:jkt] he/him]

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.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 19

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1465160.patch

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});

Comment 20

[Boris Zbarsky [: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?

Comment 21

[Abdulrahman Alqabandi]

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


<video controls="">
  <source src="javascript:console.dir(this)" type="video/mp4">
</video>

Comment 22

[Dão Gottwald [:dao]]

(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 23

[Dão Gottwald [:dao]]

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?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 25

[Jonathan Kingston [:jkt] he/him]

> 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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

As a sec-high affecting more than just trunk, this patch needs sec-approval before it can land.

Comment 27

[Jonathan Kingston [:jkt] he/him]

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.

Comment 28

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

(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.

Comment 29

[Abdulrahman Alqabandi]

(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'.

Comment 30

[Jonathan Kingston [:jkt] he/him]

> 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.

Comment 31

[:Gijs (he/him)]

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.

Comment 32

[Jonathan Kingston [:jkt] he/him]

Removing flag to add back in the check for Pocket.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Ah, bug 1466801 tracks that.

Comment 35

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1465160.patch

Comment 36

[:Gijs (he/him)]

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...).

Comment 37

[Jonathan Kingston [:jkt] he/him]

Attachment: bug-1465160.patch

Comment 38

[Jonathan Kingston [:jkt] he/him]

Sec approval should be fine from Al's previous comment 28.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21a2d648e367c61d60655974dff7abc803f09029

Comment 40

[Abdulrahman Alqabandi]

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?

Comment 41

[Jonathan Kingston [:jkt] he/him]

> 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.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21a2d648e367

Please request Beta approval on this when you get a chance.

Comment 43

[Jonathan Kingston [:jkt] he/him]

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

Comment 44

[:Gijs (he/him)]

(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 45

[Ryan VanderMeulen [:RyanVM]]

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

Fixes a uXSS introduced during the Fx61 cycle. Approved for 61.0b12.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d9d187e5b04

Comment 47

[Rares Doghi, Desktop QA]

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.

Comment 48

[Daniel Veditz [:dveditz]]

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