MEDIUM: h1: allow to preserve keep-alive on T-E + C-L

In 2.5-dev9, commit 631c7e866 ("MEDIUM: h1: Force close mode for invalid
uses of T-E header") enforced a recently arrived new security rule in the
HTTP specification aiming at preventing a class of content-smuggling
attacks involving HTTP/1.0 agents. It consists in handling the very rare
T-E + C-L requests or responses in close mode.

It happens it does have an impact of a rare few and very old clients
(probably running insecure TLS stacks by the way) that continue to send
both with their POST requests. The impact is that for each and every
request they'll have to reconnect, possibly negotiating a full TLS
handshake that becomes harmful to the machine in terms of CPU computation.

This commit adds a new option "h1-do-not-close-on-insecure-transfer-encoding"
that does exactly what it says, it just asks not to close on such messages,
even though the message continues to be sanitized and C-L dropped. It means
that the risk is only between the sender and haproxy, which is limited, and
might be the only acceptable solution for such environments having to deal
with broken implementations.

The cases are so rare that it should not need to be backported, or in the
worst case, to the latest LTS if there is any demand.

(cherry picked from commit 2dab1ba84b11fe43baa91642ffcddb90e9ec09d2)
Signed-off-by: Willy Tarreau <w@1wt.eu>

doc/configuration.txt

@@ -1275,6 +1275,7 @@ The following keywords are supported in the "global" section :
    - h1-accept-payload-with-any-method
    - h1-case-adjust
    - h1-case-adjust-file
+   - h1-do-not-close-on-insecure-transfer-encoding
    - h2-workaround-bogus-websocket-clients
    - hard-stop-after
    - harden.reject-privileged-ports.tcp
@@ -1871,6 +1872,32 @@ h1-accept-payload-with-any-method
   However, it may be an issue with some old clients. In this case, this global
   option may be set.
 
+h1-do-not-close-on-insecure-transfer-encoding
+  As mandated by the HTTP/1.1 specification (RFC9112#6.1), the presence of both
+  a Transfer-Encoding header field and a Content-Length header field in the
+  same message represents a serious risk of conveying a content smuggling
+  attack if there are any HTTP/1.0 agent anywhere in the upstream of downstream
+  chain, and when facing this, an agent must absolutely close the connection
+  after the response so as to prevent any exploitation. But this may have a
+  performance impact on some very old clients, especially if they need to
+  renegotiate a TLS connection for every request. This option is present to
+  ask HAProxy not to enforce this rule, and to just sanitize the message but
+  leave the connection alive after the response. This may only be done when
+  absolutely certain that no HTTP/1.0 agents are present in the chain and that
+  all implementations before HAProxy are fully HTTP/1.1 compliant regarding the
+  rules that apply to these header fields. In any case, HAProxy will continue
+  to ignore and drop the extraneous Content-Length header so as not to confuse
+  the next hop.
+
+  When enabling this option to work around an old broken client or server, it
+  is important to understand that regardless of the need or not for this
+  option, such an agent violating this rule faces a risk to see its messages
+  truncated by old agents that would consider Content-Length and ignore
+  Transfer-Encoding, since the cumulated size of the encoded chunk sizes are
+  not being accounted for. As such, the rule above is not just a matter of
+  security but also of taking care of getting rid of agents that may face
+  communication trouble due to incompatibilities with older ones.
+
 h1-case-adjust <from> <to>
   Defines the case adjustment to apply, when enabled, to the header name
   <from>, to change it to <to> before sending it to HTTP/1 clients or

include/haproxy/h1.h

@@ -150,6 +150,8 @@ union h1_sl {                          /* useful start line pointers, relative t
 	} st;                          /* status line : field, length */
 };
 
+extern int h1_do_not_close_on_insecure_t_e;
+
 int h1_headers_to_hdr_list(char *start, const char *stop,
                            struct http_hdr *hdr, unsigned int hdr_num,
                            struct h1m *h1m, union h1_sl *slp);

src/h1.c

@@ -16,10 +16,17 @@
 
 #include <haproxy/api.h>
 #include <haproxy/base64.h>
+#include <haproxy/cfgparse.h>
 #include <haproxy/h1.h>
 #include <haproxy/http-hdr.h>
 #include <haproxy/tools.h>
 
+/* by default, RFC9112#6.1 applies, t-e combined with c-l represents a risk of
+ * smuggling if it crosses another 1.0 agent so we must close at the end of the
+ * transaction. But it may cause difficulties to some very old broken devices.
+ */
+int h1_do_not_close_on_insecure_t_e = 0;
+
 /* Parse the Content-Length header field of an HTTP/1 request. The function
  * checks all possible occurrences of a comma-delimited value, and verifies
  * if any of them doesn't match a previous value. It returns <0 if a value
@@ -1202,7 +1209,9 @@ int h1_headers_to_hdr_list(char *start, const char *stop,
 		if (h1m->flags & H1_MF_XFER_ENC) {
 			if (h1m->flags & H1_MF_CLEN) {
 				/* T-E + C-L: force close and remove C-L */
-				h1m->flags |= H1_MF_CONN_CLO;
+				if (!h1_do_not_close_on_insecure_t_e)
+					h1m->flags |= H1_MF_CONN_CLO;
+
 				h1m->flags &= ~H1_MF_CLEN;
 				h1m->curr_len = h1m->body_len = 0;
 				hdr_count = http_del_hdr(hdr, ist("content-length"));
@@ -1316,3 +1325,23 @@ void h1_calculate_ws_output_key(const char *key, char *result)
 	/* encode in base64 the hash */
 	a2base64(hash_out, 20, result, 29);
 }
+
+/* config parser for global "h1-do-not-close-on-insecure-transfer-encoding" */
+static int cfg_parse_h1_do_not_close_insecure_t_e(char **args, int section_type, struct proxy *curpx,
+                                                const struct proxy *defpx, const char *file, int line,
+                                                char **err)
+{
+	if (too_many_args(0, args, err, NULL))
+		return -1;
+
+	h1_do_not_close_on_insecure_t_e = 1;
+	return 0;
+}
+
+/* config keyword parsers */
+static struct cfg_kw_list cfg_kws = {{ }, {
+	{ CFG_GLOBAL, "h1-do-not-close-on-insecure-transfer-encoding", cfg_parse_h1_do_not_close_insecure_t_e },
+	{ 0, NULL, NULL },
+}};
+
+INITCALL1(STG_REGISTER, cfg_register_keywords, &cfg_kws);

src/mux_h1.c

@@ -2627,9 +2627,11 @@ static size_t h1_make_eoh(struct h1s *h1s, struct h1m *h1m, struct htx *htx, siz
 	if (!(h1s->flags & H1S_F_HAVE_O_CONN)) {
 		if ((h1m->flags & (H1_MF_XFER_ENC|H1_MF_CLEN)) == (H1_MF_XFER_ENC|H1_MF_CLEN)) {
 			/* T-E + C-L: force close */
-			h1s->flags = (h1s->flags & ~H1S_F_WANT_MSK) | H1S_F_WANT_CLO;
 			h1m->flags &= ~H1_MF_CLEN;
-			TRACE_STATE("force close mode (T-E + C-L)", H1_EV_TX_DATA|H1_EV_TX_HDRS, h1s->h1c->conn, h1s);
+			if (!h1_do_not_close_on_insecure_t_e) {
+				h1s->flags = (h1s->flags & ~H1S_F_WANT_MSK) | H1S_F_WANT_CLO;
+				TRACE_STATE("force close mode (T-E + C-L)", H1_EV_TX_DATA|H1_EV_TX_HDRS, h1s->h1c->conn, h1s);
+			}
 		}
 		else if ((h1m->flags & (H1_MF_VER_11|H1_MF_XFER_ENC)) == H1_MF_XFER_ENC) {
 			/* T-E + HTTP/1.0: force close */