From 001fb1a5488b5059ca4c26a82dd4f00e9850f1b4 Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Fri, 6 Sep 2024 09:16:16 +0200 Subject: [PATCH] BUG/MEDIUM: mux-h1/mux-h2: Reject upgrades with payload on H2 side only Since 1d2d77b27 ("MEDIUM: mux-h1: Return a 501-not-implemented for upgrade requests with a body"), it is no longer possible to perform a protocol upgrade for requests with a payload. The main reason was to be able to support protocol upgrade for H1 client requesting a H2 server. In that case, the upgrade request is converted to a CONNECT request. So, it is not possible to convey a payload in that case. But, it is a problem for anyone wanting to perform upgrades on H1 server using requests with a payload. It is uncommon but valid. So, now, it is the H2 multiplexer responsibility to reject upgrade requests, on server side, if there is a payload. An INTERNAL_ERROR is returned for the H2S in that case. On H1 side, the upgrade is now allowed, but only if the server waits for the end of the request to return the 101-Switching-protocol response. Indeed, it is quite hard to synchronise the frontend side and the backend side in that case. Asking to servers to fully consume the request payload before returned the response seems reasonable. This patch should fix the issue #2684. It could be backported after a period of observation, as far as 2.4 if possible. But only if it is not too hard. It depends on "MINOR: mux-h1: Set EOI on SE during demux when both side are in DONE state". --- src/h1_htx.c | 4 +--- src/mux_h1.c | 10 +++++++++- src/mux_h2.c | 6 ++++++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/h1_htx.c b/src/h1_htx.c index 472169aec5..0f76cea8d4 100644 --- a/src/h1_htx.c +++ b/src/h1_htx.c @@ -182,11 +182,9 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx flags |= h1m_htx_sl_flags(h1m); /* Remove Upgrade header in problematic cases : - * - body present * - "h2c" or "h2" token specified as token */ - if (((flags & (HTX_SL_F_CONN_UPG|HTX_SL_F_BODYLESS)) == HTX_SL_F_CONN_UPG) || - ((h1m->flags & (H1_MF_CONN_UPG|H1_MF_UPG_H2C)) == (H1_MF_CONN_UPG|H1_MF_UPG_H2C))) { + if ((h1m->flags & (H1_MF_CONN_UPG|H1_MF_UPG_H2C)) == (H1_MF_CONN_UPG|H1_MF_UPG_H2C)) { int i; for (i = 0; hdrs[i].n.len; i++) { diff --git a/src/mux_h1.c b/src/mux_h1.c index 88523a991f..058474f625 100644 --- a/src/mux_h1.c +++ b/src/mux_h1.c @@ -2130,8 +2130,16 @@ static size_t h1_process_demux(struct h1c *h1c, struct buffer *buf, size_t count } if ((h1m->flags & H1_MF_RESP) && - ((h1s->meth == HTTP_METH_CONNECT && h1s->status >= 200 && h1s->status < 300) || h1s->status == 101)) + ((h1s->meth == HTTP_METH_CONNECT && h1s->status >= 200 && h1s->status < 300) || h1s->status == 101)) { + if (h1s->req.state != H1_MSG_DONE) { + TRACE_STATE("Reject tunnel because request is not finished", H1_EV_RX_DATA|H1_EV_H1S_BLK, h1c->conn, h1s); + h1s->flags |= H1S_F_PARSING_ERROR; + htx->flags |= HTX_FL_PARSING_ERROR; + h1_capture_bad_message(h1s->h1c, h1s, h1m, buf); + break; + } h1_set_tunnel_mode(h1s); + } else { if (h1s->req.state < H1_MSG_DONE || h1s->res.state < H1_MSG_DONE) { /* Unfinished transaction: block this input side waiting the end of the output side */ diff --git a/src/mux_h2.c b/src/mux_h2.c index 55e5045195..7ed33c912e 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -5989,6 +5989,12 @@ static size_t h2s_snd_bhdrs(struct h2s *h2s, struct htx *htx) if ((sl->flags & HTX_SL_F_CONN_UPG) && isteqi(list[hdr].n, ist("connection"))) { /* rfc 7230 #6.1 Connection = list of tokens */ struct ist connection_ist = list[hdr].v; + + if (!(sl->flags & HTX_SL_F_BODYLESS)) { + TRACE_STATE("cannot convert upgrade for request with payload", H2_EV_TX_FRAME|H2_EV_TX_HDR, h2c->conn, h2s); + goto fail; + } + do { if (isteqi(iststop(connection_ist, ','), ist("upgrade"))) {