ssl: fail gracefully on Alert instead of ClientHello

[Gernot Vormayr]

When openssl encounters an error after it already connected, but before sending a ClientHello, it sends an Alert (might happen with other libraries too). This can happen with, e.g., curl on debian buster with curl --tls-max 1.0 https://localhost/ due to /etc/ssl/openssl.cnf specifying 1.2 as MinVersion. Since nginx inspects the first byte provided by the client and only invokes the ssl part if a ClientHello is detected (https://trac.nginx.org/nginx/browser/nginx/src/http/ngx_http_request.c#L729), it tries to interpret the Alert as plain http, leading to the following log output (2 lines error log, 1 line access log):

2019/08/08 09:59:12 [info] 11#11: *9 client sent invalid method while reading client request line, client: 127.0.0.1, server: host0.domain0.bd, request: "P"
2019/08/08 09:59:12 [info] 11#11: *9 shutdown() failed (107: Transport endpoint is not connected) while reading client request line, client: 127.0.0.1, server: host0.domain0.bd, request: "P"
127.0.0.1 - - [08/Aug/2019:09:59:12 +0000] "\x15\x03\x01\x00\x02\x02P" 400 157 "-" "-"

The attached patch handles this case by logging the error, followed by closing the connection:

2019/08/08 10:00:18 [info] 188#188: *1 peer sent SSL alert while SSL handshaking, client: 127.0.0.1, server: 0.0.0.0:443

[Maxim Dounin]

