1189082 - (CVE-2016-1942) location bar continues displaying wyciwyg URI and resource URI if user tries to navigate to it manually

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Jordi Chancel]

Attachment: TESTCASE1.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150630154324

Steps to reproduce:

When you are on a malicious webpage which contains a link like : 

wyciwyg://https://www.google.com/  , 

copy this link and make a right click into the location bar and click on the selection :"Past and Go" 

All these steps lead to a Location Bar Spoofing Vulnerbility 

Steps with the testcase :
-1 : copy the URL into the link in the testcase webpage, make a right click into the location bar and click "Past and Go".
-2 : after all these steps , Location Bar is Spoofed.


Actual results:

With the wyciwyg:// protocol it's possible to spoof the location bar by a simple interraction like drag and drop a link into the location bar or copy the link URL and try to go on this URL by a right click into the location bar and a click on "Past and Go".


Expected results:

With the wyciwyg:// protocol it's possible to spoof the location.

code a patch of the "past and go" of an url with the protocol wyciwyg:// don't change the URL of the original webpage can be a possible idea to resolve this vulnerbility.

Comment 1

[Jordi Chancel]

Attachment: TESTCASE v1.1.html

Comment 2

[:Gijs (he/him)]

The "wyciwyg" protocol prefix remains in the location bar for me when I use your testcase, and the globe icon remains, well, a globe icon (not a lock or https or anything). Am I missing something?

Comment 3

[Jordi Chancel]

*I think* it is exactly the expected result of this vulnerability/bug which is described in comment0.

i will upload a demonstration video for show you what is the real expected result of the Testcase_v1.1.html for a better understanding of the expected result of this vulnerability/bug .

( I will upload the video quickly (in some hours) and i will advertise you when it is uploaded ).

-Thanks

Comment 4

[Jordi Chancel]

Attachment: Demonstration video.html

Really sorry for the delay.

With this video you can look how the PoC works and what is its effect/impact.

Thanks to validate this security bug and try to say hat is its severity.

Thanks

Comment 5

[:Gijs (he/him)]

I don't think I have the expertise to decide on a security severity here, but it seems like a bug in frontend code for leaving the URI in place when the load fails.

Comment 6

[Jordi Chancel]

Matt Wobensmith , can define the sec-severity and if this report can have the flag : "sec-bounty: ? " , please ?

Thanks.

Comment 7

[Jordi Chancel]

Daniel Veditz, can define the sec-severity of this Vulnerability please?

Thanks.

Comment 8

[Daniel Veditz [:dveditz]]

It really doesn't help when you CC lots of folks irrelevant to the bug -- it just adds to their bugspam level and leads to more and more people turning off bug notifications. Completely counter-productive. If you have questions about the severity rating of a bug or its bounty state it is better to send mail to the security@ list than to clutter up the bug.

Comment 9

[Daniel Veditz [:dveditz]]

FWIW I get the following on the browser (not web) console:

A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Fri Sep 11 2015 09:18:14 GMT-0700 (PDT)
Full Message: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.loadURIWithOptions]
Full Stack: JS frame :: chrome://browser/content/browser.js :: _loadURIWithFlags :: line 12097
JS frame :: chrome://browser/content/tabbrowser.xml :: loadURIWithFlags :: line 6090
JS frame :: chrome://browser/content/tabbrowser.xml :: loadURIWithFlags :: line 3577
JS frame :: chrome://browser/content/utilityOverlay.js :: openLinkIn :: line 343
JS frame :: chrome://browser/content/utilityOverlay.js :: openUILinkIn :: line 203
JS frame :: chrome://browser/content/urlbarBindings.xml :: handleCommand/continueOperation/loadCurrent :: line 350
JS frame :: chrome://browser/content/urlbarBindings.xml :: continueOperation :: line 390
JS frame :: chrome://browser/content/urlbarBindings.xml :: handleCommand/< :: line 332
JS frame :: chrome://browser/content/urlbarBindings.xml :: _canonizeURL/< :: line 457
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 867
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 746
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.scheduleWalkerLoop/< :: line 688
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 browser.js:12097:0

The above line numbers are from a Release (40.0.3) test, they're different in Nightly but the basically same JS stack (Nightly doesn't have the scheduleWalkerLoop one at the end).

Comment 10

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

Minusing this for bounty as a sec-low rated issue. If you can find a better attack based on this, Jordi, please let us know and we will reconsider the rating.

Comment 11

[:Gijs (he/him)]

Attachment: Patch v0.1

Marco, does this make sense? I'm not super-aware of how our location bar code works in regards to stuff like this, and it seems this reverts the typed value only under certain circumstances, so I'm not sure if this is sufficient or if there's something else I should be doing.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8664685 [details] [diff] [review]
Patch v0.1

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

