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

schannel_recv treats no bytes available as end-of-stream #244

Closed
chris-araman opened this Issue Apr 29, 2015 · 17 comments

Comments

Projects
None yet
4 participants
@chris-araman
Contributor

chris-araman commented Apr 29, 2015

A recent change caused a regression in the behavior of schannel_recv.

Previously, when no more data was available to read, schannel_recv would return -1. After this change, schannel_recv returns 0:
https://sourceforge.net/p/curl/bugs/1462/

This in turn changes the behavior of curl_easy_recv. It used to return CURLE_AGAIN in this scenario, but now returns CURLE_OK and 0 bytes received, as if we've reached the end of the stream.

This in turn causes HTTPS requests to fail intermittently on Windows with errors like CURLE_GOT_NOTHING.

The behavior is reproducible with libcurl 7.42.1.

@chris-araman

This comment has been minimized.

Show comment
Hide comment
@chris-araman

chris-araman Apr 29, 2015

Contributor

Regressed here:
145c263

Contributor

chris-araman commented Apr 29, 2015

Regressed here:
145c263

mback2k added a commit that referenced this issue May 2, 2015

schannel: fix regression in schannel_recv
#244

Commit 145c263 changed the behavior when Curl_read_plain returns
CURLE_AGAIN. We now handle CURLE_AGAIN and SEC_I_CONTEXT_EXPIRED
correctly.
@mback2k

This comment has been minimized.

Show comment
Hide comment
@mback2k

mback2k May 2, 2015

Member

Thanks, merge request #245 just landed in aa99a63.

Member

mback2k commented May 2, 2015

Thanks, merge request #245 just landed in aa99a63.

@mback2k mback2k closed this May 2, 2015

@mback2k mback2k self-assigned this May 2, 2015

jay added a commit to jay/curl that referenced this issue May 3, 2015

schannel: alternate solution for issue 244. draft1
- return 0 if len == 0. that will have to be documented.
- continue on and process the caches regardless of raw recv
- if decrypted data will be returned then set the error code to CURLE_OK
and return its count
- if decrypted data will not be returned and the connection has closed
(eg nread == 0) then return 0 and CURLE_OK
- if decrypted data will not be returned and the connection *hasn't*
closed then set the error code to CURLE_AGAIN --only if an error code
isn't already set-- and return -1
- narrow the Win2k workaround to only Win2k

'schannel_recv treats no bytes available as end-of-stream'
curl#244
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay May 3, 2015

Member

Re sourceforge bug 1462 it basically was schannel_recv returning the count of encrypted bytes returned by raw recv (via Curl_read_plain) rather than the count of decrypted bytes when encrypted bytes were received but no decrypted data was available (either pre or post processed from the cache and/or that just received data). schannel_recv should never return the count of encrypted bytes so that was definitely a bug and was solved by returning 0 in the case no data was decrypted. And that causes this issue.

@chris-araman it looks like you have already figured that out but for anyone else who's interested that's the background.

@mback2k regarding the recent commits related to this issue:

If you go with Chris' solution then when a raw recv fails schannel_recv returns immediately rather than continuing to process any already cached encrypted and decrypted data. That doesn't look right to me, shouldn't it continue on and handle the caches as it did before? For performance even if not required.

Also is it possible for schannel_recv to still return 0 when no decrypted data is available or !len. The former would be a problem, the latter I don't know and it isn't documented for curl_easy_recv what should happen when buflen is 0.

How about:

  • return 0 if len == 0. that will have to be documented.
  • continue on and process the caches regardless of raw recv
  • if decrypted data will be returned then set the error code to CURLE_OK and return its count
  • if decrypted data will not be returned and the connection has closed (eg nread == 0) then return 0 and CURLE_OK
  • if decrypted data will not be returned and the connection hasn't closed then set the error code to CURLE_AGAIN --only if an error code isn't already set-- and return -1
  • narrow the Win2k workaround to only Win2k

A draft is here.

Member

jay commented May 3, 2015

Re sourceforge bug 1462 it basically was schannel_recv returning the count of encrypted bytes returned by raw recv (via Curl_read_plain) rather than the count of decrypted bytes when encrypted bytes were received but no decrypted data was available (either pre or post processed from the cache and/or that just received data). schannel_recv should never return the count of encrypted bytes so that was definitely a bug and was solved by returning 0 in the case no data was decrypted. And that causes this issue.

