From: Willy Tarreau <w@1wt.eu>
Date: Thu, 25 Feb 2016 15:15:19 +0000 (+0100)
Subject: BUG/MEDIUM: chunks: always reject negative-length chunks
X-Git-Tag: v1.7-dev2~73
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=320ec2a7454a84a6bb318a8a132d0300b712937f;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: chunks: always reject negative-length chunks

The recent addition of "show env" on the CLI has revealed an interesting
design bug. Chunks are supposed to support a negative length to indicate
that they carry no data. chunk_printf() sets this size to -1 if the string
is too large for the buffer. At a few places in the http engine we may end
up with trash.len = -1. But bi_putchk(), chunk_appendf() and a few other
chunks consumers don't consider this case as possible and will use such a
chunk, possibly restoring an invalid string or trying to copy -1 bytes.

This fix takes care of clarifying the situation in a backportable way
where such sizes are used, so that a negative length indicating an error
remains present until the chunk is reinitialized or overwritten. But a
cleaner design adjustment needs to be done so that there's a clear contract
on how to use these chunks. At first glance it doesn't seem *that* useful
to support negative sizes, so probably this is what should change.

This fix must be backported to 1.6 and 1.5.
---

diff --git a/include/common/chunk.h b/include/common/chunk.h
index 851c7a65e2..b74c767488 100644
--- a/include/common/chunk.h
+++ b/include/common/chunk.h
@@ -67,7 +67,7 @@ static inline void chunk_init(struct chunk *chk, char *str, size_t size)
 static inline int chunk_initlen(struct chunk *chk, char *str, size_t size, int len)
 {
 
-	if (size && len > size)
+	if (len < 0 || (size && len > size))
 		return 0;
 
 	chk->str  = str;
@@ -112,7 +112,7 @@ static inline int chunk_strcat(struct chunk *chk, const char *str)
 
 	len = strlen(str);
 
-	if (unlikely(chk->len + len >= chk->size))
+	if (unlikely(chk->len < 0 || chk->len + len >= chk->size))
 		return 0;
 
 	memcpy(chk->str + chk->len, str, len + 1);
@@ -134,7 +134,7 @@ static inline int chunk_strcat(struct chunk *chk, const char *str)
  */
 static inline char *chunk_newstr(struct chunk *chk)
 {
-	if (chk->len + 1 >= chk->size)
+	if (chk->len < 0 || chk->len + 1 >= chk->size)
 		return NULL;
 
 	chk->str[chk->len++] = 0;
@@ -166,7 +166,7 @@ static inline void chunk_destroy(struct chunk *chk)
  */
 static inline char *chunk_dup(struct chunk *dst, const struct chunk *src)
 {
-	if (!dst || !src || !src->str)
+	if (!dst || !src || src->len < 0 || !src->str)
 		return NULL;
 
 	if (dst->size)
diff --git a/src/channel.c b/src/channel.c
index 755d2d9cb0..4728986cba 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -77,7 +77,7 @@ int bo_inject(struct channel *chn, const char *msg, int len)
 	if (len == 0)
 		return -1;
 
-	if (len > chn->buf->size) {
+	if (len < 0 || len > chn->buf->size) {
 		/* we can't write this chunk and will never be able to, because
 		 * it is larger than the buffer. This must be reported as an
 		 * error. Then we return -2 so that writers that don't care can
@@ -142,6 +142,9 @@ int bi_putblk(struct channel *chn, const char *blk, int len)
 	if (unlikely(channel_input_closed(chn)))
 		return -2;
 
+	if (len < 0)
+		return -3;
+
 	max = channel_recv_limit(chn);
 	if (unlikely(len > max - buffer_len(chn->buf))) {
 		/* we can't write this chunk right now because the buffer is
diff --git a/src/chunk.c b/src/chunk.c
index 1359adc0c0..e2511076ba 100644
--- a/src/chunk.c
+++ b/src/chunk.c
@@ -111,7 +111,7 @@ int chunk_appendf(struct chunk *chk, const char *fmt, ...)
 	va_list argp;
 	int ret;
 
-	if (!chk->str || !chk->size)
+	if (chk->len < 0 || !chk->str || !chk->size)
 		return 0;
 
 	va_start(argp, fmt);
@@ -136,6 +136,9 @@ int chunk_htmlencode(struct chunk *dst, struct chunk *src)
 	int olen, free;
 	char c;
 
+	if (dst->len < 0)
+		return dst->len;
+
 	olen = dst->len;
 
 	for (i = 0; i < src->len; i++) {
@@ -177,6 +180,9 @@ int chunk_asciiencode(struct chunk *dst, struct chunk *src, char qc)
 	int olen, free;
 	char c;
 
+	if (dst->len < 0)
+		return dst->len;
+
 	olen = dst->len;
 
 	for (i = 0; i < src->len; i++) {