As long as alerts are allowed before ClientHello, these should be simply passed to SSL layer, much like with other handshake messages. As long as they are not, current behaviour looks correct (expect for additional shutdown() error, which shouldn't happen, though it looks like a separate issue, and not nginx one - as the socket is clearly connected, ENOTCONN error returned by shutdown() looks like a kernel bug).

I don't think I see anything in TLS RFCs which makes alerts explicitly illegal before ClientHello. On the other hand, it doesn't looks like they are meaningful before ClientHello.

Trivial test with the following test patch:

diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -726,7 +726,10 @@ ngx_http_ssl_handshake(ngx_event_t *rev)
     }
 
     if (n == 1) {
-        if (buf[0] & 0x80 /* SSLv2 */ || buf[0] == 0x16 /* SSLv3/TLSv1 */) {
+        if (buf[0] & 0x80 /* SSLv2 */
+            || buf[0] == 0x16 /* SSLv3/TLSv1 */
+            || buf[0] == 0x15 /* SSLv3/TLSv1 */)
+        {
             ngx_log_debug1(NGX_LOG_DEBUG_HTTP, rev->log, 0,
                            "https ssl handshake: 0x%02Xd", buf[0]);
 

suggests that OpenSSL itself thinks alerts are not allowed before ClientHello, as it reports an unexpected message error in such situation (tested with latest OpenSSL 1.1.1c):

2019/08/08 18:52:15 [info] 45965#100054: *1 SSL_do_handshake() failed (SSL: error:140940F4:SSL routines:ssl3_read_bytes:unexpected message) while SSL handshaking, client: 127.0.0.1, server: 0.0.0.0:8443

Given the above, you may want to report the problem to OpenSSL - it should either not try to send any alerts in this case, or handle them.

[Gernot Vormayr]

Replying to mdounin:

As long as alerts are allowed before ClientHello, these should be simply passed to SSL layer, much like with other handshake messages. As long as they are not, current behaviour looks correct (expect for additional shutdown() error, which shouldn't happen, though it looks like a separate issue, and not nginx one - as the socket is clearly connected, ENOTCONN error returned by shutdown() looks like a kernel bug).

According to a traffic dump nginx sends a HTTP 400 page back, which gets answered with an RST - so depending on timing it could be, that at the point shutdown is called, the socket is already closed.

I don't think I see anything in TLS RFCs which makes alerts explicitly illegal before ClientHello. On the other hand, it doesn't looks like they are meaningful before ClientHello.

I also tried to find something in the RFCs before sending this - couldn't find anything that clearly states anything about Alert before ClientHello. However, ​https://tools.ietf.org/html/rfc8446#section-6.2 says that when an error is detected, that the detecting party should send an alert and must close the connection immediately after. At the point in time this happens, the connection was already made - but the handshake did not yet occur; the RFC states that e.g. due to memory allocation failure one can send internal error, which could happen while creating the ClientHello. So my interpretation would be that it is legal to have an Alert instead of a ClientHello. But I'm not an expert and would not insist on this.

Trivial test with the following test patch:

diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -726,7 +726,10 @@ ngx_http_ssl_handshake(ngx_event_t *rev)
     }
 
     if (n == 1) {
-        if (buf[0] & 0x80 /* SSLv2 */ || buf[0] == 0x16 /* SSLv3/TLSv1 */) {
+        if (buf[0] & 0x80 /* SSLv2 */
+            || buf[0] == 0x16 /* SSLv3/TLSv1 */
+            || buf[0] == 0x15 /* SSLv3/TLSv1 */)
+        {
             ngx_log_debug1(NGX_LOG_DEBUG_HTTP, rev->log, 0,
                            "https ssl handshake: 0x%02Xd", buf[0]);
 

suggests that OpenSSL itself thinks alerts are not allowed before ClientHello, as it reports an unexpected message error in such situation (tested with latest OpenSSL 1.1.1c):

2019/08/08 18:52:15 [info] 45965#100054: *1 SSL_do_handshake() failed (SSL: error:140940F4:SSL routines:ssl3_read_bytes:unexpected message) while SSL handshaking, client: 127.0.0.1, server: 0.0.0.0:8443

This is the first version I tried, where I got the same result. Well the relevant part in openssl states

/*
 * If we've got this far and still haven't decided on what version
 * we're using then this must be a client side alert we're dealing with
 * (we don't allow heartbeats yet). We shouldn't be receiving anything
 * other than a ClientHello if we are a server.
 */

So yeah a bit of a mixed feeling - on one hand they say this must be a client side alert - on the other hand they say there should be only ClientHello.

Given the above, you may want to report the problem to OpenSSL - it should either not try to send any alerts in this case, or handle them.

I'd say a bit of both:

• In hindsight my patch is not good since it tries to handle that itself while this should be handled by the SSL layer
• I agree with you that this should be forwarded to the SSL layer - so I think that the patch you wrote should go in
• OpenSSL does definitely do something wrong here: It sends an Alert as response to an alert while the spec clearly says that both parties "must immediately close the connection without sending or receiving any additional data"
• Without your patch the current implementation imho also violates this rule (it receives an alert and interprets that as application data and sends something back)

[Maxim Dounin]

Replying to gvormayr@…:

Replying to mdounin:

As long as alerts are allowed before ClientHello, these should be simply passed to SSL layer, much like with other handshake messages. As long as they are not, current behaviour looks correct (expect for additional shutdown() error, which shouldn't happen, though it looks like a separate issue, and not nginx one - as the socket is clearly connected, ENOTCONN error returned by shutdown() looks like a kernel bug).

According to a traffic dump nginx sends a HTTP 400 page back, which gets answered with an RST - so depending on timing it could be, that at the point shutdown is called, the socket is already closed.

Depending on the timing, the socket might be reset, and the error could be ECONNRESET (though this is not something shutdown() is expected to indicate as an error), but not ENOTCONN. This looks like a bug in the kernel you are using, likely due to incorrect optimizations in the TCP handling on loopback.

• I agree with you that this should be forwarded to the SSL layer - so I think that the patch you wrote should go in

We can consider it, but the only thing it does is an optimization of a particular case with garbage got from the client. Are you seeing a lot of these for some reason?

[Gernot Vormayr]

Replying to mdounin:

Replying to gvormayr@…:

Replying to mdounin:

As long as alerts are allowed before ClientHello, these should be simply passed to SSL layer, much like with other handshake messages. As long as they are not, current behaviour looks correct (expect for additional shutdown() error, which shouldn't happen, though it looks like a separate issue, and not nginx one - as the socket is clearly connected, ENOTCONN error returned by shutdown() looks like a kernel bug).

According to a traffic dump nginx sends a HTTP 400 page back, which gets answered with an RST - so depending on timing it could be, that at the point shutdown is called, the socket is already closed.

Depending on the timing, the socket might be reset, and the error could be ECONNRESET (though this is not something shutdown() is expected to indicate as an error), but not ENOTCONN. This looks like a bug in the kernel you are using, likely due to incorrect optimizations in the TCP handling on loopback.

I tried to find out what is happening here, since this is a bit peculiar. Well according to the manpage of shutdown, ECONNRESET is not something it returns (no idea why that is...) and I traced a bit around the linux kernel and read a bit of source code and found out, that the following is happening:

1. Upon arrival of the RST, tcp_reset is called, which sets sk->sk_err to ECONNRESET and calls tcp_done, which further sets sk->sk_state to TCP_CLOSE.
2. The shutdown syscall calls inet_shutdown, which sets the return value to ENOTCONN if sk->sk_state is TCP_CLOSE (so sk_err gets ignored here and then it goes on to call tcp_shutdown).

Now depending on timing and scheduling (whether 1 happens before 2) ENOTCONN or no error gets returned from shutdown (I managed to get both outcomes in multiple tests).

I fear finding out the reason why they chose ENOTCONN and not ECONNRESET for shutdown is beyond my detective capabilities (didn't find any explaining comment or anything) - only thing I found out is, that it has been this way in the linux kernel since at least 2.6.

• I agree with you that this should be forwarded to the SSL layer - so I think that the patch you wrote should go in

We can consider it, but the only thing it does is an optimization of a particular case with garbage got from the client. Are you seeing a lot of these for some reason?

So far I've seen this only during SSL testing (so not really a lot of these). I'd say that this thing is not particularly interesting in any way since nothing bad can happen. The only thing I see that can be happening here is a WTF from a user/admin/developer since the request line "\x15\x03\x01\x00\x02\x02P" looks a bit strange. So I'd say you can close this.

[Maxim Dounin]

Replying to gvormayr@…:

Replying to mdounin:

Depending on the timing, the socket might be reset, and the error could be ECONNRESET (though this is not something shutdown() is expected to indicate as an error), but not ENOTCONN. This looks like a bug in the kernel you are using, likely due to incorrect optimizations in the TCP handling on loopback.

Well according to the manpage of shutdown, ECONNRESET is not something it returns (no idea why that is...)

That's because shutdown() is not expected to fail regardless of error conditions on the socket as long as socket is valid. It can only fail if ​something incorrect provided as arguments - that is, not a file descriptor (EBADF), invalid how argument (EINVAL), a socket which was never connected (ENOTCONN), or not a socket at all (ENOTSOCK).

and I traced a bit around the linux kernel and read a bit of source code and found out, that the following is happening:

1. Upon arrival of the RST, tcp_reset is called, which sets sk->sk_err to ECONNRESET and calls tcp_done, which further sets sk->sk_state to TCP_CLOSE.
2. The shutdown syscall calls inet_shutdown, which sets the return value to ENOTCONN if sk->sk_state is TCP_CLOSE (so sk_err gets ignored here and then it goes on to call tcp_shutdown).

Now depending on timing and scheduling (whether 1 happens before 2) ENOTCONN or no error gets returned from shutdown (I managed to get both outcomes in multiple tests).

I fear finding out the reason why they chose ENOTCONN and not ECONNRESET for shutdown is beyond my detective capabilities (didn't find any explaining comment or anything) - only thing I found out is, that it has been this way in the linux kernel since at least 2.6.

So it looks like it is broken for a long time, yet this wasn't observed previously. Either way, this is to be fixed in kernel, and is irrelevant to the particular issue.

• I agree with you that this should be forwarded to the SSL layer - so I think that the patch you wrote should go in

We can consider it, but the only thing it does is an optimization of a particular case with garbage got from the client. Are you seeing a lot of these for some reason?

So far I've seen this only during SSL testing (so not really a lot of these). I'd say that this thing is not particularly interesting in any way since nothing bad can happen. The only thing I see that can be happening here is a WTF from a user/admin/developer since the request line "\x15\x03\x01\x00\x02\x02P" looks a bit strange. So I'd say you can close this.

Well, the request line in question actually explains more than the unexpected message error from OpenSSL, which is basically the same but does not contain any information about the message. Ok, closing this.