BUG/CRITICAL: http: don't update msg->sov once data start to leave the buffer

Commit bb2e669 ("BUG/MAJOR: http: correctly rewind the request body
after start of forwarding") was incorrect/incomplete. It used to rely on
CF_READ_ATTACHED to stop updating msg->sov once data start to leave the
buffer, but this is unreliable because since commit a6eebb3 ("[BUG]
session: clear BF_READ_ATTACHED before next I/O") merged in 1.5-dev1,
this flag is only ephemeral and is cleared once all analysers have
seen it. So we can start updating msg->sov again each time we pass
through this place with new data. With a sufficiently large amount of
data, it is possible to make msg->sov wrap and validate the if()
condition at the top, causing the buffer to advance by about 2GB and
crash the process.

Note that the offset cannot be controlled by the attacker because it is
a sum of millions of small random sizes depending on how many bytes were
read by the server and how many were left in the buffer, only because
of the speed difference between reading and writing. Also, nothing is
written, the invalid pointer resulting from this operation is only read.

Many thanks to James Dempsey for reporting this bug and to Chris Forbes for
narrowing down the faulty area enough to make its root cause analysable.

This fix must be backported to haproxy 1.5.

diff --git a/include/types/channel.h b/include/types/channel.h
index 88a52a412866d5ed4e97b17e9aaf79ae22281ee5..8bc395830348c5e2c318e9d3a9c87646b5405929 100644 (file)
--- a/include/types/channel.h
+++ b/include/types/channel.h
@@ -105,7 +105,7 @@
 #define CF_STREAMER       0x00010000  /* the producer is identified as streaming data */
 #define CF_STREAMER_FAST  0x00020000  /* the consumer seems to eat the stream very fast */
 
-/* unused: 0x00040000 */
+#define CF_WROTE_DATA     0x00040000  /* some data were sent from this buffer */
 #define CF_ANA_TIMEOUT    0x00080000  /* the analyser timeout has expired */
 #define CF_READ_ATTACHED  0x00100000  /* the read side is attached for the first time */
 #define CF_KERN_SPLICING  0x00200000  /* kernel splicing desired for this channel */

diff --git a/src/proto_http.c b/src/proto_http.c
index f0dd0c83d7b920b2931b40b80f31b5d9620421df..24948508563e3a58e24f2f427204270cc08b463e 100644 (file)
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -4919,8 +4919,8 @@ void http_end_txn_clean_session(struct session *s)
        s->req->cons->conn_retries = 0;  /* used for logging too */
        s->req->cons->exp       = TICK_ETERNITY;
        s->req->cons->flags    &= SI_FL_DONT_WAKE; /* we're in the context of process_session */
-       s->req->flags &= ~(CF_SHUTW|CF_SHUTW_NOW|CF_AUTO_CONNECT|CF_WRITE_ERROR|CF_STREAMER|CF_STREAMER_FAST|CF_NEVER_WAIT|CF_WAKE_CONNECT);
-       s->rep->flags &= ~(CF_SHUTR|CF_SHUTR_NOW|CF_READ_ATTACHED|CF_READ_ERROR|CF_READ_NOEXP|CF_STREAMER|CF_STREAMER_FAST|CF_WRITE_PARTIAL|CF_NEVER_WAIT);
+       s->req->flags &= ~(CF_SHUTW|CF_SHUTW_NOW|CF_AUTO_CONNECT|CF_WRITE_ERROR|CF_STREAMER|CF_STREAMER_FAST|CF_NEVER_WAIT|CF_WAKE_CONNECT|CF_WROTE_DATA);
+       s->rep->flags &= ~(CF_SHUTR|CF_SHUTR_NOW|CF_READ_ATTACHED|CF_READ_ERROR|CF_READ_NOEXP|CF_STREAMER|CF_STREAMER_FAST|CF_WRITE_PARTIAL|CF_NEVER_WAIT|CF_WROTE_DATA);
        s->flags &= ~(SN_DIRECT|SN_ASSIGNED|SN_ADDR_SET|SN_BE_ASSIGNED|SN_FORCE_PRST|SN_IGNORE_PRST);
        s->flags &= ~(SN_CURR_SESS|SN_REDIRECTABLE|SN_SRV_REUSED);
 
@@ -5463,7 +5463,7 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
                         * such as last chunk of data or trailers.
                         */
                        b_adv(req->buf, msg->next);
-                       if (unlikely(!(s->rep->flags & CF_READ_ATTACHED)))
+                       if (unlikely(!(s->req->flags & CF_WROTE_DATA)))
                                msg->sov -= msg->next;
                        msg->next = 0;
 
@@ -5515,7 +5515,7 @@ int http_request_forward_body(struct session *s, struct channel *req, int an_bit
  missing_data:
        /* we may have some pending data starting at req->buf->p */
        b_adv(req->buf, msg->next);
-       if (unlikely(!(s->rep->flags & CF_READ_ATTACHED)))
+       if (unlikely(!(s->req->flags & CF_WROTE_DATA)))
                msg->sov -= msg->next + MIN(msg->chunk_len, req->buf->i);
 
        msg->next = 0;

diff --git a/src/stream_interface.c b/src/stream_interface.c
index 67a5234fa86ebddfbf5e0efd275d070bec95620e..9f7e979edf51b0e7a156acf9a867d048770d59de 100644 (file)
--- a/src/stream_interface.c
+++ b/src/stream_interface.c
@@ -658,7 +658,7 @@ static void si_conn_send(struct connection *conn)
        if (chn->pipe && conn->xprt->snd_pipe) {
                ret = conn->xprt->snd_pipe(conn, chn->pipe);
                if (ret > 0)
-                       chn->flags |= CF_WRITE_PARTIAL;
+                       chn->flags |= CF_WRITE_PARTIAL | CF_WROTE_DATA;
 
                if (!chn->pipe->data) {
                        put_pipe(chn->pipe);
@@ -702,7 +702,7 @@ static void si_conn_send(struct connection *conn)
 
                ret = conn->xprt->snd_buf(conn, chn->buf, send_flag);
                if (ret > 0) {
-                       chn->flags |= CF_WRITE_PARTIAL;
+                       chn->flags |= CF_WRITE_PARTIAL | CF_WROTE_DATA;
 
                        if (!chn->buf->o) {
                                /* Always clear both flags once everything has been sent, they're one-shot */