bpo-31711: Fix for calling SSLSocket.send with empty input. #7559

rkondratenko · 2018-06-09T14:03:21Z

SSLSocket in Python wraps ordinary sockets and provides SSLSocket.send with same behaviour (except for nonblocking differences mentiond in the documentation). Internally cPython uses OpenSSL as SSL library.
SSLSocket.send() calls to SSL_write() with data to send. However calling SSL_write() with zero-length buffer is undefined behaviour. To emulate ordinary socket behaviour in this case and not to trigger
undefined behaviour from OpenSSL we explicitly check for zero-length buffer before calling SSL_write().

https://bugs.python.org/issue31711

It should behave as ordinary socket as promised in the docs.

the-knights-who-say-ni · 2018-06-09T14:03:23Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

Thanks again to your contribution and we look forward to looking at it!

rkondratenko · 2018-06-09T14:26:03Z

Signed CLA

njsmith · 2018-06-09T19:26:14Z

Thanks for taking this on! Two suggestions:

I think the code will be simpler and easier to understand if you move the check for b->len much earlier, before we get into the actual I/O operations. It looks like a good place would be next to the check for if (b->len > INT_MAX) { ... }
You'll need to add a test, to Lib/test/ssl.py

rkondratenko · 2018-06-15T10:20:18Z

@njsmith I adopted your suggestions. Please take a look.

vadmium · 2018-06-16T12:27:14Z

Lib/test/test_ssl.py

@@ -2578,6 +2578,14 @@ def test_echo(self):
                               chatty=True, connectionchatty=True,
                               sni_name=hostname)

+        ## Testing that SSLSopcet can handle empty input


Is this meant to be SSLSocket?

rkondratenko · 2018-07-03T10:59:58Z

Any onther suggestions? Are you ready to merge?

njsmith · 2018-08-01T07:24:37Z

