5:07 p.m.
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(a)gmail.com>
Signed-off-by: Eric Blake <eblake(a)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
7:10 a.m.
New subject: [nbdkit PATCH] server: Fix off-by-one for maximum block_status length [CVE-XXX]
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(a)gmail.com>
Signed-off-by: Eric Blake <eblake(a)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
8:01 a.m.
New subject: [nbdkit PATCH] server: Fix off-by-one for maximum block_status length [CVE-XXX]
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(a)gmail.com>
> Signed-off-by: Eric Blake <eblake(a)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
10:34 a.m.
New subject: [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
0
days inactive
1
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones