nginx doesn't close http/2 connection on error '400' (redirected)

[RuStrannik@…]

Problem: when built-in error '400' redirected to custom handler, and used incorrect request method, nginx does not perform any actions, just hangs keeping connection open.

Expected behaviour: error handler redirected and either connection closed without response (case A) of specified response returned and connection closed as well (case B).

Server config:

server {
        listen 443 ssl http2 default_server;
        listen [::]:443 ssl http2 default_server;

        include snippets/snakeoil.conf;

        error_page 400 /drop;
        #error_page 400 =200 /drop; # makes no difference
        # Case 'A'
        location = /drop { return 444; }
        # Case 'B'
        #location = /drop { return 200 "error\n"; } # doesn't work either

        location / { return 200 "server ok!\n"; }
}

Request to cause issue:

curl --request "whatever" --insecure -v https://123.123.123.123

Output:

*   Trying 123.123.123.123:443...
* Connected to 123.123.123.123 (123.123.123.123) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*  CAfile: /etc/ssl/certs/ca-certificates.crt
*  CApath: /etc/ssl/certs
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_256_GCM_SHA384
* ALPN, server accepted to use h2
* Server certificate:
*  subject: CN=localhost.localdomain
*  start date: Nov 16 00:53:58 2020 GMT
*  expire date: Nov 14 00:53:58 2030 GMT
*  issuer: CN=localhost.localdomain
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x55b42a177fe0)
> whatever / HTTP/2
> Host: 123.123.123.123
> user-agent: curl/7.74.0
> accept: */*
>
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* old SSL session ID is stale, removing
* Connection state changed (MAX_CONCURRENT_STREAMS == 128)!

<now hangs here infinitely>

UPD: fixed typos and removed unrelated condition.

[Maxim Dounin]

Thanks for reporting this, looks like another variant of #274, which was missed in 1812f1d79d84.

I don't see any issues with return 200 ... though, it works fine in my tests and is expected to work fine. Could you please re-check if you are indeed seeing any issues with it?

The following patch should fix the issue with return 444:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1677031176 -10800
#      Wed Feb 22 04:59:36 2023 +0300
# Node ID f51b18f379ae98cdd41fc9e8e1cceacdf2543ebb
# Parent  cffaf3f2eec8fd33605c2a37814f5ffc30371989
HTTP/2: socket leak with "return 444" in error_page (ticket #2455).

Similarly to ticket #274 (7354:1812f1d79d84), early request finalization
without ngx_http_run_posted_requests() call resulted in a connection
hang (a socket leak) if "return 444" was used in error_page 400, and
400 (Bad Request) error was generated in ngx_http_v2_state_process_header()
due to invalid request headers or pseudo-headers.

diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
--- a/src/http/v2/ngx_http_v2.c
+++ b/src/http/v2/ngx_http_v2.c
@@ -1730,6 +1730,7 @@ ngx_http_v2_state_process_header(ngx_htt
     size_t                      len;
     ngx_int_t                   rc;
     ngx_table_elt_t            *h;
+    ngx_connection_t           *fc;
     ngx_http_header_t          *hh;
     ngx_http_request_t         *r;
     ngx_http_v2_header_t       *header;
@@ -1789,6 +1790,7 @@ ngx_http_v2_state_process_header(ngx_htt
     }
 
     r = h2c->state.stream->request;
+    fc = r->connection;
 
     /* TODO Optimization: validate headers while parsing. */
     if (ngx_http_v2_validate_header(r, header) != NGX_OK) {
@@ -1886,6 +1888,8 @@ error:
 
     h2c->state.stream = NULL;
 
+    ngx_http_run_posted_requests(fc);
+
     return ngx_http_v2_state_header_complete(h2c, pos, end);
 }
 

Review and testing appreciated.

Disclaimer: please note that configuring error pages for 400 (Bad Request) errors is generally a bad idea, it's known to cause various issues due to lack of otherwise required request properties.

[RuStrannik@…]

Thank you for posting patch so quickly.

Can confirm, 'return 200' actually works on 'nginx/1.23.1', my bad, sorry.
Was also testing this issue on a different server with 'nginx/1.22.0' and 'return 200' does not work there.

Regarding the configuring error pages, I wanted to minimize server exposure and minimize resources spent on processing requests from scripts and scanners, especially to malformed ones, as there is absolutely no value in doing that.

Have not tested patched and recompiled nginx yet.

[Maxim Dounin]

Replying to RuStrannik@…:

Can confirm, 'return 200' actually works on 'nginx/1.23.1', my bad, sorry.
Was also testing this issue on a different server with 'nginx/1.22.0' and 'return 200' does not work there.

There should be no difference between nginx 1.22.0 and nginx 1.23.1 regarding this particular issue. The issue might appear on forced request termination, such as with return 444;, or when subrequests are used in the response. Simple return 200 "error\n"; shouldn't be a problem (unless it also triggers subrequests, such as due to add_before_body or add_after_body elsewhere in the configuration).

Just to make sure I've tested the configuration provided with nginx 1.22.0 and, as expected, wasn't able to reproduce any issues with return 200.... If you are indeed seeing issues with return 200... on a server with nginx 1.22.0, it would be interesting to see a configuration which demonstrates the issue.

[Maxim Dounin <mdounin@…>]

In 8147:72627f1cf09e/nginx:

HTTP/2: socket leak with "return 444" in error_page (ticket #2455).

Similarly to ticket #274 (7354:1812f1d79d84), early request finalization
without calling ngx_http_run_posted_requests() resulted in a connection
hang (a socket leak) if the 400 (Bad Request) error was generated in
ngx_http_v2_state_process_header() due to invalid request headers and
"return 444" was used in error_page 400.

[Maxim Dounin]

Fix committed and available in nginx 1.23.4 and 1.24.0, closing this. Thanks to all involved.