NTLM loops when connection persistence is not available #256

Closed
frenche opened this Issue May 4, 2015 · 13 comments

Comments

Projects
None yet
3 participants
@frenche
Contributor

frenche commented May 4, 2015

Hi,

The following 'curl' command is doomed to fail since NTLM requires a persistent connection to complete the negotiation.

# curl http://my.host.com/ -v -0 -u usera:passa --ntlm
...
curl: (47) Maximum (50) redirects followed

I think we could be more graceful, this commit fixes it for me:
frenche@e0db49a

I can do a pull request if it is fine.
And as soon as I understand how to to it properly :)

Thanks,
Isaac B.

@michael-o

This comment has been minimized.

Show comment
Hide comment
@michael-o

michael-o May 5, 2015

Member

How is NTLM supposed to work where you cannot persist over the connection?

Member

michael-o commented May 5, 2015

How is NTLM supposed to work where you cannot persist over the connection?

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche May 5, 2015

Contributor

It is supposed to fail as authentication-failed (and return 401 and content to the app).
Looping and then failing on max-redirects doesn't sound the best way to handle it.

Contributor

frenche commented May 5, 2015

It is supposed to fail as authentication-failed (and return 401 and content to the app).
Looping and then failing on max-redirects doesn't sound the best way to handle it.

@michael-o

This comment has been minimized.

Show comment
Hide comment
@michael-o

michael-o May 5, 2015

Member

Now I see, you have enabled HTTP 1.0, right? Why should this fail if the server sends a keep-alive?

Member

michael-o commented May 5, 2015

Now I see, you have enabled HTTP 1.0, right? Why should this fail if the server sends a keep-alive?

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche May 5, 2015

Contributor

On Tue, May 5, 2015 at 9:55 PM, Michael Osipov notifications@github.com
wrote:

Now I see, you have enabled HTTP 1.0, right? Why should this fail if the
server sends a keep-alive?

  • Yes (that's the '-0').
  • Not sure, but I think client has to request keep-alive first.

Anyway, the suggested code verifies after fact if the connection is getting
close while negotiating and fails auth (so there was no keep-alive, maybe
better to also check that keep-alive aren't supported so if the connection
was closing for a different reason we should restart auth as it may
succeed).

Thanks.

Contributor

frenche commented May 5, 2015

On Tue, May 5, 2015 at 9:55 PM, Michael Osipov notifications@github.com
wrote:

Now I see, you have enabled HTTP 1.0, right? Why should this fail if the
server sends a keep-alive?

  • Yes (that's the '-0').
  • Not sure, but I think client has to request keep-alive first.

Anyway, the suggested code verifies after fact if the connection is getting
close while negotiating and fails auth (so there was no keep-alive, maybe
better to also check that keep-alive aren't supported so if the connection
was closing for a different reason we should restart auth as it may
succeed).

Thanks.

@michael-o

This comment has been minimized.

Show comment
Hide comment
Member

michael-o commented May 5, 2015

Here is the deal: http://en.wikipedia.org/wiki/HTTP_persistent_connection#HTTP_1.0

Pretty non-standard 👎

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche May 5, 2015

Contributor

Not sure I understand what you mean by the last comment.

Contributor

frenche commented May 5, 2015

Not sure I understand what you mean by the last comment.

@michael-o

This comment has been minimized.

Show comment
Hide comment
@michael-o

michael-o May 6, 2015

Member

I referred to this

Under HTTP 1.0, there is no official specification for how keepalive operates

Member

michael-o commented May 6, 2015

I referred to this

Under HTTP 1.0, there is no official specification for how keepalive operates

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 6, 2015

Member

@michael-o I think that is beside the point, I believe @frenche's case should still be handled better than looping like crazy and his patch is probably a good step in the right direction.

Member

bagder commented May 6, 2015

@michael-o I think that is beside the point, I believe @frenche's case should still be handled better than looping like crazy and his patch is probably a good step in the right direction.

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche May 6, 2015

Contributor

@michael-o note that I've encounter this in prod not in lab testings.
I think it's important to cover such edge cases even though it is becomes tedious sometimes.

@bagder any specific additional steps in mind?

Thanks!

Contributor

frenche commented May 6, 2015

@michael-o note that I've encounter this in prod not in lab testings.
I think it's important to cover such edge cases even though it is becomes tedious sometimes.

@bagder any specific additional steps in mind?

Thanks!

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 10, 2015

Member

@frenche well, a gold star would be handed out if you'd accompany the patch with a test case! =)

Member

bagder commented May 10, 2015

@frenche well, a gold star would be handed out if you'd accompany the patch with a test case! =)

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche May 11, 2015

Contributor

Hi,

On Sun, May 10, 2015 at 4:56 PM, Daniel Stenberg
notifications@github.com wrote:

@frenche well, a gold star would be handed out if you'd accompany the patch with a test case! =)

Anything for a golden star :)

But I think we already have a test case for it, namely:
https://github.com/bagder/curl/blob/master/tests/data/test159

The reason this test currently passes without the fix is because the
'fake' server response is 'HTTP 1.1' and it does not send a
"Connection: close" header.
So it seem 'libcurl' upgrades to 'HTTP 1.1' and keeps the connection
open (although the user requested 1.0, in other words it decides the
HTTP version only based on server response).
I wonder if that is what we expect in such case.

Basically if I change the the 'fake' server response to 'HTTP 1.0' it
fails without the fix and succeeds with the fix applied.
However the 'HTTP 1.1' server response seem okay according to RFC 2145
although I'd expect it to close the connection in order to behave as
'HTTP 1.0'.

Quote from RFC 2145:
One consequence of these rules is that an HTTP/1.1 message sent to an
HTTP/1.0 recipient (or a recipient whose version is unknown) MUST be
constructed so that it remains a valid HTTP/1.0 message when all
headers not defined in the HTTP/1.0 specification [1] are removed.

So this lead me to test leaving the 'fake' server response as 'HTTP
1.1' and adding 'Connection: close' to the response (in test 159).
However my fix did not work for this case because we apparently only
check for 'Connection' header after 'Curl_http_input_auth()' call (the
code flow isn't clear to me but gdb indicated that).
Which means my fix would also probably cause the request to fail when
using 'HTTP 1.0' along with 'Connection: Keep-Alive' although it could
succeed this way.

Currently trying to think of a better alternative for the fix.
Any insight is very welcome.

Thanks,
Isaac B.

Contributor

frenche commented May 11, 2015

Hi,

On Sun, May 10, 2015 at 4:56 PM, Daniel Stenberg
notifications@github.com wrote:

@frenche well, a gold star would be handed out if you'd accompany the patch with a test case! =)

Anything for a golden star :)

But I think we already have a test case for it, namely:
https://github.com/bagder/curl/blob/master/tests/data/test159

The reason this test currently passes without the fix is because the
'fake' server response is 'HTTP 1.1' and it does not send a
"Connection: close" header.
So it seem 'libcurl' upgrades to 'HTTP 1.1' and keeps the connection
open (although the user requested 1.0, in other words it decides the
HTTP version only based on server response).
I wonder if that is what we expect in such case.

Basically if I change the the 'fake' server response to 'HTTP 1.0' it
fails without the fix and succeeds with the fix applied.
However the 'HTTP 1.1' server response seem okay according to RFC 2145
although I'd expect it to close the connection in order to behave as
'HTTP 1.0'.

Quote from RFC 2145:
One consequence of these rules is that an HTTP/1.1 message sent to an
HTTP/1.0 recipient (or a recipient whose version is unknown) MUST be
constructed so that it remains a valid HTTP/1.0 message when all
headers not defined in the HTTP/1.0 specification [1] are removed.

So this lead me to test leaving the 'fake' server response as 'HTTP
1.1' and adding 'Connection: close' to the response (in test 159).
However my fix did not work for this case because we apparently only
check for 'Connection' header after 'Curl_http_input_auth()' call (the
code flow isn't clear to me but gdb indicated that).
Which means my fix would also probably cause the request to fail when
using 'HTTP 1.0' along with 'Connection: Keep-Alive' although it could
succeed this way.

Currently trying to think of a better alternative for the fix.
Any insight is very welcome.

Thanks,
Isaac B.

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche May 19, 2015

Contributor

Hi again,

The function Curl_http_readwrite_headers() is one big loop and we need to hook after all the headers have been parsed as they can come in any order.

Please review PR #280 I think is OK.
I edited test 159 instead of creating a new one (hope it is done OK).

Thanks!

Contributor

frenche commented May 19, 2015

Hi again,

The function Curl_http_readwrite_headers() is one big loop and we need to hook after all the headers have been parsed as they can come in any order.

Please review PR #280 I think is OK.
I edited test 159 instead of creating a new one (hope it is done OK).

Thanks!

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 31, 2015

Member

Merged the fix.

Member

bagder commented May 31, 2015

Merged the fix.

@bagder bagder closed this May 31, 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

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