[Libguestfs] Re: [nbdkit PATCH] server: Fix off-by-one for maximum block_status length [CVE-XXX]
On Wed, Apr 23, 2025 at 08:01:55AM -0500, Eric Blake via Libguestfs wrote:
My additional analysis:
>
> blocks[i].length = length = MIN (e.length, UINT32_MAX);
>
> The extent we're sending has e.length is >= 4G, which we truncate to
> UINT32_MAX == 4G-1.
The ONLY time length < e.length is if we truncated an extent that was
4G or larger to 4G-1. All other times, length == e.length.
>
> pos += length;
>
> pos += 4G-1
If there was only one extent, then pos == offset+length; if there was
more than one extent, then pos > offset+length. We may still have pos
< offset+count, depending on whether the extents seen so far have
summed up past count.
>
> In the previous code:
>
> if (pos > offset + count) /* this must be the last block */
Because we only accept a 32-bit count, the largest value we can see is
a count of 4G-1. If there was more than one extent (even a one-byte
extent followed by a 4G-1 extent), we are guaranteed that pos >
offset+count. But the NBD protocol says that the last extent in our
reply must start prior to offset+count (when req_one is off, our last
extent may extend beyond the client's limit, but no new extents should
be volunteered to the client).
> break;
>
> branch not taken so we reach:
>
> assert (e.length <= length);
>
> e.length == 4G, length == 4G-1, so we assert here.
Yep. And if compiled with NDEBUG, the assertion doesn't trigger, but
the loop could continue and try to send the client an extent that
starts after the maximum offset permitted, so even if we don't crash,
we'd be violating the NBD spec.
>
> In the updated code, the assert is avoided, which means this branch
> must now be taken, which means the client requested 'offset' == 0 &
> 'count' must be exactly 4G-1, I think? (Or any relative request)
>
> if (pos >= offset + count) /* this must be the last block */
> break;
The combination of user requesting len=4G-1 and the plugin answering
with a single extent >= 4G is what I tested. I still need to analyze
if there are other conditions where the off-by-one could also bite us
where we provide too many extents in reply to the user without
triggering the assertion. But it looks like the assertion can only be
triggered if the user requests len 4G-1.
I could not find any other ways to tickle the assertion failure (any
split of the plugin's report into more than one extent will either
break the loop, or satisfy the assertion); but the following DOES
coerce the server into sending too many extents in violation of the
protocol without triggering the assertion:
$ ext="
echo 0 1 0
echo 1 $((4*1024*1024*1024-1)) 1
echo $((4*1024*1024*1024)) 1G 0
"
$ nbdkit -f eval --filter=ddrescue get_size='echo 5G' extents="$ext" \
--run 'nbdsh --base-allocation --uri "$uri" -c "
def f(m,o,e,s):
print(e)
h.block_status(2**32-1,1,f)
"'
[4294967295, 1, 1073741824, 0]
and is also fixed by my patch to correctly print
[4294967295, 1]
I'll update the test to additionally cover that in v2 of the patch,
while still waiting for secalert's answer on a CVE.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org