Prevent segfault while calling srv.ssl_cert_handle #1055
Conversation
In some cases lscf->srv.ssl_cert_handler may be null pointer. Probably it's caused by some https clients which do something wrong during ssl handshake. In any case we should not call null pointer function.
|
|
||
| rc = lscf->srv.ssl_cert_handler(r, lscf, L); | ||
|
|
||
| if (lscf->srv.ssl_cert_handler) { |
There was a problem hiding this comment.
This pointer can never be NULL when it is already in this ngx_http_lua_ssl_cert_handler function. And we never observe NULL pointers in some user company's global SSL gateway network. I guess it's something else, like some 3rd-party NGINX C modules or 3rd-party Lua C or FFI libraries you are using, which corrupt memory. This does not look like the culprit. Better dig things up instead of working around the real issue which might be somewhere else.
There was a problem hiding this comment.
I agree, I have to go deeper and find out the real reason of null pointer.
But, anyway,
- There is only one 3rd party module I use, besides shipped with nginx and openresty: https://github.com/newobj/nginx-x-rid-header/blob/master/ngx_x_rid_header_module.c, it's quite simple and i'ts not the culprit for my mind.
- I have used my build of nginx for rather long time, with no segfaults just untill I've added only one line: "ssl_certificate_by_lua_block { return }"
- there are no any other block of lua code in my config
- It seems, I've somehow localized this issue,. My nginx crashes after request from IP addresses which use okhttp/3.3.1 client (it's java http client for android)
Some more info I've posted to https://groups.google.com/forum/#!topic/openresty-en/ym1oPD9fyLo
|
FInally I manage to reproduce this bug: (I't also reproducabe with latest nginx/openresrty) No ssl certificate in second virtualhost, but configtest is OK. Wrong certificate, but no segmentation fault Now, uncomment certificate by lua block: Shoud this issue be addressed to nginx team or openresty? |
|
@petrovich-ua Will you try reproducing it with the latest OpenResty 1.11.2.3 release? See https://openresty.org/en/download.html Also, please ensure you are using OpenSSL 1.0.2k with the following patch applied: Also, enabling the nginx debugging logs (i.e., those |
|
@petrovich-ua Okay, I see what's going on here. I don't think it makes sense to use 2 virtual servers listening on the same SSL port but with only one of them configuring |
|
Or make |
|
@ghedo Yeah, that makes sense. It indeed makes little sense to make this directive server-level when SNI is not checked at that point (unless the virtual servers use different IPs or different ports that can differentiate themselves). |
It was reproducible yesterday in this case:
Ok, I'll do it in few hours if it is still actual.
Why not? First server is my case is default server with it's own locations and backends. It stands in front of huge main application which serves many domains. In my case I just forgot to set ssl_certificate in second server. Yes, it is misconfiguration. But it was not an issue, because fist server had wildcard certificate, so everything worked perfectly until I've turned ssl_certificate_by_lua on. |
|
@petrovich-ua As I've mentioned above, |
I agree. |
|
@petrovich-ua Yeah, we have to break existing configurations for this. Mind to create a PR for this change? Thanks! |
Not sure if I correctly understand you. ("Do you mind if I" vs "Do you mind if you") I think my knowlege of nginx architecture does not allow me to implement this change in reasonable time frame. I can try, but have no idea how long may it take. I will not protest against if you close this PR and open a new one with more correct description. |
|
@petrovich-ua I was asking you to do a new PR for that change. I think the following change should be sufficient for the code base itself: diff --git a/src/ngx_http_lua_module.c b/src/ngx_http_lua_module.c
index 72f934d0..df599d53 100644
--- a/src/ngx_http_lua_module.c
+++ b/src/ngx_http_lua_module.c
@@ -509,14 +509,14 @@ static ngx_command_t ngx_http_lua_cmds[] = {
NULL },
{ ngx_string("ssl_certificate_by_lua_block"),
- NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_BLOCK|NGX_CONF_NOARGS,
+ NGX_HTTP_MAIN_CONF|NGX_CONF_BLOCK|NGX_CONF_NOARGS,
ngx_http_lua_ssl_cert_by_lua_block,
NGX_HTTP_SRV_CONF_OFFSET,
0,
(void *) ngx_http_lua_ssl_cert_handler_inline },
{ ngx_string("ssl_certificate_by_lua_file"),
- NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
+ NGX_HTTP_MAIN_CONF|NGX_CONF_TAKE1,
ngx_http_lua_ssl_cert_by_lua,
NGX_HTTP_SRV_CONF_OFFSET,
0,But we also need to update the test suite (under |
|
OK, I've changed my mind. It still makes sense to use diff --git a/src/ngx_http_lua_ssl_certby.c b/src/ngx_http_lua_ssl_certby.c
index 84e30931..5abbe759 100644
--- a/src/ngx_http_lua_ssl_certby.c
+++ b/src/ngx_http_lua_ssl_certby.c
@@ -193,6 +193,7 @@ ngx_http_lua_ssl_cert_handler(ngx_ssl_conn_t *ssl_conn, void *data)
ngx_http_lua_srv_conf_t *lscf;
ngx_http_core_loc_conf_t *clcf;
ngx_http_lua_ssl_ctx_t *cctx;
+ ngx_http_core_srv_conf_t *cscf;
c = ngx_ssl_get_connection(ssl_conn);
@@ -298,6 +299,16 @@ ngx_http_lua_ssl_cert_handler(ngx_ssl_conn_t *ssl_conn, void *data)
c->log->action = "loading SSL certificate by lua";
+ if (lscf->srv.ssl_cert_handler == NULL) {
+ cscf = ngx_http_get_module_srv_conf(r, ngx_http_core_module);
+
+ ngx_log_error(NGX_LOG_ALERT, c->log, 0,
+ "no ssl_certificate_by_lua* defined in "
+ "server %V", &cscf->server_name);
+
+ goto failed;
+ }
+
rc = lscf->srv.ssl_cert_handler(r, lscf, L);
if (rc >= NGX_OK || rc == NGX_ERROR) {
diff --git a/t/139-ssl-cert-by.t b/t/139-ssl-cert-by.t
index e6d0da8a..1513d6e9 100644
--- a/t/139-ssl-cert-by.t
+++ b/t/139-ssl-cert-by.t
@@ -11,7 +11,7 @@ my $openssl_version = eval { `$NginxBinary -V 2>&1` };
if ($openssl_version =~ m/built with OpenSSL (0|1\.0\.(?:0|1[^\d]|2[a-d]).*)/) {
plan(skip_all => "too old OpenSSL, need 1.0.2e, was $1");
} else {
- plan tests => repeat_each() * (blocks() * 6 + 10);
+ plan tests => repeat_each() * (blocks() * 6 + 6);
}
$ENV{TEST_NGINX_HTML_DIR} ||= html_dir();
@@ -1676,3 +1676,193 @@ close: 1 nil
--- no_error_log
[error]
[alert]
+
+
+
+=== TEST 20: some server {} block missing ssl_certificate_by_lua* handlers (literal server name)
+--- http_config
+ server {
+ listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+ server_name test.com;
+
+ ssl_certificate_by_lua_block { print("ssl cert by lua is running!") }
+ ssl_certificate ../../cert/test.crt;
+ ssl_certificate_key ../../cert/test.key;
+
+ server_tokens off;
+ location /timers {
+ default_type 'text/plain';
+ content_by_lua_block {
+ ngx.timer.at(0.1, function() ngx.sleep(0.3) end)
+ ngx.timer.at(0.11, function() ngx.sleep(0.3) end)
+ ngx.timer.at(0.09, function() ngx.sleep(0.3) end)
+ ngx.sleep(0.2)
+ ngx.say(ngx.timer.running_count())
+ }
+ more_clear_headers Date;
+ }
+ }
+
+ server {
+ listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+ server_name test2.com;
+ }
+--- config
+ server_tokens off;
+ lua_ssl_trusted_certificate ../../cert/test.crt;
+
+ location /t {
+ content_by_lua_block {
+ do
+ local sock = ngx.socket.tcp()
+
+ sock:settimeout(2000)
+
+ local ok, err = sock:connect("unix:$TEST_NGINX_HTML_DIR/nginx.sock")
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local sess, err = sock:sslhandshake(nil, "test2.com", true)
+ if not sess then
+ ngx.say("failed to do SSL handshake: ", err)
+ return
+ end
+
+ ngx.say("ssl handshake: ", type(sess))
+
+ local req = "GET /timers HTTP/1.0\r\nHost: test.com\r\nConnection: close\r\n\r\n"
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send http request: ", err)
+ return
+ end
+
+ ngx.say("sent http request: ", bytes, " bytes.")
+
+ while true do
+ local line, err = sock:receive()
+ if not line then
+ -- ngx.say("failed to receive response status line: ", err)
+ break
+ end
+
+ ngx.say("received: ", line)
+ end
+
+ local ok, err = sock:close()
+ ngx.say("close: ", ok, " ", err)
+ end -- do
+ -- collectgarbage()
+ }
+ }
+
+--- request
+GET /t
+--- response_body
+connected: 1
+failed to do SSL handshake: handshake failed
+
+--- error_log eval
+[
+qr/\[alert\] .*? no ssl_certificate_by_lua\* defined in server test2.com/,
+qr/\[crit\] .*? SSL_do_handshake\(\) failed\b/,
+]
+
+
+
+=== TEST 21: some server {} block missing ssl_certificate_by_lua* handlers (regex server name)
+--- http_config
+ server {
+ listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+ server_name test.com;
+
+ ssl_certificate_by_lua_block { print("ssl cert by lua is running!") }
+ ssl_certificate ../../cert/test.crt;
+ ssl_certificate_key ../../cert/test.key;
+
+ server_tokens off;
+ location /timers {
+ default_type 'text/plain';
+ content_by_lua_block {
+ ngx.timer.at(0.1, function() ngx.sleep(0.3) end)
+ ngx.timer.at(0.11, function() ngx.sleep(0.3) end)
+ ngx.timer.at(0.09, function() ngx.sleep(0.3) end)
+ ngx.sleep(0.2)
+ ngx.say(ngx.timer.running_count())
+ }
+ more_clear_headers Date;
+ }
+ }
+
+ server {
+ listen unix:$TEST_NGINX_HTML_DIR/nginx.sock ssl;
+ server_name ~test2\.com;
+ }
+--- config
+ server_tokens off;
+ lua_ssl_trusted_certificate ../../cert/test.crt;
+
+ location /t {
+ content_by_lua_block {
+ do
+ local sock = ngx.socket.tcp()
+
+ sock:settimeout(2000)
+
+ local ok, err = sock:connect("unix:$TEST_NGINX_HTML_DIR/nginx.sock")
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local sess, err = sock:sslhandshake(nil, "test2.com", true)
+ if not sess then
+ ngx.say("failed to do SSL handshake: ", err)
+ return
+ end
+
+ ngx.say("ssl handshake: ", type(sess))
+
+ local req = "GET /timers HTTP/1.0\r\nHost: test.com\r\nConnection: close\r\n\r\n"
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send http request: ", err)
+ return
+ end
+
+ ngx.say("sent http request: ", bytes, " bytes.")
+
+ while true do
+ local line, err = sock:receive()
+ if not line then
+ -- ngx.say("failed to receive response status line: ", err)
+ break
+ end
+
+ ngx.say("received: ", line)
+ end
+
+ local ok, err = sock:close()
+ ngx.say("close: ", ok, " ", err)
+ end -- do
+ -- collectgarbage()
+ }
+ }
+
+--- request
+GET /t
+--- response_body
+connected: 1
+failed to do SSL handshake: handshake failed
+
+--- error_log eval
+[
+qr/\[alert\] .*? no ssl_certificate_by_lua\* defined in server ~test2\\\.com/,
+qr/\[crit\] .*? SSL_do_handshake\(\) failed\b/,
+] |
…listen on the same port or unix domain socket file path *and* some of them are using ssl_certificate_by_lua* configurations while some are not. thanks petrovich-ua for the report and original patch in #1055.
|
Merged my version of the patch to master instead. Thanks! |
3 participants
In some cases lscf->srv.ssl_cert_handler may be null pointer. Probably it's caused by some https clients which do something wrong during ssl handshake.
In any case we should not call null pointer function.
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.