::: browser/base/content/urlbarBindings.xml
@@ +399,5 @@
> +                });
> +              } catch (ex) {
> +                // This load can throw an exception in certain cases, which means
> +                // we'll want to replace the URL with the loaded URL:
> +                this.handleRevert();

The cases where handleRevert() is a no-op should not affect this fix, so it would be fine...

Though, at this point I wonder why openUILinkIn throws, but it still loads a uri. I would expect that it wouldn't throw and load at the same time.
Could you please check what is throwing in _loadURIWithFlags and why? I think something in the first load is throwing, we enter the catch and that throws again. Which of the 2 calls actually causes the load?

Is E10SUtils.canLoadURIInProcess doing the right choice for wysiwyg, do we support those in the chrome process or should we enforce content?
I wonder if there's an e10s bug hiding here, or a bug caused by e10s-ifying this code.

Comment 13

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #12)
> Comment on attachment 8664685 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8664685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/urlbarBindings.xml
> @@ +399,5 @@
> > +                });
> > +              } catch (ex) {
> > +                // This load can throw an exception in certain cases, which means
> > +                // we'll want to replace the URL with the loaded URL:
> > +                this.handleRevert();
> 
> The cases where handleRevert() is a no-op should not affect this fix, so it
> would be fine...
> 
> Though, at this point I wonder why openUILinkIn throws, but it still loads a
> uri.

It doesn't the wyciwyg URL doesn't load, that's why the document in the window stays the same. It throws, and doesn't load anything.

> Could you please check what is throwing in _loadURIWithFlags and why? I
> think something in the first load is throwing, we enter the catch and that
> throws again. Which of the 2 calls actually causes the load?
> 
> Is E10SUtils.canLoadURIInProcess doing the right choice for wysiwyg, do we
> support those in the chrome process or should we enforce content?
> I wonder if there's an e10s bug hiding here, or a bug caused by e10s-ifying
> this code.

I just checked, and this behaves the same way in 29.0, though without an error visible in the error console.

I can look at this in more detail, but at the same time, it seems sensible that if we're trying to load something, and that fails, we should revert the contents of the URL bar if they have been changed (like in paste & go). Either that or open this up and wontfix, because we're effectively doing the same thing that we'd do if you pasted the URL and didn't "go" (or hit enter, or whatever) - we show the user's typed stuff.

Comment 14

[Marco Bonardo [:mak]]

Oh ok, I didn't look at the test code, and I should have. If it doesn't load then it makes more sense.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8664685 [details] [diff] [review]
Patch v0.1

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

Yes, this should do.

I still have some doubts regarding the fact openUILinkIn throws this way, mostly cause we use this API all around, and it is throwing in some code that was added for e10s (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#913). We should likely just handle the failures, after all if loading failed, we can't do miracles.

Comment 16

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/5ee6279df679

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ee6279df679

Comment 18

[Jordi Chancel]

i have found the same vulnerability with resource:// ( resource://www.google.com ) , can i open a new bug report or the fix in this bug resolve this?

Comment 19

[:Gijs (he/him)]

(In reply to Jordi Chancel from comment #18)
> i have found the same vulnerability with resource:// (
> resource://www.google.com ) , can i open a new bug report or the fix in this
> bug resolve this?

I can't reproduce this on Nightly, so I believe the fix here has addressed this.

Comment 20

[Marco Bonardo [:mak]]

Gijs, not sure why, but I seem to still be able to reproduce the bugs according to comment 0 in current Nightly...

Comment 21

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #20)
> Gijs, not sure why, but I seem to still be able to reproduce the bugs
> according to comment 0 in current Nightly...

That's odd; I cannot... are you testing on a clean profile? What site is loaded when you use "paste and go" ?

Comment 22

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Marco Bonardo [::mak] from comment #20)
> > Gijs, not sure why, but I seem to still be able to reproduce the bugs
> > according to comment 0 in current Nightly...
> 
> That's odd; I cannot... are you testing on a clean profile? What site is
> loaded when you use "paste and go" ?

yep, I'm testing with ./mach run, after paste&go I see wyciwyg://https://www.google.com/

Looks like openUILinkIn is not throwing an exception...

Comment 23

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #22)
> (In reply to :Gijs Kruitbosch from comment #21)
> > (In reply to Marco Bonardo [::mak] from comment #20)
> > > Gijs, not sure why, but I seem to still be able to reproduce the bugs
> > > according to comment 0 in current Nightly...
> > 
> > That's odd; I cannot... are you testing on a clean profile? What site is
> > loaded when you use "paste and go" ?
> 
> yep, I'm testing with ./mach run, after paste&go I see
> wyciwyg://https://www.google.com/
> 
> Looks like openUILinkIn is not throwing an exception...

It looks like this is because the patch fixed the non-e10s case but not the e10s case, and ./mach run has e10s enabled (and I don't, it breaks too much stuff). I'll clone this report to fix the e10s case as well.

Comment 24

[Jordi Chancel]

Attachment: PoC n°1 URL Spoofing using wyciwyg URL scheme.html

Comment 25

[Jordi Chancel]

Attachment: PoC n°2 URL Spoofing using resource URL scheme.html