[nbdkit PATCH] server: Fix off-by-one for maximum block_status length [CVE-XXX]
Latent since the introduction of the .extents callback. The loop intentionally truncates to 2**32-1 bytes, but condition to end the loop early used > while the assertion after the loop used <=, meaning that the assertion can fire for any plugin that returns an extent of 2**32 or larger. Fixes: 26455d45 ('server: protocol: Implement Block Status "base:allocation".', v1.11.10) Reported-by: Nikolay Ivanets <stenavin@gmail.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- We still need to sort out if this gets a CVE number; either way, the commit title needs an update, and if it does get a CVE we need to also post a security alert message to the list. tests/Makefile.am | 2 ++ server/protocol.c | 2 +- tests/test-eval-extents.sh | 60 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100755 tests/test-eval-extents.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index 13c0457c..ffe42c78 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -881,6 +881,7 @@ TESTS += \ test-eval.sh \ test-eval-file.sh \ test-eval-exports.sh \ + test-eval-extents.sh \ test-eval-cache.sh \ test-eval-dump-plugin.sh \ test-eval-disconnect.sh \ @@ -889,6 +890,7 @@ EXTRA_DIST += \ test-eval.sh \ test-eval-file.sh \ test-eval-exports.sh \ + test-eval-extents.sh \ test-eval-cache.sh \ test-eval-dump-plugin.sh \ test-eval-disconnect.sh \ diff --git a/server/protocol.c b/server/protocol.c index d428bfc8..b4b1c162 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -499,7 +499,7 @@ extents_to_block_descriptors (struct nbdkit_extents *extents, (*nr_blocks)++; pos += length; - if (pos > offset + count) /* this must be the last block */ + if (pos >= offset + count) /* this must be the last block */ break; /* If we reach here then we must have consumed this whole diff --git a/tests/test-eval-extents.sh b/tests/test-eval-extents.sh new file mode 100755 index 00000000..08c8d670 --- /dev/null +++ b/tests/test-eval-extents.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright Red Hat +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires_run +requires_plugin eval +requires_nbdsh_uri +requires nbdsh --base-allocation + +files="eval-extents.out" +rm -f $files +cleanup_fn rm -f $files + +# Trigger an off-by-one bug introduced in v1.11.10 and fixed in v1.43.6 +export script=' +def f(context, offset, extents, status): + print(extents) + +h.block_status(2**32-1, 0, f) +' +nbdkit eval \ + get_size='echo 5G' \ + pread='dd if=/dev/zero count=$3 iflag=count_bytes' \ + extents='echo 0 5G 0' \ + --run 'nbdsh --base-allocation --uri "$uri" -c "$script"' \ + > eval-extents.out +cat eval-extents.out +grep -E '\[4294967295, 0]' eval-extents.out -- 2.49.0
On Tue, Apr 22, 2025 at 05:07:17PM -0500, Eric Blake via Libguestfs wrote:
Latent since the introduction of the .extents callback. The loop intentionally truncates to 2**32-1 bytes, but condition to end the loop early used > while the assertion after the loop used <=, meaning that the assertion can fire for any plugin that returns an extent of 2**32 or larger.
Fixes: 26455d45 ('server: protocol: Implement Block Status "base:allocation".', v1.11.10) Reported-by: Nikolay Ivanets <stenavin@gmail.com> Signed-off-by: Eric Blake <eblake@redhat.com> ---
We still need to sort out if this gets a CVE number; either way, the commit title needs an update, and if it does get a CVE we need to also post a security alert message to the list.
tests/Makefile.am | 2 ++ server/protocol.c | 2 +- tests/test-eval-extents.sh | 60 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100755 tests/test-eval-extents.sh
diff --git a/tests/Makefile.am b/tests/Makefile.am index 13c0457c..ffe42c78 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -881,6 +881,7 @@ TESTS += \ test-eval.sh \ test-eval-file.sh \ test-eval-exports.sh \ + test-eval-extents.sh \ test-eval-cache.sh \ test-eval-dump-plugin.sh \ test-eval-disconnect.sh \ @@ -889,6 +890,7 @@ EXTRA_DIST += \ test-eval.sh \ test-eval-file.sh \ test-eval-exports.sh \ + test-eval-extents.sh \ test-eval-cache.sh \ test-eval-dump-plugin.sh \ test-eval-disconnect.sh \ diff --git a/server/protocol.c b/server/protocol.c index d428bfc8..b4b1c162 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -499,7 +499,7 @@ extents_to_block_descriptors (struct nbdkit_extents *extents, (*nr_blocks)++;
pos += length; - if (pos > offset + count) /* this must be the last block */ + if (pos >= offset + count) /* this must be the last block */ break;
AIUI ... 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. pos += length; pos += 4G-1 In the previous code: if (pos > offset + count) /* this must be the last block */ break; branch not taken so we reach: assert (e.length <= length); e.length == 4G, length == 4G-1, so we assert here. 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;
/* If we reach here then we must have consumed this whole diff --git a/tests/test-eval-extents.sh b/tests/test-eval-extents.sh new file mode 100755 index 00000000..08c8d670 --- /dev/null +++ b/tests/test-eval-extents.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright Red Hat +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires_run +requires_plugin eval +requires_nbdsh_uri +requires nbdsh --base-allocation
I think it's marginally better to use: requires nbdsh --base-allocation --version which should be the same test.
+files="eval-extents.out" +rm -f $files +cleanup_fn rm -f $files + +# Trigger an off-by-one bug introduced in v1.11.10 and fixed in v1.43.6 +export script=' +def f(context, offset, extents, status): + print(extents) + +h.block_status(2**32-1, 0, f)
And here the requested offset == 0 & count == 4G-1, as deduced above.
+' +nbdkit eval \ + get_size='echo 5G' \ + pread='dd if=/dev/zero count=$3 iflag=count_bytes' \ + extents='echo 0 5G 0' \ + --run 'nbdsh --base-allocation --uri "$uri" -c "$script"' \ + > eval-extents.out +cat eval-extents.out +grep -E '\[4294967295, 0]' eval-extents.out --
It seems OK, ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Wed, Apr 23, 2025 at 01:10:58PM +0100, Richard W.M. Jones wrote:
On Tue, Apr 22, 2025 at 05:07:17PM -0500, Eric Blake via Libguestfs wrote:
Latent since the introduction of the .extents callback. The loop intentionally truncates to 2**32-1 bytes, but condition to end the loop early used > while the assertion after the loop used <=, meaning that the assertion can fire for any plugin that returns an extent of 2**32 or larger.
Fixes: 26455d45 ('server: protocol: Implement Block Status "base:allocation".', v1.11.10) Reported-by: Nikolay Ivanets <stenavin@gmail.com> Signed-off-by: Eric Blake <eblake@redhat.com> ---
pos += length; - if (pos > offset + count) /* this must be the last block */ + if (pos >= offset + count) /* this must be the last block */ break;
AIUI ...
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.
pos += length;
pos += 4G-1
In the previous code:
if (pos > offset + count) /* this must be the last block */ 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
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
participants (2)
-
Eric Blake -
Richard W.M. Jones