cURL looses Cookie information when CURLOPT_FOLLOWLOCATION is true #697

Closed
smos opened this Issue Mar 2, 2016 · 22 comments

Projects

None yet

4 participants

@smos
smos commented Mar 2, 2016

I did this

On Debian 7.9 with cURL 7.26.0-1+wheezy13 and PHP 5.3.3-7+squeeze19 we can succesfully login into Siemens C470/N300 DECT base stations. After upgrading to Debian 8 and cURL 7.38.0 this no longer works. I've also tested this on Debian 9 (Stretch/sid) with cURL 7.47.0 and the problem exists there still.

No cookies were saved to the Cookie JAR on either of these occasions.

After debugging somewhat I appear to have found the issue, the authentication is succesful, and the host returns a cookie with a 303 redirect. This cookie is then lost for any subsequent request, nor is it written to the cookie jar.

Apparently there has been a change since cURL 7.26 that no longer saves the cookie on redirect. Unfortunate. It appears I was not the only one that ran into this issue.

  • FOLLOWLOCATION is true: It does not even write a cookie jar header. The file is 0 bytes.
  • FOLLOWLOCATION is false: we only get a cookie header in the cookie jar.

I expected the following

I expected a successful login where the cookie is saved to the jar when the login page redirects as 7.26.0 previously did. Perhaps this was changed for security reasons but with a side effect. I could not find a option for cURL to save this cookie on redirect.

curl/libcurl version

seth@lsautom:~$ curl --version
curl 7.47.0 (x86_64-pc-linux-gnu) libcurl/7.47.0 GnuTLS/3.4.9 zlib/1.2.8 libidn/1.32 libssh2/1.5.0 nghttp2/1.7.1 librtmp/2.3
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets

operating system

Debian Stretch/sid ~02-03-2016

seth@lsautom:~$ php -v
PHP 5.6.17-3 (cli)
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2015 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2015, by Zend Technologies

Example output:

seth@lsautom:~$ php -f curltest.php |more
cookie jar /tmp/telecookieYrUzx4
Authentication Error: HTTP/1.1 303 See Other
Connection: Keep-Alive
Location: http://10.9.43.20/home.html
Server:
**Set-Cookie: key=14683720; path=/; expires=Sat, 21-12-2037 00:00:00 GMT**
Content-Type: text/html
Content-Length: 736

**HTTP/1.1 303 See Other**
Connection: Keep-Alive
Location: http://10.9.43.20/login.html
Server:
Content-Type: text/html
Content-Length: 738

HTTP/1.1 200 OK
Cache-Control: no-store
Connection: Keep-Alive
Pragma: no-cache
Transfer-Encoding: chunked
ETag: /login.html
Server:
Content-Type: text/html

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<html>
<head>
  <title>N300A IP</title>

Login not succesful after redirect. The cookie file is empty.

seth@lsautom:~$ ls -l /tmp/telecookieYrUzx4
-rw------- 1 seth Automatisering 0 mrt  2 15:48 /tmp/telecookieYrUzx4

Example PHP code

<?php
        //$USERNAME = 'guest';
        $PASSWORD = '0000';
        $SERVERURL = 'http://10.9.43.20/login.html';
        $cookiejar = tempnam('/tmp/', 'telecookie');                                            # will have to change path for windows
        echo "cookie jar {$cookiejar}\n";

        $ch = curl_init();
        curl_setopt($ch, CURLOPT_URL, $SERVERURL . '#');
        curl_setopt($ch, CURLOPT_HEADER, 1);
        curl_setopt($ch, CURLOPT_FOLLOWLOCATION, 1);
        curl_setopt($ch, CURLOPT_COOKIEFILE, $cookiejar);                                       # reads cookies from this file, but more importantly turns on cookie engine
        curl_setopt($ch, CURLOPT_COOKIEJAR, $cookiejar);                                        # writes cookies to this file on cleanup, probably not necessary
        curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false);                                        # the Fiery uses a self-signed, non fqdn ssl cert
        curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false);                                        # the Fiery uses a self-signed, non fqdn ssl cert

        curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);                                         # allows curl_exec to send to a variable
        curl_setopt($ch, CURLOPT_POST, true);

        # these are what gets posted to the REST API to login - it sets a session cookie that gets used for future requests
        $loginpostfields = 'language=1' . '&password=' . $PASSWORD;

        curl_setopt($ch, CURLOPT_POSTFIELDS, $loginpostfields);
        $curlout = curl_exec($ch);
        if(curl_errno($ch)) {
    echo 'Connection Error: ' . curl_error($ch);
    exit;
        }
        elseif('{"authenticated":true}' != $curlout) {
                echo 'Authentication Error: ' . $curlout;
                exit;
        }


        curl_setopt($ch, CURLOPT_POST, false);
        //curl_setopt($ch, CURLOPT_URL, $SERVERURL . 'consumables');
        $curlout = curl_exec($ch);
        $consumables = json_decode($curlout, true);
        print_r($consumables);


        curl_close($ch);
        unlink($cookiejar);
