classification
Title: SSL_shutdown needs SSL_read() until SSL_ERROR_ZERO_RETURN
Type: Stage: patch review
Components: SSL Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: alex, christian.heimes, dstufft, janssen, martin.panter, njs
Priority: high Keywords: patch

Created on 2017-05-23 03:44 by njs, last changed 2018-05-23 13:55 by christian.heimes.

Pull Requests
URL Status Linked Edit
PR 6977 closed christian.heimes, 2018-05-18 20:22
Messages (8)
msg294216 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2017-05-23 03:44
The SSL_shutdown man page says that if it returns 0, and an SSL_ERROR_SYSCALL is set, then SSL_ERROR_SYSCALL should be ignored - or at least I think that's what it's trying to say. See the RETURN VALUES section. I think this means we should only raise this error if the return value is <0? Though I suppose we need to clear out the error queue in any case.

I ended up changing the code that was triggering this for other reasons and now I'm not hitting it, so it's not exactly urgent for me, but FYI... I was getting SSLSyscallError exceptions from code using memory BIOs and where everything was fine. The test case had one task continually sending data into an SSLObject-based stream while the other end called SSLObject.unwrap() and then ended up continually getting SSLWantRead and reading more data -- after a few cycles of reading it got SSLSyscalError instead.
msg294245 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-05-23 11:47
Maybe Issue 10808?
msg294299 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-05-24 00:12
Which OS and OpenSSL version are you on?
msg294301 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2017-05-24 00:19
Debian testing, x86-64, with:

Python 3.5.3rc1 (default, Jan  3 2017, 04:40:57) 
[GCC 6.3.0 20161229] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ssl
>>> ssl.OPENSSL_VERSION
'OpenSSL 1.1.0e  16 Feb 2017'
msg301472 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-09-06 14:11
If I understand the man page of SSL_shutdown correctly, than SSL_shutdown() must be called a second time when the first time returned 0. But it does not say how an application shall behave if the second call to SSL_shutdown() also returns 0.

OpenSSL does not contain an example for bidirectional shutdown. s_client.c only does unidirectional shutdown.

cURL just ignores the result:

/*
 * This function is called when an SSL connection is closed.
 */
void Curl_ossl_close(struct connectdata *conn, int sockindex)
{
  struct ssl_connect_data *connssl = &conn->ssl[sockindex];

  if(connssl->handle) {
    (void)SSL_shutdown(connssl->handle);
    SSL_set_connect_state(connssl->handle);

    SSL_free (connssl->handle);
    connssl->handle = NULL;
  }
  if(connssl->ctx) {
    SSL_CTX_free (connssl->ctx);
    connssl->ctx = NULL;
  }
}
msg301491 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2017-09-06 17:43
My reading of the man page is that if SSL_shutdown returns 0, this means that it succeeded at doing the first phase of shutdown. If there are errors then they should be ignored, because it actually succeeded.

If you want to then complete the second phase of shutdown, of course, you have to call it again, but that's no different than any other use of SSL_shutdown.

If two calls to SSL_shutdown both return zero, then that's definitely a bug in OpenSSL. A return value of zero means that previously the SSL_SENT_SHUTDOWN flag was not set, and now it is set, so that can only happen once per connection. But that's orthogonal to the SSL_ERROR_SYSCALL issue.
msg317028 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-05-18 13:09
The issue can occur when the peer sends data while processing the close notify alert.

The meaningless SSL_ERROR_SYSCALL in SSL_shutdown() is even more severe with OpenSSL 1.1.1 and TLS 1.3. In case the client only writes and never reads, SSL_shutdown fails to unwrap the socket. The issue is caused by the fact that the session ticket is transferred after the handshake. With a SSL_read(), the ticket is stuck on the wire and may not be consumed until OpenSSL waits for close notify TLS alert.

In https://github.com/openssl/openssl/issues/6262#issuecomment-389987860 Kurt wrote:

> The peer is still allowed to send data after receiving the "close notify" event. If the peer did send data it need to be processed by calling SSL_read() before calling SSL_shutdown() a second time. SSL_read() will indicate the end of the peer data by returning <= 0 and SSL_get_error() returning SSL_ERROR_ZERO_RETURN. It is recommended to call SSL_read() between SSL_shutdown() calls.

I hacked SSL_read() into the code and the problem did no longer occur for me. My workaround is not a proper solution, though. The shutdown code is slightly complicated (to say the least).
msg317407 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-05-23 13:55
The session ticket issue in TLS 1.3 handshake will be fixed by upstream patch https://github.com/openssl/openssl/pull/6340.

We still need to drain the SSL socket to remove pending application data before the second SSL_shutdown() call, but it's no longer critical to get basic TLS 1.3 support working.

Ned, I'm downgrading this issue from blocker to high.
History
Date User Action Args
2018-05-23 13:55:30christian.heimessetpriority: deferred blocker -> high

messages: + msg317407
2018-05-18 20:22:21christian.heimessetkeywords: + patch
stage: patch review
pull_requests: + pull_request6632
2018-05-18 13:09:05christian.heimessetpriority: normal -> deferred blocker

title: SSL_shutdown can return meaningless SSL_ERROR_SYSCALL -> SSL_shutdown needs SSL_read() until SSL_ERROR_ZERO_RETURN
messages: + msg317028
versions: + Python 3.8
2017-09-06 17:43:18njssetmessages: + msg301491
2017-09-06 14:11:38christian.heimessetmessages: + msg301472
2017-05-24 00:19:54njssetmessages: + msg294301
2017-05-24 00:12:13christian.heimessetmessages: + msg294299
2017-05-23 11:47:48martin.pantersetnosy: + martin.panter
messages: + msg294245
2017-05-23 03:44:13njscreate