Closed Bug 105602 Opened 23 years ago Closed 23 years ago

Set-Cookie: ... ; secure algorithm flawed

Categories

(Core :: Networking: Cookies, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla0.9.6

People

(Reporter: brendanx, Assigned: morse)

References

()

Details

(Keywords: testcase)

Attachments

(3 files, 6 obsolete files)

Hopefully, someone can simply enlighten me, because this bug seems like such a
huge security hole to me, I find it hard to believe that a browser could ship
with it.

For the life of me, using the Set-Cookie header, I do not seem to be able to get
Netscape 6.0 or 6.1 to recognize a cookie as 'secure', that is only ever to send
it over a secure (https) connection.

The browser has a great little cookie list in Edit/Preferences/Privacy and
Security/Cookies/View Stored Cookies.  Each entry in that list shows its various
properties, including 'Server Secure'.  I have never seen a cookie say 'Yes' in
the 'Server Secure' field.  I only ever see 'No'.

Here is a cookie header I send from a server that Internet Explorer handles as I
would expect, but for which Netscape 6.x shows 'Server Secure: No' and seems
perfectly happy to send as clear text over a normal 'http' connection:

Set-Cookie: session=6420E5AA380E444F96B1D8131A18A7BC; path=/; secure
Just to test this on a high use public site, I tried logging into
http://www.hotmail.com with cookie prompting turned on in Internet Explorer.  It
showed a single secure cookie 'MSPSec' during the log on.

I tried the same log on using Netscape, and then reviewed the cookie list, but
'MSPSec' was listed as 'Server Secure: No'.

I am not proud, and would be happy to change my cookie headers, if there is a
work-around to my problem.  Will Set-Cookie2 work better?

You probably want to fix the browser to recognize the formats currently in high
use on the Web, but I would also like to know if there is a work-around, or just
something minor I have missed. I tried messing around with spaces and
semi-colons but have had no luck.
Oops.  My bad.  I am relatively new to this database, and I missed searching 
CLOSED bugs.  The main problem was already reported and fixed between 6.0 and 
6.1.

I did test 6.1, but it appears that my test case on 6.1 was not valid for the 
generalization I draw above.  I had abandoned testing my own cookies, and only 
tried the MSPSec cookie set by 'passport.com'.  Unfortunately for them, they 
set their secure cookie to a path '/ppsecure'.  That breaks the 'secure' 
parsing code at

http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookies.cpp#965

Since it does the following:

Set-Cookie: MSPSec=value; path=/ppsecure; secure

(1) Strip off string to first semi-colon

path=/ppsecure; secure

(2) Search for 'secure'

path=/pp>secure; secure

(3) Test to see if preceding character is space or semi-colon.

Oops.  'p' is not a space or semi-colon, so it is not secure.

May not be such a huge deal for them, since it appears to me that the their 
downfall, the ppsecure path, also protects them quite a bit by making it 
unlikely that anyone would ever request something with that path name over a 
non-secure connection.

Still you probably want to fix it before someone tries to set a secure cookie 
on a domain name that includes secure in it.

Set-Cookie: name=value; domain=.schwabsecure.com; secure
Severity: critical → normal
Summary: Set-Cookie: ... ; secure doesn't appear to function → Set-Cookie: ... ; secure algorithm flawed
->cookies.
Assignee: asa → syd
Component: Browser-General → Editor: Composer
QA Contact: doronr → sujay
.
Assignee: syd → morse
Component: Editor: Composer → Cookies
QA Contact: sujay → tever
Reporter, thank you for your excellent analysis task.  Yes, there definitely is 
a flaw in the logic that tests for the secure attribute.

I'm attaching a patch that I believe will fix the problem.  However I do not 
have a test site for demonstrating the problem.  The site you posted, 
http://www.hotmail.com, does not appear to demonstrate it, at least not when I 
tried it.

If you have the ability to test out the patch to see if it corrects the problem, 
it would be greatly appreciated.

cc'ing alecf and jag for reviews.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → mozilla0.9.6
really cc'ing jag and alecf this time.  Please review patch.  Thanks.
ack, this whole parsing mechanism is frightening. First of all, you're
dereferencing *(ptr-1) - which by my calculations could be one character before
the whole string begins, if the string begins with ";" - which means that a
malicious web site could set a cookie "; foo" which would cause the cookie code
to start stomping on memory... and to make matters worse, you're casting a
(const char*) to a (char*) in order to stomp all over this string.

What you should be doing is strdup()'ing this string if you're going to stomp
all over it. What happens if someone passes in a literal char[] string into your
function? you'll be stomping all over read-only memory, and cause the program to
crash. |const| is not something to simply cast away.

Why not use nsCRT::strtok() - it safely parses out substrings of a char*?
I realize the code already does this and you're certainly not making it any
worse, but this is just nasty. 
Alec, your first assertion about the -1 is wrong, because the worst it could be 
is the semi-colon that was found.  I had the same concern at first.

;secure
>;secure         //find first semi-colon
\0>secure        //null it out
\0>secure        //find first occurance of secure
\0>secure        //check ptr - 1 (oh, it's \0)

Also, I think we are in a buffer that contains the 'Set-Cookie:' of the 
header.  Not sure but, even so, the -1 is safe.  This does of course point out 
another flaw in the code that I haven't yet mentioned.  

Set-Cookie: name=value;secure

won't be secure:
name=value>;secure         //find first semi-colon
name=value\0>secure        //null it out
name=value\0>secure        //find first occurance of secure
name=value\0>secure        //check ptr - 1 (oh, it's \0, and not space or semi)

So, that type of cookie fails to be secure too.  Not all that likely, but still 
exceptable according to the spec.

Agree on the casting issue (probably single thread memory, but browsers are 
multi-threaded), and the idea of tokenizing the string instead of searching it 
would simplify dealing with so many special cases.

On the repro of this bug, you have to actually have a hotmail account on 
www.hotmail.com.  Not hard to create.  After you log onto your account, you 
will see that a cookie 'MSPSec' has been added to the domain 'passport.com'.  
It should be secure.
I had the same comment about the -1 comment being incorrect and left Alec a 
voicemail to discuss this.

And, yes, we are in a buffer that contains the set-cookie part of the header.

Oops, you are correct about the case of "name=value;secure" failing to be a 
secure cookie.  The fix of course is to change

 if (((cPre==' ') || (cPre==';')) && ...

to

 if (((cPre==' ') || (cPre==';') || cPre=='\0')) && ...

I'll try your suggestions for reproducing the bug.  Thanks.
I'm able to reproduce the problem now, and I've checked that the patch indeed 
corrects it.
since I only check my voice mail maybe once a week, let's continue the
discussion here in the open. You haven't addressed the issue of writing to a
const char* string, nor using nsCRT::strtok() to parse the string. Maybe I'm
wrong about nsCRT::strtok, but I'd appreciate you at least addressing it.
Attachment #54249 - Attachment is obsolete: true
My vote would be to resolve the 'const char*' -> 'char*' discrepancy, either by 
strdup'ing, or by finding the caller, and resolving whether it is safe to 
change the function prototype to more accurately represent the way the function 
uses the parameter.  That cast is just an accident waiting to happen.

Strtok might have yielded simpler code if used from the start, but a rewrite at 
this point for that benefit is not likely to be worth the effort, and might 
just yield a new, different set of bugs.
Attaching a patch that uses strdup rather than recasting.  However it now 
requires that we free the newly-allocated memory before returning and there are 
numerous returns from withing the routine.
Attached patch using strdup (obsolete) — Splinter Review
Attachment #54291 - Attachment is obsolete: true
I attached a template class that acts like a pointer, but frees the associated 
pointer when it goes out of scope.  If you want to keep the memory, you just 
ptr.Detach().

We use something like this in our project, and it makes code like a strdup in a 
function with a lot of returns a lot simpler.  Also, it is exception safe, if 
anything should happen to throw.
With all the focus on strdup and strtok, nobody even notice that my patch was 
indeed flawed.

semicolon is a pointer to the start of the cookie attributes.  I was 
incrementing the value of semicolon each time around the loop when searching for 
the string "secure".  So after I find secure, semicolon no longer points to 
the start of that attribute string but rather to the point in the attribute 
string following "secure".  In the next section of code I'm going to search 
for "path" starting from where semicolon points.  So if the path attribute 
precedes the secure attribute, I will miss it.
Whatever was I thinking.  The \0 is not needed as one of the delimiter 
characters in strtok.  Attaching corrected patch.
Attachment #54616 - Attachment is obsolete: true
Attachment #54315 - Attachment is obsolete: true
Okay, right, time to look at the patch.

char * token = strtok(semi_colon, " ;");
while (token) {
  if (PL_strcasecmp(token, "secure") == 0) {
         isSecure = PR_TRUE;
         break;
      }
  token = strtok(nsnull, " ;");
  }

So, you switched to using strtok, and now, as predicted, I believe you have 
introduced new issues.  If you are going to use strtok at all in this function, 
I think you need to use it for all the possible attributes of the cookie.  One 
big while loop with if conditionals like:

char * token = strtok(semi_colon, " ;");
while (token)
   {
   if (strnicmp(token, "domain=", 7) == 0)
      ...
   else if (strnicmp(token, "path=", 5) == 0)
      ...
   else if (strnicmp(token, "expires=", 8) == 0)
      ...
   else if (stricmp(token, "secure", 6) == 0)
      ...
   else
      {
      // Ignore unknown cookie parameter
      }
   token = strtok(nsnull, " ;");
   }

Read the strtok documentation carefully, and then run the following code 
through a debugger:

#include <string.h>

void main()
	{
	char* sz = strdup("path=/x; domain=.y.com; expires=ddd; secure");
	char* t = strtok(sz, " ;");
	while (t)
		{
		t = strtok(NULL, " ;");
		}
	}

Put a breakpoint in the loop, and watch the memory for sz.

From the documentation, "Each call to strtok modifies strToken by inserting a 
null character after the token returned by that call."  Running through the 
debugger verified for me that these modified characters are not actually 
replaced when advancing to the next token.

So, if your tokenizing for secure occurs at the top of your function, you now 
proceed to the remainder of your function with a string that contains \0 at the 
previous delimiter locations.

This means:

path=/x; domain=.y.com; expires=ddd; secure

becomes:

path=/x\0 domain=.y.com\0 expires=ddd\0 secure

which means that subsequent searches on the pointer to the head of this string 
memory, like:

ptr = PL_strcasestr(semi_colon, "path=");

....

ptr = PL_strcasestr(semi_colon, "domain=");

are not guaranteed of any particular result.  Diff's are not always the easiest 
to read, so let me know if I have misread your modification to the function.
Brendan MacLean is correct, I misused strtok.  The problem in my patch is that 
after I check for "secure", there will be nulls throughout the variable called 
semicolon, and then the next test for "path=" will fail.

There are three ways we can go here.

1. Just fix the bug

Fix the bug that has been reported without any attempt to convert to using 
strtok.  This is basically my patch entitled "using strdup" but with a slight 
change so as not to increment semicolon each time around the loop.  
Independentally, file a separate bug to convert this parsing over to using 
strtok someday in the future.

2. Use strtok for the parsing of "secure"

This is basically my patch entitled "removing \0 from delimiter arg of strtok" 
but with a slight modification so that a temporary string is established which 
strtok can modify without modifying the original semicolon string.

3. Use strtok throughout this module

This is a much more ambitious undertaken that stands a good chance of 
introducing regressions.

My own leaning is to do #1 and I think that MacLean is suggesting that as well.  
The super-reviewer wants #2.  So far I have not heard anybody advocate number 3.  
So I will produce two more patches -- one to fix the flawed patch for #1 and the 
other to fix the flawed patch for #2.
Attached patch revised patch for solution #2 (obsolete) — Splinter Review
Attachment #54626 - Attachment is obsolete: true
If you are not going to do #3, then I don't see the point in doing #2.  As I 
said before, #3 could yield a much cleaner function in the end, but is likely 
to take more time to stabilize again.

I don't really think #2 is a major improvement in the code, and it introduces a 
new mechanism of parsing (and another strdup).  Seems the worst of both worlds 
to me: spot-fix with higher risk.

Make a minor, low-risk fix, #1, or go for a much cleaner function at a higher 
risk, #3.
Regarding solution #3, the complete conversion to strtok is more involved 
than it appears to be from the skeleton that MacLean posted.  For example, in 
his skeleton there are things like

   else if (strnicmp(token, "path=", 5) == 0)

So we are expecting a token of "path=".  However if the string contained

   ....path=/a/b...

we would never get "path=" as a token because '/' is not one of the delimiters.  
Ok, so we should then put "=" as a delimiter and test for the token "path".  
Then call strtok once more and make sure it is followed by an '='.  Then call 
strtok again and get the value of path.

IMO, strtok is too powerful a function to use here.  It might be the function of 
choice for parsing a real programming language.  But strcasestr is more 
efficient for extracting values from a string like the cookie header.
I use 'strnicmp' (note the 'n'), not 'stricmp'.  That means I was expecting a 
token _beginning with_ 'path=...', and I would find it in your example.

Oy...
Or maybe we want a combination of strtok and strcasestr such as:

char * attribute = strtok(semi_colon, ";"); // note space is no longer a delim
while (attribute) {
  if (ptr = PL_strcasestr(attribute, "path=")) {
    // extract the path value from ptr+5
  } else if (ptr = PL_strcasestr(attribute, "domain=")) {
    // extract the domain value from ptr+6
  ...
  }
}

That is, use strtok to get the attributes which are semi-colon delimited, and 
then use strcasestr to examine the contents of each attribute.
Sort of, only you really want strnicmp, since the string needs to begin 
with 'path=' or 'domain=' or 'expires='.  Strcasestr will find it in the middle 
of the token, which is not really correct.  So you can use strnicmp, or you 
could use '((ptr = PL_strcasestr(attribute, "path=")) && prt == attribute)'.

It is not all that likely that you would find it anywhere but the beginning, 
but wouldn't you rather the code just be correct.  Not sure I understand your 
problem with 'strnicmp' or the equivalent in the local dialect.
well, you're also using strtok() instead of nsCRT::strtok() which was what I
requested earlier. it won't make solution #2 any cleaner, but it will avoid
using libc's strtok(), which is verboten in mozilla.

I do see the point in #2 even if #3 won't happen. I think that #3 should
eventually happen but for now I find #2 far easier to read and maintain than #1. 

Why? Because as a C++ developer, I am familiar with the semantics of
strtok()-like functions. If I ever need to fix a bug in this code, then I will
understand what is going on here. If I have to mentally step through this
handwritten boolean logic, I am more prone to error in both understanding what
is going on, not to mention more likely to introduce a new bug.
Okay, fine.  I don't have a huge problem with #2.  If it makes the C++ 
developers among us happy ;-) go for it.

I'd add a comment about the side-effect that strtok has of inserting null 
chars, lest someone decide to 'improve' your code and remove the extra 
allocation.
Comment on attachment 54730 [details] [diff] [review]
patch #2 modified to use nsCRT::strtok

r=jag
Attachment #54730 - Flags: review+
Attachment #54700 - Attachment is obsolete: true
Comment on attachment 54730 [details] [diff] [review]
patch #2 modified to use nsCRT::strtok

yup, this all looks correct to me. Thanks.
sr=alecf
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
test cases for this problem coming up.
Keywords: testcase
QA Contact: tever → cookieqa
V/fixed:
I used:

javascript:document.cookie=("MSPSec=value; path=/ppsecure; secure")

and confirmed that it is marked as secure in cookie manager.
Status: RESOLVED → VERIFIED
QA Contact: core.networking.cookies → benc
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: