BUG/MEDIUM: htx: Reserve a block descriptor for zero-sized blocks

htx_reserve_nxblk() only refused to reserve a new block when
<blksz> was greater than the free space available for the payload, as
reported by htx_free_data_space(). However this function returns 0 in two
different cases: when there is exactly enough room left for a block
descriptor and no payload, but also when there is not even enough room for
a block descriptor. Both cases cannot be distinguished when a zero-sized
block is added, because the test is then always false.

In that case, the function goes on and hands out a block descriptor that
does not fit in the blocks table. Concretely, with <head> set to 0 and less
than sizeof(struct htx_blk) bytes left in the message, the defragmentation
has nothing to reclaim and the descriptor is placed inside the payload
part, silently overwriting the end of the last block. In addition, the tail
is incremented, so htx_used_space() becomes greater than the message size
and htx_free_space() wraps around to a huge value. None of the sanity
checks at the end of the function is able to detect it.

The free space is now explicitly tested against the size of a block
descriptor. This way, a zero-sized block is refused when there is no room
left for its descriptor. It is fixed in htx_reserve_nxblk() to also cover
htx_add_data_atonce(), which has the same flaw in its own test, and
htx_add_header()/htx_add_trailer(), which compute a null size when both the
name and the value are empty.

The only known caller able to request a zero-sized block is the
decompression filter, when there is no room left to drain the decompressed
data. It was introduced in 3.5-dev, so no released version should be
affected. However the flaw exists in the HTX API since its introduction in
1.9, so this patch may be backported as hardening to all stable versions.

Reported-by: Asjid Kalam <asjid.kalam@gmail.com>

diff --git a/src/htx.c b/src/htx.c
index cf70ffe2a428b30c913d7e53368c831607be8db0..1ff7ccce8737d5f11a8b3fb9c4ff063e8cdb66c3 100644 (file)
--- a/src/htx.c
+++ b/src/htx.c
@@ -159,7 +159,14 @@ static struct htx_blk *htx_reserve_nxblk(struct htx *htx, uint32_t blksz)
        struct htx_blk *blk;
        uint32_t tail, headroom, tailroom;
 
-       if (blksz > htx_free_data_space(htx))
+       /* A block descriptor must always be reserved, even for a zero-sized
+        * block. htx_free_data_space() returns 0 when there is not even enough
+        * room for a descriptor, so it cannot be used alone here: a zero-sized
+        * block would pass the test and a descriptor would then be placed
+        * outside of the blocks table, overwriting the payload.
+        */
+       if (blksz > htx_free_data_space(htx) ||
+           htx_free_space(htx) < sizeof(struct htx_blk))
                return NULL; /* full */
 
        if (htx->head == -1) {
@@ -184,7 +191,6 @@ static struct htx_blk *htx_reserve_nxblk(struct htx *htx, uint32_t blksz)
        else if (htx->head > 0) {
                htx_defrag_blks(htx);
                tail = htx->tail + 1;
-               BUG_ON(htx_pos_to_addr(htx, tail) < htx->tail_addr);
        }
        else
                goto defrag;
@@ -218,6 +224,7 @@ static struct htx_blk *htx_reserve_nxblk(struct htx *htx, uint32_t blksz)
                /* need to defragment the message before inserting upfront */
                htx_defrag(htx, NULL, 0);
                tail = htx->tail + 1;
+               BUG_ON(htx_pos_to_addr(htx, tail) < htx->tail_addr + blksz);
                blk = htx_get_blk(htx, tail);
                blk->addr = htx->tail_addr;
                htx->tail_addr += blksz;