CURLOPT_COOKIELIST and Set-Cookie without a domain #292

Closed
jay opened this Issue May 25, 2015 · 8 comments

Comments

Projects
None yet
2 participants
@jay
Member

jay commented May 25, 2015

I struggled with the best way to explain to the user that using CURLOPT_COOKIELIST and Set-Cookie without a domain means the cookie is used for any domain and will not be modified. The issue was brought up on the mailing list recently. I came up with some basic warning that I added just now in b18a165. But should Set-Cookie without a domain really mean that? It seems too late to change the behavior, so it should be documented.

Also it isn't yet documented that a cookie without a domain is saved with hostname "unknown" and I wonder if it should be. It seems they are not later loaded as any-domain cookies when they are imported. But also it's been like that for so long maybe some users expect that. However what's the goal of saving as unknown, because when you import it it's not like it's going to be used for all domains again. It might be fitting to warn the user of that as well. And what if someone actually has a hostname "unknown"?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 25, 2015

Member

A Set-Cookie: received without a set domain should use the domain actually used in the URL to get that cookie. I believe that's in the cookie spec (and I believe we even have tests for this case). I think the case is different when CURLOPT_COOKIELIST is used as it then doesn't know for which domain that cookie is used.

I don't think an actual cookie received over the wire can end up without a domain, so the case for that is only when passed in via the libcurl API. Or am I wrong?

I'll agree that using "unknown" as domain name is not really reliable, and it would make sense to invent a better way to handle these cookies. I think the point is that it cannot be blank so it needs to be stored somehow and actually I don't think we've ever really fully thought through this case.

Member

bagder commented May 25, 2015

A Set-Cookie: received without a set domain should use the domain actually used in the URL to get that cookie. I believe that's in the cookie spec (and I believe we even have tests for this case). I think the case is different when CURLOPT_COOKIELIST is used as it then doesn't know for which domain that cookie is used.

I don't think an actual cookie received over the wire can end up without a domain, so the case for that is only when passed in via the libcurl API. Or am I wrong?

I'll agree that using "unknown" as domain name is not really reliable, and it would make sense to invent a better way to handle these cookies. I think the point is that it cannot be blank so it needs to be stored somehow and actually I don't think we've ever really fully thought through this case.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay May 25, 2015

Member

I don't think an actual cookie received over the wire can end up without a domain, so the case for that is only when passed in via the libcurl API. Or am I wrong?

It's specific to CURLOPT_COOKIELIST as far as I can tell.

Member

jay commented May 25, 2015

I don't think an actual cookie received over the wire can end up without a domain, so the case for that is only when passed in via the libcurl API. Or am I wrong?

It's specific to CURLOPT_COOKIELIST as far as I can tell.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 26, 2015

Member

I think we could start with not using "unknown" for the domain in the file. The question is what we should do instead. Just refuse to save it?

Member

bagder commented May 26, 2015

I think we could start with not using "unknown" for the domain in the file. The question is what we should do instead. Just refuse to save it?

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay May 26, 2015

Member

I think we could start with not using "unknown" for the domain in the file. The question is what we should do instead. Just refuse to save it?

I can't think of any reason to continue except we could break a user's script. I've been racking my brain to figure out just what such a script does though.

Member

jay commented May 26, 2015

I think we could start with not using "unknown" for the domain in the file. The question is what we should do instead. Just refuse to save it?

I can't think of any reason to continue except we could break a user's script. I've been racking my brain to figure out just what such a script does though.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 31, 2015

Member

I think we should refuse to save it to start with, as then we maintain the functionality at least but we stop the creation of a domain that wasn't used to begin with.

Member

bagder commented May 31, 2015

I think we should refuse to save it to start with, as then we maintain the functionality at least but we stop the creation of a domain that wasn't used to begin with.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jun 1, 2015

Member