@chris-araman it looks like you have already figured that out but for anyone else who's interested that's the background.

@mback2k regarding the recent commits related to this issue:

If you go with Chris' solution then when a raw recv fails schannel_recv returns immediately rather than continuing to process any already cached encrypted and decrypted data. That doesn't look right to me, shouldn't it continue on and handle the caches as it did before? For performance even if not required.

Also is it possible for schannel_recv to still return 0 when no decrypted data is available or !len. The former would be a problem, the latter I don't know and it isn't documented for curl_easy_recv what should happen when buflen is 0.

How about:

  • return 0 if len == 0. that will have to be documented.
  • continue on and process the caches regardless of raw recv
  • if decrypted data will be returned then set the error code to CURLE_OK and return its count
  • if decrypted data will not be returned and the connection has closed (eg nread == 0) then return 0 and CURLE_OK
  • if decrypted data will not be returned and the connection hasn't closed then set the error code to CURLE_AGAIN --only if an error code isn't already set-- and return -1
  • narrow the Win2k workaround to only Win2k

A draft is here.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay May 8, 2015

Member

Reopening this due to problems with aa99a63 as described above and since confirmed. Please do not use that fix in production, we're still working on this issue.

Member

jay commented May 8, 2015

Reopening this due to problems with aa99a63 as described above and since confirmed. Please do not use that fix in production, we're still working on this issue.

@mback2k

This comment has been minimized.

Show comment
Hide comment
@mback2k

mback2k May 19, 2015

Member

@jay Did you see my inline comments about your proposed solution? Thanks for that, btw.

Member

mback2k commented May 19, 2015

@jay Did you see my inline comments about your proposed solution? Thanks for that, btw.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay May 19, 2015

Member

@mback2k I did, sorry I left you hanging. I started a second draft and wanted to post it before I got back to you. So far it is similar to the first. I'll probably have it up within a week.

Member

jay commented May 19, 2015

@mback2k I did, sorry I left you hanging. I started a second draft and wanted to post it before I got back to you. So far it is similar to the first. I'll probably have it up within a week.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jun 7, 2015

Member

I just finished draft 4, the branch is here if anyone wants to try it out.

Member

jay commented Jun 7, 2015

I just finished draft 4, the branch is here if anyone wants to try it out.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jun 16, 2015

Member

@bagder I'm pretty much near the end of my work on this but I don't have a review yet. I'd like to get this into 7.43 because as it stands now we have problems with schannel. What I have in the branch covers more corner cases than what's in master now.

It's likely further changes will be needed to address that data-always-pending issue but I cannot reproduce ..yet.

Still I am down to the wire here, please let me know what you think I should do.

Member

jay commented Jun 16, 2015

@bagder I'm pretty much near the end of my work on this but I don't have a review yet. I'd like to get this into 7.43 because as it stands now we have problems with schannel. What I have in the branch covers more corner cases than what's in master now.

It's likely further changes will be needed to address that data-always-pending issue but I cannot reproduce ..yet.

Still I am down to the wire here, please let me know what you think I should do.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 16, 2015

Member

@mback2k is really the most clueful person to get input from on schannel details. I can only like check for code style violations and I know you're very good at not doing such...

So it is really a question of confidence level. I will package the release in ~8 hours so if you truly believe that this patch improves the situation then merge it. If you have doubts I think it is better to be safe than sorry. We could even consider a 7.43.1 release later if we feel a need to get some more bug fixes out before 7.44.0 time. I trust your judgement here!

Member

bagder commented Jun 16, 2015

@mback2k is really the most clueful person to get input from on schannel details. I can only like check for code style violations and I know you're very good at not doing such...

So it is really a question of confidence level. I will package the release in ~8 hours so if you truly believe that this patch improves the situation then merge it. If you have doubts I think it is better to be safe than sorry. We could even consider a 7.43.1 release later if we feel a need to get some more bug fixes out before 7.44.0 time. I trust your judgement here!

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

schannel: schannel_recv overhaul
This commit is several drafts squashed together. The changes from each
draft are noted below. If any changes are similar and possibly
contradictory the change in the latest draft takes precedence.

Bug: #244
Reported-by: Chris Araman

%%
%% Draft 1
%%
- return 0 if len == 0. that will have to be documented.
- continue on and process the caches regardless of raw recv
- if decrypted data will be returned then set the error code to CURLE_OK
and return its count
- if decrypted data will not be returned and the connection has closed
(eg nread == 0) then return 0 and CURLE_OK
- if decrypted data will not be returned and the connection *hasn't*
closed then set the error code to CURLE_AGAIN --only if an error code
isn't already set-- and return -1
- narrow the Win2k workaround to only Win2k

%%
%% Draft 2
%%
- Trying out a change in flow to handle corner cases.

%%
%% Draft 3
%%
- Back out the lazier decryption change made in draft2.

%%
%% Draft 4
%%
- Some formatting and branching changes
- Decrypt all encrypted cached data when len == 0
- Save connection closed state
- Change special Win2k check to use connection closed state

%%
%% Draft 5
%%
- Default to CURLE_AGAIN in cleanup if an error code wasn't set and the
connection isn't closed.

%%
%% Draft 6
%%
- Save the last error only if it is an unrecoverable error.

Prior to this I saved the last error state in all cases; unfortunately
the logic to cover that in all cases would lead to some muddle and I'm
concerned that could then lead to a bug in the future so I've replaced
it by only recording an unrecoverable error and that state will persist.

- Do not recurse on renegotiation.

Instead we'll continue on to process any trailing encrypted data
received during the renegotiation only.

- Move the err checks in cleanup after the check for decrypted data.

In either case decrypted data is always returned but I think it's easier
to understand when those err checks come after the decrypted data check.

%%
%% Draft 7
%%
- Regardless of len value go directly to cleanup if there is an
unrecoverable error or a close_notify was already received. Prior to
this change we only acknowledged those two states if len != 0.

- Fix a bug in connection closed behavior: Set the error state in the
cleanup, because we don't know for sure it's an error until that time.

- (Related to above) In the case the connection is closed go "greedy"
with the decryption to make sure all remaining encrypted data has been
decrypted even if it is not needed at that time by the caller. This is
necessary because we can only tell if the connection closed gracefully
(close_notify) once all encrypted data has been decrypted.

- Do not renegotiate when an unrecoverable error is pending.

%%
%% Draft 8
%%
- Don't show 'server closed the connection' info message twice.

- Show an info message if server closed abruptly (missing close_notify).
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jun 17, 2015

Member

I will package the release in ~8 hours so if you truly believe that this patch improves the situation then merge it.

Landed in 3e7ec1e.

Member

jay commented Jun 17, 2015

I will package the release in ~8 hours so if you truly believe that this patch improves the situation then merge it.

Landed in 3e7ec1e.

@chris-araman

This comment has been minimized.

Show comment
Hide comment
@chris-araman

chris-araman Jun 17, 2015

Contributor

I'm testing 7.43.0 out now. Looks good so far. Thanks, @jay and @mback2k!

Contributor

chris-araman commented Jun 17, 2015

I'm testing 7.43.0 out now. Looks good so far. Thanks, @jay and @mback2k!

@mback2k

This comment has been minimized.

Show comment
Hide comment
@mback2k
Member

mback2k commented Jun 17, 2015

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jun 18, 2015

Member

No problem guys. I'm closing this but Marc if you have some time before the next release and you are able to do a review that would be great.

Member

jay commented Jun 18, 2015

No problem guys. I'm closing this but Marc if you have some time before the next release and you are able to do a review that would be great.

@jay jay closed this Jun 18, 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

schannel: fix regression in schannel_recv
curl#244

Commit 145c263 changed the behavior when Curl_read_plain returns
CURLE_AGAIN. We now handle CURLE_AGAIN and SEC_I_CONTEXT_EXPIRED
correctly.

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

schannel: schannel_recv overhaul
This commit is several drafts squashed together. The changes from each
draft are noted below. If any changes are similar and possibly
contradictory the change in the latest draft takes precedence.

Bug: curl#244
Reported-by: Chris Araman

%%
%% Draft 1
%%
- return 0 if len == 0. that will have to be documented.
- continue on and process the caches regardless of raw recv
- if decrypted data will be returned then set the error code to CURLE_OK
and return its count
- if decrypted data will not be returned and the connection has closed
(eg nread == 0) then return 0 and CURLE_OK
- if decrypted data will not be returned and the connection *hasn't*
closed then set the error code to CURLE_AGAIN --only if an error code
isn't already set-- and return -1
- narrow the Win2k workaround to only Win2k

%%
%% Draft 2
%%
- Trying out a change in flow to handle corner cases.