The PR looks good to me, and I think the change is a good idea. @tiran, are you OK with this change? (Conversation in [bpo-31711](https://bugs.python.org/issue31711) was inconclusive.)

tiran · 2018-08-01T08:38:07Z

I see a potential issue with this patch. Simply speaking, TLS/SSL has two different kinds of messages. There is application data and protocol data. SSL_write and SSL_read not only transmits application data, but they also flush and check for protocol data. That's why a read operation may also send data and a write operation may also read data.

This patch breaks the assumption that send() also takes care of protocol data. With TLS 1.2, it's not a big issue. Most critical protocol data like session and client cert auth are handled in the TLS handshake. However in TLS 1.3, the server side sends a TLS client auth request much later. Usually it occurs after the first write. Speaking of handshake, with this patch, send() may no longer establish a handshake with do_handshake_on_connect=False, too.

I'm -1 on this patch. Let's figure out how to flush / check protocol data and handle implicit handshake correctly.

tiran · 2018-08-01T08:38:44Z

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.


Missing space after /*.

tiran · 2018-08-01T08:39:29Z

Modules/_ssl.c

+        * SSLSocket imitates normal socket in Python.
+        * We have to guard against empty input here. */
+        Py_XDECREF(sock);
+        return PyLong_FromLong(0);


This breaks implicit handshake and possible TLS 1.3 edge cases, see comment.

bedevere-bot · 2018-08-01T08:39:35Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

njsmith · 2018-08-01T09:23:26Z

But in the current module, sslsock.send(b"") doesn't do any of those things, it just invokes "undefined behavior" and kills the connection. So surely this patch is still an improvement? And is it even possible to do any of those things you said without actually sending or receiving data?

rkondratenko · 2018-08-01T09:50:34Z

I did a quick check of OpenSSL docs. There seems to be no easy way to process protocol messages without any other side-effects. Best candidate seems to be SSL_peek[_ex] but is has standard warning:

When a read function operation has to be repeated because SSL_get_error(3) returned SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE, it must be repeated with the same arguments.

This means that we would have to save a flag in SSLSocket to call SSL_peek again with the same parameters on next calls. This would probably violate even more implicit assumtions.

Also, to send data before the end of the handshake in TLS 1.3 one has to explicitly call SSL_write_early_data. Current module doesn't do this anyway.

slingamn · 2019-05-31T11:06:43Z

I recently encountered this issue and was very surprised by it. I very much agree that this patch improves on the current behavior.

Can anything be done to move forward with this?

sylver · 2019-12-04T15:43:58Z

Just encountered this problem. What's the status of this issue ?
It seems staled for a year and a half now.

njsmith · 2019-12-04T23:18:07Z

@tiran This still seems like an improvement to me, and I don't think your concerns are applicable to this case, but I don't want to just ignore them either...

tiran · 2021-04-19T03:59:23Z

Thanks to PEP 644 the issue will be fixed in 3.10 by using SSL_read_ex and SSL_write_ex() functions. I couldn't use the functions earlier because Python had to support older OpenSSL versions and LibreSSL. #25468

Fix for calling SSLSocket.send with empty input.

9a3f389

It should behave as ordinary socket as promised in the docs.

the-knights-who-say-ni added the CLA not signed label Jun 9, 2018

bedevere-bot added the awaiting review label Jun 9, 2018

Added news entry for previous commit

8cd70f8

ned-deily removed the CLA not signed label Jun 9, 2018

the-knights-who-say-ni added the CLA signed label Jun 9, 2018

ned-deily requested review from tiran and njsmith June 9, 2018 15:55

rkondratenko added 2 commits June 14, 2018 16:15

Moving check for empty input in SSLSocket.send()

7c90059

Adding test for empty input in SSLSocket.send()

49fa684

vadmium reviewed Jun 16, 2018

View reviewed changes

Fixing misprint in comment

8ed37b4

tiran requested changes Aug 1, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Aug 1, 2018

Added missing space in comments

94fbeb1

slingamn mentioned this pull request Dec 24, 2019

bpo-31711: On SSLObject.write method, added assert that data has content. #17671

Closed

csabella requested a review from tiran January 26, 2020 01:03

csabella added awaiting core review and removed awaiting changes labels Jan 26, 2020

tiran closed this Apr 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31711: Fix for calling SSLSocket.send with empty input. #7559

bpo-31711: Fix for calling SSLSocket.send with empty input. #7559

rkondratenko commented Jun 9, 2018 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented Jun 9, 2018

rkondratenko commented Jun 9, 2018

njsmith commented Jun 9, 2018

rkondratenko commented Jun 15, 2018

vadmium Jun 16, 2018

rkondratenko commented Jul 3, 2018

njsmith commented Aug 1, 2018 •

edited by bedevere-bot

Loading

tiran commented Aug 1, 2018

tiran Aug 1, 2018

tiran Aug 1, 2018

bedevere-bot commented Aug 1, 2018

njsmith commented Aug 1, 2018

rkondratenko commented Aug 1, 2018

slingamn commented May 31, 2019

sylver commented Dec 4, 2019

njsmith commented Dec 4, 2019

tiran commented Apr 19, 2021

bpo-31711: Fix for calling SSLSocket.send with empty input. #7559

bpo-31711: Fix for calling SSLSocket.send with empty input. #7559

Conversation

rkondratenko commented Jun 9, 2018 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Jun 9, 2018

rkondratenko commented Jun 9, 2018

njsmith commented Jun 9, 2018

rkondratenko commented Jun 15, 2018

vadmium Jun 16, 2018

Choose a reason for hiding this comment

rkondratenko commented Jul 3, 2018

njsmith commented Aug 1, 2018 • edited by bedevere-bot Loading

tiran commented Aug 1, 2018

tiran Aug 1, 2018

Choose a reason for hiding this comment

tiran Aug 1, 2018

Choose a reason for hiding this comment

bedevere-bot commented Aug 1, 2018

njsmith commented Aug 1, 2018

rkondratenko commented Aug 1, 2018

slingamn commented May 31, 2019

sylver commented Dec 4, 2019

njsmith commented Dec 4, 2019

tiran commented Apr 19, 2021

rkondratenko commented Jun 9, 2018 •

edited by bedevere-bot

Loading

njsmith commented Aug 1, 2018 •

edited by bedevere-bot

Loading