BUG/MAJOR: htx: Fix htx_defrag() when an HTX block is expanded

When an HTX block is expanded, a defragmentation may be performed first to
have enough space to copy the new data. When it happens, the meta data of
the HTX message must take account of the new data length but copied data are
still unchanged at this stage (because we need more space to update the
message content). And here there is a bug because the meta data are updated
by the caller. It means that when the blocks content is copied, the new
length is already set. Thus a block larger than the reality is copied and
data outside the buffer may be accessed, leading to a crash.

To fix this bug, htx_defrag() is updated to use an extra argument with the
new meta data to use for the referenced block. Thus the caller does not need
to update the HTX message by itself. However, it still have to update the
data.

Most of time, the bug will be encountered in the HTTP compression
filter. But, even if it is highly unlikely, in theory it is also possible to
hit it when a HTTP header (or only its value) is replaced or when the
start-line is changed.

This patch must be backported as far as 2.0.

diff --git a/include/haproxy/htx.h b/include/haproxy/htx.h
index 7de316e333b8b3de191925007b0d28bab99809b9..e94e3da474240422974fcc6bf34d5e6b71dc39e2 100644 (file)
--- a/include/haproxy/htx.h
+++ b/include/haproxy/htx.h
@@ -32,7 +32,7 @@
 
 extern struct htx htx_empty;
 
-struct htx_blk *htx_defrag(struct htx *htx, struct htx_blk *blk);
+struct htx_blk *htx_defrag(struct htx *htx, struct htx_blk *blk, uint32_t info);
 struct htx_blk *htx_add_blk(struct htx *htx, enum htx_blk_type type, uint32_t blksz);
 struct htx_blk *htx_remove_blk(struct htx *htx, struct htx_blk *blk);
 struct htx_ret htx_find_offset(struct htx *htx, uint32_t offset);

diff --git a/src/htx.c b/src/htx.c
index 28b2c47b679aced97089d1d973460eef8ecaa996..3b49e78d17a970c2e3d81ca6aae84f06f85dc600 100644 (file)
--- a/src/htx.c
+++ b/src/htx.c
@@ -16,12 +16,15 @@
 struct htx htx_empty = { .size = 0, .data = 0, .head  = -1, .tail = -1, .first = -1 };
 
 /* Defragments an HTX message. It removes unused blocks and unwraps the payloads
- * part. A temporary buffer is used to do so. This function never fails. if
- * <blk> is not NULL, we replace it by the new block address, after the
- * defragmentation. The new <blk> is returned.
+ * part. A temporary buffer is used to do so. This function never fails. Most of
+ * time, we need keep a ref on a specific HTX block. Thus is <blk> is set, the
+ * pointer on its new position, after defrag, is returned. In addition, if the
+ * size of the block must be altered, <blkinfo> info must be provided (!=
+ * 0). But in this case, it remains the caller responsibility to update the
+ * block content.
  */
 /* TODO: merge data blocks into one */
