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".

diff --git a/src/h1_htx.c b/src/h1_htx.c
index 472169aec55077fef9c9e5bf64377001b3e0040c..0f76cea8d4280fab6f22084adbe374fe6a2c8f5a 100644 (file)
--- 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 88523a991fc2a0b73fcfca3cdeec85fdfa01334a..058474f62517aa463d061734f55ef9d1456497ad 100644 (file)
--- 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 55e50451957f4182e57f9bdba05999d88ab744bf..7ed33c912e2338ad7a0b8bf58e2ea3b0d98a0745 100644 (file)
--- 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"))) {