Closed
Bug 105602
Opened 23 years ago
Closed 23 years ago
Set-Cookie: ... ; secure algorithm flawed
Categories
(Core :: Networking: Cookies, defect)
Tracking
()
VERIFIED
FIXED
mozilla0.9.6
People
(Reporter: brendanx, Assigned: morse)
References
()
Details
(Keywords: testcase)
Attachments
(3 files, 6 obsolete files)
1.42 KB,
text/plain
|
Details | |
2.99 KB,
patch
|
Details | Diff | Splinter Review | |
3.13 KB,
patch
|
jag+mozilla
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
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
Comment 2•23 years ago
|
||
->cookies.
Assignee: asa → syd
Component: Browser-General → Editor: Composer
QA Contact: doronr → sujay
.
Assignee: syd → morse
Component: Editor: Composer → Cookies
QA Contact: sujay → tever
Assignee | ||
Comment 4•23 years ago
|
||
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
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
really cc'ing jag and alecf this time. Please review patch. Thanks.
Comment 7•23 years ago
|
||
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.
Reporter | ||
Comment 8•23 years ago
|
||
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.
Assignee | ||
Comment 9•23 years ago
|
||
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.
Assignee | ||
Comment 10•23 years ago
|
||
I'm able to reproduce the problem now, and I've checked that the patch indeed corrects it.
Comment 11•23 years ago
|
||
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.
Assignee | ||
Comment 12•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #54249 -
Attachment is obsolete: true
Reporter | ||
Comment 13•23 years ago
|
||
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.
Assignee | ||
Comment 14•23 years ago
|
||
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.
Assignee | ||
Comment 15•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #54291 -
Attachment is obsolete: true
Reporter | ||
Comment 16•23 years ago
|
||
Reporter | ||
Comment 17•23 years ago
|
||
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.
Comment 18•23 years ago
|
||
See bug 104346.
Assignee | ||
Comment 19•23 years ago
|
||
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.
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
Whatever was I thinking. The \0 is not needed as one of the delimiter characters in strtok. Attaching corrected patch.
Assignee | ||
Comment 22•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #54616 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #54315 -
Attachment is obsolete: true
Reporter | ||
Comment 23•23 years ago
|
||
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.
Assignee | ||
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #54626 -
Attachment is obsolete: true
Reporter | ||
Comment 27•23 years ago
|
||
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.
Assignee | ||
Comment 28•23 years ago
|
||
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.
Reporter | ||
Comment 29•23 years ago
|
||
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...
Assignee | ||
Comment 30•23 years ago
|
||
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.
Reporter | ||
Comment 31•23 years ago
|
||
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.
Comment 32•23 years ago
|
||
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.
Reporter | ||
Comment 33•23 years ago
|
||
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.
Assignee | ||
Comment 34•23 years ago
|
||
Comment 35•23 years ago
|
||
Comment on attachment 54730 [details] [diff] [review] patch #2 modified to use nsCRT::strtok r=jag
Attachment #54730 -
Flags: review+
Assignee | ||
Updated•23 years ago
|
Attachment #54700 -
Attachment is obsolete: true
Comment 36•23 years ago
|
||
Comment on attachment 54730 [details] [diff] [review] patch #2 modified to use nsCRT::strtok yup, this all looks correct to me. Thanks. sr=alecf
Assignee | ||
Comment 37•23 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 38•21 years ago
|
||
test cases for this problem coming up.
Keywords: testcase
QA Contact: tever → cookieqa
Comment 39•20 years ago
|
||
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.
Description
•