On Wed, Apr 23, 2025 at 04:06:45PM -0500, Eric Blake wrote:
> There has been an off-by-one bug in the code for .extents since the
> introduction of that callback. Remember, internally the code allows
> plugins to report on extents with 64-bit lengths, but the protocol
> only supports 32-bit block status calls (nbdkit will need to create
> plugin version 3 before it can support NBD's newer 64-bit block
> status). As such, the server loop intentionally truncates a plugin's
> large extent to 2**32-1 bytes. But in the process of checking whether
> the loop should exit early, or if any additional extents should be
> reported to the client, the server used 'pos > offset+count' instead
> of >=, which is one byte too far. If the client has requested exactly
> 2**32-1 bytes, and the plugin's first extent has that same length, the
> code erroneously proceeds on to the plugin's second extent. Worse, if
> the plugin's first extent has 2**32 bytes or more, it was truncated to
> 2**31-1 bytes, but not completely handled, and the failure to exit the
> loop early means that the server then fails the assertion:
>
> nbdkit: ../../server/protocol.c:505: extents_to_block_descriptors:
> Assertion `e.length <= length' failed.
>
> The single-byte fix addresses both symptoms, while the added test
> demonstrates both when run on older nbdkit (the protocol violation
> when the plugin returns 2**32-1 bytes in the first extent, and the
> assertion failure when the plugin returns 2**32 or more bytes in the
> first extent).
>
> The problem can only be triggered by a client request for 2**32-1
> bytes; anything smaller is immune. The problam also does not occur
> for plugins that do not return extents information beyond the client's
> request, or if the first extent is smaller than the client's request.
>
> The ability to cause the server to die from an assertion failure can
> be used as a denial of service attack against other clients.
> Mitigations: if you require the use of TLS, then you can ensure that
> you only have trusted clients that won't trigger a block status call
> of length 2**32-1 bytes. Also, you can use "--filter=blocksize-policy
> blocksize-minimum=512" to reject block status attempts from clients
> that are not sector-aligned.
>
> Fixes: 26455d45 ('server: protocol: Implement Block Status
> "base:allocation".', v1.11.10)
> Reported-by: Nikolay Ivanets <stena...@gmail.com>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> Acked-by: Richard W.M. Jones <rjo...@redhat.com>
> ---
> tests/Makefile.am | 2 ++
> server/protocol.c | 2 +-
> tests/test-eval-extents.sh | 71 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 74 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..adc2b5b0
> --- /dev/null
> +++ b/tests/test-eval-extents.sh
> @@ -0,0 +1,71 @@
> +#!/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 --version
> +
> +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)
> +
> +# First, probe where the server should return 2 extents.
> +h.block_status(2**32-1, 2, f)
> +
> +# Next, probe where the server has exactly 2**32-1 bytes in its first extent.
> +h.block_status(2**32-1, 1, f)
> +
> +# Now, probe where the first extent has to be truncated.
> +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 4G 1; echo 4G 1G 2' \
> + --run 'nbdsh --base-allocation --uri "$uri" -c "$script"' \
> + > eval-extents.out
> +cat eval-extents.out
> +cat <<EOF | diff -u - eval-extents.out
> +[4294967294, 1, 1073741824, 2]
> +[4294967295, 1]
> +[4294967295, 1]
> +EOF
^^ I really need to use this 'diff -' trick in a few other places ...
Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
_______________________________________________
Libguestfs mailing list -- guestfs@lists.libguestfs.org
To unsubscribe send an email to guestfs-le...@lists.libguestfs.org
