Closed Bug 473587 Opened 11 years ago Closed 11 years ago

Invalid port numbers load, but should report errors

Categories

(Core :: Networking, defect, P3)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: christopher.sullo, Assigned: benjamin)

References

Details

(Keywords: fixed1.9.0.7, fixed1.9.1, Whiteboard: [sg:low spoof])

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) 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:1.9.0.5) 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.
Whiteboard: [sg:low spoof]
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #357173 - Flags: superreview?
Attachment #357173 - Flags: review?
Attachment #357173 - Flags: superreview?(dbaron)
Attachment #357173 - Flags: superreview?
Attachment #357173 - Flags: review?(dbaron)
Attachment #357173 - Flags: review?
Comment on attachment 357173 [details] [diff] [review]
Detect overflow in ToInteger, rev. 1

r+sr=dbaron
Attachment #357173 - Flags: superreview?(dbaron)
Attachment #357173 - Flags: superreview+
Attachment #357173 - Flags: review?(dbaron)
Attachment #357173 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/59f4175487b6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Flags: blocking1.9.1+
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #357173 - Flags: approval1.9.0.7?
Whiteboard: [sg:low spoof] → [sg:low spoof][needs 1.9.1 baking - 01/22]
Flags: wanted1.9.0.x+
Comment on attachment 357173 [details] [diff] [review]
Detect overflow in ToInteger, rev. 1

Approved for 1.9.0.7, a=dveditz for release-drivers.

Does this fix alpha ports too? that is http://www.mozilla.com:foo/ == port 0.
Attachment #357173 - Flags: approval1.9.0.7? → approval1.9.0.7+
Whiteboard: [sg:low spoof][needs 1.9.1 baking - 01/22] → [sg:low spoof]
Fixed in CVS for 1.9.0.7
Keywords: fixed1.9.0.7
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 → ---
Flags: wanted1.8.1.x?
Flags: blocking1.8.1.next?
Assignee: benjamin → nobody
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.
Assignee: nobody → jduell.mcbugs
Flags: wanted1.8.1.x? → wanted1.8.1.x+
Whiteboard: [sg:low spoof] → [sg:low spoof] partial fix checked in
Dan: Can we either remove the approval1.9.0.7 flag or add the fixed1.9.0.7 keyword back so this no longer shows in queries?
On my list.   Low-to-medium priority.
Priority: P2 → P3
Flags: blocking1.8.1.next?
Attachment #357173 - Flags: approval1.9.0.7+ → approval1.9.0.7-
Comment on attachment 357173 [details] [diff] [review]
Detect overflow in ToInteger, rev. 1

Tree closed for 1.9.0.7 -- 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 1.9.0.7 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.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 479485
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: approval1.9.0.7- → approval1.9.0.7+
Group: core-security → mozilla-confidential
Whiteboard: [sg:low spoof] embargo until bug 479485 fixed → [sg:low spoof]
Group: mozilla-confidential
You need to log in before you can comment on or make changes to this bug.