1233610 - IDN service should return NS_ERROR_MALFORMED_URI when punycode representation of the URL is longer than 63 characters (instead of NS_ERROR_FAILURE)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase

JavaScript error: resource://app/components/FeedConverter.js, line 538: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIOService.newURI]

Security-sensitive in case this is a bug in the URL code (which seems possible given how weird the input has to be).

Comment 1

[:Gijs (he/him)]

The page does:

var u = new URL("file:");
u.pathname = "aaa\u0003s\u0003xxxxxxxxxx\u0003\u0003\u0003y\u009Bz\u0003h\u0003hhhhh\\sssss\u0003";
u.protocol = "pcast";

The feed converter's newURI function gets called with:

pcast:///aaa%03s%03xxxxxxxxxx%03%03%03y%C2%9Bz%03h%03hhhhh%5Csssss%03

as the spec, which means it calls into the IO service with:

http:///aaa%03s%03xxxxxxxxxx%03%03%03y%C2%9Bz%03h%03hhhhh%5Csssss%03

which fails.

Which means this reduces to just running this:

Services.io.newURI("http:///" + encodeURIComponent("aaa\u0003s\u0003xxxxxxxxxx\u0003\u0003\u0003y\u009Bz\u0003h\u0003hhhhh\\sssss\u0003"), null, null)

in the browser console, which produces the same exception. It seems to come from trying to understand the gobbledygook as a hostname (which is how this gets parsed, apparently?) which then breaks somehow. I stopped looking at a certain point. It certainly seems fine and not sec-sensitive to me, but maybe Valentin can provide more details based on his work in bug 1199430.

Comment 2

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

The error code comes from gIDN->ConvertToDisplayIDN
It happens now because the %C2%9B part is not ascii, and ends up being decoded, which triggers the IDNA.
The IDNA then returns an error code because the punycode representation of the URL is longer than 63 characters.
Seems to me like this is the correct behaviour, and not a bug.

Comment 3

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #2)
> The error code comes from gIDN->ConvertToDisplayIDN
> It happens now because the %C2%9B part is not ascii, and ends up being
> decoded, which triggers the IDNA.
> The IDNA then returns an error code because the punycode representation of
> the URL is longer than 63 characters.
> Seems to me like this is the correct behaviour, and not a bug.

I wonder if we should be throwing NS_ERROR_MALFORMED_URI instead, though. This is what happened on earlier versions of Firefox; I don't really know what changed it (I haven't tried to find a regression window).

Also, curiosity question: if this is not a bug, why did you assign yourself to it? :-)

Comment 4

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

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Valentin Gosu [:valentin] from comment #2)
> > The error code comes from gIDN->ConvertToDisplayIDN
> > It happens now because the %C2%9B part is not ascii, and ends up being
> > decoded, which triggers the IDNA.
> > The IDNA then returns an error code because the punycode representation of
> > the URL is longer than 63 characters.
> > Seems to me like this is the correct behaviour, and not a bug.
> 
> I wonder if we should be throwing NS_ERROR_MALFORMED_URI instead, though.
> This is what happened on earlier versions of Firefox; I don't really know
> what changed it (I haven't tried to find a regression window).

I'm pretty sure the regression window starts after landing bug 412457. Before that we didn't do any percent decoding of hostnames.
I guess we could change nsIDNService to return NS_ERROR_MALFORMED_URI instead.

> 
> Also, curiosity question: if this is not a bug, why did you assign yourself
> to it? :-)

I set the asignee a few days ago, and forgot about it when I submitted the comment. :)

Comment 5

[:Gijs (he/him)]

I'd like to mark this no longer sec-sensitive, but I can't do that. :-\

Comment 6

[Andrew McCreight [:mccr8]]

Unhiding per comment 5.

Comment 7

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

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

Comment 8

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

Attachment: MozReview Request: Bug 1233610 - IDN service should return NS_ERROR_MALFORMED_URI instead of NS_ERROR_FAILURE r?mcmanus

Review commit: https://reviewboard.mozilla.org/r/29861/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29861/

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 8705120 [details]
MozReview Request: Bug 1233610 - IDN service should return NS_ERROR_MALFORMED_URI instead of NS_ERROR_FAILURE r?mcmanus

https://reviewboard.mozilla.org/r/29861/#review26763

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/749795f067db

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/749795f067db

Comment 12

[Jesse Ruderman]

Changing the error message doesn't really solve my problem, but I guess that's covered by bug 949926.