From b4d05093bc89f71377230228007e69a1434c1a0c Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Mon, 1 Sep 2014 20:35:55 +0200 Subject: [PATCH] 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. (cherry picked from commit a2d002aeb669a9bbca2dcd3e6be71615f435e9e6) --- include/types/channel.h | 2 +- src/proto_http.c | 8 ++++---- src/stream_interface.c | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/types/channel.h b/include/types/channel.h index 88a52a4..8bc3958 100644 --- 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 a47f0a1..4d27b2c 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -4886,8 +4886,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); @@ -5430,7 +5430,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; @@ -5482,7 +5482,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 67a5234..9f7e979 100644 --- 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 */ -- 1.9.3