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

[Judson Wilson]

> 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.

The behavior I am attempting to produce matches what I am
observing in the Apache web server (assuming default
configuration), so it's not unheard of for HTTPS.

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

I am researching ways of auditing TLS communications
in a read-only way. One method of achieving this is by having the TLS
client give the keyblock to an auditor after the client is sure that the
server
will not accept any more data protected by the keys (assume the software
for the client and server are controlled by the same party). From my
investigation of nginx and apache, if a client receives close_notify,
they can be assured that no more data will be accepted by the server
under the key (both web server software packages immediately
destroy the SSL object after calling SSL_shutdown).

Unfortunately, nginx has this case where a client would attempt to
close a connection, but never get close_notify in response.
It seems like simply obeying the TLS standard would fix this.
(close_notify IS sent on "connection: close" header or keepalive
timeout as I would expect.)

> 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.

Yes truncation attacks were not on my mind - that's a different
problem.

I'd be happy to look into this a further if you think this work has
a chance of being incorporated.

Thanks,
Judson

On Mon, Dec 7, 2015 at 10:31 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:

> 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/
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20151207/e1622169/attachment.html>