?>



@bagder
Member
bagder commented Mar 2, 2016

The cookie engine has no knowledge of redirects and never had, which makes this sound strange to me. The cookie jar is only ever saved on the closing of the easy handle. Is there are chance that your code exits before that in the redirect case?

@smos
smos commented Mar 4, 2016

I glanced at Cookie.c briefly and read the following comment "The 'lineptr' parameter is a full "Set-cookie:" line as received from a server."

That makes me think that the parameter is initialized fresh upon redirect and any previous Cookie information is now gone. Perhaps previous versions did not init this parameter again upon redirect, saving the Cookie information.

Regardless, the old combo of php5-curl 5.3 and libcurl 7.26 saved the Cookie. Now I'll just have to find out why.

The application we built remotely logs into the Siemens DECT base stations we administer, and we also configure them this way using a PHP script for our SIP PBX. We've been using this application for quite a while now, as early as Debian 6.0 and it's worked with redirects since that time :)

I'm now setting up a VM to see if I can track down where the changes came from. I'm not entirely sure if this is caused in changes from php5-curl 5.3 to php5-curl 5.6 or from the change from curl 7.26 to 7.38.

Completely rewriting the curl Class I use in our code to save the Cookie manually and scrape the headers from the redirect seems really backwards to me. I think cURL was made explicitly to not have to do such a thing ;)

@smos
smos commented Mar 4, 2016

I've uploaded some transferlogs for stdout and stderr from Debian 7 and Debian 8. If you look at the err log from Debian 8 with regards to the POST request on the login form I see that it picks up on the Cookie.

* upload completely sent off: 24 out of 24 bytes
< HTTP/1.1 303 See Other
< Connection: Keep-Alive
< Location: http://10.9.43.20/home.html
* Server  is not blacklisted
< Server: 
* Added cookie key="78238433" for domain 10.9.43.20, path /, expire 0
< Set-Cookie: key=78238433; path=/; expires=Sat, 21-12-2037 00:00:00 GMT
< Content-Type: text/html
< Content-Length: 736

It says it adds the cookie apparently with the 303 reponse. The next request is for home.html, but it performs this as a GET request. This one is performed without the cookie and any subsequent page is for login.html.

At this point the base station is locked and you can't get in anymore, you have to powercycle the base station to make it forget the session.

transferlog.debian8.err.txt
transferlog.debian7.err.txt
transferlog.debian8.txt
transferlog.debian7.txt

@smos
smos commented Mar 4, 2016

Backported 7.38.0 from Jessie onto Wheezy and that is enough to break it. (With PHP staying the same) I'm trying to build intermediate versions but this is my first time building Debian packages.

@jay
Member
jay commented Mar 4, 2016

Confirmed

@jay jay added the HTTP label Mar 4, 2016
@smos
smos commented Mar 4, 2016

I came as far today as 7.28.0 before the workday ended. Fastest method I found for building a debian package with an older version is clone the git tree from http://anonscm.debian.org/cgit/collab-maint/curl.git/log/?ofs=100 and then check out different points in time and build from that using
git checkout git rev-list -n 1 --before="2013-06-18" master

I can then build a new package using
debuild -b -uc -us

I can probably downgrade our production box by building a older package this way without entirely trashing the entire system :) Unfortunately the base station got buggered again so I can test over the weekend.

@bagder
Member
bagder commented Mar 4, 2016

I think trying to find or setup a way we can reproduce this problem with the current version is a better spent time.

@jay
Member
jay commented Mar 4, 2016

Bisected to 4cfbb20, I'm on it


curl -v -b cookies.txt -c cookies.txt -d "language=1&password=0000" -L http://127.0.0.1:8000/login.html

HTTP/1.1 302 Found
Connection: Keep-Alive
Location: http://127.0.0.1:8000/login.html
Server: 
Set-Cookie: key=14683720; path=/; expires=Sat, 21-12-2037 00:00:00 GMT
Content-Type: text/html
Content-Length: 730

XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXX



@bagder
Member
bagder commented Mar 4, 2016

Oh, the expire string is indeed funny looking. Does it fail to parse that and then expire the cookie?

@jay jay self-assigned this Mar 4, 2016
@jay
Member
jay commented Mar 4, 2016

It's the date, 21-12-2037 doesn't work but 21-Dec-2037 does, that is why expiry is 0 and is being purged. I'll consult the RFC and see if this is correct, however would there be any disadvantage to adding a fix for this even if it's not complaint?

@bagder
Member
bagder commented Mar 4, 2016

Our date parser is already accepting lots of non compliant formats. I won't mind if we support this one too, if we can do that without breaking something else...

@jay
Member
jay commented Mar 5, 2016

Basically anything that could possibly relate to this (netscape cookie spec and date specs in RFCs 822, 850, 1036, 1123, 2965, 6265) specify a date format in the order of day month year with a no digit month. Edit: the order varies, however the most common is day month year. 6265 outlines an algorithm for handling the tokens but do not indicate a preferred order.

However the date syntax spec in 822 on which some of the others based off of (and also suggest compliance with) has dd mm yy in the comment. I know I'm really reaching here but mm is the shorthand for a two digit month (as opposed to mmm), so maybe someone misread that RFC.

     date-time   =  [ day "," ] date time        ; dd mm yy
                                                 ;  hh:mm:ss zzz

     day         =  "Mon"  / "Tue" /  "Wed"  / "Thu"
                 /  "Fri"  / "Sat" /  "Sun"

     date        =  1*2DIGIT month 2DIGIT        ; day month year
                                                 ;  e.g. 20 Jun 82

     month       =  "Jan"  /  "Feb" /  "Mar"  /  "Apr"
                 /  "May"  /  "Jun" /  "Jul"  /  "Aug"
                 /  "Sep"  /  "Oct" /  "Nov"  /  "Dec"

IE 9/10 and FF 38/47.0a1 parse the date but Chrome 49 won't and treats it as a session cookie.

It seems to me there could be two issues here, first we aren't sending this session cookie on redirect and second we aren't parsing this cookie's date format which turns it into a session cookie.

As far as relaxing the date parser it looks like if I change it without being specific to cookies then situations like 01-02-38 it could be Feb 1 or Jan 2. However since in the case of cookies the format is always day month year we could do some special handling specifically for them before after or during parsing (for example convert dd-mm to dd-mmm before parsing). The no session cookie on redirect might be a separate issue and I haven't tested yet to see whether that's specific to this cookie or all cookies.

@bagder
Member
bagder commented Mar 5, 2016

