Ignore EPIPE when sending NewSessionTickets in TLSv1.3 #6944

mattcaswell · 2018-08-13T19:22:03Z

If a client sends data to a server and then immediately closes without
waiting to read the NewSessionTickets then the server can receive EPIPE
when trying to write the tickets and never gets the opportunity to read
the data that was sent. Therefore we ignore EPIPE when writing out the
tickets in TLSv1.3

Fixes #6904

mattcaswell · 2018-08-13T19:22:39Z

Set to WIP because I'd like feedback on the approach. Also I've not done any kind of test yet.

kroeckx · 2018-08-13T20:48:05Z

ssl/statem/statem_srvr.c

You should use get_last_sys_error() instead of errno

kroeckx · 2018-08-13T20:49:25Z

It should probably also handle at least ECONNRESET.

I'm not at all convinced this is a good idea.

kroeckx · 2018-08-13T21:40:41Z

I think we should fix the problem that this is trying to fix, I'm just not sure that this approach is the correct one.

mattcaswell · 2018-08-13T22:06:14Z

I think we should fix the problem that this is trying to fix, I'm just not sure that this approach is the correct one.

I'm not married to this approach if there are alternative suggestions?

mdounin · 2018-08-17T13:02:20Z

I'm not married to this approach if there are alternative suggestions?

Just in case, a better solution might be to issue new tickets right after the server's Finished message, as suggested by RFC 8446:

   Note: Although the resumption master secret depends on the client's
   second flight, a server which does not request client authentication
   MAY compute the remainder of the transcript independently and then
   send a NewSessionTicket immediately upon sending its Finished rather
   than waiting for the client Finished.  This might be appropriate in
   cases where the client is expected to open multiple TLS connections
   in parallel and would benefit from the reduced overhead of a
   resumption handshake, for example.

This is how BoringSSL handles it, and it seems to produce less clutter from application point of view.

davidben · 2018-08-17T15:55:08Z

Small correction: we currently only do that in 0-RTT. It wasn't for this issue but because otherwise the server sees a read-triggered write and applications are often confused by "unusual" I/O patterns. (Read-triggered write is less avoidable on the client, but 0-RTT is sufficiently complex on the client that asking them to handle I/O in general is minor by comparison.)

I don't think we'd thought about this particular issue much. Half-RTT tickets might paper over it a bit but you could still be sad depending on send buffer sizes and the like.

mattcaswell · 2018-08-22T14:39:55Z

Just in case, a better solution might be to issue new tickets right after the server's Finished message, as suggested by RFC 8446:

I'm not sure that really fixes the problem (it just hides it). I'm also worried about creating sessions before the client has been authenticated. So I'm not keen on this approach.

If a client sends data to a server and then immediately closes without waiting to read the NewSessionTickets then the server can receive EPIPE when trying to write the tickets and never gets the opportunity to read the data that was sent. Therefore we ignore EPIPE when writing out the tickets in TLSv1.3 Fixes openssl#6904

mattcaswell · 2018-08-22T14:42:32Z

It should probably also handle at least ECONNRESET.

I'm not clear on when a write will return ECONNRESET and when it will return EPIPE. A quick search on google didn't particularly help. Are you able to explain?

mattcaswell · 2018-08-22T14:44:43Z

I took this out of WIP, and updated the use of errno to get_last_sys_error(). I've not changed it to check for ECONNRESET yet (awaiting feedback as per my question above).

kroeckx · 2018-08-22T16:57:23Z

See https://stackoverflow.com/questions/2974021/what-does-econnreset-mean-in-the-context-of-an-af-local-socket

mattcaswell · 2018-08-23T09:27:59Z

I'm not clear on when a write will return ECONNRESET and when it will return EPIPE. A quick search on google didn't particularly help. Are you able to explain?

Thanks for the link. So reading the answer in that link it seems to me that the behaviour is quite platform and socket type dependent. But, at least on Linux for TCP sockets, we could get ECONNRESET in a scenario where we are writing two tickets. The first ticket writes successfully as far as OpenSSL is concerned, but the data has not been written out by the time we get a close from the client. When we then attempt to write the second ticket we would get ECONNRESET.

So, I think you are right - we do need to handle ECONNRESET too.

mattcaswell · 2018-08-23T09:36:42Z

Fixup commit pushed adding a check for ECONNRESET.

t-j-h · 2018-08-31T23:49:16Z

ssl/statem/statem_srvr.c

+#if defined(EPIPE) && defined(ECONNRESET)
+            if (SSL_get_error(s, 0) == SSL_ERROR_SYSCALL
+                    && (get_last_sys_error() == EPIPE
+                        || get_last_sys_error() == ECONNRESET)) {


I think you will find that this is too late to be calling get_last_sys_error in that the underlying functions hit state that is reset in the calls in code paths from the read/write through until here. You need to look at get_last_socket_error() in its usage and reference the error values that are saved at the point of the IO call. That has to be checked.

There is also the function BIO_sock_non_fatal_error (poorly named) where it has the set of things for non-blocking IO - and it would make sense to me to follow the same style and have a function we pass the error to which makes the determination so we can add/remove items.

I think you will find that this is too late to be calling get_last_sys_error in that the underlying functions hit state that is reset in the calls in code paths from the read/write through until here.

I don't think so. We don't try and write anything to the underlying socket until we call statem_flush(). Up until that point we just buffer anything we attempt to write. The statem_flush() causes the buffering BIO to write out all of its data to the underlying socket - and then we immediately test the value of get_last_sys_error(). So there really shouldn't be any code paths that reset this state.

You need to look at get_last_socket_error() in its usage and reference the error values that are saved at the point of the IO call

The difference between get_last_socket_error() and get_last_sys_error is that, on Windows, the former calls GetLastError() whilst the latter calls WSAGetLastError(). I'm not enough of a Windows programmer to know if one is preferred over the other. I don't think there are any error values saved immediately at the point of the IO call. But as long as we don't do any more system calls between where the error is and the point where we check it then that shouldn't be necessary.

There is also the function BIO_sock_non_fatal_error (poorly named) where it has the set of things for non-blocking IO - and it would make sense to me to follow the same style and have a function we pass the error to which makes the determination so we can add/remove items.

Ok - I can do that

The difference between get_last_socket_error() and get_last_sys_error is that, on Windows, the former calls GetLastError() whilst the latter calls WSAGetLastError()

Sorry, its the other way around.

mattcaswell · 2018-09-03T13:56:43Z

I pushed a fixup commit that pulls the error code checking out into a separate function so that we can easily add new values. I also added a call to clear_sys_error() to make sure we don't pick up any stale state.

mattcaswell · 2018-09-04T10:14:31Z

Pushed. Thanks.

If a client sends data to a server and then immediately closes without waiting to read the NewSessionTickets then the server can receive EPIPE when trying to write the tickets and never gets the opportunity to read the data that was sent. Therefore we ignore EPIPE when writing out the tickets in TLSv1.3 Fixes #6904 Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #6944)

bernd-edlinger · 2018-10-31T08:49:51Z

Using errno values won't work when the BIO is a memory stream, right?

mattcaswell · 2018-10-31T09:31:23Z

Using errno values won't work when the BIO is a memory stream, right?

Well - yes - but then I don't think there is a problem if the BIO is a memory stream. We shouldn't get these errors writing to a mem BIO because there is no concept of the peer having closed the connection to the mem BIO. The application may then encounter EPIPE if it later attempts to copy the contents of the mem BIO into a socket fd. But that's really within the application's remit to handle. I don't think we can do much about that.

According to openssl/openssl#6944

* update openssl to v1.1.x * fix bug in Socket:shutdown * Improve tls test cases * add TLSv1_3 ciphers to DEFAULT_CIPHERS * update test keys and certificates to 2048 bits. * Ignore EPIPE and ECONNRESET for TLSv1_3 According to openssl/openssl#6944 * catch luv_tcp_getpeername error sometime, Socket:address() generate error EINVAL: invalid argument

This is important especially when TLS 1.3 is used. > TLSv1.3 sessions are not established during the main handshake. > Instead, after the main handshake is complete, a server may send some > NewSessionTicket messages to the client. If a client sends messages and then closes the connection immediately without reading any NewSessionTickets from the server, the server may not read the messages that were already sent by the client. This issue has been fixed in openssl/openssl#6944, but a full TCP shutdown is still required to make the server read the data. Also: openssl/openssl#6904 Signed-off-by: László Várady <laszlo.varady@protonmail.com>

This is important especially when TLS 1.3 is used. > TLSv1.3 sessions are not established during the main handshake. > Instead, after the main handshake is complete, a server may send some > NewSessionTicket messages to the client. If a client sends messages and then closes the connection immediately without reading any NewSessionTickets from the server, the server may not read the messages that were already sent by the client. This issue has been fixed in openssl/openssl#6944, but a full TCP shutdown is still required to make the server read the data. Also: openssl/openssl#6904 Backported from OSE: da03117a89aab7158d9a6db4d275d0358a71323c Signed-off-by: László Várady <laszlo.varady@protonmail.com> Signed-off-by: Attila Szakacs <attila.szakacs@oneidentity.com>

mattcaswell mentioned this pull request Aug 13, 2018

How can a client write and shutdown TLS 1.3 so that server reads the sent data? #6904

Closed

kroeckx reviewed Aug 13, 2018

View reviewed changes

ssl/statem/statem_srvr.c Outdated

Copy link

Member

kroeckx Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use get_last_sys_error() instead of errno

mattcaswell added this to the 1.1.1 milestone Aug 20, 2018

mattcaswell force-pushed the ignore-epipe branch from 0cf5bb2 to 620ab0c Compare August 22, 2018 14:43

mattcaswell changed the title ~~WIP: Ignore EPIPE when sending NewSessionTickets in TLSv1.3~~ Ignore EPIPE when sending NewSessionTickets in TLSv1.3 Aug 22, 2018

fixup! Ignore EPIPE when sending NewSessionTickets in TLSv1.3

cec045c

t-j-h reviewed Aug 31, 2018

View reviewed changes

fixup! Ignore EPIPE when sending NewSessionTickets in TLSv1.3

04446ab

t-j-h approved these changes Sep 3, 2018

View reviewed changes

mattcaswell closed this Sep 4, 2018

zhaozg mentioned this pull request Apr 1, 2019

update openssl to v1.1.x luvit/luvit#1062

Merged

zhaozg added a commit to zhaozg/luvit that referenced this pull request Apr 1, 2019

Ignore EPIPE and ECONNRESET for TLSv1_3

77289c6

According to openssl/openssl#6944

MrAnno mentioned this pull request Jan 9, 2020

Fix TLS 1.3, memory leaks, and race condition in loggen syslog-ng/syslog-ng#3064

Merged

Uh oh!

Ignore EPIPE when sending NewSessionTickets in TLSv1.3 #6944

Ignore EPIPE when sending NewSessionTickets in TLSv1.3 #6944

Uh oh!

Conversation

mattcaswell commented Aug 13, 2018

Uh oh!

mattcaswell commented Aug 13, 2018

Uh oh!

kroeckx Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

kroeckx commented Aug 13, 2018

Uh oh!

kroeckx commented Aug 13, 2018

Uh oh!

mattcaswell commented Aug 13, 2018

Uh oh!

mdounin commented Aug 17, 2018

Uh oh!

davidben commented Aug 17, 2018

Uh oh!

mattcaswell commented Aug 22, 2018

Uh oh!

mattcaswell commented Aug 22, 2018

Uh oh!

mattcaswell commented Aug 22, 2018

Uh oh!

kroeckx commented Aug 22, 2018

Uh oh!

mattcaswell commented Aug 23, 2018

Uh oh!

mattcaswell commented Aug 23, 2018

Uh oh!

t-j-h Aug 31, 2018

Choose a reason for hiding this comment

Uh oh!

mattcaswell Sep 3, 2018

Choose a reason for hiding this comment

Uh oh!

mattcaswell Sep 3, 2018

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Sep 3, 2018

Uh oh!

mattcaswell commented Sep 4, 2018

Uh oh!

bernd-edlinger commented Oct 31, 2018

Uh oh!

mattcaswell commented Oct 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants