"secure=" attribute on cookie header is not being observed

VERIFIED FIXED

Status

()

VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: morse, Assigned: morse)

Tracking

Trunk
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

16 years ago
The RFC2109 cookie spec specifies the syntax of the set-cookie header as 
follows:

        set-cookie      =       "Set-Cookie:" cookies
        cookies         =       1#cookie
        cookie          =       NAME "=" VALUE *(";" cookie-av)
        NAME            =       attr
        VALUE           =       value
        cookie-av       =       "Comment" "=" value
                        |       "Domain" "=" value
                        |       "Max-Age" "=" value
                        |       "Path" "=" value
                        |       "Secure"
                        |       "Version" "=" 1*DIGIT

So it's very clear that the secure attribute is not to have an equal sign 
following it.  However not all sites observe that.  An offender is 
https://online.lloydstsb.co.uk/customer.ibc (see bug 164260 comment 20).  
Furthermore, IE allows an equal sign in this case so they will recognize 
"secure=<anything> as making the cookie secure.  Oddly enough, that means that 
"secure=no" will make it secure as well.  But since the site violated the spec 
to start with, they deserve whatever they get. ;-)

Here is an example that demonstrates the problem:

  <html>
    <script>
      document.cookie = "a=b;secure=yes";
      alert(document.cookie);
    </script>
    <body>
      hi
    </body>
  </html>

If the above were executed from other than an https site, the intent is that we 
should not see the cookie displayed in the alert.  IE indeed does not display 
it.  Mozilla and N4 do display it.

So the bug being reported here is that mozilla should behave as IE does and 
accept secure=<anything> as making the cookie secure.
(Assignee)

Comment 1

16 years ago
Created attachment 102667 [details] [diff] [review]
accept "secure=<anything>" as making a cookie secure
Comment on attachment 102667 [details] [diff] [review]
accept "secure=<anything>" as making a cookie secure

It looks like you're using a strtok loop to find the "secure" attribute, and
strcasestr searches for all of the other attributes. THis code would run faster
if you got rid of the strtok look and used strcasestr for the "secure"
attribute as well. Also, there's no reason to traverse the string for "secure"
and again for "secure=" - I think a single match function should suffice for
both.
(Assignee)

Comment 3

16 years ago
Mitch, the strtok is necessary because we need to find the token "secure" and
make sure it is not in any other context, such as being on a path.  See bug
105602 for details.

So given that we need to test for "secure" as a token and not as a substring,
the next question is how do we test for "secure=".  That one it makes sense to
test for as a substring.  Of course it would probably pick up such things as
"xsecure=" but that's totally meaningless in the cookie header so if we get
fooled by it, it's the site's own fault.
Comment on attachment 102667 [details] [diff] [review]
accept "secure=<anything>" as making a cookie secure

OK, r=mstoltz. I think we could make some significant performance improvements
to this code, but I don't want that to delay a needed security fix.
Attachment #102667 - Flags: review+
Comment on attachment 102667 [details] [diff] [review]
accept "secure=<anything>" as making a cookie secure

