402287 - register{Protocol,Content}Handler should only be allowed from same host as handler

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This is to avoid the scenario where a user is tricked into setting up a handler to a major site that goes through a redirector for information stealing purposes.

Requesting blocking-1.9?

Comment 2

[Robert Sayre]

Not a regression, registerContentHandler is on branch.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Requesting reconsideration for, at the very least, wanted+.  The reasoning here that it's hard to add restrictions post facto if people come to depend on not having them, whereas it's easier to start conservative and remove restrictions later.

Comment 4

[Florian Quèze [:florian]]

If we really want to add this restriction, we could probably add a pref to switch that restriction off so that we can still use our curent testcases.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Florian also pointed out to me at lunch that this policy doesn't prevent the phishing/redirection attack we had in mind: if someone is able to convince a user to accept their own site setting up a handler that redirects through them to Google Mail, they'll just host the registration on the same site as redirector.

In the interested of starting off this feature from a generally conservative posture, I still think this probably deserves wanted+, but it's not as important as I thought.

Comment 6

[Jesse Ruderman]

That's why I want the hostname to be displayed in the pick-a-handler dialog.

Comment 7

[Daniel Veditz [:dveditz]]

If we had this kind of restriction on adding search plugins that feature would be a lot less useful. No mycroft.mozdev.org :-(

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Since we don't know of any attacks that this would prevent, I assert that this is not blocking+, and at most wanted+.  Setting back to blocking? for re-evaluation.

Comment 9

[Jesse Ruderman]

I thought comment 0 described the attack pretty clearly.  This needs to be fixed for Firefox 3 because otherwise sites will start relying on being able to do it the unsafe way.

Comment 10

[Florian Quèze [:florian]]

(In reply to comment #9)
> I thought comment 0 described the attack pretty clearly.

This restriction would not protect against the attack described in comment 0.
See comment 5.

Comment 11

[Jesse Ruderman]

It would protect against that attack as long as the hostname is displayed in the pick-an-application dialog.

Comment 12

[Florian Quèze [:florian]]

(In reply to comment #11)
> It would protect against that attack as long as the hostname is displayed in
> the pick-an-application dialog.
> 

This is bug 402620.

Comment 13

[Mike Schroepfer]

Based on comment #12 taking off blocking list..

Comment 14

[Jesse Ruderman]

Schrep, you might have misread comment 12.  Both bugs need to be fixed to prevent malicious sites from suggesting bogus (e.g. redirecting) URLs at mail.google.com as mailto: handlers.

Comment 15

[Florian Quèze [:florian]]

(In reply to comment #14)
> Schrep, you might have misread comment 12.  Both bugs need to be fixed to
> prevent malicious sites from suggesting bogus (e.g. redirecting) URLs at
> mail.google.com as mailto: handlers.
> 

No.
If evil.com wants to register a redirecting handler to mail.google.com, the url of the handler will be http://evil.com/something, and registerProtocolHandler will be called from evil.com
Fixing this bug would not prevent anything in this attack. We should probably WONTFIX it.

Maybe you mean "a protocol handler should not be allowed to redirect to a different host" but it's not what this bug is about.

Comment 16

[Jesse Ruderman]

I'm talking about redirecting the other way: evil.com registers a protocol handler pointing to https://mail.google.com/redirect?url=http://evil.com/collector, and Firefox shows mail.google.com as the host for the handler.

Comment 17

[Florian Quèze [:florian]]

Are there really some urls like https://mail.google.com/redirect?url=%s that work and are hosted with the same hostname as a useful webapp?

The only redirector I could find on a google website is http://www.google.com/url?q=%s and there are a lot of other parameters to check (probably with session data) that the redirecting page is trusted. If the check fails a warning is displayed: http://www.google.com/url?q=http://evil.com

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

I didn't realize what Jesse meant either.  I'm of two minds about this bug; do we have any sort of data or even gut feel about whether this actually happens in the wild?  It seems to be arguably a web-site bug.

Comment 19

[Jesse Ruderman]

Open redirects and "open framing" are very common and are not usually considered to be vulnerabilities.  See http://sla.ckers.org/forum/read.php?3,505,10958 for tons of examples.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

OK, ick.  This should indeed block.

Comment 21

[Florian Quèze [:florian]]

Attachment: wip

Adds the restriction with a hidden pref to disable it so that it doesn't bother us while testing the feature.

Comment 22

[Jeff Walden [:Waldo]]

(In reply to comment #21)
> Adds the restriction with a hidden pref to disable it so that it doesn't bother
> us while testing the feature.

Mochitest gives you the ability to run tests on any of a set of arbitrary domains, or to run tests on a single domain (see the Mochitest page on MDC).  I don't see why a hidden pref is necessary here -- you should have more than enough control over this for automated tests to not need this.  Further, you shouldn't be writing ad-hoc tests instead of automated tests, because ad-hoc tests lose their utility after being used, whereas automated ones are useful essentially forever.

Comment 23

[Florian Quèze [:florian]]

By "while testing the feature" I meant while we finish the UI for registering a protocol/content handler. This restriction will basically break all the testcases we currently have.

And btw, this restriction sucks. We need to have it by default, but some advanced users who are able to read the url and check themselves that there is not a redirection hidden in it may want to disable it.

Comment 24

[Florian Quèze [:florian]]

Attachment: patch

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Florian and I had a bit of discussion about whether we should loosen the restriction here to allow the entire eTLD+1 by default rather than just the hostname.

My gut is that we should stay conservative and keep with the hostname and liberalize as necessary.  That said, my feeling about it isn't that strong.  One question I have is about what level of certainty the eTLD+1 service gives us; does it ever make mistakes?

Other thoughts?  dveditz?  jesse?

Comment 26

[Jesse Ruderman]

My gut feeling matches dmose's.  We shouldn't use eTLD+1 matches for everything.

Comment 27

[Jesse Ruderman]

See also bug 413567, "Figure out how we should display hostnames in the protocol handling dialog".

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 290606 [details] [diff] [review]
patch

>Index: browser/components/feeds/src/WebContentConverter.js

>+    // We also reject handlers registered from a different host (see bug 402287)
>+    // The pref allows us to test the feature
>+    var pb = Cc["@mozilla.org/preferences-service;1"].
>+             getService(Ci.nsIPrefService).getBranch(null);

Could just getService(Ci.nsIPrefBranch) and remove the call to getBranch(), the pref service QIs to nsIPrefBranch and forwards calls to the root branch.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpref/src/nsPrefService.h&rev=1.25&mark=51,61,62#51

>+    if ((!pb.prefHasUserValue(PREF_ALLOW_DIFFERENT_HOST) ||
>+         !pb.getBoolPref(PREF_ALLOW_DIFFERENT_HOST)) &&
>+        aContentWindow.location.host != uri.host)

nsLocation::GetHost() actually returns the URI's hostPort, so perhaps you should use location.hostName instead (which returns just .host)? I'm not sure whether you want to actually be comparing origins rather than just the host's name, but I guess the host name is probably sufficient.

>+      throw("Permision denied to add " + uri.spec + " as a content or protocol handler");

Comment 29

[Florian Quèze [:florian]]

Attachment: patch (ready for checkin)

Tests will be added in bug 402788.

Comment 30

[Florian Quèze [:florian]]

Checking in browser/components/feeds/src/WebContentConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/WebContentConverter.js,v  <--  WebContentConverter.js
new revision: 1.26; previous revision: 1.25
done
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.275; previous revision: 1.274
done

Comment 31

[Phil Ringnalda (:philor)]

Needs a touch of dev-doc, since I wound up having to read the code to find out why I suddenly couldn't use it while not doing anything devmo told me not to do.

Comment 32

[Eric Shepherd [:sheppy]]

Fixed.

Comment 33

[Heather Arthur [:harth]]

Attachment: test case in mochitest for this bug

Comment 34

[Florian Quèze [:florian]]

(In reply to comment #33)
> Created an attachment (id=323108) [details]
> test case in mochitest for this bug
> 

I added some tests for this bug as part of the patch for bug 402788. Did you see these? Was there something that wasn't tested right?

Comment 35

[cmtalbert]

(In reply to comment #34)
> (In reply to comment #33)
> > Created an attachment (id=323108) [details] [details]
> > test case in mochitest for this bug
> > 
> 
> I added some tests for this bug as part of the patch for bug 402788. Did you
> see these? Was there something that wasn't tested right?
> 
Ah, this would be my fault.  Heather is just starting out with test development.  I directed her to do this test as I was unaware of the ones you had written, Florian.

Comment 36

[Justin Dolske [:Dolske]]

Comment on attachment 323108 [details] [diff] [review]
test case in mochitest for this bug

Clearing review, since there already seem to be tests for this.