libcurl does not accept cookies for localhost when built --with-libpsl #658

Closed
m6w6 opened this Issue Feb 15, 2016 · 20 comments

Projects

None yet

5 participants

@m6w6
Contributor
m6w6 commented Feb 15, 2016

Cross-filing from rockdaboot/libpsl#48

Whatever the outcome in above issue will be, I think curl needs to address this issue, too.

Currently libcurl won't accept any cookies from single-label domains, i.e. bare hostnames like localhost, when built --with-libpsl support.

Two questions which come immediately to mind are:

  • should curl ask PSL when cookie-domain equals http-host?
  • should curl ask PSL when http-host is a single label?
@bagder
Member
bagder commented Feb 15, 2016

Agreed. Let's first work out the issue in the libpsl end and then based on that we figure what if anything we need to do here.

@bagder bagder self-assigned this Feb 15, 2016
@bagder
Member
bagder commented Feb 15, 2016

One thing that was made clear with this: we need a way for specific requests/handles to disable the use of PSL even if that libcurl version was built to enable it by default.

@bagder bagder referenced this issue in rockdaboot/libpsl Feb 16, 2016
Closed

Any single label is considered a TLD #48

@remicollet

Better to write here ;)

Looking at other consumer of libpsl (e.g. wget), it seems using psl_is_cookie_domain_acceptable could be a better solution than psl_is_public_suffix.

psl_is_cookie_domain_acceptable(psl, localhost, localhost) => 1

See: http://git.savannah.gnu.org/cgit/wget.git/tree/src/cookies.c#n527

@rockdaboot
Contributor

Could you try this patch and give feedback ? I am sorry, off-office in 5 mins... back in a few hours.

