1310987 - Insecure URL decoding poses risk of invalid URL redirection

Status: RESOLVED WORKSFORME

(Core :: Networking, defect, P3)

Description

[Shailesh Suthar]

Attachment: test.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161013141419

Steps to reproduce:

1. Create a hyperlink tag <a> like below in a html file:
<a href="http://example.com%5C.mozilla.com">http://example.com%5C.mozilla.com</a>
2. Open file and Click on link. 
3. You will be redirected on http://example.com\.mozilla.com instead of http://example.com%5C.mozilla.com
4. "Server not found" error will be shown and in address bar .mozilla.com will be shown highlighted as domain name.
5. Click on address bar and press Enter.
6. You will be redirected on http://example.com instead of mozilla.com website.
7. Done


Actual results:

Characters from subdomain of any link should not be decoded. Actually, %2f, %3f, %23 and all other characters are decoded at this point but only %5c is decoded into "\" character (You can verify using attached html file).

Google Chrome and Internet Explorer browsers consider such as an invalid link. Safari redirects on http://example.com%5c.mozilla.com but only Firefox redirects on http://example.com\.mozilla.com. 

When user is redirected on http://example.com\.mozilla.com using this way, .mozilla.com is shown highlighted so there is few risk of invalid redirection.


Expected results:

Redirect user on http://example.com%5c.mozilla.com (without any decoding for subdomain)
OR
URLs with subdomain which contains HEX character should be considered as INVALID URL and there should be no redirection.

Comment 1

[:Gijs (he/him)]

Anne, what does the spec say here?

Comment 2

[Anne (:annevk)]

It says the host parser should fail here: https://url.spec.whatwg.org/#host-parsing (see the bit that mentions "\").

Comment 3

[:Gijs (he/him)]

Valentin, sounds like this is a bug in our URI parser?

Comment 4

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

The URL parser does not allow \ in the hostname. However, this behaviour is caused by several things:
1. Backslash replacement (bug 652186)
2. Percent unescaping in the hostname - it only unescapes characters that are valid in the hostname, in order to maintain backwards compatibility. Since \ is not allowed, %5C is not unescaped.
3. The url bar unescapes everything. In browser.js::losslessDecodeURI() it calls decodeURI(value) and proceeds to reencode whitespace.

A few solutins for this are:
1. Disallow %5C in the hostname. Doable, but implementing it would add another pass, making nsStandardURL even slower.
2. Not do any decoding in the URL bar. This would fix a lot of other use cases, but I got a lot of push-back when I proposed this.
3. We should not decode %5C in losslessDecodeURI, or reencode the resulting \ (but only in the hostname). This seems tricky.
4. I think we already have a bug for this - If the user just clicks the URL bar without changing anything, the URL bar should load the same URL (gBrowser.currentURI) instead of what's currently in the URL bar.

I think the best is 4.

Comment 5

[Anne (:annevk)]

> Percent unescaping in the hostname - it only unescapes characters that are valid in the hostname, in order to maintain backwards compatibility.

This is not compliant. Compatibility with what? We should just decode it and then declare it an error. That's what other browsers appear to be doing too.

Comment 6

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

(In reply to Anne (:annevk) from comment #5)
> > Percent unescaping in the hostname - it only unescapes characters that are valid in the hostname, in order to maintain backwards compatibility.
> 
> This is not compliant. Compatibility with what? We should just decode it and
> then declare it an error. That's what other browsers appear to be doing too.

Addons :(
A lot of addons have an encoded @ (%40) in their name and resource:// path

Comment 7

[Anne (:annevk)]

We could special case resource:// rather than all of the web?

Comment 8

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #6)
> (In reply to Anne (:annevk) from comment #5)
> > > Percent unescaping in the hostname - it only unescapes characters that are valid in the hostname, in order to maintain backwards compatibility.
> > 
> > This is not compliant. Compatibility with what? We should just decode it and
> > then declare it an error. That's what other browsers appear to be doing too.
> 
> Addons :(
> A lot of addons have an encoded @ (%40) in their name and resource:// path

Can we limit the workaround here to only %40/"@" and/or potentially only for resource:// ? That would solve this issue without breaking add-ons, right?

> A few solutins for this are:
> 2. Not do any decoding in the URL bar. This would fix a lot of other use
> cases, but I got a lot of push-back when I proposed this.

AIUI not doing this would show IDN hostnames as escaped, and more generally do the same for any non-ascii query parameters or hash/ref portions. Is that right? If so, that is likely not desirable, no.

> 3. We should not decode %5C in losslessDecodeURI, or reencode the resulting
> \ (but only in the hostname). This seems tricky.
> 4. I think we already have a bug for this - If the user just clicks the URL
> bar without changing anything, the URL bar should load the same URL
> (gBrowser.currentURI) instead of what's currently in the URL bar.

We could do this, but the same problem would then occur if/when the user modifies any extraneous portions of the URL. FWIW, I am a little confused as to how we're doing highlighting wrong in the initial case - we should be using the hostname value that the nsIURI gives us... that might be a separate bug.


FWIW, I am not convinced there's much point keeping this sec-sensitive. It's a bug, but I don't see how it's a security issue as filed. Al?

Comment 9

[:Gijs (he/him)]

+ni for comment #8:

(In reply to :Gijs Kruitbosch from comment #8)
> FWIW, I am not convinced there's much point keeping this sec-sensitive. It's
> a bug, but I don't see how it's a security issue as filed. Al?

Comment 10

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

(In reply to Anne (:annevk) from comment #7)
> We could special case resource:// rather than all of the web?

Yes. I filed 1311107.

Also, this bug is basically a dupe of bug 1026938 and/or bug 1163959.

Comment 11

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #10)
> Also, this bug is basically a dupe of bug 1026938 and/or bug 1163959.

Is it? The first is about query/hash parameters. This issue has other consequences, it feels like.

Comment 12

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

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Valentin Gosu [:valentin] from comment #10)
> > Also, this bug is basically a dupe of bug 1026938 and/or bug 1163959.
> 
> Is it? The first is about query/hash parameters. This issue has other
> consequences, it feels like.

I agree. But fixing one of the others should automatically fix this one too.

I have a simple patch that fixes this issue, but it also fails multiple tests, so it probably needs more work.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d40da30f474cf0841548f6eb3af4194eb946883

Comment 13

[effi]

Attachment: mike.svg

Comment 14

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 16

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

Firefox no longer allows percent encoding in the hostname, so this issue no longer applies.
It may change the path or query - but we have bug 1163959 for that.