Making sure it gets treated as a session cookie when the date parser fails seems like a decent first priority. (I'm on vacation atm so I only cheer from the sidelines right now)

@jay
Member
jay commented Mar 5, 2016

It is treated as a session cookie but not in the function that purges expired cookies so that's a problem. That function will check against expires if there's an expirestr, however it doesn't take into account what happens when expirestr parsing failed and therefore expires is 0. How about this:

diff --git a/lib/cookie.c b/lib/cookie.c
index 1fd97c0..558b6a7 100644
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -304,17 +304,17 @@ static void remove_expired(struct CookieInfo *cookies)
 {
   struct Cookie *co, *nx, *pv;
   curl_off_t now = (curl_off_t)time(NULL);

   co = cookies->cookies;
   pv = NULL;
   while(co) {
     nx = co->next;
-    if((co->expirestr || co->maxage) && co->expires < now) {
+    if(co->expires && co->expires < now) {
       if(co == cookies->cookies) {
         cookies->cookies = co->next;
       }
       else {
         pv->next = co->next;
       }
       cookies->numcookies--;
       freecookie(co);

A side effect of this would be cookies that are read in the Netscape format with an expiry of 0 would expire as well, which I think is correct, no?

The date thing as I noted in my edit is more complicated than I originally thought. The RFCs do not explicitly spell out the order as day month year although it's implied various places. And the latest RFC 6265 allows the tokens in more than one order and gives an algorithm to process them. The dd-mm-yyyy format is not compliant. And I read up on curl_getdate. I could either add the dd-mm-yyyy format to that function as acceptable always or make a flag and a stub or a new function that can only be used internally and is specific to cookies, this way the public api will not be affected.

@jay
Member
jay commented Mar 5, 2016

Uh that side effect statement is wrong. It seems that by the current logic cookies that were read in the Netscape format cannot be purged by remove_expired and with the proposed change to that logic those cookies can now be purged the same as Set-Cookie style cookies, which afaict would be correct.

@bagder
Member
bagder commented Mar 5, 2016

It looks correct to me!

@jay jay added a commit that referenced this issue Mar 5, 2016
@jay jay cookie: Don't expire session cookies in remove_expired
Prior to this change cookies with an expiry date that failed parsing
and were converted to session cookies could be purged in remove_expired.

Bug: #697
Reported-by: Seth Mos
20de9b4
@captain-caveman2k
Member

Unfortunately this seems to break Test 46 removing the www.fake.come and www.loser.com entries:

steve@server:~/Development/curl/tests$ ./runtests.pl 46
test 0046...[HTTP, get cookies and store in cookie jar]
46: output (log/jar46) FAILED:
--- log/check-expected 2016-03-06 13:51:35.397771300 +0000
+++ log/check-generated 2016-03-06 13:51:35.397771300 +0000
@@ -2,8 +2,6 @@
# https://curl.haxx.se/docs/http-cookies.html[LF]
# This file was generated by libcurl! Edit at your own risk.[LF]
[LF]
-www.fake.come FALSE / FALSE 1022144953 cookiecliente si[LF]
-www.loser.com FALSE / FALSE 1139150993 UID 99[LF] 127.0.0.1 FALSE / FALSE 1739150993 mooo indeed[LF]
#HttpOnly_127.0.0.1 FALSE /want FALSE 1739150993 mooo2 indeed2[LF] 127.0.0.1 FALSE /want FALSE 0 empty [LF]
- abort tests

For more detail please see the autobuilds.

@bagder
Member
bagder commented Mar 6, 2016

I can confirm that. That is of course because those are expired cookies and the newly modified expiry function will now (correctly) expire them! I propose the fix for this is to bump the numerical to the 2 billion range.

@bagder bagder added a commit that referenced this issue Mar 6, 2016
@bagder bagder test46: change cookie expiry date
Since two of the cookies would now otherwise expire and cause the test
to fail after commit 20de9b4

Discussed in #697
e6293cf
@bagder
Member
bagder commented Mar 6, 2016

Fixed in commit e6293cf

@smos
smos commented Mar 7, 2016

I just verified that this fix works for me, I put a patched version on our production box and it's doing it's thing again. Now, to contact people to get 250 base stations power cycled. :)

# Netscape HTTP Cookie File
# http://curl.haxx.se/docs/http-cookies.html
# This file was generated by libcurl! Edit at your own risk.

10.9.43.20      FALSE   /       FALSE   0       key     85586456
@jay
Member
jay commented Mar 7, 2016

Thanks for the update Seth. Please be aware there are two issues here:

  1. Cookies with an invalid expiry date of dd-mm-yy[yy] are converted to session cookies
  2. Cookies with an invalid expiry date are incorrectly expired

I've only solved #2 not #1. As I mentioned earlier both FF and IE accept that date format. I have written a function to correctly handle the format, and I will upload it for review soon.

Right now because #2 is solved but #1 is not your cookie is being turned into a session cookie because its expiry date can't be parsed. That will probably be fine as long as you don't junk session cookies, but please tread carefully until we fully resolve this.

@bagder
Member
bagder commented Mar 12, 2016

I consider this issue fixed, as it was brought back to the state it was before. Fixing the parsing of that non-compliant date format could be considered an added bonus but nothing to lose any sleep over since it has to be a very small fraction of the world's cookies using that.

@bagder bagder closed this Mar 23, 2016
@RubtsovAV RubtsovAV added a commit to RubtsovAV/serps-http-client-curl that referenced this issue May 9, 2016
@RubtsovAV RubtsovAV Added the cURL version requirements
In that version was fixed the bug, which looses Cookie information when CURLOPT_FOLLOWLOCATION is true. 

curl/curl#697
4e3aa82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment