SSL_shutdown() return value <0

[Maxim Dounin]

Hello!

On Mon, Dec 10, 2018 at 09:46:28PM +0100, Jan Prachař wrote:

> Hello, I would like to ask about this piece of code from function
> ngx_ssl_shutdown:
> 
>     n = SSL_shutdown(c->ssl->connection);
> 
>     sslerr = 0;
> 
>     /* before 0.9.8m SSL_shutdown() returned 0 instead of -1 on errors
> */
> 
>     if (n != 1 && ERR_peek_error()) {
>         sslerr = SSL_get_error(c->ssl->connection, n);
>     }
> 
> 
> 
> Why don't you check SSL_get_error always if n < 0, but only if also
> ERR_peer_error() returns non-zero value?

When this code was written, SSL_shutdown() never returned negative 
values.  Instead, it returned 0 on errors.  ERR_peek_error() was 
used to filter out if there was a real error or not - but it looks 
like it does so incorrectly, see below.

> According to a documentation of SSL_shutdown, you should check result
> of SSL_get_error() and take appropriate action if it returns
> SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE,e.g. call SSL_shutdown
> again, if SSL_shutdown would block on writing to SSL connection.
> 
> If ERR_peek_error() is not zero, which mean some internal OpenSSL error
> occured, SSL_get_error will return SSL_ERROR_SSL, won't it?

The ERR_peek_error() is used to filter out cases when no real 
error occured, and calling SSL_get_error() returned meaningless 
SSL_ERROR_SYSCALL.  See

http://mailman.nginx.org/pipermail/nginx/2008-January/003084.html

for details on why it was introduced.

Though it looks like it also filters out any real 
connection-specific errors as well, including SSL_ERROR_WANT_READ 
or SSL_ERROR_WANT_WRITE.  I don't think this was an intention.

I suspect the original idea was to filter out errors when 
SSL_get_error() returns SSL_ERROR_SYSCALL in the case when "an EOF 
was observed that violates the protocol", as per SSL_get_error() 
documentation:

       SSL_ERROR_SYSCALL
           Some I/O error occurred.  The OpenSSL error queue may contain more
           information on the error.  If the error queue is empty (i.e.
           ERR_get_error() returns 0), ret can be used to find out more about
           the error: If ret == 0, an EOF was observed that violates the
           protocol.  If ret == -1, the underlying BIO reported an I/O error
           (for socket I/O on Unix systems, consult errno for details).

Probably this needs to be rewritten.

> I have also tried to change the condition to just n < 0, and came to
> antoher issue. If client closes connection prematurely, there is
> usually SSL_write, that has failed with error WANT_WRITE. If then the
> SSL_shutdown is called repeatedly, it causes OpenSSL error (SSL:
> error:1409F07F:SSL routines:ssl3_write_pending:bad write retry),
> because pending SSL_write should have been called first.

In many places we try to avoid doing actual SSL shutdown if we 
know there was an error and/or we know the connection was already 
closed, by using c->ssl->no_send_shutdown flag.  Existing cases 
might not be enough though.

-- 
Maxim Dounin
http://mdounin.ru/