diff --git a/lib/cookie.c b/lib/cookie.c
index c542476..3033837 100644
--- a/lib/cookie.c
+++ b/lib/cookie.c
@@ -798,9 +798,9 @@ Curl_cookie_add(struct SessionHandle *data,
   /* Check if the domain is a Public Suffix and if yes, ignore the cookie.
      This needs a libpsl compiled with builtin data. */
   if(co->domain && !isip(co->domain) && (psl = psl_builtin()) != NULL) {
-    if(psl_is_public_suffix(psl, co->domain)) {
-      infof(data, "cookie '%s' dropped, domain '%s' is a public suffix\n",
-            co->name, co->domain);
+    if(!psl_is_cookie_domain_acceptable(psl, domain, co->domain)) {
+      infof(data, "cookie '%s' dropped, domain '%s' must not set cookies for '%s'\n",
+            co->name, domain, co->domain);
       freecookie(co);
       return NULL;
     }
@rockdaboot
Contributor

Curl_cookie_add() is sometimes called with domain=NULL - in this case we have to skip the call to psl_is_cookie_domain_acceptable(), the check should be:

if(domain && co->domain && !isip(co->domain) && (psl = psl_builtin()) != NULL) {

In this case test1136 fails - I think it is a failure in the test itself:

 1136: output (log/jar1136.txt) FAILED:
--- log/check-expected  2016-02-16 16:17:35.498629587 +0100
+++ log/check-generated 2016-02-16 16:17:35.498629587 +0100
@@ -4,3 +4,4 @@
 [LF]
 .www.example.ck        TRUE    /       FALSE   0       test2   allowed2[LF]
 .www.ck        TRUE    /       FALSE   0       test4   allowed4[LF]
+.z-1.compute-1.amazonaws.com   TRUE    /       FALSE   0       test5   forbidden5[LF]

 - abort tests

In tests/log/trace1136:

15:17:35.628968 == Info: psl_is_cookie_domain_acceptable(0x7f01504cca30, z-1.compute-1.amazonaws.com, z-1.compute-1.amazonaws.com)
15:17:35.628972 == Info: Added cookie test5="forbidden5" for domain z-1.compute-1.amazonaws.com, path /, expire 0

It is correct to accept cookies from z-1.compute-1.amazonaws.com if the cookie-domain is also z-1.compute-1.amazonaws.com.

@kdudka
Collaborator
kdudka commented Mar 2, 2016

curl tests 46, 62, 179, 506, and 1216 also fail with the above patch applied. Is this expected?

@rockdaboot
Contributor

I still have not updated curl since I made the above change (just freshly compiled via 'make check'):

$ cd tests
$ ./runtests.pl -n 46 62 179 506 1216
********* System characteristics ******** 
* curl 7.47.2-DEV (x86_64-pc-linux-gnu) 
* libcurl/7.47.2-DEV OpenSSL/1.0.2g zlib/1.2.8 libidn/1.32 libpsl/0.11.0 (+libicu/55.1) nghttp2/1.8.0
* Features: IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets PSL 
* Host: blitz-lx
* System: Linux blitz-lx 4.4.0-1-amd64 #1 SMP Debian 4.4.2-3 (2016-02-21) x86_64 GNU/Linux
* Servers: HTTP-IPv6 HTTP-unix FTP-IPv6 
* Env: 
***************************************** 
test 0046...[HTTP, get cookies and store in cookie jar]
-pd--oe--- OK (1   out of 5  , remaining: 00:08)
test 0062...[HTTP, send cookies when using custom Host:]
-pd---e--- OK (2   out of 5  , remaining: 00:03)
test 0179...[HTTP using proxy and cookies with path checks]
-pd---e--- OK (3   out of 5  , remaining: 00:01)
test 0506...[HTTP with shared cookie list (and dns cache)]
s----oe--- OK (4   out of 5  , remaining: 00:00)
test 1216...[HTTP cookie domains tailmatching the host name]
-pd---e--- OK (5   out of 5  , remaining: 00:00)
TESTDONE: 5 tests out of 5 reported OK: 100%
TESTDONE: 5 tests were considered during 2 seconds.

$ cd ..
$ src/curl --version
curl 7.47.2-DEV (x86_64-pc-linux-gnu) libcurl/7.47.2-DEV OpenSSL/1.0.2g zlib/1.2.8 libidn/1.32 libpsl/0.11.0 (+libicu/55.1) nghttp2/1.8.0
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets PSL
@rockdaboot
Contributor

Hmmm, curl --version says PSL, but no libpsl is linked here:

$ ldd src/.libs/curl
        linux-vdso.so.1 (0x00007ffd297e9000)
        libcurl.so.4 => /usr/lib/x86_64-linux-gnu/libcurl.so.4 (0x00007f29c8b58000)
        libssl.so.1.0.2 => /usr/lib/x86_64-linux-gnu/libssl.so.1.0.2 (0x00007f29c88ef000)
        libcrypto.so.1.0.2 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.0.2 (0x00007f29c848b000)
        libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007f29c8270000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f29c7ecc000)
        libnghttp2.so.14 => /usr/lib/x86_64-linux-gnu/libnghttp2.so.14 (0x00007f29c7ca7000)
        libidn.so.11 => /usr/lib/x86_64-linux-gnu/libidn.so.11 (0x00007f29c7a73000)
        librtmp.so.1 => /usr/lib/x86_64-linux-gnu/librtmp.so.1 (0x00007f29c7856000)
        libssh2.so.1 => /usr/lib/x86_64-linux-gnu/libssh2.so.1 (0x00007f29c762c000)
        libgssapi_krb5.so.2 => /usr/lib/x86_64-linux-gnu/libgssapi_krb5.so.2 (0x00007f29c73e1000)
        libkrb5.so.3 => /usr/lib/x86_64-linux-gnu/libkrb5.so.3 (0x00007f29c710a000)
        libk5crypto.so.3 => /usr/lib/x86_64-linux-gnu/libk5crypto.so.3 (0x00007f29c6ed9000)
        libcom_err.so.2 => /lib/x86_64-linux-gnu/libcom_err.so.2 (0x00007f29c6cd5000)
        liblber-2.4.so.2 => /usr/lib/x86_64-linux-gnu/liblber-2.4.so.2 (0x00007f29c6ac6000)
        libldap_r-2.4.so.2 => /usr/lib/x86_64-linux-gnu/libldap_r-2.4.so.2 (0x00007f29c6874000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f29c6657000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f29c6453000)
        /lib64/ld-linux-x86-64.so.2 (0x00005640a4142000)
        libgnutls.so.30 => /usr/lib/x86_64-linux-gnu/libgnutls.so.30 (0x00007f29c611c000)
        libhogweed.so.4 => /usr/lib/x86_64-linux-gnu/libhogweed.so.4 (0x00007f29c5ee7000)
        libnettle.so.6 => /usr/lib/x86_64-linux-gnu/libnettle.so.6 (0x00007f29c5caf000)
        libgmp.so.10 => /usr/lib/x86_64-linux-gnu/libgmp.so.10 (0x00007f29c5a2c000)
        libgcrypt.so.20 => /lib/x86_64-linux-gnu/libgcrypt.so.20 (0x00007f29c574b000)
        libkrb5support.so.0 => /usr/lib/x86_64-linux-gnu/libkrb5support.so.0 (0x00007f29c553f000)
        libkeyutils.so.1 => /lib/x86_64-linux-gnu/libkeyutils.so.1 (0x00007f29c533a000)
        libresolv.so.2 => /lib/x86_64-linux-gnu/libresolv.so.2 (0x00007f29c5123000)
        libsasl2.so.2 => /usr/lib/x86_64-linux-gnu/libsasl2.so.2 (0x00007f29c4f07000)
        libp11-kit.so.0 => /usr/lib/x86_64-linux-gnu/libp11-kit.so.0 (0x00007f29c4ca2000)
        libtasn1.so.6 => /usr/lib/x86_64-linux-gnu/libtasn1.so.6 (0x00007f29c4a8f000)
        libgpg-error.so.0 => /lib/x86_64-linux-gnu/libgpg-error.so.0 (0x00007f29c487a000)
        libffi.so.6 => /usr/lib/x86_64-linux-gnu/libffi.so.6 (0x00007f29c4671000)

What is going on ?

@rockdaboot
Contributor

Ups, why this ?

libcurl.so.4 => /usr/lib/x86_64-linux-gnu/libcurl.so.4

I would expect when compiling curl (incl. libcurl), it would be linked with it's own library version by default...

@kdudka
Collaborator
kdudka commented Mar 2, 2016

I am pretty sure that curl is linked with its own library at build time but the run-time shared library resolution is something else. RPATH inside the curl executable is set to the target installation location, not the build-tree location. Thanks to that, no install-time relink is necessary.

If you run src/curl instead of src/.libs/curl, the libtool wrapper should take care of setting the environment for you, so that the "correct" libcurl is used:

$ src/.libs/curl --version
curl 7.47.2-DEV (x86_64-unknown-linux-gnu) libcurl/7.43.0 NSS/3.22 Basic ECC zlib/1.2.8 libidn/1.32 libssh2/1.7.0 nghttp2/1.8.0
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz HTTP2 UnixSockets PSL 

$ src/curl --version
curl 7.47.2-DEV (x86_64-unknown-linux-gnu) libcurl/7.47.2-DEV OpenSSL/1.0.2f zlib/1.2.8 libidn/1.32 libpsl/0.7.0 (+libicu/54.1) libssh2/1.7.0 nghttp2/1.8.0
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: IDN IPv6 Largefile NTLM NTLM_WB SSL libz HTTP2 UnixSockets PSL 
@kdudka
Collaborator
kdudka commented Mar 2, 2016

As for the failing test-cases, they can be actually fixed by adding the NULL-check for domain in Curl_cookie_add() on top of your original patch as you suggest above. I overlooked that comment while trying it previously. Thanks for the patch!

@kdudka
Collaborator
kdudka commented Mar 3, 2016

I have applied the following patch to fix curl in Fedora:

http://pkgs.fedoraproject.org/cgit/rpms/curl.git/tree/0001-curl-7.47.1-psl-localhost.patch?id=e2daf982

Unless anybody objects, I would apply it upstream, too. I am not against fixing psl_is_public_suffix() but the above patch would make it work regardless the version of libpsl.

@rockdaboot
Contributor

psl_is_public_suffix() needs no fix. It works regarding the rules of the Mozilla PSL.
It is simply the wrong function for checking the cookie domains.

@bagder
Member
bagder commented Mar 3, 2016

The function is said to return true if a domain is a public suffix (that's even how it is named), but it returns true even for suffixes that aren't listed as public suffixes. And according to @rockdaboot, that's how it is supposed to work.

But yes. we can certainly work-around this by using a different function.

@rockdaboot
Contributor

psl_is_public_suffix() alone isn't enough to check whether a cookie domain is valid or not - you need some code around it, and that is what psl_is_cookie_domain_acceptable() which in turn calls psl_unregistrable_domain().
You can see above that test1136 was buggy - the bug was detected by using psl_is_cookie_domain_acceptable().

Even if I would change the behavior of psl_is_public_suffix(), the prior code would still be buggy.
That is why I wouldn't call the above patch 'work-around' - it is simply a bug fix.

@bagder
Member
bagder commented Mar 3, 2016

The problem I think you refer to, is about handling cookies with a "single label", not about public suffixes. How we should deal with them is one issue. I believe we introduced this when we removed the dot-counting for accepting cookies in commit 85b9dc8.

Another issue (this one) is that libpsl says that the domain is a public suffix, when it isn't. For that, we have a work-around with another function call.

Do not conflate PSL and single-label domain names.

@rockdaboot
Contributor

The usage of psl_is_public_suffix() prior to the above patch was a mistake/bug. The bug was recognized due to the 'single label issue' (which is not an issue regarding the PSL rules - basic discussion should be done with the maintainers/inventors of the PSL, I am not affiliated with Mozilla/PSL and thus simply the wrong person).
Test1136 is an example that just shows the same bug with a multiple label domain.

Originally, @m6w6 asked two questions (that's what this issue is about):

Two questions which come immediately to mind are:
should curl ask PSL when cookie-domain equals http-host?
should curl ask PSL when http-host is a single label?

And these questions are adressed with the above patch. psl_is_cookie_domain_acceptable() cares for cookie-domain==http-host and also for http-host being a single label. This may be redundant regarding the existing cookie code (and might be optimized in another patch), but it should work for the issuers cases. If you think this is wrong, please give some domain/cookie-domain examples to show that and we can further think about it.

@bagder
Member
bagder commented Mar 3, 2016

I already explained that you don't consider this to be a libpsl bug, but I do.

@rockdaboot
Contributor

Yes I know. But you ignore my answer. libpsl follows the PSL rules. If you consider the PSL rules to be wrong (that's how I understood you), you should discuss this with the right people.

If you think the PSL rules are ok (including the implicit '*' rule that you were talking about), but the libpsl implementation is buggy, give an example and I can work on it.

@bagder
Member
bagder commented Mar 3, 2016

I don't ignore your answer, I just disagree with your take on this. I don't think Mozilla/PSL dictates how your library's function call with a specific name is supposed to work.

@kdudka kdudka closed this in c140bd7 Mar 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment