Closed Bug 1233610 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jruderman, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files)

Attached file 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).
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.
Group: firefox-core-security → core-security
Component: RSS Discovery and Preview → Networking
Flags: needinfo?(valentin.gosu)
Product: Firefox → Core
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.
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
(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? :-)
(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. :)
Assignee: valentin.gosu → nobody
I'd like to mark this no longer sec-sensitive, but I can't do that. :-\
Summary: FeedConverter.js: NS_ERROR_FAILURE: nsIIOService.newURI → IDN service should return NS_ERROR_MALFORMED_URI when punycode representation of the URL is longer than 63 characters (instead of NS_ERROR_FAILURE)
Group: core-security → network-core-security
Unhiding per comment 5.
Group: network-core-security
Attachment #8705120 - Flags: review?(mcmanus) → review+
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
https://hg.mozilla.org/mozilla-central/rev/749795f067db
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Changing the error message doesn't really solve my problem, but I guess that's covered by bug 949926.
You need to log in before you can comment on or make changes to this bug.