From 9fd5aa8ada7b62ec43488a54d80986e41370471f Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 6 Aug 2019 15:21:45 +0200 Subject: [PATCH] BUG/MEDIUM: mux-h2: do not recheck a frame type after a state transition Patrick Hemmer reported a rare case where the H2 mux emits spurious RST_STREAM(STREAM_CLOSED) that are triggered by the send path and do not even appear to be associated with a previous incoming frame, while the send path never emits such a thing. The problem is particularly complex (hence its rarity). What happens is that when data are uploaded (POST) we must refill the sending stream's window by sending a WINDOW_UPDATE message (and we must refill the connection's too). But in a highly bidirectional traffic, it is possible that the mux's buffer will be full and that there is no more room to build this WINDOW_UPDATE frame. In this case the demux parser switches to the H2_CS_FRAME_A state, noting that an "acknowledgement" is needed for the current frame, and it doesn't change the current stream nor frame type. But the stream's state was possibly updated (typically OPEN->HREM when a DATA frame carried the ES flag). Later the data can leave the buffer, wake up h2_io_cb(), which calls h2_send() to send pending data, itself calling h2_process_mux() which detects that there are unacked data in the connection's window so it emits a WINDOW_UPDATE for the connection and resets the counter. so it emits a WINDOW_UPDATE for the connection and resets the counter. Then h2_process() calls h2_process_demux() which continues the processing based on the current frame type and the current state H2_CS_FRAME_A. Unfortunately the protocol compliance checks matching the frame type against the current state are still present. These tests are designed for new frames only, not for those in progress, but they are not limited by frame types. Thus the current DATA frame is checked again against the current stream state that is now HREM, and fails the test with a STREAM_CLOSED error. The quick and backportable solution consists in adding the test for this ACK and bypass all these checks that were already validated prior to the state transition. A better long-term solution would consist in having a new state between H and P indicating the frame is new and needs to be checked ("N" for new?) and apply the protocol tests only in this state. In addition everywhere we decide to send a window update, we should send a stream WU first if there are unacked data for the current stream. Last, rcvd_s should always be reset when transitioning to FRAME_H (and a BUGON for this in dev would help). The bug will be way harder to trigger on 2.0 than on 1.8/1.9 because we have a ring buffer for the connection so the buffer full situations are extremely rare. This fix must be backpored to all versions having H2 (as far as 1.8). Special thanks to Patrick for providing exploitable traces. --- src/mux_h2.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/mux_h2.c b/src/mux_h2.c index 2b34508fb..89a8c0b38 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -2368,7 +2368,13 @@ static void h2_process_demux(struct h2c *h2c) } } - /* Only H2_CS_FRAME_P and H2_CS_FRAME_A here */ + /* Only H2_CS_FRAME_P, H2_CS_FRAME_A and H2_CS_FRAME_E here. + * H2_CS_FRAME_P indicates an incomplete previous operation + * (most often the first attempt) and requires some validity + * checks for the frame and the current state. The two other + * ones are set after completion (or abortion) and must skip + * validity checks. + */ tmp_h2s = h2c_st_by_id(h2c, h2c->dsi); if (tmp_h2s != h2s && h2s && h2s->cs && @@ -2386,6 +2392,9 @@ static void h2_process_demux(struct h2c *h2c) if (h2c->st0 == H2_CS_FRAME_E) goto strm_err; + if (h2c->st0 == H2_CS_FRAME_A) + goto valid_frame_type; + if (h2s->st == H2_SS_IDLE && h2c->dft != H2_FT_HEADERS && h2c->dft != H2_FT_PRIORITY) { /* RFC7540#5.1: any frame other than HEADERS or PRIORITY in @@ -2507,6 +2516,7 @@ static void h2_process_demux(struct h2c *h2c) } #endif + valid_frame_type: switch (h2c->dft) { case H2_FT_SETTINGS: if (h2c->st0 == H2_CS_FRAME_P)