Changes in openssl master wrt SSL_shutdown()

[mmuhlenhoff.wikimedia.org@…]

ngx_ssl_shutdown() currently doesn't consider a -1 return value for openssl's SSL_shutdown() function. So far the comment

/* SSL_shutdown() never returns -1, on error it returns 0 */

was correct, but there's a recent change in openssl master which is also backported to the openssl 1.0.2 stable branch (not yet released), which changes that:
​https://github.com/openssl/openssl/commit/64193c8218540499984cd63cda41f3cd491f3f59

[Maxim Dounin]

The commit in question actually reverts one case which returns -1. The comment was correct before OpenSSL 0.9.8m, when SSL_shutdown() was changed to return -1 (see ​this commit).

As it looks like people tend to complain a lot about this comment (this is the 3rd question I see in the last couple of months), here is a patch:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1455332396 -10800
#      Sat Feb 13 05:59:56 2016 +0300
# Node ID 643527b585ec08b45bd812d436e34c771650063e
# Parent  965e4223e7025006d646b81165a70aaa0a7fdc54
SSL: fixed SSL_shutdown() comment.

diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -1797,7 +1797,7 @@ ngx_ssl_shutdown(ngx_connection_t *c)
 
     sslerr = 0;
 
-    /* SSL_shutdown() never returns -1, on error it returns 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);

[mmuhlenhoff.wikimedia.org@…]

Thanks for fixing up the comment. The changes made to ssl/ssl_lib.c from
​https://github.com/openssl/openssl/commit/64193c8218540499984cd63cda41f3cd491f3f59 also have an impact on nginx: On a busy server we're seeing a lot of log messages like this:

2016/02/12 14:31:45 [crit] 8106#8106: *82 SSL_shutdown() failed (SSL: error:140E0197:SSL routines:SSL_shutdown:shutdown while in init) while SSL handshaking, client: X.X.X.X, server: 0.0.0.0:443

I'm wondering whether these should really be logged with "critical" severity? After all nginx can't do anything about a misbehaving SSL client and the shutdown is handled gracefully on the openssl side.

[Maxim Dounin]

This is logged at critical severity as this is not an error nginx expects to happen at this point. The error in question was just added in OpenSSL 1.0.2f (commit ​f73c737), so nginx knows nothing about this error and complains loudly.

Something like this should reduce logging severity as appropriate (untested):

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1455631090 -10800
#      Tue Feb 16 16:58:10 2016 +0300
# Node ID 7b5564208e67810689e5841a0f27584d732b1c97
# Parent  50fb3fd79f76c4d62e330650bb6047194c972352
SSL: logging level of "shutdown while in init".

diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -1942,6 +1942,9 @@ ngx_ssl_connection_error(ngx_connection_
 #ifdef SSL_R_INAPPROPRIATE_FALLBACK
             || n == SSL_R_INAPPROPRIATE_FALLBACK                     /*  373 */
 #endif
+#ifdef SSL_R_SHUTDOWN_WHILE_IN_INIT
+            || n == SSL_R_SHUTDOWN_WHILE_IN_INIT                     /*  407 */
+#endif
             || n == 1000 /* SSL_R_SSLV3_ALERT_CLOSE_NOTIFY */
             || n == SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE             /* 1010 */
             || n == SSL_R_SSLV3_ALERT_BAD_RECORD_MAC                 /* 1020 */

[Maxim Dounin]

Alternatively, something like this could be used to avoid calling SSL_shutdown() during SSL handshakes at all. This should also fix "calling a function you should not call" messages observed with OpenSSL 1.0.2f.

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1455640522 -10800
#      Tue Feb 16 19:35:22 2016 +0300
# Node ID fe3b7aa9c4bf62785d4cfe448efe818dbed1072a
# Parent  27010543f3b619709a68a9272a5e3d90fbf8e468
SSL: avoid calling SSL_shutdown() during handshake (ticket #901).

diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -1767,6 +1767,19 @@ ngx_ssl_shutdown(ngx_connection_t *c)
     int        n, sslerr, mode;
     ngx_err_t  err;
 
+    if (!c->ssl->handshaked) {
+        /*
+         * OpenSSL 1.0.2f complains if SSL_shutdown() is called during
+         * an SSL handshake, while previous versions always return 0.
+         * Avoid calling SSL_shutdown() if handshake wasn't completed.
+         */
+
+        SSL_free(c->ssl->connection);
+        c->ssl = NULL;
+
+        return NGX_OK;
+    }
+
     if (c->timedout) {
         mode = SSL_RECEIVED_SHUTDOWN|SSL_SENT_SHUTDOWN;
         SSL_set_quiet_shutdown(c->ssl->connection, 1);

This looks like a slightly better solution.

[Maxim Dounin <mdounin@…>]

In 6407:062c189fee20/nginx:

SSL: avoid calling SSL_shutdown() during handshake (ticket #901).

This fixes "called a function you should not call" and
"shutdown while in init" errors as observed with OpenSSL 1.0.2f
due to changes in how OpenSSL handles SSL_shutdown() during
SSL handshakes.

[Maxim Dounin]

Fix committed.

[anubhav2301@…]

Replying to mdounin:

Fix committed.

I used Nginx 1.8.1 stable version with OpenSSL 1.0.2g stable version. But I am getting error #942.

Do let me know which version should I use to resolve this issue ? Please help.

[Maxim Dounin]

The patch in question is present in nginx 1.9.12 and up, quote from ​CHANGES:

    *) Workaround: "called a function you should not call" and "shutdown
       while in init" messages might appear in logs when using OpenSSL
       1.0.2f.