Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Curl cookie handling bug with spaces #195

Closed
cromestant opened this issue Apr 1, 2015 · 18 comments
Closed

Curl cookie handling bug with spaces #195

cromestant opened this issue Apr 1, 2015 · 18 comments
Assignees
Labels

Comments

@cromestant
Copy link

Hello,
I just ran into a wonky behavior in curl 's cookie handling when spaces exist in the cookie:
here is a gist with the trace:https://gist.github.com/cromestant/9452ad9912c27882867c

to reproduce I created two PHP files

//cookie.php

<?php
header("Set-Cookie: CHARZID=this is ma cookie; path=/");
header("Location: http://www.google.com");

and
//cookies2.php

<?php
header("Set-Cookie: CHARZID = this is ma cookie; path=/");
header("Location: http://www.google.com");

the only difference is the space in the cookie name=value .
I looked at the RFC and it seems like a valid cookie header.

If you curl it and use a cookie jar you will see that cookie 2 does not set the value
you can test here:
http://test dot tigocloud dot net slash cookie.php
and cookie2.php

@cromestant
Copy link
Author

from IRC #curlusers
gevaerts: http://paste.debian.net/164323/ makes this work again
[12:05pm] gevaerts: That's a (very) partial revert of 7c21c1c

@gevaerts
Copy link
Contributor

gevaerts commented Apr 1, 2015

The following patch fixes this for me (but probably breaks the test suite and maybe some actual real things as well):

