Closed Bug 1301621 Opened 3 years ago Closed 3 years ago

Parse URL ports as 16 bit

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

See https://www.w3.org/Bugs/Public/show_bug.cgi?id=26446 , https://github.com/whatwg/url/commit/c3a454c073d6bf9f24d8ee2b5c3e6e006b943777

We currently parse ports as 32 bit.

This doesn't seem to bypass any security checks (e.g. fetch has a concept of a "bad" or forbidden port), since network requests with larger port numbers just don't happen (instead of overflowing or something).

But we should fix the parser and perhaps also the size of the argument in nsIURI.
I only changed the parsing logic. Should we update nsIURI and nsIURIParser to use 16 bit too? I'm not sure if that would be too far-reaching a change?
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Comment on attachment 8789691 [details]
Bug 1301621 - Parse URL ports as 16 bit;

https://reviewboard.mozilla.org/r/77824/#review76198

This gets a tentative r+.
I'm not sure if this is the best place for the check, or if it should be in nsStandardURL. Either way, be sure to test this on try. I expect at least some tests to break.

PS. We can't make the nsIURI attribute 16 bit, because a negative value has a certain meaning in this context. Just a check that we are within limits will do.
Attachment #8789691 - Flags: review?(valentin.gosu) → review+
Whiteboard: [necko-active]
Should I add a redundant check to nsStandardURL?
(In reply to Manish Goregaokar [:manishearth] from comment #4)
> Should I add a redundant check to nsStandardURL?

I don't think you need a redundant one - maybe a MOZ_ASSERT?
You do ned to add a check to nsStandardURL::SetPort though, as that doesn't go through the parser.
See Also: → 1300052
I'm not sure how the netwerk/test/unit/test_bug652761.js, netwerk/test/unit/test_invalidport.js, and toolkit/components/search/tests/xpcshell/test_location_error.js failures work. Those tests expect it to fail, but they error out at

0:00.91 LOG: Thread-1 ERROR NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]
NetUtil_newURI@resource://gre/modules/NetUtil.jsm:206:16

Not sure what's going on? It throws an error which we expect will happen, but doesn't pass the test?

All the other failures are expected-passes or *I think* intermittents.
(In reply to Manish Goregaokar [:manishearth] from comment #7)
> I'm not sure how the netwerk/test/unit/test_bug652761.js,
> netwerk/test/unit/test_invalidport.js, and
> toolkit/components/search/tests/xpcshell/test_location_error.js failures
> work. Those tests expect it to fail, but they error out at
> 
> 0:00.91 LOG: Thread-1 ERROR NS_ERROR_MALFORMED_URI: Component returned
> failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]
> NetUtil_newURI@resource://gre/modules/NetUtil.jsm:206:16
> 
> Not sure what's going on? It throws an error which we expect will happen,
> but doesn't pass the test?
> 
> All the other failures are expected-passes or *I think* intermittents.

Previous to this patch, parsing URLs with huge ports would work, even though the channel would fail to open that URL/port.
Now it fails when creating the URL. So we need to change the tests to something similar to:
Assert.throws(() => { _code_that_will_throw_ }, "invalid port");

Maybe add a comment indicating this bug number.
Try build successful, with two unexpected passes which I forgot to update (https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ecc288f5a3d&selectedJob=27216044). Kicked off a try build with reduced scope to ensure it's fixed.

Should I autoland this? Not sure, given that you gave a tentative r+ :)
(In reply to Manish Goregaokar [:manishearth] from comment #13)
> Should I autoland this? Not sure, given that you gave a tentative r+ :)
You could land it, but I just realized there's still something left to do, which is the default port.
We need to change SetDefaultPort and Init() to also apply the same restriction. You can update the patch,or we can deal with it in bug 1300052. I'd also like some unit tests to for all the codepaths.
I'm not sure if there's anything that still needs testing? test_bug652761.js handles the error, and the WPT tests already deal with the URI changing and stuff like that. Are there some other checks I need?
Oh, I see, you mean by testing nsSTandardURI directly. Will do
Tests pass now (nto sure why gecko-decision timed out, but the tests were properly spawned). r? on the newly added test?
Patch looks great! r=valentin
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc9963bf6d98
Parse URL ports as 16 bit; r=valentin
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/820356f508df
Backed out changeset fc9963bf6d98 for xpcshell bustage
Failures seem to only happen on windows and android, not sure why. Trying with a nonexistant domain; https://treeherder.mozilla.org/#/jobs?repo=try&revision=c506fe143268
Mark, any idea how to fix this test?

https://hg.mozilla.org/try/file/e38d66ad91e0/toolkit/components/search/tests/xpcshell/test_location_error.js

It used to use port 111111111, but now this port causes a malformed uri error (previously no error would be thrown, but the network request would never happen). Changing it to a lower port works on linux and osx, but windows and android fail to find the error code. Not sure what's going on here; any way we could tweak this test so it doesn't rely on the existence of ports which blackhole requests?
Flags: needinfo?(markh)
This is checking a particular code path in the geoip for the default search engine lookup, dealing with a class of network error.  I'm sorry, but can't remember the specific details of what error state was being provoked. Florian might have more insights...
Flags: needinfo?(markh) → needinfo?(florian)
I haven't touched this test nor the geoip lookup (I touched everything that happens after it though), so I'm not sure what this test exactly verifies, but it seems to me that any error would do, maybe we could have the test setup a local http server, and point the test to an URL that'll return a 404?
Flags: needinfo?(florian)
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/483725b39bb2
Parse URL ports as 16 bit; r=valentin
https://hg.mozilla.org/mozilla-central/rev/483725b39bb2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.