From 9a3f389fc5d14f5b3ebe80b913ede44e2a7480dc Mon Sep 17 00:00:00 2001 From: Rostislav Kondratenko Date: Sat, 9 Jun 2018 16:48:31 +0300 Subject: [PATCH 1/6] Fix for calling SSLSocket.send with empty input. It should behave as ordinary socket as promised in the docs. --- Modules/_ssl.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 2bce4816d26fe7..9db253288aa328 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2276,10 +2276,18 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b) } do { - PySSL_BEGIN_ALLOW_THREADS - len = SSL_write(self->ssl, b->buf, (int)b->len); - _PySSL_UPDATE_ERRNO_IF(len <= 0, self, len); - PySSL_END_ALLOW_THREADS + if (b->len > 0) { + /*Sending 0 bytes to SSL_write is undefined behaviour re OpenSSL documentation. + * SSLSocket imitates normal socket in Python. + * We have to guard against empty input here. */ + PySSL_BEGIN_ALLOW_THREADS + len = SSL_write(self->ssl, b->buf, (int)b->len); + _PySSL_UPDATE_ERRNO_IF(len <= 0, self, len); + PySSL_END_ALLOW_THREADS + } else { + len = 0; + _PySSL_UPDATE_ERRNO_IF(0, self, len); // Resetting errno + } err = self->ssl_errno; if (PyErr_CheckSignals()) @@ -2310,7 +2318,7 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b) } while (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE); Py_XDECREF(sock); - if (len > 0) + if (len > 0 || b->len == 0) return PyLong_FromLong(len); else return PySSL_SetError(self, len, __FILE__, __LINE__); From 8cd70f8668ff7b9dc084847276c00596cccda782 Mon Sep 17 00:00:00 2001 From: Rostislav Kondratenko Date: Sat, 9 Jun 2018 17:16:32 +0300 Subject: [PATCH 2/6] Added news entry for previous commit --- .../next/Library/2018-06-09-17-15-35.bpo-31711.sOj9dl.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2018-06-09-17-15-35.bpo-31711.sOj9dl.rst diff --git a/Misc/NEWS.d/next/Library/2018-06-09-17-15-35.bpo-31711.sOj9dl.rst b/Misc/NEWS.d/next/Library/2018-06-09-17-15-35.bpo-31711.sOj9dl.rst new file mode 100644 index 00000000000000..5d761ea9668076 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-06-09-17-15-35.bpo-31711.sOj9dl.rst @@ -0,0 +1,2 @@ +Avoiding triggering undefined behaviour from SSL_write when calling +SSLSocket.send() with empty input. From 7c9005908ab9d939df0cd892f71f11b5ece2408b Mon Sep 17 00:00:00 2001 From: Rostislav Kondratenko Date: Thu, 14 Jun 2018 16:15:00 +0300 Subject: [PATCH 3/6] Moving check for empty input in SSLSocket.send() --- Modules/_ssl.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 9db253288aa328..b9d6f27b05de68 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2247,6 +2247,13 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b) "string longer than %d bytes", INT_MAX); goto error; } + if (b->len == 0) { + /*Sending 0 bytes to SSL_write is undefined behaviour per OpenSSL documentation. + * SSLSocket imitates normal socket in Python. + * We have to guard against empty input here. */ + Py_XDECREF(sock); + return PyLong_FromLong(0); + } if (sock != NULL) { /* just in case the blocking state of the socket has been changed */ @@ -2276,18 +2283,10 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b) } do { - if (b->len > 0) { - /*Sending 0 bytes to SSL_write is undefined behaviour re OpenSSL documentation. - * SSLSocket imitates normal socket in Python. - * We have to guard against empty input here. */ - PySSL_BEGIN_ALLOW_THREADS - len = SSL_write(self->ssl, b->buf, (int)b->len); - _PySSL_UPDATE_ERRNO_IF(len <= 0, self, len); - PySSL_END_ALLOW_THREADS - } else { - len = 0; - _PySSL_UPDATE_ERRNO_IF(0, self, len); // Resetting errno - } + PySSL_BEGIN_ALLOW_THREADS + len = SSL_write(self->ssl, b->buf, (int)b->len); + _PySSL_UPDATE_ERRNO_IF(len <= 0, self, len); + PySSL_END_ALLOW_THREADS err = self->ssl_errno; if (PyErr_CheckSignals()) @@ -2318,7 +2317,7 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b) } while (err == SSL_ERROR_WANT_READ || err == SSL_ERROR_WANT_WRITE); Py_XDECREF(sock); - if (len > 0 || b->len == 0) + if (len > 0) return PyLong_FromLong(len); else return PySSL_SetError(self, len, __FILE__, __LINE__); From 49fa684470fc6c94213aaeced07462e9e1a3c682 Mon Sep 17 00:00:00 2001 From: Rostislav Kondratenko Date: Thu, 14 Jun 2018 16:16:00 +0300 Subject: [PATCH 4/6] Adding test for empty input in SSLSocket.send() --- Lib/test/test_ssl.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py index 73d3e3bbcdaeb8..8f16a9a1459616 100644 --- a/Lib/test/test_ssl.py +++ b/Lib/test/test_ssl.py @@ -2466,7 +2466,7 @@ def server_params_test(client_context, server_context, indata=b"FOO\n", sys.stdout.write( " client: sending %r...\n" % indata) s.write(arg) - outdata = s.read() + outdata = s.read() if len(arg) > 0 else b'' if connectionchatty: if support.verbose: sys.stdout.write(" client: read %r\n" % outdata) @@ -2578,6 +2578,14 @@ def test_echo(self): chatty=True, connectionchatty=True, sni_name=hostname) + ## Testing that SSLSopcet can handle empty input + with self.subTest(client=ssl.PROTOCOL_TLS_CLIENT, server=ssl.PROTOCOL_TLS_SERVER): + server_params_test(client_context=client_context, + server_context=server_context, + indata = b'', + chatty=True, connectionchatty=True, + sni_name=hostname) + client_context.check_hostname = False with self.subTest(client=ssl.PROTOCOL_TLS_SERVER, server=ssl.PROTOCOL_TLS_CLIENT): with self.assertRaises(ssl.SSLError) as e: From 8ed37b4c56903b12028961e6366ecf0d72036a02 Mon Sep 17 00:00:00 2001 From: Rostislav Kondratenko Date: Mon, 18 Jun 2018 12:31:17 +0300 Subject: [PATCH 5/6] Fixing misprint in comment --- Lib/test/test_ssl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py index 8f16a9a1459616..a572cf718e6c13 100644 --- a/Lib/test/test_ssl.py +++ b/Lib/test/test_ssl.py @@ -2578,7 +2578,7 @@ def test_echo(self): chatty=True, connectionchatty=True, sni_name=hostname) - ## Testing that SSLSopcet can handle empty input + ## Testing that SSLSocket can handle empty input with self.subTest(client=ssl.PROTOCOL_TLS_CLIENT, server=ssl.PROTOCOL_TLS_SERVER): server_params_test(client_context=client_context, server_context=server_context, From 94fbeb1cd14103ced4e40060d2f314377a80a429 Mon Sep 17 00:00:00 2001 From: Rostislav Kondratenko Date: Wed, 1 Aug 2018 12:30:40 +0300 Subject: [PATCH 6/6] Added missing space in comments --- Modules/_ssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index b9d6f27b05de68..95743a8bc32e7f 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2248,7 +2248,7 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b) goto error; } if (b->len == 0) { - /*Sending 0 bytes to SSL_write is undefined behaviour per OpenSSL documentation. + /* Sending 0 bytes to SSL_write is undefined behaviour per OpenSSL documentation. * SSLSocket imitates normal socket in Python. * We have to guard against empty input here. */ Py_XDECREF(sock);