Closed Bug 174104 Opened 22 years ago Closed 22 years ago

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

Categories

(Core :: Networking: Cookies, defect)

Product:
Core
Component:
Networking: Cookies
Platform:
x86
Windows NT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: morse, Assigned: morse)

Details

Attachments

(1 file, 2 obsolete files)

fix up inefficiency with strlen
2.52 KB, patch
security-bugs
: review+
dveditz
: superreview+
chofmann
: approval+
Details | Diff | Splinter Review 
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.
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.
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.)
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.


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>
Attached patch mimic behavior of IE exactly (obsolete) — — Splinter Review 

    
  

        Attachment #102667 -
        Attachment is obsolete: true

    
    

    
  
This is on the 1.2 list.  How is it coming?

    
    

    
  
> 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.

    
    

    
  
> 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.

    
    

    
  


  

        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

    
    

    
  
dan do you want me to adt plus this?

    
  
Keywords: mozilla1.0.2

    
    

    
  
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+

    
  
Keywords: mozilla1.0.2 → mozilla1.0.2+

    
    

    
  
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+

    
    

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

    
    

    
  
verified1.0.2 - win NT4, linux rh6, mac osX

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

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: