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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel