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)
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 @@
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 @@
* 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 @@
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 @@
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 @@
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 */