[PATCH] SSL: shutdown cleanly when other endpoint starts shutdown

[Maxim Dounin]

Hello!

On Sat, Dec 05, 2015 at 07:17:54AM +0000, Judson Wilson wrote:

> # HG changeset patch
> # User Judson Wilson <wilson.judson at gmail.com>
> # Date 1449296759 0
> #      Sat Dec 05 06:25:59 2015 +0000
> # Node ID f41799d322f02c8998a800953d81e7274a9d3376
> # Parent  cb31017e961b4a54e83c4fc1be46c18842696207
> SSL: shutdown cleanly when other endpoint starts shutdown
> 
> Before this change, if the other endpoint sends an SSL close_notify, nginx
> will kill the SSL connection without sending a close_notify in response.
> This behavior does not follow RFC 5246 section 7.2.1:
> 
>   Unless some other fatal alert has been transmitted, each party is
>   required to send a close_notify alert before closing the write side
>   of the connection.
> 
> This change fixes this behavior in this specific situation, causing
> nginx to reply with a close_notify before shutting down the conneciton.
> 
> diff -r cb31017e961b -r f41799d322f0 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c	Wed Dec 02 19:17:19 2015 -0800
> +++ b/src/event/ngx_event_openssl.c	Sat Dec 05 06:25:59 2015 +0000
> @@ -1472,7 +1472,6 @@
>      }
>  
>      c->ssl->no_wait_shutdown = 1;
> -    c->ssl->no_send_shutdown = 1;
>  
>      if (sslerr == SSL_ERROR_ZERO_RETURN || ERR_peek_error() == 0) {
>          ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0,
> @@ -1480,6 +1479,8 @@
>          return NGX_DONE;
>      }
>  
> +    c->ssl->no_send_shutdown = 1;
> +
>      ngx_ssl_connection_error(c, sslerr, err, "SSL_read() failed");
>  
>      return NGX_ERROR;

There is a problem with this change: in most cases in practice 
close from a client means that the socket is no longer open, and 
trying to send a close_notify alert won't do anything good but 
will result in RST instead.  Common practice seems to be don't 
send close_notify alerts in such a case (e.g., I see Chrome doing 
the same), and section 7.2.1 mentions this as well.

Note well that section 7.2.1 you refer to is about avoiding 
truncation attacks.  On the other hand, the code path you are 
editing doesn't ensure that a close_notify notify alert was 
received from the other side.  That is, sending a close_notify 
alert will make truncation attacks easier, not stop them.

If you still think that changes in this area are needed - please 
provide some more details on what you are trying to fix.

-- 
Maxim Dounin
http://nginx.org/