Closed Bug 142803 Opened 22 years ago Closed 22 years ago

Port is being stored as part of cookie domain

Categories

(Core :: Networking: Cookies, defect, P2)

x86
Windows 2000
defect

Tracking

()

VERIFIED FIXED
mozilla1.2beta

People

(Reporter: dennis, Assigned: morse)

References

()

Details

Attachments

(3 files, 2 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc1)
Gecko/20020417
BuildID:    2002041711

SSL form checked by JavaScript, redirect appends :443 to domain name, resulting
in loss of session state.  Session state is cookie based.  Cookie not sent when
requested domain is www.jlchicago.ord:443 instead of www.jlchicago.org ... You
would have to begin the purchase process to see the error. You would not need to
actually buy anything though.  On this site you will need to enter a credit card
number to get to the point where the error occurs.  4111111111111111 will work
for that purpose.  


Reproducible: Always
Steps to Reproduce:
1. HTML Form in SSL that uses an onSubmit="return [true|false]" javascript
handler, action uses relative path.
2. Fill out form correctly, submit it.
3. page redirect includes :443 on end of domain.

Actual Results:  Loss of cookie-based session state.

Expected Results:  Should not append :443 to domain.
To cookies for starters.
Assignee: Matti → morse
Status: UNCONFIRMED → NEW
Component: Browser-General → Cookies
Ever confirmed: true
QA Contact: imajes-qa → tever
I tried this using both n4.x and mozilla.  In both cases, I submitted the form 
and was taken to the site

   
https://www.jlchicago.org:443/index.cfm/y/index.cfm/fa/mkp.home/cfid/716528/cfto
ken/68636654/y/index.htm

where only the digit fields (716528 and 68636654) differed between n4.x and 
mozilla.  But on both cases there was a :443 appended at the end.  So mozilla 
behaves exactly like 4.x

I then tried this with IE and in this case I got to the following site instead

https://www.jlchicago.org/index.cfm/y/index.cfm/fa/mkp.confirm/y/index.htm

Need to investigate further to determine whether the IE behavior is correct or 
the Netscape one is correct.  In any event, the site was designed to expect the 
IE behavior and only works correctly with IE but not with netscape (neither 4.x 
nor mozilla).

In any event, if this is a netscape but, it certainly is of low priority since 
it existed in n4 all this time and we lived with it.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2beta
Priority: -- → P2
Target Milestone: mozilla1.2beta → mozilla1.1beta
Attaching the relevant headers in the traffic that I captured when using the 
mozilla browser.  Note that the first 9 requests are do not involve any port, 
and the cookies are set and read.  Then request 10 through 50 are to port 443 
and do not involve any cookies.  Request 51 is also to port 443 and its response 
sets some cookies.
Problem is due to the fact that mozilla is including port number as part of the 
domain attribute of the cookie.  As far as I can tell, that is incorrect.  The 
RFC2109 spec states:

> The term request-host ... refers to the value the client would send to the
> server as ... the host (but not port) ... of the HTTP request line.

> 4.3.1 Interpreting Set-Cookie
> ...
> Domain Defaults to the request-host.
Attaching patch to no longer store port as part of cookie domain attribute.

This involves the following changes:

1. getting cookies and setting cookies no longer includes url_port when calling 
ExtractUrlPart

2. Since port is no longer extracted, there is no longer any reason to strip the 
port number in various places of the code.

3. Existing cookie files might include some cookies that have port numbers.  
This will cause duplicate cookies to be sent to the server.  To solve this, 
cookies with port numbers will be discarded when reading in the cookie file.
Summary: Form checked by JavaScript, redirect appends :443 to domain name, resulting in loss of session state → Port is being stored as part of cookie domain
Comment on attachment 90920 [details] [diff] [review]
leaving port number off when storing cookie domain

r=sgehani
Attachment #90920 - Flags: review+
Comment on attachment 90920 [details] [diff] [review]
leaving port number off when storing cookie domain

Is CookieCompare() used? I couldn't find a reference to it. If so maybe you
could zap the dead code as part of this patch (ran across it while
double-checking users of ->host).

>     /* check for bad legacy cookies (domain not starting with a dot) */
>     if (new_cookie->isDomain && *new_cookie->host != '.') {
>+      /* bad cookie, discard it */
>+      continue;
>+    }
>+
>+    /* check for bad legacy cookies (domain containing a port) */
>+    if (strchr(new_cookie->host, ':')) {
>       /* bad cookie, discard it */
>       continue;
>     }

If you're never stripping ports then won't an old cookie stored with a port
never match? in that case can we just leave it around until is expires away on
its own?

An extra strchr() times the max number of cookies at startup isn't going to
kill us. My main concern here is that both of these continues leak the
discarded cookie and its half a dozen or so allocated strings.
Attachment #90920 - Flags: needs-work+
> Is CookieCompare() used?

No.  Looks like it got consumed by cookie_CheckForPrevCookie at some time, and 
never got removed.  I'll yank it out now.

