Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#901 closed defect (fixed)

Changes in openssl master wrt SSL_shutdown()

Reported by: mmuhlenhoff.wikimedia.org@… Owned by:
Priority: minor Milestone:
Component: nginx-core Version: 1.9.x
Keywords: Cc:
uname -a:
nginx -V: 1.9.11

Description

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

Change History (8)

comment:1 Changed 4 years ago by mdounin

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);

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by mdounin

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 */

comment:4 Changed 4 years ago by mdounin

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.

comment:5 Changed 4 years ago by 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.

comment:6 follow-up: Changed 4 years ago by mdounin

  • Resolution set to fixed
  • Status changed from new to closed

Fix committed.

comment:7 in reply to: ↑ 6 Changed 4 years ago by anubhav2301@…

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.

Last edited 4 years ago by anubhav2301@… (previous) (diff)

comment:8 Changed 4 years ago by mdounin

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.
Note: See TracTickets for help on using tickets.