[Libguestfs] Re: [nbdkit PATCH v2 1/2] server: Fix off-by-one for maximum block_status length [CVE-XXX]

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 <stenavin(a)gmail.com&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; Acked-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- 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