791597 - (CVE-2017-5417) Dragging and dropping URLs leads to address bar spoofing

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Michal Zalewski]

Demo: http://lcamtuf.coredump.cx/urldrag/

Comment 1

[Daniel Veditz [:dveditz]]

This seems to be related to other bugs that arise because we show the in-progress URL in the addressbar while still showing the old content. Except in this case the old content actually changed (visually) in the meanwhile.

If we did something like Safari's in-urlbar progress meter it would show that the new page was not yet loaded. Or maybe we could hook any page events after the navigation started and revert the URL if any script runs?

Comment 2

[Michal Zalewski]

The Safari approach seems half-assed too, to be honest (though it's better than nothing).

Perhaps some sort of a more explicit "transitional" indicator would make sense (e.g., explicitly show "Navigating from <origin_a> to <origin_b>..." in the address bar)?

FWIW, for most other types of navigation, you defer updating the address bar until you ditch the old document and are ready to start rendering the new one. This can still be abused to some extent (*), but at least, there is no point at which the scripts from the old page are still running and control the docuemnt, and the address bar is showing an unrelated origin. 

(*) The already-rendered view stays on the screen until you have some HTML to speculatively render instead. An evil server that sleeps at the right moment can result in the old but non-working page being shown while the new URL appears above. But other than scaring the user and doing some far-fetched phishing attacks, that doesn't seem too bad.

Comment 3

[Michal Zalewski]

Also note that the transitional indicator as described above would fix a lot of the UI awkwardness that resulted from previous security fixes - for example, that clicking on a link or hitting "back" provides almost no UI feedback, but entering an URL in the address bar does.

Comment 6

[:Gijs (out for now; he/him)]

Attachment: Patch v0.1

This is a bit evil, but we can't call this.handleRevert() because at the point where we run this code, we've started loading new things and so it doesn't work.

This represents a pretty significant change in behaviour, but per the discussion in bug 1296837, I don't see any other reasonable way of fixing this. Having an "activity" indicator in the URL bar as suggested in the comments here feels like a kludge if the URL bar display is still clearly lying and not something the user actually typed and intentionally wanted to load.

Obviously, this fix doesn't address the same vulnerability when manually typing out (or copy-pasting) a URL (instead of drag/drop), but it feels to me like that would be a lot harder to make work, both because (a) it's more actions for the user, so fewer users will complete the 'funnel', and (b) it seems more explicit about what is going on (ie the user is more likely to be suspicious).

I don't think we want to wholesale shift to the IE behaviour because it entails losing the user's typing until the new page starts loading, which we already do sometimes (because bugs) and has been a source of frustration for power users. I'm kind of nervous about making that trade-off here because of the predictable backlash, but I don't see anything else that's reasonable (short of wontfixing this entirely).

Comment 7

[Marco Bonardo [:mak]]

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

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

RE: the paste case. I agree it's not as bad as the D&D case, should probably be evaluated by the security team though. It's definitely more work for the user and less likely to mislead someone, but we have paste&go that removes some of the steps. FWIW fixing paste&go would not be different than fixing this one (http://searchfox.org/mozilla-central/rev/904bf9addd03b03d4cad11b82f19f43d875b7f27/browser/base/content/urlbarBindings.xml#112) so maybe we could evaluate doing that now as a bonus?

Could you write a very simple test here? I'm not blocking on that, but it should not take a lot of time (if it does, just ditch it and land this).
I think It should be feasible by using something like the test-case in bug 1296837 (or an sjs file), synthtesizeDrop and checking the urlbar inputfield value. Just remember our harness cannot reach into the outside world, so it should point to a non-existing example.com:randomport.

Regardless, r=me cause the idea looks simple and valid.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] from comment #7)
> Comment on attachment 8811210 [details] [diff] [review]
> Patch v0.1
> 
> Review of attachment 8811210 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> RE: the paste case. I agree it's not as bad as the D&D case, should probably
> be evaluated by the security team though. It's definitely more work for the
> user and less likely to mislead someone, but we have paste&go that removes
> some of the steps. FWIW fixing paste&go would not be different than fixing
> this one
> (http://searchfox.org/mozilla-central/rev/
> 904bf9addd03b03d4cad11b82f19f43d875b7f27/browser/base/content/urlbarBindings.
> xml#112) so maybe we could evaluate doing that now as a bonus?

I looked at this, but I'm worried about there being effectively no visual response to the user that something new loads. In particular, you won't know *what* you pasted in, so if your clipboard had something other than what you thought, you're going to be confused. Given that there are now web clipboard APIs as well, that might open up other avenues for exploits instead of this one?

(for mitigation, I looked at a way to know when the "old" document is removed in favour of the thing we're loading so we can set the URL ASAP if userTypedValue is still null, but I couldn't find a good notification for that...)

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #8)
> I looked at this, but I'm worried about there being effectively no visual
> response to the user that something new loads. In particular, you won't know
> *what* you pasted in, so if your clipboard had something other than what you
> thought, you're going to be confused.

If I understand correctly, your concern is that in the dnd case you see what you are dragging and in any case is a non-interruptible operation. Instead in the D&D case, being by its own nature async and interruptible, you may not be able to tell what you are pasting.
In any case, you would see the old address and the throbber, right? so you would know "something" is loading. You wouldn't know what.
Actually when dragging a link the only indicator of what you are dragging may be the "statusbar" tooltip url. but I see how the 2 operations have different timelines and thus d&d looks less confusing.
I'm fine leaving the paste case separate, probably worth filing its own bug and if we don't want to handle it, should just be wontfixed.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > I looked at this, but I'm worried about there being effectively no visual
> > response to the user that something new loads. In particular, you won't know
> > *what* you pasted in, so if your clipboard had something other than what you
> > thought, you're going to be confused.
> 
> If I understand correctly, your concern is that in the dnd case you see what
> you are dragging and in any case is a non-interruptible operation. Instead
> in the D&D case, being by its own nature async and interruptible, you may
> not be able to tell what you are pasting.

Correct, though the page can manipulate the drag data...

> In any case, you would see the old address and the throbber, right? so you
> would know "something" is loading. You wouldn't know what.

Yes.

> Actually when dragging a link the only indicator of what you are dragging
> may be the "statusbar" tooltip url. but I see how the 2 operations have
> different timelines and thus d&d looks less confusing.

Right. I'm also less concerned with there being less feedback, I guess.

> I'm fine leaving the paste case separate, probably worth filing its own bug
> and if we don't want to handle it, should just be wontfixed.

Will do.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6036fb9dca03914f3c6466e34d70d231487108

Comment 11

[:Gijs (out for now; he/him)]

Filed bug 1323452 for the paste and go case (which has many more steps to exploit and is stranger than "drag this thing", so marked it sec-low and left open).

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/3f6036fb9dca
https://hg.mozilla.org/mozilla-central/rev/b52932a0811b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider taking on 52 ahead of the next ESR as well?

Comment 14

[:Gijs (out for now; he/him)]

Attachment: Patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: it's possible to spoof users by convincing them to drag and drop URLs into the location bar that take a long time to load.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet, but this should be straightforward
[Needs manual test from QE? If yes, steps to reproduce]: Up to QE. There are automated tests, but if we want a manual test, it's pretty easy:

1. open http://lcamtuf.coredump.cx/urldrag/
2. drag the link in that page to the address bar and drop it there

Expected: url bar reverts to the URL on the lcamtuf.coredump.cx domain, rather than showing the dropped link (which is a custom port on a google.com domain)

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: it's reasonably low-risk
[Why is the change risky/not risky?]: it's a minor JS change (2 lines of code), comes with automated tests, and I'm not aware of any regression bugs having been filed about it, and we still have ample betas left to test it and back it out if we do find problems.
[String changes made/needed]: nope

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8834370 [details] [diff] [review]
Patch for beta

fix address bar spoofing issue in beta52

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b180354560dd

Comment 17

[Andrei Vaida [:avaida]]

Flagging this for verification, instructions in Comment 14.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/b180354560dd

Comment 19

[Andrei Vaida [:avaida]]

Reproduced the issue on an affected build (50.1.0, 20161208153507) using the str from Comment 14 on Ubuntu 16.04 x64.

This is verified fixed on Ubuntu 16.04 x64, Windows 10 x64 and macOS 10.12.3 using:

    - 52.0b8-build1 (20170220070057),
    - 53.0a2 (2017-02-23),

the Location Bar displays lcamtuf.coredump.cx domain after drag and drop.