bugfix: a segmentation fault might occur when SSL renegotiation happens #1355

tokers · 2018-07-24T14:56:06Z

Please see #1354 for more details.

Since it is tough to distinguish whether current connection is upgraded
to HTTP/2 in the ngx_http_lua_ssl_cert_handler context. This PR just
returns 0 (marks failed) when SSL renegotiation happens. Nginx already
prohibited the SSL renegotiation, so it is safe.

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

spacewander · 2018-07-25T02:12:21Z

AFAIK, server side SSL renegotiation is disable in Nginx.
Maybe you are using OpenSSL 1.1.0+, so the renegotiation is not disable yet.
This was fixed in the master branch of Nginx, see https://trac.nginx.org/nginx/ticket/1376

tokers · 2018-07-25T02:37:42Z

@spacewander The usage of SSL_OP_NO_RENEGOTIATION in Nginx was added recently.
It seems that the option can disable the SSL renegotiation completely. But for the Nginx older than 1.15.2, there is a time window between Nginx detects SSL renegotiation and takes the corresponding action, so this segmentation fault can still happens.

I don't know what is your new plan for compatible with the Nginx 1.15.x series, if there is no such plan, maybe it's better to fixup this?

spacewander · 2018-07-25T02:49:14Z

@tokers

for the Nginx older than 1.15.2, there is a time window between Nginx detects SSL renegotiation and takes the corresponding action, so this segmentation fault can still happens.

Why not backport the patch to the Nginx older than 1.15.2? Server SSL renegotiation is disable with the intention to avoid security risk. Even if nobody is using SSL 3.0, work around a fixed bug is not so nice.

tokers · 2018-07-25T03:05:13Z

@spacewander The purpose is not "work around a fixed bug" but solves the segmentation fault problem. Of course put the patch for old Nginx is also ok.

spacewander · 2018-07-25T10:30:13Z

@tokers
What about wrapping this change with specific version macro?
This change is only required for some versions of Nginx built with some versions of OpenSSL.

spacewander · 2018-07-25T10:35:20Z

src/ngx_http_lua_ssl_certby.c

@@ -201,6 +201,10 @@ ngx_http_lua_ssl_cert_handler(ngx_ssl_conn_t *ssl_conn, void *data)

    c = ngx_ssl_get_connection(ssl_conn);

+    if (c->ssl->renegotiation) {
+        return 0;


IMHO, we should return -1 and log with error message.
In the ideal world, in which Nginx and OpenSSL work perfectly, server side renegotiation is disable.
Also, skip the ssl_cert_by_lua phase and use the dummy ssl certificate is not always expected in the renegotiation.

Why should we return -1? -1 represents the current stage is still in progress.

@tokers
My bad. Yes, it should return 0.

@spacewander
I have updated this change.

tokers · 2018-07-25T11:14:37Z

@spacewander

What about wrapping this change with specific version macro?
This change is only required for some versions of Nginx built with some versions of OpenSSL.

Okay, I think we can check the Nginx version number and the SSL_OP_NO_RENEGOTIATION macro.

agentzh · 2018-07-25T18:15:00Z

@tokers Is it possible to add a test case to cover this?

tokers · 2018-07-26T02:13:55Z

@agentzh

It seems difficult for me, I don't know how to manipulate Test::Nginx to perform the SSL renegotiation.

spacewander · 2018-07-26T03:23:05Z

@tokers
What about running the OpenSSL client manually?
https://github.com/openresty/lua-resty-core/blob/master/t/ssl.t#L2176

tokers · 2018-07-26T03:24:43Z

@spacewander
Thanks! I will imitate it.

Please see #1354 for more details. Since it is tough to distinguish whether current connection is upgraded to HTTP/2 in the ngx_http_lua_ssl_cert_handler context. This PR just returns 0 (marks failed) when SSL renegotiation happens. Nginx already prohibited the SSL renegotiation, so it is safe.

tokers · 2018-07-27T04:34:08Z

@agentzh @spacewander test was added. This problem only occurs when OpenSSL version is between 1.1.0 to 1.1.0g.

mergify · 2020-06-26T00:38:56Z