Bug #3186: HTTP 2 connections not properly closed - Lighttpd - lighty labs

Actions

Copy link

Bug #3186

closed

HTTP 2 connections not properly closed

Added by gjoe over 1 year ago. Updated over 1 year ago.

Status:

Fixed

Priority:

Normal

Category:

core

Target version:

1.4.69

ASK QUESTIONS IN Forums:

Description

We discovered that HTTP 2 connections are not properly closed once they are done. We discovered this issue with a client written in Go. In our setup, this lead to a problem with Cloudflare which rejected the response.

In the network traffic we see that the Go client (127.0.0.1) closes the connection but then still receives an additional packet afterwards. Connections to other servers behave differently.

I reproduced it in the most recent release Lighttpd 1.4.68. Here is a minimal config:

server.modules = (
    "mod_openssl",
    "mod_redirect",
)

server.document-root = "/var/www/" 
server.bind = "" 
ssl.engine = "enable" 
ssl.pemfile = "/etc/lighttpd/certs/lighttpd.pem" 

# the bug only manifests with a redirect
url.redirect = ("^/test$" => "%0/")

Here are the steps to reproduce it on Ubuntu 22.04:

1. Install golang and lighttpd (I also reproduced it with a self-compiled 1.4.68)
2. Run Lighttpd with the config above.
3. Copy the attached script somewhere, then run:

go mod init example/hello
go get golang.org/x/net/http2
go run test.go

The output is

Response status: 404 Not Found
2023/01/19 14:37:35 protocol error: received DATA after END_STREAM
Response status: 403 Forbidden

Files

Download all files

brokenhttp2.png (21.8 KB) brokenhttp2.png		gjoe, 2023-01-19 14:30
test.go (778 Bytes) test.go		gjoe, 2023-01-19 14:34

Actions

Copy link

Updated by gstrauss over 1 year ago

Status changed from New to Patch Pending
Target version changed from 1.4.xx to 1.4.69

Thank you very much for the very detailed bug report and reproduction instructions.

It appears that when lighttpd sends END_STREAM with HEADERS frame, the state transition was not properly marked, and so lighttpd would then send END_STREAM with a DATA frame with no content. Here is a candidate patch which avoids sending END_STREAM twice in that case.

--- a/src/h2.c
+++ b/src/h2.c
@@ -2042,6 +2042,12 @@ h2_send_hpack (request_st * const r, connection * const con, const char *data, u

     headers.u[2] = htonl(r->h2id);

+    if (flags & H2_FLAG_END_STREAM)
+        /*++r->h2state;*//*expect H2_STATE_OPEN or H2_STATE_HALF_CLOSED_REMOTE*/
+        r->h2state = (r->h2state == H2_STATE_HALF_CLOSED_REMOTE)
+          ? H2_STATE_CLOSED
+          : H2_STATE_HALF_CLOSED_LOCAL;
+
     /* similar to h2_send_data(), but unlike DATA frames there is a HEADERS
      * frame potentially followed by CONTINUATION frame(s) here, and the final
      * HEADERS or CONTINUATION frame here has END_HEADERS flag set.
@@ -2735,7 +2741,11 @@ h2_send_end_stream_data (request_st * const r, connection * const con)
 void
 h2_send_end_stream (request_st * const r, connection * const con)
 {
-    if (r->h2state == H2_STATE_CLOSED) return;
+    if (r->h2state == H2_STATE_CLOSED
+        || r->h2state == H2_STATE_HALF_CLOSED_LOCAL) {
+        r->h2state = H2_STATE_CLOSED;
+        return;
+    }
     if (r->state != CON_STATE_ERROR && r->resp_body_finished) {
         /* CON_STATE_RESPONSE_END */
         if (r->gw_dechunk && r->gw_dechunk->done

Actions

Copy link

Updated by gstrauss over 1 year ago

Modified patch

--- a/src/h2.c
+++ b/src/h2.c
@@ -2042,6 +2042,12 @@ h2_send_hpack (request_st * const r, connection * const con, const char *data, u

     headers.u[2] = htonl(r->h2id);

+    if (flags & H2_FLAG_END_STREAM)
+        /*++r->h2state;*//*expect H2_STATE_OPEN or H2_STATE_HALF_CLOSED_REMOTE*/
+        r->h2state = (r->h2state == H2_STATE_HALF_CLOSED_REMOTE)
+          ? H2_STATE_CLOSED
+          : H2_STATE_HALF_CLOSED_LOCAL;
+
     /* similar to h2_send_data(), but unlike DATA frames there is a HEADERS
      * frame potentially followed by CONTINUATION frame(s) here, and the final
      * HEADERS or CONTINUATION frame here has END_HEADERS flag set.
@@ -2704,6 +2710,7 @@ h2_send_cqdata (request_st * const r, connection * const con, chunkqueue * const
 static void
 h2_send_end_stream_data (request_st * const r, connection * const con)
 {
+  if (r->h2state != H2_STATE_HALF_CLOSED_LOCAL) {
     union {
       uint8_t c[12];
       uint32_t u[3];          /*(alignment)*/
@@ -2720,6 +2727,7 @@ h2_send_end_stream_data (request_st * const r, connection * const con)
     /*(ignore window updates when sending 0-length DATA frame with END_STREAM)*/
     chunkqueue_append_mem(con->write_queue,  /*(+3 to skip over align pad)*/
                           (const char *)dataframe.c+3, sizeof(dataframe)-3);
+  }

     if (r->h2state != H2_STATE_HALF_CLOSED_REMOTE) {
         /* set timestamp for comparison; not tracking individual stream ids */

Actions

Copy link