Closed Bug 473587 Opened 11 years ago Closed 11 years ago
Invalid port numbers load, but should report errors
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:184.108.40.206) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:220.127.116.11) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729) Invalid port numbers in URLs work in Firefox. There appears to be a bug where the port number can be 'wrapped' to a different number, such that a large number can be used to represent 80 or 443. For example: http://mozilla.org:90194313296/ will load mozilla.org port 80 http://mozilla.org:90194313659/ will load mozilla.org port 443 Since this port number is invalid, Firefox should report an error and not load the URL. This may have security implications if extensions or other software is validating URLs. Note: this number translation happens before other security events (for example, a number representing a restricted port still reports as restricted). A second case is when the port number is a text string. In this case, Firefox will ignore the port number. For example, the URL: http://example.com:example.org/ will load example.com without error. This could be used in a phishing attack to trick an unsuspecting user to visiting the site in the left portion of the URL, since many users will assume the right portion is the site to be visited. Firefox should also generate an error in this case. Reproducible: Always Steps to Reproduce: 1. load URLs in details section. Actual Results: Errors were not reported. Expected Results: Errors should be reported to the user. This happens when tested on Firefox Mac and Windows.
Status: UNCONFIRMED → NEW
Component: General → Networking
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → networking
http://mxr.mozilla.org/mozilla/source/netwerk/base/src/nsURLParsers.cpp#605 Relies on nsTString_CharT::ToInteger http://mxr.mozilla.org/mozilla/source/xpcom/string/src/nsTStringObsolete.cpp#144 I suppose we should overflow-check this... It's also interesting that this code appears to auto-detect hex number encoding, although simple in-browser testing seems to indicate this doesn't work.
Comment on attachment 357173 [details] [diff] [review] Detect overflow in ToInteger, rev. 1 r+sr=dbaron
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Whiteboard: [sg:low spoof] → [sg:low spoof][needs 1.9.1 baking - 01/22]
Comment on attachment 357173 [details] [diff] [review] Detect overflow in ToInteger, rev. 1 Approved for 18.104.22.168, a=dveditz for release-drivers. Does this fix alpha ports too? that is http://www.mozilla.com:foo/ == port 0.
Attachment #357173 - Flags: approval22.214.171.124? → approval126.96.36.199+
Whiteboard: [sg:low spoof][needs 1.9.1 baking - 01/22] → [sg:low spoof]
Fixed in CVS for 188.8.131.52
I'm not sure this is a sufficient fix, we need something in the URL parsers as well. Nothing was done about the alpha case, and overflows now result in loading the default port. Instead both should be invalid URIs in my opinion. It doesn't make a lot of sense that http://www.mozilla.org:232323/ is an invalid URI while http://mozilla.org:90194313296/ happily loads something other than what it looks like.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Verifying this, everything seems to just redirect to the default port now with no error so http://mozilla.org:90194313296/ just goes to http://mozilla.org, as does http://mozilla.org:90194313659/. Is this really the desired fix? If so, I can mark it as verified.
How does it handle "ports" like this with the patch? http://mozilla.org:cnn.com/ Personally, I'd prefer that if a port was not between 1-65535 it gave an error and did not load anything (and of course still gave errors with 'restricted' ports).
Jason: want a crack at fixing the rest of this one? This will get you into the guts of the URL parser.
Dan: Can we either remove the approval184.108.40.206 flag or add the fixed220.127.116.11 keyword back so this no longer shows in queries?
On my list. Low-to-medium priority.
Priority: P2 → P3
Attachment #357173 - Flags: approval18.104.22.168+ → approval22.214.171.124-
Comment on attachment 357173 [details] [diff] [review] Detect overflow in ToInteger, rev. 1 Tree closed for 126.96.36.199 -- we'll approve this for the appropriate 1.9.0.x release when we get the rest of the bug fixed.
FWIW... this was checked in for 188.8.131.52 already, and is in the release. I'd like to mark this one FIXED and open another one for the remaining work, since otherwise it will be very confusing.
Filed the followup as bug 479485
Assignee: jduell.mcbugs → benjamin
Whiteboard: [sg:low spoof] partial fix checked in → [sg:low spoof] embargo until bug 479485 fixed
Attachment #357173 - Flags: approval184.108.40.206- → approval220.127.116.11+
Group: core-security → mozilla-confidential
Whiteboard: [sg:low spoof] embargo until bug 479485 fixed → [sg:low spoof]
You need to log in before you can comment on or make changes to this bug.