>+      if (PL_strcasecmp(token, "secure") == 0 || PL_strcasestr(semi_colon, "secure=")) {

Not the most efficient way to do this, but...

What happens in the case "...;secure = yes;..."? The spec doesn't mention the
spaces, but you've got code elsewhere because IE allows it in other places.

sr=dveditz
Attachment #102667 - Flags: superreview+
If you're doing this for IE compatibility, it seems to me you'd want to get the
compatibility right.  Does IE accept things that have "secure=" but don't begin
with it?  Does IE accept things that begin with "secure", but not followed by an
= sign?  (I'm particularly hesitant about the former, since you don't want to be
more buggy than IE -- then they could be forced to imitate us.)
(Assignee)

Comment 7

16 years ago
After some careful experimenting with IE, here is how I believe it is behaving:

Ignoring spaces or horizontal tabs, the word "secure" must be immediately
preceded by a semicolon and immediately followed by either a semicolon, an equal
sign, or a null.  If these conditions are satisfied, then the cookie will be
recognized as being a secure cookie, and otherwise it will not be.

I will revise the patch to mimic identically the behavior of IE.

(Assignee)

Comment 8

16 years ago
Here is a testbed for determining if a cookie is being recognized as secure or
not.  You invoke this page by giving it a query string consisting of the
contents of a set-cookie header.  If the cookie value is then displayed, it was
not recognized as a secure cookie (assuming you do not host this on an https
server).

For example, you might invoke it with

   "http://www.myserver.com/test.htm?a=b;secure=yes

<html>
  <head>
    <script>

      function GetQueryString() {
        var i;
        var url = location.href;
        var re = /\?/;
        var query = url.search(re);
        if (query == -1) {
          return;
        }
        document.cookie = url.slice(query+1);
        alert(document.cookie);
      }

    GetQueryString();

    </script>
  </head>
  <body>
    hello
  </body>
</html>
(Assignee)

Comment 9

16 years ago
Created attachment 104090 [details] [diff] [review]
mimic behavior of IE exactly
(Assignee)

Updated

16 years ago
Attachment #102667 - Attachment is obsolete: true
This is on the 1.2 list.  How is it coming?
(Assignee)

Comment 11

16 years ago
> This is on the 1.2 list.  How is it coming?

It's waiting for a review from mstoltz, and after that a super-review from dveditz.
Comment on attachment 104090 [details] [diff] [review]
mimic behavior of IE exactly

>+    ptr = PL_strcasestr(semi_colon, "secure");
>+    while (ptr) {
>+      char * ptr2 = ptr + PL_strlen("secure");

It's inefficient to calculate the length of "secure" on every iteration.
Consider this:
>+    static const char secureToken[] = "secure";
>+    ptr = PL_strcasestr(semi_colon, secureToken);
>+    while (ptr) {
>+      char * ptr2 = ptr + sizeof(secureToken) -1; // -1 for the null terminator

That way, the length of "secure" is computed at compile time.


>+        if (*ptr == ';' || *ptr == '\0') { // note: '\0' means we got to beginning of string

Is the byte before the start of an allocated buffer always zero? On every
platform? I've never seen it done this way, and I'm nervous about searching
back past the beginning of a buffer like this. Instead of looking for *ptr ==
'\0', you should look for ptr == localString, which will also indicate the
beginning of the buffer.

I'd like to see an updated patch before signing off on this.
(Assignee)

Comment 13

16 years ago
> Is the byte before the start of an allocated buffer always zero?

In this case it is.  Look further up and you'll see the line:

      *semi_colon++ = '\0';

What was done here is that a search was made for the semicolon that separates
the <name>=<value> part of the set-cookie header from the attributes that follow
it.  When that semicolon was found, it was replaced with a null.  So from then
on the code is dealing with the attribute part of the string which follows the null.

> Instead of looking for *ptr == '\0', you should look for ptr == localString,

There is no longer a variable called localString.

Attaching patch to fix the strlen inefficiency.
(Assignee)

Comment 14

16 years ago
Created attachment 104429 [details] [diff] [review]
fix up inefficiency with strlen
Attachment #104090 - Attachment is obsolete: true
Comment on attachment 104429 [details] [diff] [review]
fix up inefficiency with strlen

OK, everything looks good.
r=mstoltz
Attachment #104429 - Flags: review+
Comment on attachment 104429 [details] [diff] [review]
fix up inefficiency with strlen

sr=dveditz for fixing this bug, but I think this whole section needs to be
re-written. There seems to be a different approach to parsing each cookie
attribute (example, this seems to be the only one that tries to validate what
preceeds the keyword) and that's got to lead to some odd/rare bugs.
Attachment #104429 - Flags: superreview+
Would it be worth taking this on the branch as well? This was found on a banking
site, and despite the real error being on their end they may consider Gecko
insecure leading to evangelism problems.
Keywords: adt1.0.2

Comment 18

16 years ago
dan do you want me to adt plus this?
(Assignee)

Updated

16 years ago
Keywords: mozilla1.0.2

Comment 19

16 years ago
Comment on attachment 104429 [details] [diff] [review]
fix up inefficiency with strlen

a=chofmann for 1.0.2 branch and 1.2 trunk
Attachment #104429 - Flags: approval+

Updated

16 years ago
Keywords: mozilla1.0.2 → mozilla1.0.2+

Comment 20

16 years ago
Plussing for adt, sent Stephen email asking for checkin on the branch now that
we have drivers approval.
Keywords: adt1.0.2 → adt1.0.2+
(Assignee)

Comment 21

16 years ago
Patch checked in on both branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Keywords: fixed1.0.2
Resolution: --- → FIXED

Comment 22

16 years ago
verified1.0.2 - win NT4, linux rh6, mac osX

internal testcase at http://bubblegum/tever/bug174104.html
Keywords: fixed1.0.2 → verified1.0.2

Comment 23

16 years ago
verified trunk - 11/01/02 builds - win NT4, linux rh6, mac osX
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.