1199289 - Malformed http-auth like URL may issue a web research.

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mathieu Goessens]

Hi,

Entering http://login:[e-mail] in the address bar issue an http request using the http auth: basic mechanism [1] [2].

As it sends potential user credentials in an known unsecure way, it does only after prompting the user for his/her explicit permission, following a security advice from [3].

However without the http:// , entering login:[e-mail] in the address bar issue a web request to the default search engine, leaking potential credentials to this search engine.

Although login:[e-mail] is not a valid URL, firefox adds an implicit http:// if a user enter example.org. So IHMO it it would be both safer and more in the following of the least surprise principle if it does so for login:[e-mail] instead of issuing a web request.

Tested in firefox 40 Linux amd64.

Other browsers behaviours:

- Google Chrome/Chromium 44 Ubuntu 15.04 redirect both http://a:[e-mail] and a:[e-mail] to example.org. Did not verify if it sends credentials.

- An older chromium version (Debian wheezy's one: chromium 37) has the same behaviour as firefox 40: http://a:[e-mail] issue a request to example.org , a:[e-mail] redirect the request to the search engine.

- Safari 8.0.8 OS 10.10.5 opens a popup for both http://a:[e-mail] and a:[e-mail] saying it does not know how to open the URL.

- curl -v a:[e-mail] issue an http request to example.org with those credentials.


Refs:

[1] https://tools.ietf.org/html/rfc2617
[2] https://tools.ietf.org/html/rfc2616
[3] https://tools.ietf.org/html/rfc1945#section-12.4

[ cc'd Patrick and Valentin from Necko and Sylvestre for Release mgmt.
Bug marked as confidential in doubt and following Sylvestre's Advice ]

Comment 1

[:Gijs (he/him)]

I'm pretty sure this is nsDefaultURIFixup deciding that "loginname" is not a protocol we recognize and therefore deciding you want to search. Implemented in bug 982428.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Maybe if has a "bogus protocol" and a @ we can safely assume it's a user/password and send the user to example.org - maybe we should always do that... even if the user inputs http:[e-mail]

Comment 3

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #2)
> Maybe if has a "bogus protocol" and a @ we can safely assume it's a
> user/password and send the user to example.org - maybe we should always do
> that... even if the user inputs http:[e-mail]

This currently tries to log you in with username "password" and an empty password, but it does go to example.org.

And yes, the @ thing should probably be taken into account here. I was just clarifying that this isn't a networking issue. I'd also offer to work on this, but I'm on PTO all of next week so I likely couldn't see it through.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Valentin Gosu [:valentin] from comment #2)
> > Maybe if has a "bogus protocol" and a @ we can safely assume it's a
> > user/password and send the user to example.org - maybe we should always do
> > that... even if the user inputs http:[e-mail]
> 
> This currently tries to log you in with username "password" and an empty
> password, but it does go to example.org.

Still probably not what you want. The colon has an actual meaning when there's an @ present.

> And yes, the @ thing should probably be taken into account here. I was just
> clarifying that this isn't a networking issue. I'd also offer to work on
> this, but I'm on PTO all of next week so I likely couldn't see it through.

I'll give it a shot.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Prevent URIFixup from leaking user:password to search engine

Comment 6

[Valentin Gosu [:valentin] (he/him)]

The attached patch seems to fix the problem, but it slightly breaks 2 unit tests:
browser_urlHighlight.js and browser_urlbarTrimURLs.js for URLs such as http://user:[e-mail]/

https://treeherder.mozilla.org/logviewer.html#?job_id=10935293&repo=try

The reason is that before this patch, the URL couldn't be trimmed (removing the http:// part) in the URL bar without making the protocol change. Now that works, so it messes up some of our assumptions.

Fixing browser_urlbarTrimURLs.js is easy, we just need to change the expected value.
For browser_urlHighlight.js we need to change the implementation of urlbarBindings.xml::formatValue to create a fixup URI before checking the protocol, or figure out a better way of marking that the URL has been trimmed.

Comment 7

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #6)
> For browser_urlHighlight.js we need to change the implementation of
> urlbarBindings.xml::formatValue to create a fixup URI before checking the
> protocol,

I looked at this before (bug 1199601 and the fixed blocked bug) but I don't think it's trivial to do this because of the different ways the URL gets changed etc. (ie there are a number of callpaths, and it isn't always the case that the location bar content is a real URI, etc. etc. - see also discussions with Dão in bug 1195976.

> or figure out a better way of marking that the URL has been
> trimmed.

I think the simplest thing will be to fix the regular expression we use in formatValue to work (or at least err on the side of caution) for trimmed URIs. See also the patch I attached on bug 1195976.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Gijs, do you think you could take this bug? You obviously have more insight into the problem, and I'm a bit caught up with other issues at the moment.

Comment 9

[:Gijs (he/him)]

I can look, but I'm digging myself out of PTO so I dunno with what timing I will be able to do so.

Comment 10

[:Gijs (he/him)]

So there's an issue here that this notation is essentially indistinguishable from protocol:[e-mail].

In particular: mailto:[e-mail] . :-)

(which the fixup service turns into a http URL with your patch, and doesn't without it.)

We could make a specific exception for mailto, but I don't know to what extent that works. Valentin, can you suggest what we should be doing here?

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Then it's possible there's a bug in my implementation. I suspect ioService->GetProtocolHandler returns a handler for the mailto protocol. If that's the case (and for any other protocol which has a handler - resource:, ws:, etc), we should not assume that it's missing a protocol.

Comment 12

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #11)
> Then it's possible there's a bug in my implementation. I suspect
> ioService->GetProtocolHandler returns a handler for the mailto protocol.

It returns the default external protocol handler on my OS X system, at least (specifically, in the JS console, Services.io.getProtocolHandler("default") == Services.io.getProtocolHandler("mailto") is true. I don't know why; I would have expected to be able to distinguish working and not-working external protocols.

I'm also vaguely uncomfortable that:

myusername:[e-mail]

and

myusernamethatisaprotocol:[e-mail]

behave differently depending on whether the username corresponds to a protocol. I guess that can't really be helped.

Equally, I would ideally not want to re-break sticking "define:folding@home" in the URL bar.

> If
> that's the case (and for any other protocol which has a handler - resource:,
> ws:, etc), we should not assume that it's missing a protocol.

I mean, on beta right now, I can put mailto:[e-mail] in the address bar, and it opens my mail client. So clearly the existing check for "known" protocols that is in the code *works*. It's past 10pm here so I'm not sure *why* offhand. I'll check again tomorrow if you don't beat me to it.

Comment 13

[:Gijs (he/him)]

There's a broken check in nsDefaultURIFixup that checks the wrong fixup flag when checking the external protocol service (scheme typos instead of keyword url).

But also, your patch avoids even creating the fixed URI, and instead it should wait for the check on the external protocol handler (that then uses the keyword fixup).

I have a fixed up patch now, but I still need to fix the browser tests and I don't have time to finish that up right now. Going to steal so I do it on Monday.

Comment 14

[:Gijs (he/him)]

Attachment: Patch

This fixes the issue at hand without breaking mailto:[e-mail], or other URIs where an external protocol handler has been registered.

For the tests, this uses the URI fixup service to figure out the protocol and ensure that the URI is valid, and prefixes http:// back on in case it got removed. It seems trimURL only ever removes http (and never ftp? I swear it used to do that...) and so this approach seems to work for me.

I experimented with using the fixup URI instead of the regexp for highlighting, but I am paranoid that the fixup will add slashes (see bugs mentioned earlier in this bug with URIs like foo.org?bar) and/or otherwise not match the content of the URL bar, thereby causing the matching to be off. Looking for just the domain in the URI would cause issues if the domain was repeated in a subdomain or the username/password. I added testcases for those, and ended up reverting most of the implementation bits to their previous state. I think the testcases are useful in case we revisit the implementation, so I left them in, though they could be removed if people felt that was necessary.

I also adapted the tests to deal with the stripping of http:// for user:pass URLs.

r?dao for the browser/ tests, and Valentin, do you feel comfortable r+'ing the rest and/or do you think I need to ping someone else (bz/smaug) for the docshell bits?

Comment 15

[:Gijs (he/him)]

Comment on attachment 8660776 [details] [diff] [review]
Patch

Ugh, this breaks "define:foo" in the location bar. Haven't figured out why yet. :-(

Comment 16

[:Gijs (he/him)]

Attachment: Patch v1.1

(In reply to :Gijs Kruitbosch from comment #13)
> There's a broken check in nsDefaultURIFixup that checks the wrong fixup flag
> when checking the external protocol service (scheme typos instead of keyword
> url).

So while I continue to think this is not ideal, we can't use the keyword URI flag because that will get unset for things that make a valid URI (even if we don't know how to use that URI) in docshell before we pass flags to the fixup service:

https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4562-4576

So let's not shave that yak here...

Comment 17

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf7af7969edd

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8660791 [details] [diff] [review]
Patch v1.1

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

I am not a docshell peer, but the fixup URI bits and tests look alright to me.

Comment 19

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #17)
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf7af7969edd

Two issues:
- on win8, there is an external protocol handler for the "unknown" protocol, which breaks that test. Trivial fix is using "gobbledygook" for the pre-"user" portion.
- on all of windows, the test for //user:[e-mail]:8080 doesn't work because the leading "/" only triggers file URI fixup on Unix, not on Windows

All the bc test failures look unrelated.

Comment 20

[:Gijs (he/him)]

Attachment: Patch v1.2

This fixes the test failures on my machine.

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a361294f4b9

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8661761 [details] [diff] [review]
Patch v1.2

>--- a/browser/base/content/test/general/browser_urlHighlight.js
>+++ b/browser/base/content/test/general/browser_urlHighlight.js

>+  testVal("<http://mozilla.com:mozilla.com@>mozilla.com", "<mozilla.com:mozilla.com@>mozilla.com");

Please call testVal("<mozilla.com:mozilla.com@>mozilla.com") instead and revert the changes to the testVal function. We trim before we highlight, so you're not adding to the testing surface here as far as highlighting is concerned. Trimming tests belong in browser_urlbarTrimURLs.js.

>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml

>+          // Ignore if we couldn't make a URI out of this, the URI resulted in a search,
>+          // or the URI has a non-http(s)/ftp protocol.
>+          if (!uriInfo.fixedURI || uriInfo.keywordProviderName ||
>+              ["http", "https", "ftp"].indexOf(uriInfo.fixedURI.scheme) == -1) {
>             return;
>+          }

nit: please put 'uriInfo.keywordProviderName ||' on a new line

>+          let trimmedLength = 0;
>+          if (uriInfo.fixedURI.scheme == "http" && !value.startsWith("http://")) {
>+            value = "http://" + value;
>+            trimmedLength = "http://".length;
>+          }

Please document what you're doing here / why you're doing this.

>+            let fixupDomain = uriInfo.fixedURI.host;
>+            baseDomain = Services.eTLD.getBaseDomainFromHost(fixupDomain);

nit: baseDomain = Services.eTLD.getBaseDomainFromHost(uriInfo.fixedURI.host);

Makes it clearer that you're not using the fixed domain for anything else below this line.

Comment 22

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/417e00a78107

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/417e00a78107

Comment 24

[:Gijs (he/him)]

Comment on attachment 8661761 [details] [diff] [review]
Patch v1.2

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: user:pass information gets leaked to search engines
[Describe test coverage new/current, TreeHerder]: has test coverage in this path
[Risks and why]: medium-low - the code change is straightforward, but there is a potential this will cause inadvertent changes to URLs that currently intentionally (from the perspective of the user) go to search engines, or (less likely) to external programs. The automated tests are extensive enough, and considering we're just days *after* the last merge, that I think the risk is worth taking in order to cut the amount of time until this security bug is addressed - that and getting more baking time on beta (so with more users) to see if it did inadvertently end up breaking something.
[String/UUID change made/needed]: no

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8661761 [details] [diff] [review]
Patch v1.2

Sec-moderate, medium risk, touch a user facing feature, 42 is a special release, I prefer if this rides the 43 train.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9034d20bf95d

Comment 27

[Mathieu Goessens]

Thanks a lot everybody for your reactivity and your work on this bug :)

Comment 28

[Mathieu Goessens]

[ Adding [mozfr-community] whiteboard ans cc'd pascalc for tracking ; sorry for the noise ]

Comment 29

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 40 RC (20150807085045) under Ubuntu 12.04 64-bit.

Verified fixed on Firefox 44.0a2 (2015-12-07/08) and Firefox 43 beta 9 (20151203163240) under Ubuntu 12.04 32-bit, Windows 10 64-bit and Mac OS X 10.11.1.

Comment 30

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

On consultation with Dan, we're not doing an advisory for this one due to the lack of overall danger to users.