> If you're never stripping ports then won't an old cookie stored with a port
> never match? in that case can we just leave it around until is expires away
> on its own?

Yes, that's probably true.  It's just that we already got burned when we added 
dots to the beginning of domain names and wound up with both dotted and undotted 
cookies, both of which were being sent back to the host.  That regression led to 
the code that you see for removing legacy cookies not having the dot.  So that's 
why I felt it best to clean out these legacy cookies as well.

> My main concern here is that both of these continues leak the discarded
> cookie and its half a dozen or so allocated strings.

Good catch.  I just added code to plug the leak.  Attaching new patch.
Attachment #90920 - Attachment is obsolete: true
Comment on attachment 91120 [details] [diff] [review]
stop cookie leak, delete dead code

bringing r=sgehani forward from previous patch
Attachment #91120 - Flags: review+
One issue that I'd like people to think about before approving this patch is 
whether it is correct to ignore the port as far as cookies are concerned.  I 
think so (based on my reading of the RFC).  But it will be a significant change 
to our cookie behavior and I want to make sure it is the correct thing to do.

It might break some websites.  If they are few and minor ones, we can solve that 
with evangelism.  However I believe that IE must be ingnoring ports as well 
(otherwise how could the site in this bug report have worked with IE) in which 
case we should be safe.

There might also be security implications to not including the port test, so I'm 
cc'ing mitch on this.
That's interesting. I thought that cookies are to be port-specific since there
can be different sites on different ports of the same host (example:
http://www.openvms.compaq.com and http://www.openvms.compaq.com:8000 use
different cookies).
Internally we have several development web sites all on one server with ports
80, 81, 82, etc. They all have different cookies with overlapping names (since
they all have Username, etc.). This problem isn't really significant to me but I
mention it anecdotally since at least some people may depend on the current
behavior.
Another quirk but I'd guess this one's pretty common.
Dan, thanks for your comments.  They were most informative.

Would you agree from the reading of the RFC2109 spec that ports should be 
ignored (comment 5)?  But if appears that if we take this patch, then we will 
break your website.  If this is too widespread, then we are going to have to 
look for a different solution.

I presume that your website works with IE.  Can you look at the IE cookie file 
and see what is being stored.
Keywords: nsbeta1
Target Milestone: mozilla1.1beta → mozilla1.2beta
My gut feeling is that ports should always count - they could very well point to
a different server under different administration etc. I would vote to make this
an evangelism issue.

If we find a large number of sites that do not work because of our current
implementation I could reconsider, but since there are no dupes and this bug was
found relatively late, I don't think this is a common situation.
I know the following argument probably represents a small segment of the 
population, but I think Mozilla should adhere to the standard (leaving the 
port out) for 2 reasons:

1. IE does it this way.
2. If Mozilla includes the port it makes it impossible to share cookies across 
servers running on different ports.  For example, let's say a user visits a 
site running on nonstandard ports (http:8000 and https:8443 for example) and 
the site requires ssl login.  When the user is redirected to the non-ssl part 
of the site he will no longer be logged in.  And what's more, if parts of the 
site are accessible only via login and non-ssl, he won't ever be able to see 
them because he cannot login securely and have his login persist.
I think it is very important that Mozilla, and all other browsers, adhere to
published standards in general, and specifically to the cookie spec as laid out
in RFC2109.  Many eCommerce sites, including those that I have built, rely on
the ability to switch seamlessly between HTTP and HTTPS while keeping
cookie-based session information intact.  RFC2109 leaves no room for doubt: the
port is to be ignored in matching the domain name of the cookie.  All versions
of Internet Explorer (and pre-Mozilla Netscape) that I have tested behave
according to this spec, at least with regard to stripping the port.  Therefore,
it is incumbent upon those web sites that are out of spec to make changes to
accomodate the spec, not vice versa.
Install this JSP in any JSP container that supports SSL, and is using cookies
for session tracking.  Hit the page under HTTP, and then hit it under HTTPS
(using ports other than 80 and 443; e.g., 8000 and 8001).  If the browser is
behaving according to RFC2109, then the session identifiers should be the same.
 If the browser is out of spec, then the session identifiers will be different.
So the spec, IE, and web developers all agree that the port should be ignored.
Why don't we check this patch in?

I'll sr the patch...
Comment on attachment 91120 [details] [diff] [review]
stop cookie leak, delete dead code

sr=roc+moz
Attachment #91120 - Flags: superreview+
The issue was not standard compliance or the lack of an sr that kept me from
persuing this.  Rather I had let this one lie because I was afraid of breaking
websites.  Comment 14 implied that we would break websites and this to me said
that IE does check for ports.  But from what is said in comment 17 and comment
18, it seems that IE does not check ports.  (Comment 18 is incorrect about
pre-mozilla netscape -- it was the basis for the current mozilla code so N4.x
does check ports.)

OK, if there is agreement that IE does not check ports, then I'll check this in
(as soon as I fix the bit rot).  But I'll monitor the criticisms carefully, and
if it turns out that it causes us to start breaking websites that work fine in
IE, I'll back it out immediately.
Sounds reasonable. Check it in!
I stand corrected with regard to pre-Mozilla Netscape.  I just ran session-snoop.jsp (see 
my previous attachment) from Netscape Communicator 4.7 on a Mac, and Stephen 
Morse is correct: this bug has been present in Netscape since before Mozilla.  But the 
spec is clear, and IE 5 (Mac/Windows) and IE 6 (Windows) ignore the port in matching 
the cookie, as per spec.  Thanks for the attention and quick response on this issue, guys.
Attachment #91120 - Attachment is obsolete: true
Comment on attachment 99671 [details]
Simple JSP to test cookie behavior

r=sgehani
sr=roc+moz
(bringing reviews forward)
Attachment #99671 - Flags: superreview+
Attachment #99671 - Flags: review+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
I've added this comment to bugzilla for bugs 99311 and 142803 but thought
it worth bringing to your attention via email as well. Hope thats ok and
hope you'll excess the somewhat long post.

Came across this bug with cookies and port numbers yesterday. Looking at
the bugzilla history it appears (not completely confirmed) that mozilla's
behaviour has been swinging from working one way to working another several
times over the last few builds. I just wanted to add my opinion that the
current behaviour in 1.2beta is correct and and as per the RFCs. I also
wanted to point out this change to those people who have 'fixed' or requested
incorrect (IMO) behaviour for previous bugs so that do not reopen the old bugs 
and cause the behaviour to change back in future releases.

The bug relates to the default domain (also called host in the cookie manager)
that cookies are stored under/sent back to and weather or not to store port
numbers if they are used.

There are two RFC for cookies, 2109 (For set-cookie) and 2965 (For set-cookie2)

In RFC 2109 
in section 4.3.1 Interpreting Set-Cookie
it states 
  "Domain Defaults to the request-host. "
And in section 2 TERMINOLOGY
it states 
 "The terms request-host and request-URI refer to the values the client 
  would send to the server as, respectively, the host (but not port) and 
  abs_path portions of the absoluteURI (http_URL) of the HTTP request line. 
  Note that request-host must be a FQHN."
 
In RFC 2965
in section 3.3.1 Interpreting Set-Cookie2 
it states
  "Domain Defaults to the effective request-host. "
it also states
  " Port    The default behavior is that a cookie MAY be 
            returned to any request-port. "
And in section 1 TERMINOLOGY
it states
  " The terms request-host and request-URI refer to the values the client 
    would send to the server as, respectively, the host (but not port) and 
    abs_path portions of the absoluteURI (http_URL) of the HTTP request line. "
  (Just like RFC 2109)


My interpretation of these is that port numbers should not be used for recording
cookie domains unless a set-cookie2 header explicitly defines port number.

When a cookie is sent to mozilla with no domain specified

It appears that the behaviour for

0.9.3 stores port number 
0.9.4 does not store the port number
0.9.5 stores port number

And the tested behaviour for

1.0.1 stores port number
1.1 stores port number
1.2b does not store port number


The first bug in bugzilla relating to this is 99311 reported by
hniksic@arsdigita.com
who to summarise requested that a cookie set by
	delirium:1280
be a seperate cookie domain to a cookie set by 
	delirium:9800

The fix for this bug was to store port number for unspecified domains.

There are then a couple of similar bugs (bug numbers 134194, 130545) around the
0.9.8/0.9.9 builds that seem related but I did not find enough info in the 
bug reports to be sure that this is same thing.

Then bug 142803 'port numbers being stored as part of cookie' reported 
by dennis@gorillapolymedia.com requesting that port numbers not be 
stored with cookies results in the correct (IMO) behaviour being 
implemented for build 1.2beta.

For The discussion for bug 142803 I agree with commet 5 that port numbers 
should not be stored according to RFC's. And comments 17 and 18 are also very 
valid. I can also confirm (from testing) that IE (5.5 & 6) does ignore ports 
in this kind of situation.

As far as breaking websites (comment 14) goes. This is similar to the symptoms
that caused hniksic@arsdigita.com to report bug 99311. It is only when a site
uses different ports and the same name that problems might arise. If a shared
server implements name based (or ip based) virtual hosting this problem can
not arise. For IP based each ip must have a different name (although multiple
names can point to same IP) so for cookies this is same as name based virtual
hosting. Each site will automatically be distinguised by its name. In my
experience the number of site that exist that use different ports and the same
name that require seperation is very small especially in comparision with
the number of of sites that use http/https transparently and require sharing.
VERIFIED: fixed.
Mozilla 1.4a & b, allplats.

As long as I have tested cookies (since Jan), no ports have shown up in "site"
field.
Status: RESOLVED → VERIFIED
QA Contact: tever → cookieqa
You need to log in before you can comment on or make changes to this bug.