-struct htx_blk *htx_defrag(struct htx *htx, struct htx_blk *blk)
+struct htx_blk *htx_defrag(struct htx *htx, struct htx_blk *blk, uint32_t blkinfo)
 {
        struct buffer *chunk = get_trash_chunk();
        struct htx *tmp = htxbuf(chunk);
@@ -38,6 +41,7 @@ struct htx_blk *htx_defrag(struct htx *htx, struct htx_blk *blk)
        new  = 0;
        addr = 0;
        tmp->size = htx->size;
+       tmp->data = 0;
 
        /* start from the head */
        for (old = htx_get_head(htx); old != -1; old = htx_get_next(htx, old)) {
@@ -45,25 +49,31 @@ struct htx_blk *htx_defrag(struct htx *htx, struct htx_blk *blk)
                if (htx_get_blk_type(oldblk) == HTX_BLK_UNUSED)
                        continue;
 
-               newblk = htx_get_blk(tmp, new);
-               newblk->addr = addr;
-               newblk->info = oldblk->info;
                blksz = htx_get_blksz(oldblk);
+               memcpy((void *)tmp->blocks + addr, htx_get_blk_ptr(htx, oldblk), blksz);
 
                /* update the start-line position */
                if (htx->first == old)
                        first = new;
 
+               newblk = htx_get_blk(tmp, new);
+               newblk->addr = addr;
+               newblk->info = oldblk->info;
+
                /* if <blk> is defined, save its new position */
-               if (blk != NULL && blk == oldblk)
+               if (blk != NULL && blk == oldblk) {
+                       if (blkinfo)
+                               newblk->info = blkinfo;
                        blkpos = new;
+               }
 
-               memcpy((void *)tmp->blocks + addr, htx_get_blk_ptr(htx, oldblk), blksz);
-               new++;
+               blksz = htx_get_blksz(newblk);
                addr += blksz;
-
+               tmp->data += blksz;
+               new++;
        }
 
+       htx->data = tmp->data;
        htx->first = first;
        htx->head = 0;
        htx->tail = new - 1;
@@ -174,7 +184,7 @@ static struct htx_blk *htx_reserve_nxblk(struct htx *htx, uint32_t blksz)
        else {
          defrag:
                /* need to defragment the message before inserting upfront */
-               htx_defrag(htx, NULL);
+               htx_defrag(htx, NULL, 0);
                tail = htx->tail + 1;
                blk = htx_get_blk(htx, tail);
                blk->addr = htx->tail_addr;
@@ -584,11 +594,6 @@ struct htx_blk *htx_replace_blk_value(struct htx *htx, struct htx_blk *blk,
        if (!ret)
                return NULL; /* not enough space */
 
-       /* Before updating the payload, set the new block size and update HTX
-        * message */
-       htx_set_blk_value_len(blk, v.len + delta);
-       htx->data += delta;
-
        if (ret == 1) { /* Replace in place */
                if (delta <= 0) {
                        /* compression: copy new data first then move the end */
@@ -600,6 +605,10 @@ struct htx_blk *htx_replace_blk_value(struct htx *htx, struct htx_blk *blk,
                        memmove(old.ptr + new.len, old.ptr + old.len, (v.ptr + v.len) - (old.ptr + old.len));
                        memcpy(old.ptr, new.ptr, new.len);
                }
+
+               /* set the new block size and update HTX message */
+               htx_set_blk_value_len(blk, v.len + delta);
+               htx->data += delta;
        }
        else if (ret == 2) { /* New address but no defrag */
                void *ptr = htx_get_blk_ptr(htx, blk);
@@ -618,26 +627,32 @@ struct htx_blk *htx_replace_blk_value(struct htx *htx, struct htx_blk *blk,
 
                /* Copy value after old part, if any */
                memcpy(ptr, old.ptr + old.len, (v.ptr + v.len) - (old.ptr + old.len));
-       }
-       else { /* Do a degrag first */
-               struct buffer *tmp = get_trash_chunk();
-
-               /* Copy the header name, if any */
-               chunk_memcat(tmp, n.ptr, n.len);
 
-               /* Copy value before old part, if any */
-               chunk_memcat(tmp, v.ptr, old.ptr - v.ptr);
+               /* set the new block size and update HTX message */
+               htx_set_blk_value_len(blk, v.len + delta);
+               htx->data += delta;
+       }
+       else { /* Do a degrag first (it is always an expansion) */
+               struct htx_blk tmpblk;
+               int32_t offset;
 
-               /* Copy new value */
-               chunk_memcat(tmp, new.ptr, new.len);
+               /* use tmpblk to set new block size before defrag and to compute
+                * the offset after defrag
+                */
+               tmpblk.addr = blk->addr;
+               tmpblk.info = blk->info;
+               htx_set_blk_value_len(&tmpblk, v.len + delta);
 
-               /* Copy value after old part if any */
-                chunk_memcat(tmp, old.ptr + old.len, (v.ptr + v.len) - (old.ptr + old.len));
+               /* htx_defrag() will take care to update the block size and the htx message */
+               blk = htx_defrag(htx, blk, tmpblk.info);
 
-               blk = htx_defrag(htx, blk);
+               /* newblk is now the new HTX block. Compute the offset to copy/move payload */
+               offset = blk->addr - tmpblk.addr;
 
-               /* Finally, copy data. */
-               memcpy(htx_get_blk_ptr(htx, blk), tmp->area, tmp->data);
+               /* move the end first and copy new data
+                */
+               memmove(old.ptr + offset + new.len, old.ptr + offset + old.len, (v.ptr + v.len) - (old.ptr + old.len));
+               memcpy(old.ptr + offset, new.ptr, new.len);
        }
        return blk;
 }
@@ -769,15 +784,17 @@ struct htx_blk *htx_replace_header(struct htx *htx, struct htx_blk *blk,
        if (!ret)
                return NULL; /* not enough space */
 
-       /* Set the new block size and update HTX message */
-       blk->info = (type << 28) + (value.len << 8) + name.len;
-       htx->data += delta;
 
        /* Replace in place or at a new address is the same. We replace all the
         * header (name+value). Only take care to defrag the message if
         * necessary. */
        if (ret == 3)
-               blk = htx_defrag(htx, blk);
+               blk = htx_defrag(htx, blk, (type << 28) + (value.len << 8) + name.len);
+       else {
+               /* Set the new block size and update HTX message */
+               blk->info = (type << 28) + (value.len << 8) + name.len;
+               htx->data += delta;
+       }
 
        /* Finally, copy data. */
        ptr = htx_get_blk_ptr(htx, blk);
@@ -815,14 +832,16 @@ struct htx_sl *htx_replace_stline(struct htx *htx, struct htx_blk *blk, const st
        if (!ret)
                return NULL; /* not enough space */
 
-       /* Set the new block size and update HTX message */
-       htx_set_blk_value_len(blk, sz+delta);
-       htx->data += delta;
-
        /* Replace in place or at a new address is the same. We replace all the
         * start-line. Only take care to defrag the message if necessary. */
-       if (ret == 3)
-               blk = htx_defrag(htx, blk);
+       if (ret == 3)  {
+               blk = htx_defrag(htx, blk, (type << 28) + sz + delta);
+       }
+       else {
+               /* Set the new block size and update HTX message */
+               blk->info = (type << 28) + sz + delta;
+               htx->data += delta;
+       }
 
        /* Restore start-line info and flags and copy parts of the start-line */
        sl = htx_get_blk_ptr(htx, blk);