Closed Bug 1189082 (CVE-2016-1942) Opened 5 years ago Closed 4 years ago

location bar continues displaying wyciwyg URI and resource URI if user tries to navigate to it manually

Categories

(Firefox :: Address Bar, defect)

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: jordi.chancel, Assigned: Gijs)

References

(Blocks 1 open bug, )

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [post-critsmash-triage][adv-main44+])

Attachments

(4 files, 2 obsolete files)

Attached file TESTCASE1.html (obsolete) —
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.
Attached file TESTCASE v1.1.html (obsolete) —
Attachment #8640743 - Attachment is obsolete: true
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?
Flags: needinfo?(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
Flags: needinfo?(jordi.chancel)
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
Flags: needinfo?(gijskruitbosch+bugs)
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Location Bar
Ever confirmed: true
Flags: needinfo?(gijskruitbosch+bugs)
Summary: Location Bar Spoofing using the copy of a link with the protocol wyciwyg and a right click into the location bar and click on Past and Go → Location Bar Spoofing: location bar continues displaying wyciwyg URI if user tries to navigate to it manually
Group: core-security → firefox-core-security
Flags: needinfo?(mwobensmith)
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).
Flags: sec-bounty?
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.
Flags: sec-bounty? → sec-bounty-
Attached patch Patch v0.1Splinter Review
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.
Attachment #8664685 - Flags: review?(mak77)
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.
Attachment #8664685 - Flags: review?(mak77)
(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.
Flags: needinfo?(mak77)
Oh ok, I didn't look at the test code, and I should have. If it doesn't load then it makes more sense.
Flags: needinfo?(mak77)
Attachment #8664685 - Flags: review?(mak77)
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.
Attachment #8664685 - Flags: review?(mak77) → review+
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/5ee6279df679
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1209591
Group: firefox-core-security → core-security-release
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?
Flags: needinfo?(mak77)
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Gijs, not sure why, but I seem to still be able to reproduce the bugs according to comment 0 in current Nightly...
Flags: needinfo?(mak77)
Flags: needinfo?(gijskruitbosch+bugs)
(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" ?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mak77)
(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...
Flags: needinfo?(mak77)
(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.
Blocks: 1217387
Attachment #8641034 - Attachment is obsolete: true
Summary: Location Bar Spoofing: location bar continues displaying wyciwyg URI if user tries to navigate to it manually → Location Bar Spoofing: location bar continues displaying wyciwyg URI and resource URI if user tries to navigate to it manually
Summary: Location Bar Spoofing: location bar continues displaying wyciwyg URI and resource URI if user tries to navigate to it manually → Location Bar Spoofing: location bar continues displaying wyciwyg URI (eg: wyciwyg://www.google.com/ ) and resource URI (eg: resource://www.google.com/ ) if user tries to navigate to it manually
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+]
Alias: CVE-2016-1942
Summary: Location Bar Spoofing: location bar continues displaying wyciwyg URI (eg: wyciwyg://www.google.com/ ) and resource URI (eg: resource://www.google.com/ ) if user tries to navigate to it manually → location bar continues displaying wyciwyg URI and resource URI if user tries to navigate to it manually
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.