Opened 7 years ago

Closed 7 years ago

#6782 defect closed invalid (invalid)

TLS handshake code is wrong

Reported by: Andy Lutomirski Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: habnabit Branch:
Author:

Description (last modified by Jean-Paul Calderone)

Quoting from the manpage for SSL_do_handshake:

If the underlying BIO is non-blocking, SSL_do_handshake() will also return when the underlying BIO could not satisfy the needs of SSL_do_handshake() to continue the handshake. In this case a call to SSL_get_error() with the return value of SSL_do_handshake() will yield SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE. The calling process then must repeat the call after taking appropriate action to satisfy the needs of SSL_do_handshake().

The code in twisted.protocols.tls is completely wrong, and I suspect that it only works at all because OpenSSL is buggy. If we fix it, then it becomes possible to fix #6024 (because we'll actually know when the handshake is done) and we can actually report handshake errors correctly (although getting them logged in, say, a web server will require forwarding them to the underlying protocol).

Here's a partial fix. It appears to work. I don't know how to write test cases for it without resorting to some kind of end-to-end test.

_flushReceiveBIO is still wrong, but it's no worse than before. (It should handle WantWriteError.) Simiarly, _write is busted (and this may be a real-world problem if programs write more than 215 bytes in one go after the handshake is done).

If something like this is applied, it's probably worth calling pauseProducing at startup and then resuming when the handshake is done.

Attachments (1)

tls.patch (5.6 KB) - added by Andy Lutomirski 7 years ago.
Partial fix

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by Andy Lutomirski

Attachment: tls.patch added

Partial fix

comment:1 Changed 7 years ago by Jean-Paul Calderone

Description: modified (diff)

comment:2 Changed 7 years ago by Jean-Paul Calderone

Description: modified (diff)

comment:3 Changed 7 years ago by Jean-Paul Calderone

Description: modified (diff)

Thanks.

comment:4 Changed 7 years ago by Jean-Paul Calderone

Thanks for your interest in this area of Twisted. If you could include a failing unit test that demonstrates the problem, that would help a lot.

If you can't write a unit test, then any simple, self-contained, complete example that demonstrates the problem would still be of help.

comment:5 Changed 7 years ago by habnabit

Unrelatedly, the attached patch includes a topfile with a description and ticket number for a different ticket. Namely, #6768.

comment:6 Changed 7 years ago by habnabit

Cc: habnabit added

comment:7 Changed 7 years ago by Andy Lutomirski

Oddly, I don't have an actual problem that this fixes, unless you consider the fact that Twisted has no idea when the handshake finishes or how it fails to be a problem. (Or the fact that there's a decent chance that the existing code will blow up in some future OpenSSL release.)

I can probably add some function to ITLSTransport that returns a Deferred that indicates completion or failure of the handshake, and I can add tests for that.

What test infrastructure should I use here? Is there something that will let me attach to tls protocols to each other?

comment:8 Changed 7 years ago by Andy Lutomirski

And sorry about the topfile. I'll probably switch to using the git mirror, thereby restoring my sanity :)

comment:9 Changed 7 years ago by Andy Lutomirski

Hmm. I read the docs more carefully, and I'm not convinced that the code is as wrong as I thought. It seems like it's okay except that it gets a bit confused when the handshake finishes if nothing has been written and there's no data available to be read.

So I think the right thing to do is to mark this ticket invalid and move the discussion to #6204, which I'm about to submit a patch for.

comment:10 Changed 7 years ago by Jean-Paul Calderone

Resolution: invalid
Status: newclosed

So I think the right thing to do is to mark this ticket invalid and move the discussion to #6204, which I'm about to submit a patch for.

I think you mean #6024.

Thanks for the follow-up.

Closing as invalid now.

Note: See TracTickets for help on using tickets.