I wonder if it might be better to comment them out, that way if someone has a script that needs them they can still get to them?

 static char *get_netscape_format(const struct Cookie *co)
 {
   return aprintf(
+    "%s"     /* comment (cookie missing domain) */
     "%s"     /* httponly preamble */
     "%s%s\t" /* domain */
     "%s\t"   /* tailmatch */
     "%s\t"   /* path */
     "%s\t"   /* secure */
     "%" CURL_FORMAT_CURL_OFF_T "\t"   /* expires */
     "%s\t"   /* name */
     "%s",    /* value */
+    co->domain?"":"# ",

On the other hand code parsing from CURLINFO_COOKIELIST may choke on that.

I plan to change it so cookies without a domain are no longer output re CURLINFO_COOKIELIST and CURLOPT_COOKIEJAR, unless you think above could be necessary.

Member

jay commented Jun 1, 2015

I wonder if it might be better to comment them out, that way if someone has a script that needs them they can still get to them?

 static char *get_netscape_format(const struct Cookie *co)
 {
   return aprintf(
+    "%s"     /* comment (cookie missing domain) */
     "%s"     /* httponly preamble */
     "%s%s\t" /* domain */
     "%s\t"   /* tailmatch */
     "%s\t"   /* path */
     "%s\t"   /* secure */
     "%" CURL_FORMAT_CURL_OFF_T "\t"   /* expires */
     "%s\t"   /* name */
     "%s",    /* value */
+    co->domain?"":"# ",

On the other hand code parsing from CURLINFO_COOKIELIST may choke on that.

I plan to change it so cookies without a domain are no longer output re CURLINFO_COOKIELIST and CURLOPT_COOKIEJAR, unless you think above could be necessary.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 3, 2015

Member

No, I do not think we should do the commenting thing.

Member

bagder commented Jun 3, 2015

No, I do not think we should do the commenting thing.

jay added a commit that referenced this issue Jun 4, 2015

cookie: Stop exporting any-domain cookies
Prior to this change any-domain cookies (cookies without a domain that
are sent to any domain) were exported with domain name "unknown".

Bug: #292
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jun 4, 2015

Member

Ok thanks for the feedback, landed in 3013bb6.

Member

jay commented Jun 4, 2015

Ok thanks for the feedback, landed in 3013bb6.

@jay jay closed this Jun 4, 2015

jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Jun 30, 2015

spz
update of curl to version 7.43.0. Upstream RELEASE_NOTES:
Curl and libcurl 7.43.0

 Public curl releases:         147
 Command line options:         176
 curl_easy_setopt() options:   219
 Public functions in libcurl:  58
 Contributors:                 1291

This release includes the following changes:

 o Added CURLOPT_PROXY_SERVICE_NAME[11]
 o Added CURLOPT_SERVICE_NAME[12]
 o New curl option: --proxy-service-name[13]
 o Mew curl option: --service-name [14]
 o New curl option: --data-raw [5]
 o Added CURLOPT_PIPEWAIT [15]
 o Added support for multiplexing transfers using HTTP/2, enable this
   with the new CURLPIPE_MULTIPLEX bit for CURLMOPT_PIPELINING [16]
 o HTTP/2: requires nghttp2 1.0.0 or later
 o scripts: add zsh.pl for generating zsh completion
 o curl.h: add CURL_HTTP_VERSION_2

This release includes the following bugfixes:

 o CVE-2015-3236: lingering HTTP credentials in connection re-use [30]
 o CVE-2015-3237: SMB send off unrelated memory contents [31]
 o nss: fix compilation failure with old versions of NSS [1]
 o curl_easy_getinfo.3: document 'internals' in CURLINFO_TLS_SESSION
 o schannel.c: Fix possible SEC_E_BUFFER_TOO_SMALL error
 o Curl_ossl_init: load builtin modules [2]
 o configure: follow-up fix for krb5-config [3]
 o sasl_sspi: Populate domain from the realm in the challenge [4]
 o netrc: support 'default' token
 o README: convert to UTF-8
 o cyassl: Implement public key pinning
 o nss: implement public key pinning for NSS backend
 o mingw build: add arch -m32/-m64 to LDFLAGS
 o schannel: Fix out of bounds array [6]
 o configure: remove autogenerated files by autoconf
 o configure: remove --automake from libtoolize call
 o acinclude.m4: fix shell test for default CA cert bundle/path
 o schannel: fix regression in schannel_recv [7]
 o openssl: skip trace outputs for ssl_ver == 0 [8]
 o gnutls: properly retrieve certificate status
 o netrc: Read in text mode when cygwin [9]
 o winbuild: Document the option used to statically link the CRT [10]
 o FTP: Make EPSV use the control IP address rather than the original host
 o FTP: fix dangling conn->ip_addr dereference on verbose EPSV
 o conncache: keep bundles on host+port bases, not only host names
 o runtests.pl: use 'h2c' now, no -14 anymore
 o curlver: introducing new version number (checking) macros
 o openssl: boringssl build brekage, use SSL_CTX_set_msg_callback [17]
 o CURLOPT_POSTFIELDS.3: correct variable names [18]
 o curl_easy_unescape.3: update RFC reference [19]
 o gnutls: don't fail on non-fatal alerts during handshake
 o testcurl.pl: allow source to be in an arbitrary directory
 o CURLOPT_HTTPPROXYTUNNEL.3: only works with a HTTP proxy
 o SSPI-error: Change SEC_E_ILLEGAL_MESSAGE description [20]
 o parse_proxy: switch off tunneling if non-HTTP proxy [21]
 o share_init: fix OOM crash
 o perl: remove subdir, not touched in 9 years
 o CURLOPT_COOKIELIST.3: Add example
 o CURLOPT_COOKIE.3: Explain that the cookies won't be modified [22]
 o CURLOPT_COOKIELIST.3: Explain Set-Cookie without a domain [23]
 o FAQ: How do I port libcurl to my OS?
 o openssl: Use TLS_client_method for OpenSSL 1.1.0+
 o HTTP-NTLM: fail auth on connection close instead of looping [24]
 o curl_setup: Add macros for FOPEN_READTEXT, FOPEN_WRITETEXT [25]
 o curl_getdate.3: update RFC reference
 o curl_multi_info_read.3: added example
 o curl_multi_perform.3: added example
 o curl_multi_timeout.3: added example
 o cookie: Stop exporting any-domain cookies [26]
 o openssl: remove dummy callback use from SSL_CTX_set_verify()
 o openssl: remove SSL_get_session()-using code
 o openssl: removed USERDATA_IN_PWD_CALLBACK kludge
 o openssl: removed error string #ifdef
 o openssl: Fix verification of server-sent legacy intermediates [27]
 o docs: man page indentation and syntax fixes
 o docs: Spelling fixes
 o fopen.c: fix a few compiler warnings
 o CURLOPT_OPENSOCKETFUNCTION: return error at once [28]
 o schannel: Add support for optional client certificates
 o build: Properly detect OpenSSL 1.0.2 when using configure
 o urldata: store POST size in state.infilesize too [29]
 o security:choose_mech remove dead code
 o rtsp_do: remove dead code
 o docs: many HTTP URIs changed to HTTPS
 o schannel: schannel_recv overhaul [32]

This release includes the following known bugs:

 o see docs/KNOWN_BUGS (http://curl.haxx.se/docs/knownbugs.html)

This release would not have looked like this without help, code, reports and
advice from friends like these:

  Alessandro Ghedini, Alexander Dyagilev, Anders Bakken, Anthony Avina,
  Ashish Shukla, Bert Huijben, Brian Chrisman, Brian Prodoehl, Chris Araman,
  Dagobert Michelsen, Dan Fandrich, Daniel Melani, Daniel Stenberg,
  Dmitry Eremin-Solenikov, Drake Arconis, Egon Eckert, Frank Meier, Fred Stluka,
  Gisle Vanem, Grant Pannell, Isaac Boukris, Jens Rantil, Joel Depooter,
  Kamil Dudka, Linus Nielsen Feltzing, Linus Nielsen Feltzing Feltzing,
  Liviu Chircu, Marc Hoersken, Michael Osipov, Oren Souroujon, Orgad Shaneh,
  Patrick Monnerat, Patrick Rapin, Paul Howarth, Paul Oliver, Rafayel Mkrtchyan,
  Ray Satiro, Sean Boudreau, Tatsuhiro Tsujikawa, Tomas Tomecek, Viktor Szakáts,
  Ville Skyttä, Yehezkel Horowitz,
  (43 contributors)

        Thanks! (and sorry if I forgot to mention someone)

References to bug reports and discussions on issues:

 [1] = http://curl.haxx.se/mail/lib-2015-04/0095.html
 [2] = curl/curl#206
 [3] = curl/curl@5b66860#commitcomment-10473445
 [4] = curl/curl#141
 [5] = curl/curl#198
 [6] = http://curl.haxx.se/mail/lib-2015-04/0199.html
 [7] = curl/curl#244
 [8] = curl/curl#219
 [9] = curl/curl#258
 [10] = curl/curl#254
 [11] = http://curl.haxx.se/libcurl/c/CURLOPT_PROXY_SERVICE_NAME.html
 [12] = http://curl.haxx.se/libcurl/c/CURLOPT_SERVICE_NAME.html
 [13] = http://curl.haxx.se/docs/manpage.html#--proxy-service-name
 [14] = http://curl.haxx.se/docs/manpage.html#--service-name
 [15] = http://curl.haxx.se/libcurl/c/CURLOPT_PIPEWAIT.html
 [16] = http://curl.haxx.se/libcurl/c/CURLMOPT_PIPELINING.html
 [17] = curl/curl#275
 [18] = curl/curl#281
 [19] = curl/curl#282
 [20] = curl/curl#267
 [21] = http://curl.haxx.se/mail/lib-2015-05/0056.html
 [22] = http://curl.haxx.se/mail/lib-2015-05/0115.html
 [23] = http://curl.haxx.se/mail/lib-2015-05/0137.html
 [24] = curl/curl#256
 [25] = curl/curl#258 (comment)
 [26] = curl/curl#292
 [27] = https://rt.openssl.org/Ticket/Display.html?id=3621&user=guest&pass=guest
 [28] = http://curl.haxx.se/mail/lib-2015-06/0047.html
 [29] = http://curl.haxx.se/mail/lib-2015-06/0019.html
 [30] = http://curl.haxx.se/docs/adv_20150617A.html
 [31] = http://curl.haxx.se/docs/adv_20150617B.html
 [32] = curl/curl#244

jgsogo added a commit to jgsogo/curl that referenced this issue Oct 19, 2015

cookie: Stop exporting any-domain cookies
Prior to this change any-domain cookies (cookies without a domain that
are sent to any domain) were exported with domain name "unknown".

Bug: curl#292

@curl curl 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.