%%
%% Draft 3
%%
- Back out the lazier decryption change made in draft2.

%%
%% Draft 4
%%
- Some formatting and branching changes
- Decrypt all encrypted cached data when len == 0
- Save connection closed state
- Change special Win2k check to use connection closed state

%%
%% Draft 5
%%
- Default to CURLE_AGAIN in cleanup if an error code wasn't set and the
connection isn't closed.

%%
%% Draft 6
%%
- Save the last error only if it is an unrecoverable error.

Prior to this I saved the last error state in all cases; unfortunately
the logic to cover that in all cases would lead to some muddle and I'm
concerned that could then lead to a bug in the future so I've replaced
it by only recording an unrecoverable error and that state will persist.

- Do not recurse on renegotiation.

Instead we'll continue on to process any trailing encrypted data
received during the renegotiation only.

- Move the err checks in cleanup after the check for decrypted data.

In either case decrypted data is always returned but I think it's easier
to understand when those err checks come after the decrypted data check.

%%
%% Draft 7
%%
- Regardless of len value go directly to cleanup if there is an
unrecoverable error or a close_notify was already received. Prior to
this change we only acknowledged those two states if len != 0.

- Fix a bug in connection closed behavior: Set the error state in the
cleanup, because we don't know for sure it's an error until that time.

- (Related to above) In the case the connection is closed go "greedy"
with the decryption to make sure all remaining encrypted data has been
decrypted even if it is not needed at that time by the caller. This is
necessary because we can only tell if the connection closed gracefully
(close_notify) once all encrypted data has been decrypted.

- Do not renegotiate when an unrecoverable error is pending.

%%
%% Draft 8
%%
- Don't show 'server closed the connection' info message twice.

- Show an info message if server closed abruptly (missing close_notify).
@mback2k

This comment has been minimized.

Show comment
Hide comment
@mback2k

mback2k Dec 22, 2015

Member

@jay Do you think there is an easy way to test the different scenarios schannel_recv has to handle, including re-negotiation? I mean, my autobuilds and personal tests look fine, but schannel_recv was and still is quite complex.

Member

mback2k commented Dec 22, 2015

@jay Do you think there is an easy way to test the different scenarios schannel_recv has to handle, including re-negotiation? I mean, my autobuilds and personal tests look fine, but schannel_recv was and still is quite complex.

@mback2k

This comment has been minimized.

Show comment
Hide comment
@mback2k

mback2k Dec 22, 2015

Member

@jay and @bagder: All in all the current (new) schannel_recv looks a lot better and more stable regarding error handling than my original one. Thank you very much, @jay and everyone who helped.

I am just thinking about the removed Windows 2000 specific alert check. The code still checks for Windows 2000 in https://github.com/bagder/curl/blob/master/lib/vtls/schannel.c#L1113-L1163, but it just assumes that the connection was closed graceful instead of checking for a SSL/TLS alert packet.

I guess the check was removed because the SSL/TLS alert packet could also include data which is now processed (decrypted) and therefore no longer available if not cached for this purpose.

Member

mback2k commented Dec 22, 2015

@jay and @bagder: All in all the current (new) schannel_recv looks a lot better and more stable regarding error handling than my original one. Thank you very much, @jay and everyone who helped.

I am just thinking about the removed Windows 2000 specific alert check. The code still checks for Windows 2000 in https://github.com/bagder/curl/blob/master/lib/vtls/schannel.c#L1113-L1163, but it just assumes that the connection was closed graceful instead of checking for a SSL/TLS alert packet.

I guess the check was removed because the SSL/TLS alert packet could also include data which is now processed (decrypted) and therefore no longer available if not cached for this purpose.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Dec 23, 2015

Member

Marc, Thanks for your positive comments. Unfortunately I am not aware of an easy automated way to test schannel_recv. I tested it manually as I was fixing it but I don't have specific tests in the suite for each change that was made.

Renegotiation: I did not test that. It was unmodified save for the fact it no longer can cause recv recursion and I banned renegotiation when encrypted data was received after the renegotiation message but before libcurl starts it. The former was for security, the latter I got from Chromium or Microsoft's SDK (or a lack of handling that scenario in their code). The code and path for renegotiation once it leaves schannel_recv is unmodified (by me, anyway). I am unaware of a renegotiation test in the suite. Is there an easy way to test renegotiation? I will probably not get around to it myself but I think it would be a good test to have.

Win2k alert: We discussed it earlier this year in draft comments. My sentiment is the same but in addition here is more. To frame this, a connection was closed and then there was a magic byte check 0x15 in the encrypted data. My understanding of this is you were trying to detect whether or not an encrypted TLS alert has taken place (0x15) that wasn't decrypted. I don't see how we could tell from checking the encrypted data whether we've got a valid record that contains a close_notify. It's on schannel to do that, though it doesn't. So in Win2k I think it makes more sense to just assume we have the close_notify if it doesn't let us know for sure. No missing close_notify means no error, though those clients lose protection from truncation attacks.

Member

jay commented Dec 23, 2015

Marc, Thanks for your positive comments. Unfortunately I am not aware of an easy automated way to test schannel_recv. I tested it manually as I was fixing it but I don't have specific tests in the suite for each change that was made.

Renegotiation: I did not test that. It was unmodified save for the fact it no longer can cause recv recursion and I banned renegotiation when encrypted data was received after the renegotiation message but before libcurl starts it. The former was for security, the latter I got from Chromium or Microsoft's SDK (or a lack of handling that scenario in their code). The code and path for renegotiation once it leaves schannel_recv is unmodified (by me, anyway). I am unaware of a renegotiation test in the suite. Is there an easy way to test renegotiation? I will probably not get around to it myself but I think it would be a good test to have.

Win2k alert: We discussed it earlier this year in draft comments. My sentiment is the same but in addition here is more. To frame this, a connection was closed and then there was a magic byte check 0x15 in the encrypted data. My understanding of this is you were trying to detect whether or not an encrypted TLS alert has taken place (0x15) that wasn't decrypted. I don't see how we could tell from checking the encrypted data whether we've got a valid record that contains a close_notify. It's on schannel to do that, though it doesn't. So in Win2k I think it makes more sense to just assume we have the close_notify if it doesn't let us know for sure. No missing close_notify means no error, though those clients lose protection from truncation attacks.

@mback2k

This comment has been minimized.

Show comment
Hide comment
@mback2k

mback2k Dec 23, 2015

Member

@jay You could use my own test URL for that. It works by specifying different (in this case less secure) ciphers for the directory:

        <Location /renegotiate/>
                SSLCipherSuite ALL:!ADH:RC4+RSA:+HIGH:+MEDIUM:+EXP
        </Location>

And it seems to work great:

$ ./src/curl.exe -v https://stuff.marc-hoersken.de/renegotiate/
* STATE: INIT => CONNECT handle 0x31ef00; line 1103 (connection #-5000)
* Added connection 0. The cache now contains 1 members
* timeout on name lookup is not supported
*   Trying 2a00:1828:2000:378:1:80:59ee:542e...
* STATE: CONNECT => WAITCONNECT handle 0x31ef00; line 1156 (connection #0)
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0* Connected to stuff.marc-hoersken.de (2a00:1828:2000:378                                                                                                                                        :1:80:59ee:542e) port 443 (#0)
* STATE: WAITCONNECT => SENDPROTOCONNECT handle 0x31ef00; line 1253 (connection #0)
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 1/3)
* schannel: checking server certificate revocation
* schannel: sending initial handshake data: sending 191 bytes...
* schannel: sent initial handshake data: sent 191 bytes
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: failed to receive handshake, need more data
* STATE: SENDPROTOCONNECT => PROTOCONNECT handle 0x31ef00; line 1267 (connection #0)
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 1440 length 4096
* schannel: encrypted data length: 1338
* schannel: encrypted data buffer: offset 1338 length 4096
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 3994 length 4096
* schannel: encrypted data length: 132
* schannel: encrypted data buffer: offset 132 length 4096
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 2308 length 4096
* schannel: sending next handshake data: sending 182 bytes...
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 107 length 4096
* schannel: SSL/TLS handshake complete
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 3/3)
* schannel: incremented credential handle refcount = 1
* schannel: stored credential handle in session cache
* STATE: PROTOCONNECT => DO handle 0x31ef00; line 1288 (connection #0)
> GET /renegotiate/ HTTP/1.1
> Host: stuff.marc-hoersken.de
> User-Agent: curl/7.47.0-DEV
> Accept: */*
>
* STATE: DO => DO_DONE handle 0x31ef00; line 1350 (connection #0)
* STATE: DO_DONE => WAITPERFORM handle 0x31ef00; line 1477 (connection #0)
* STATE: WAITPERFORM => PERFORM handle 0x31ef00; line 1487 (connection #0)
* schannel: client wants to read 16384 bytes
* schannel: encdata_buffer resized 17408
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: encrypted data got 85
* schannel: encrypted data buffer: offset 85 length 17408
* schannel: decrypted data length: 0
* schannel: decrypted data added: 0
* schannel: decrypted data cached: offset 0 length 16384
* schannel: remote party requests renegotiation
* schannel: renegotiating SSL/TLS connection
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: sending next handshake data: sending 277 bytes...
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 1440 length 17408
* schannel: encrypted data length: 1243
* schannel: encrypted data buffer: offset 1243 length 17408
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 2683 length 17408
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 3899 length 17408
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 5339 length 17408
* schannel: encrypted data length: 1398
* schannel: encrypted data buffer: offset 1398 length 17408
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 2527 length 17408
* schannel: sending next handshake data: sending 335 bytes...
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 186 length 17408
* schannel: SSL/TLS handshake complete
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 3/3)
* schannel: incremented credential handle refcount = 2
* schannel: SSL/TLS connection renegotiated
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: decrypted data buffer: offset 0 length 16384
* schannel: schannel_recv cleanup
* schannel: client wants to read 16384 bytes
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: encrypted data got 650
* schannel: encrypted data buffer: offset 650 length 17408
* schannel: decrypted data length: 194
* schannel: decrypted data added: 194
* schannel: decrypted data cached: offset 194 length 16384
* schannel: encrypted data length: 373
* schannel: encrypted data cached: offset 373 length 17408
* schannel: decrypted data length: 299
* schannel: decrypted data added: 299
* schannel: decrypted data cached: offset 493 length 16384
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: decrypted data buffer: offset 493 length 16384
* schannel: schannel_recv cleanup
* schannel: decrypted data returned 493
* schannel: decrypted data buffer: offset 0 length 16384
* HTTP 1.1 or later with persistent connection, pipelining supported
< HTTP/1.1 404 Not Found
< Date: Wed, 23 Dec 2015 11:20:37 GMT
* Server Apache/2.4.10 (Debian) is not blacklisted
< Server: Apache/2.4.10 (Debian)
< Content-Length: 299
< Content-Type: text/html; charset=iso-8859-1
< Via: 1.1 stuff.marc-hoersken.de
<
{ [299 bytes data]
* STATE: PERFORM => DONE handle 0x31ef00; line 1645 (connection #0)
* Curl_done
100   299  100   299    0     0   1196      0 --:--:-- --:--:-- --:--:--  1196<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>404 Not Found</title>
</head><body>
<h1>Not Found</h1>
<p>The requested URL /renegotiate/ was not found on this server.</p>
<hr>
<address>Apache/2.4.10 (Debian) Server at stuff.marc-hoersken.de Port 443</address>
</body></html>

* Connection #0 to host stuff.marc-hoersken.de left intact

I am not sure if renegotation is testable using stunnel, because that does care about the actual content (in this case the HTTP URL) being transferred.

Member

mback2k commented Dec 23, 2015

@jay You could use my own test URL for that. It works by specifying different (in this case less secure) ciphers for the directory:

        <Location /renegotiate/>
                SSLCipherSuite ALL:!ADH:RC4+RSA:+HIGH:+MEDIUM:+EXP
        </Location>

And it seems to work great:

$ ./src/curl.exe -v https://stuff.marc-hoersken.de/renegotiate/
* STATE: INIT => CONNECT handle 0x31ef00; line 1103 (connection #-5000)
* Added connection 0. The cache now contains 1 members
* timeout on name lookup is not supported
*   Trying 2a00:1828:2000:378:1:80:59ee:542e...
* STATE: CONNECT => WAITCONNECT handle 0x31ef00; line 1156 (connection #0)
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0* Connected to stuff.marc-hoersken.de (2a00:1828:2000:378                                                                                                                                        :1:80:59ee:542e) port 443 (#0)
* STATE: WAITCONNECT => SENDPROTOCONNECT handle 0x31ef00; line 1253 (connection #0)
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 1/3)
* schannel: checking server certificate revocation
* schannel: sending initial handshake data: sending 191 bytes...
* schannel: sent initial handshake data: sent 191 bytes
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: failed to receive handshake, need more data
* STATE: SENDPROTOCONNECT => PROTOCONNECT handle 0x31ef00; line 1267 (connection #0)
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 1440 length 4096
* schannel: encrypted data length: 1338
* schannel: encrypted data buffer: offset 1338 length 4096
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 3994 length 4096
* schannel: encrypted data length: 132
* schannel: encrypted data buffer: offset 132 length 4096
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 2308 length 4096
* schannel: sending next handshake data: sending 182 bytes...
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 107 length 4096
* schannel: SSL/TLS handshake complete
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 3/3)
* schannel: incremented credential handle refcount = 1
* schannel: stored credential handle in session cache
* STATE: PROTOCONNECT => DO handle 0x31ef00; line 1288 (connection #0)
> GET /renegotiate/ HTTP/1.1
> Host: stuff.marc-hoersken.de
> User-Agent: curl/7.47.0-DEV
> Accept: */*
>
* STATE: DO => DO_DONE handle 0x31ef00; line 1350 (connection #0)
* STATE: DO_DONE => WAITPERFORM handle 0x31ef00; line 1477 (connection #0)
* STATE: WAITPERFORM => PERFORM handle 0x31ef00; line 1487 (connection #0)
* schannel: client wants to read 16384 bytes
* schannel: encdata_buffer resized 17408
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: encrypted data got 85
* schannel: encrypted data buffer: offset 85 length 17408
* schannel: decrypted data length: 0
* schannel: decrypted data added: 0
* schannel: decrypted data cached: offset 0 length 16384
* schannel: remote party requests renegotiation
* schannel: renegotiating SSL/TLS connection
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: sending next handshake data: sending 277 bytes...
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 1440 length 17408
* schannel: encrypted data length: 1243
* schannel: encrypted data buffer: offset 1243 length 17408
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 2683 length 17408
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 3899 length 17408
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 5339 length 17408
* schannel: encrypted data length: 1398
* schannel: encrypted data buffer: offset 1398 length 17408
* schannel: received incomplete message, need more data
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 2527 length 17408
* schannel: sending next handshake data: sending 335 bytes...
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 2/3)
* schannel: encrypted data buffer: offset 186 length 17408
* schannel: SSL/TLS handshake complete
* schannel: SSL/TLS connection with stuff.marc-hoersken.de port 443 (step 3/3)
* schannel: incremented credential handle refcount = 2
* schannel: SSL/TLS connection renegotiated
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: decrypted data buffer: offset 0 length 16384
* schannel: schannel_recv cleanup
* schannel: client wants to read 16384 bytes
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: encrypted data got 650
* schannel: encrypted data buffer: offset 650 length 17408
* schannel: decrypted data length: 194
* schannel: decrypted data added: 194
* schannel: decrypted data cached: offset 194 length 16384
* schannel: encrypted data length: 373
* schannel: encrypted data cached: offset 373 length 17408
* schannel: decrypted data length: 299
* schannel: decrypted data added: 299
* schannel: decrypted data cached: offset 493 length 16384
* schannel: encrypted data buffer: offset 0 length 17408
* schannel: decrypted data buffer: offset 493 length 16384
* schannel: schannel_recv cleanup
* schannel: decrypted data returned 493
* schannel: decrypted data buffer: offset 0 length 16384
* HTTP 1.1 or later with persistent connection, pipelining supported
< HTTP/1.1 404 Not Found
< Date: Wed, 23 Dec 2015 11:20:37 GMT
* Server Apache/2.4.10 (Debian) is not blacklisted
< Server: Apache/2.4.10 (Debian)
< Content-Length: 299
< Content-Type: text/html; charset=iso-8859-1
< Via: 1.1 stuff.marc-hoersken.de
<
{ [299 bytes data]
* STATE: PERFORM => DONE handle 0x31ef00; line 1645 (connection #0)
* Curl_done
100   299  100   299    0     0   1196      0 --:--:-- --:--:-- --:--:--  1196<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>404 Not Found</title>
</head><body>
<h1>Not Found</h1>
<p>The requested URL /renegotiate/ was not found on this server.</p>
<hr>
<address>Apache/2.4.10 (Debian) Server at stuff.marc-hoersken.de Port 443</address>
</body></html>

* Connection #0 to host stuff.marc-hoersken.de left intact

I am not sure if renegotation is testable using stunnel, because that does care about the actual content (in this case the HTTP URL) being transferred.

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