When reading the manual, it's wasn't really clear to me that the first SSL_shutdown(serverssl) would return 0. But I guess this is also not normal way to actually do a shutdown, both sides deciding at the same time to do a shutdown. I guess at least the manual can be clarified.

Yes - normally you wouldn't both simultaneously decide to shutdown at the same time. This is just for a test. Clarifications on the SSL_shutdown() docs are probably outside of the scope of this PR.

I guess not having seen it, it just falls under the "first to close the connection"

There is an extra space there

Fixed

If I understand correctly, if auto retry is off you get a fatal error.

I think you want to make sure to return SSL_ERROR_WANT_READ in case auto retry is off and the type was handshake data, like it's done below. That probably also means returning -1 for compatibility reasons.

It also seems trivial to add a call to SSLfatal() in case the type was application data.

If this is a handshake message then it should probably be processed, instead of discarded?
I mean something like that (or maybe more specific which message exactly are expected):

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 61010f4..0b983df 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -1583,14 +1583,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
         goto start;
     }
 
-    if (s->shutdown & SSL_SENT_SHUTDOWN) { /* but we have not received a
-                                            * shutdown */
-        s->rwstate = SSL_NOTHING;
-        SSL3_RECORD_set_length(rr, 0);
-        SSL3_RECORD_set_read(rr);
-        return 0;
-    }
-
     if (SSL3_RECORD_get_type(rr) == SSL3_RT_CHANGE_CIPHER_SPEC) {
         SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES,
                  SSL_R_CCS_RECEIVED_EARLY);
@@ -1644,6 +1636,14 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
         goto start;
     }
 
+    if (s->shutdown & SSL_SENT_SHUTDOWN) { /* but we have not received a
+                                            * shutdown */
+        s->rwstate = SSL_NOTHING;
+        SSL3_RECORD_set_length(rr, 0);
+        SSL3_RECORD_set_read(rr);
+        return 0;
+    }
+
     switch (SSL3_RECORD_get_type(rr)) {
     default:
         /*

I think you want to make sure to return SSL_ERROR_WANT_READ in case auto retry is off and the type was handshake data, like it's done below. That probably also means returning -1 for compatibility reasons.

Yes, this makes sense. I made that change.

It also seems trivial to add a call to SSLfatal() in case the type was application data.

I think this could be a breaking change if we did that. At the moment you can just keep calling SSL_shutdown(), ignoring any errors, until you eventually get a 1 return. If you call SSLfatal() it marks the SSL object as unusable, and any subsequent calls to SSL_shutdown() will immediately fail, i.e. you will never get a 1 return. We could just call SSLerr - but that might risk lots of stale errors on the queue.

Because we previously we considered that receiving application data during shutdown was a non fatal, recoverable, error. You want to change it into a fatal error. That's a behaviour change not a bug fix.

My recollection is that SSL_shutdown returns a SSL_ERROR_SSL.
And if an application is retrying SSL_shutdown it is unpredictable if
the SSL will recover or crash.

It actually returns SSL_ERROR_SYSCALL, because nobody bothered to actually say that there was an error, so we just return SSL_ERROR_SYSCALL. But both are fatal errors. First returning a fatal error and then returning it was successful after all doesn't make sense. Calling SSLfatal() should turn it into a SSL_ERROR_SSL.

Hmmm....I suppose existing application code has to be able to handle fatal errors to deal with stuff like the underlying TCP connection closing without a close_notify...so this would appear the same as if that occurred.

I made this case a fatal error as requested.

I don't understand why this code is moved.

me neither.

If we are in the half-shutdown state we are discarding handshake messages. Therefore we don't want to fill up the handshake fragment storage with data that we plan to discard. Therefore we move the fragment storage code until after decisions have been made about whether to discard the handshake data.

If 2 people don't understand this, it at least seems to lack a comment explaining this.

I added a comment.

I think the above 2 lines should get removed, tst == 5 is always false and is covered in the else below.

Fixed.

I know -1 and 0 are really documented as the same, but old documentation was confusing. I suggest that we return -1 in the if and 0 on the else.

The return value from this function does not necessarily translate to the return value from the top level function. -1 and 0 are documented as the same for SSL_read(), but if you've called SSL_read() then you won't get here anyway. (Almost) everywhere else in this function we return -1 after SSLfatal(), so it seems to make more sense to me to follow that. It makes little difference in practice since ssl3_shutdown() ignores the return value anyway.

Is this always SSL3_RT_APPLICATION_DATA in the else?

Well, it isn't SSL3_RT_HANDSHAKE (because we handled that in the main branch of the if), and it isn't SSL3_RT_ALERT (because that is handled in the if immediately preceding this one). If it's anything other than SSL3_RT_APPLICATION_DATA then it's illegal in this context.