From b4d05093bc89f71377230228007e69a1434c1a0c Mon Sep 17 00:00:00 2001

From: Willy Tarreau <w@1wt.eu>

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