diff --git a/lib/cookie.c b/lib/cookie.c
index f1450e9..71a02db 100644
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -407,7 +407,7 @@ Curl_cookie_add(struct SessionHandle *data,
     do {
       /* we have a <what>=<this> pair or a stand-alone word here */
       name[0]=what[0]=0; /* init the buffers */
-      if(1 <= sscanf(ptr, "%" MAX_NAME_TXT "[^;\r\n =]=%"
+      if(1 <= sscanf(ptr, "%" MAX_NAME_TXT "[^;\r\n=]=%"
                      MAX_COOKIE_LINE_TXT "[^;\r\n]",
                      name, what)) {
         /* Use strstore() below to properly deal with received cookie

From looking at the code, I think it worked before 7c21c1c (I can't easily test, autoconf issues with older builds). The code that's supposed to strip such whitespace is still there, so I assume the intent is to still support such whitespace.

@bagder bagder added the HTTP label Apr 1, 2015
@bagder bagder self-assigned this Apr 1, 2015
@bagder
Copy link
Member

bagder commented Apr 1, 2015

Thanks, I'll write up a test case (or perhaps amend an existing) for this to first repeat it and then make sure it verifies the fix - while not breaking any existing test!

@bagder
Copy link
Member

bagder commented Apr 1, 2015

Unfortunately, only this patch is not enough. It breaks test 31. I'll work on a minor improvement.

bagder added a commit that referenced this issue Apr 1, 2015
"name =value" is fine and the space should just be skipped.

Updated test 31 to also test for this.

Bug: #195
Reported-by: cromestant
Help-by: Frank Gevaerts
@bagder
Copy link
Member

bagder commented Apr 1, 2015

Please try out this version. The fix is very similar to @gevaerts', and I also extended the test case to make sure it works with a name with a trailing space.

@bagder
Copy link
Member

bagder commented Apr 2, 2015

Presumed to be fixed, closing this issue!

@bagder bagder closed this as completed Apr 2, 2015
@cromestant
Copy link
Author

I can confirm upon building and testing that the issue is resolved.
Awesome turnaround!

@bagder
Copy link
Member

bagder commented Apr 2, 2015

Thanks for confirming

@markstos
Copy link

From reading the cookie grammar in RFC 6265, it appears that whitespace is not allowed around the "=" sign, or within the value.

Should Curl be accepting invalid cookies, or emitting a warning or error when it encounters an invalid formatted cookie?

@cromestant
Copy link
Author

I might be minterpreting this, but I see

set-cookie-string = cookie-pair *( ";" SP cookie-av )

so cookie pair:

cookie-pair = cookie-name "=" cookie-value

cookie name is defined as token which is defined in
http://tools.ietf.org/html/rfc2616#section-2.2

which clearly includes spaces

and cookie value is defined as cookie octed with and without double quotes.

cookie-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
; US-ASCII characters excluding CTLs,
; whitespace DQUOTE, comma, semicolon,
; and backslash

So in looking at it I do see whitepaces as valid...

On Tue, Apr 14, 2015 at 9:47 AM, Mark Stosberg notifications@github.com
wrote:

From reading the cookie grammar in RFC 6265
http://tools.ietf.org/html/rfc6265#section-4.1.1, it appears that
whitespace is not allowed around the "=" sign, or within the value.

Should Curl be accepting invalid cookies, or emitting a warning or error
when it encounters an invalid formatted cookie?


Reply to this email directly or view it on GitHub
#195 (comment).

MSc. Charles M. Romestant F.

Merci de penser à l'environnement avant d'imprimer cet e-mail
Please think about the environment before you print this e-mail
Por favor piense en el medio ambiente antes de imprimir este e-mail

@cromestant
Copy link
Author

emphasis on whitepsace in cookie-octet added by my gmail sending.. sorryabout that

@markstos
Copy link

As you posted above, the cookie-octet is to contain: "US-ASCII characters excluding CTLs, whitespace DQUOTE, comma, semicolon, and backslash".

I read that as excluding whitespace in the cookie values.

@markstos
Copy link

The Mozilla Cookie Documentation is also clear that unencoded whitespace is now allowed in cookies. (Search for "white" on that page).

@cromestant
Copy link
Author

I agree, I misread,
however looking at the documentation available, I see a few RFCs and
discussions that are very unclear (at best) but mostly conflicting
http://www.ietf.org/rfc/rfc2965.txt specifies "non-whitespace" characters,
but then states:

Attributes (names) (attr) are case-insensitive. White space is
permitted between tokens. Note that while the above syntax
description shows value as optional, most attrs require them.

"white spaces are permitted between tokens" which if looking at the
language def previously stated:

av-pair = attr ["=" value] ; optional value
attr = token

value = token | quoted-string

could mean that between the name and = and value... as name and value
are token..

In the end the thing that preocupies me the most:

  • many apps send the space, and I've looked at lots of cookie parsing
    code that trims the spaces ( so expects them).[1]
  • Curl code seems to be a reference on this issue, and CURL's doc
    states "no white spaces" (http://curl.haxx.se/rfc/cookie_spec.html)
    yet the code was in, and then removed and now back into the tested
    version.

=> we could break compatibility with this

So I 'd say leaving it in even though it is not "proper".

[1] Chromium code has this function:

void ParseRequestCookieLine(const std::string& header_value,
ParsedRequestCookies* parsed_cookies) {
std::string::const_iterator i = header_value.begin();
while (i != header_value.end()) {
// Here we are at the beginning of a cookie.

// Eat whitespace.
while (i != header_value.end() && *i == ' ') ++i;
if (i == header_value.end()) return;

On Tue, Apr 14, 2015 at 10:39 AM, Mark Stosberg notifications@github.com
wrote:

The Mozilla Cookie Documentation
https://developer.mozilla.org/en-US/docs/Web/API/Document/cookie is
also clear that unencoded whitespace is now allowed in cookies. (Search for
"white" on that page).


Reply to this email directly or view it on GitHub
#195 (comment).

MSc. Charles M. Romestant F.

Merci de penser à l'environnement avant d'imprimer cet e-mail
Please think about the environment before you print this e-mail
Por favor piense en el medio ambiente antes de imprimir este e-mail

@bagder
Copy link
Member

bagder commented Apr 15, 2015

"CURL's doc" as mentioned there is not at all curl's doc, it is the original Netscape spec for cookies and we use that only as a historic reference now (we host it only because the original hosting place went away and it would otherwise be gone). We have RFC 6265 to follow these days. I believe we now do like the browsers do.

RFC 2965 is not a spec anyone follows, and curl doesn't either. Ignore that.

@cromestant
Copy link
Author

Great discussion, but what is the "official" policy on this in the end?
in or out, or in with a warning?

On Wed, Apr 15, 2015 at 2:48 PM, Daniel Stenberg notifications@github.com
wrote:

"CURL's doc" as mentioned there is not at all curl's doc, it is the
original Netscape spec for cookies and we use that only as a historic
reference now (we host it only because the original hosting place went away
and it would otherwise be gone). We have RFC 6265 to follow these days. I
believe we now do like the browsers do.

RFC 2965 is not a spec anyone follows, and curl doesn't either. Ignore
that.


Reply to this email directly or view it on GitHub
#195 (comment).

MSc. Charles M. Romestant F.

Merci de penser à l'environnement avant d'imprimer cet e-mail
Please think about the environment before you print this e-mail
Por favor piense en el medio ambiente antes de imprimir este e-mail

@bagder
Copy link
Member

bagder commented Apr 15, 2015

The policy is to A) follow the spec but also to B) do how "the others" do it - for maximum compatibility. Like the browsers. And they should all adhere to RFC 6265. We don't really have warnings in libcurl.

@cromestant
Copy link
Author

great!
thanks for all.

On Wed, Apr 15, 2015 at 3:10 PM, Daniel Stenberg notifications@github.com
wrote:

The policy is to A) follow the spec but also to B) do how "the others" do
it - for maximum compatibility. Like the browsers. And they should all
adhere to RFC 6265. We don't really have warnings in libcurl.


Reply to this email directly or view it on GitHub
#195 (comment).

MSc. Charles M. Romestant F.

Merci de penser à l'environnement avant d'imprimer cet e-mail
Please think about the environment before you print this e-mail
Por favor piense en el medio ambiente antes de imprimir este e-mail

jgsogo pushed a commit to jgsogo/curl that referenced this issue Oct 19, 2015
"name =value" is fine and the space should just be skipped.

Updated test 31 to also test for this.

Bug: curl#195
Reported-by: cromestant
Help-by: Frank Gevaerts
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

4 participants