The blocksize-policy filter had a minor denial-of-service security flaw, where a client could trigger the server to die from an assertion failure by sending an unaligned block status request in violation of NBD protocol recommendations (see the updated test within the patch for a sample trigger). Note that libnbd makes it difficult to trigger, as by default an unaligned request won't be sent to the server. Additionally, use of blocksize-error-policy=error is not impacted; and although the blocksize-policy filter defaults to an error policy of allow, it makes less sense to use the filter in production without opting in to blocksize-error-policy=error. Rather than complicating the blocksize-policy filter to manually munge its extents requests to an aligned boundary, I opted to instead relax the server's nbdkit_extents_aligned to support unaligned inputs by first widening the request to alignment boundaries and then truncating back to the original offset after at least one aligned extent is learned. The function still stops at the first unaligned extent, rather than trying harder to use all of the plugin's underlying information; I have plans to add a parameter in a later patch to optionally behave more like nbdkit_extents_full, but wanted this patch to focus on merely the assertion failure. An audit of all callers of nbdkit_extents_aligned shows that only blocksize-policy was vulnerable; the blocksize and swab filters only ever pass in aligned values. And while at it, I made the interface accept a 64-bit count, which makes usage easier when a client widens a request near the 4G boundary up to an alignment boundary. Since the flaw is minor, I've gone ahead and made this patch public. However, in parallel I am pursuing with Red Hat security on whether a CVE needs to be assigned. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- docs/nbdkit-filter.pod | 7 ++-- include/nbdkit-filter.h | 2 +- server/extents.c | 25 +++++++---- tests/test-blocksize-policy-extents.sh | 57 ++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 12 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 022799f4..1bb01675 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -1043,7 +1043,7 @@ A convenience function is provided to filters only which makes it easier to ensure that the client only encounters aligned extents. int nbdkit_extents_aligned (nbdkit_next *next, - uint32_t count, uint64_t offset, + uint64_t count, uint64_t offset, uint32_t flags, uint32_t align, struct nbdkit_extents *extents, int *err); @@ -1052,8 +1052,9 @@ obtained, where C<align> is a power of 2. Anywhere the underlying plugin returns differing extents within C<align> bytes, this function treats that portion of the disk as a single extent with zero and sparse status bits determined by the intersection of all underlying -extents. It is an error to call this function with C<count> or -C<offset> that is not already aligned. +extents. This function supports unaligned C<offset> or C<count>, but +the given C<extents> must begin at C<offset> and not have any extents +added yet. =head2 C<.cache> diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index ffa7fa5d..aa99af65 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -135,7 +135,7 @@ NBDKIT_EXTERN_DECL (struct nbdkit_extents *, nbdkit_extents_full, NBDKIT_ATTRIBUTE_NONNULL ((1, 5))); NBDKIT_EXTERN_DECL (int, nbdkit_extents_aligned, (nbdkit_next *next, - uint32_t count, uint64_t offset, + uint64_t count, uint64_t offset, uint32_t flags, uint32_t align, struct nbdkit_extents *extents, int *err) NBDKIT_ATTRIBUTE_NONNULL ((1, 6, 7))); diff --git a/server/extents.c b/server/extents.c index e0a2b224..8e507a94 100644 --- a/server/extents.c +++ b/server/extents.c @@ -213,7 +213,7 @@ nbdkit_add_extent (struct nbdkit_extents *exts, /* Compute aligned extents on behalf of a filter. */ NBDKIT_DLL_PUBLIC int nbdkit_extents_aligned (struct context *next_c, - uint32_t count, uint64_t offset, + uint64_t count, uint64_t offset, uint32_t flags, uint32_t align, struct nbdkit_extents *exts, int *err) { @@ -222,22 +222,25 @@ nbdkit_extents_aligned (struct context *next_c, struct nbdkit_extent *e, *e2; int64_t size; + assert (exts->extents.len == 0); + assert (exts->start == offset); + size = next->get_size (next_c); if (size == -1) { *err = EIO; return -1; } - assert (IS_ALIGNED (offset, align)); - assert (IS_ALIGNED (count, align) || offset + count == size); + exts->start = ROUND_DOWN (offset, align); + count = MIN (ROUND_UP (offset + count, align), size) - exts->start; /* Perform an initial query, then scan for the first unaligned extent. */ - if (next->extents (next_c, count, offset, flags, exts, err) == -1) + if (next->extents (next_c, count, exts->start, flags, exts, err) == -1) return -1; for (i = 0; i < exts->extents.len; ++i) { e = &exts->extents.ptr[i]; if (!IS_ALIGNED (e->length, align)) { /* If the unalignment is past align, just truncate and return early */ - if (e->offset + e->length > offset + align) { + if (e->offset + e->length > exts->start + align) { e->length = ROUND_DOWN (e->length, align); exts->extents.len = i + !!e->length; exts->next = e->offset + e->length; @@ -271,13 +274,13 @@ nbdkit_extents_aligned (struct context *next_c, CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL; extents2 = nbdkit_extents_new (e->offset + e->length, - offset + align); + exts->start + align); if (extents2 == NULL) { *err = errno; return -1; } if (next->extents (next_c, align - e->length, - offset + e->length, + exts->start + e->length, flags & ~NBDKIT_FLAG_REQ_ONE, extents2, err) == -1) return -1; @@ -298,7 +301,13 @@ nbdkit_extents_aligned (struct context *next_c, break; } } - /* Once we get here, all extents are aligned. */ + /* Once we get here, all extents are aligned. Trim back to the + * original offset if it was unaligned. + */ + e = &exts->extents.ptr[0]; + e->length -= offset - exts->start; + e->offset += offset - exts->start; + exts->start = offset; return 0; } diff --git a/tests/test-blocksize-policy-extents.sh b/tests/test-blocksize-policy-extents.sh index 46f804bb..688161ba 100755 --- a/tests/test-blocksize-policy-extents.sh +++ b/tests/test-blocksize-policy-extents.sh @@ -40,6 +40,8 @@ set -u requires_run requires_plugin data requires_nbdinfo +requires nbdsh --base-allocation --version +requires_nbdsh_uri files="blocksize-policy-extents.out" rm -f $files @@ -69,3 +71,58 @@ diff -u - blocksize-policy-extents.out <<EOF 0 4294967296 3 hole,zero 4294967296 512 0 data EOF + +# Check that unaligned requests are rejected when required +define script <<\EOF +def print_extents(context, offset, extents, err): + assert context == nbd.CONTEXT_BASE_ALLOCATION; + print(extents) + +h.set_strict_mode(0) +try: + h.block_status(511, 512, print_extents) +except nbd.Error: + print("detected misaligned count") +try: + h.block_status(512, 511, print_extents) +except nbd.Error: + print("detected misaligned offset") +h.block_status(513, 32256, print_extents) +h.block_status(1, 32768, print_extents) +EOF +export script +nbdkit data "@32k 1" --filter=blocksize-policy \ + blocksize-minimum=512 blocksize-error-policy=error \ + --run 'nbdsh --base-allocation -u "$uri" -c "$script"' \ + > blocksize-policy-extents.out +diff -u - blocksize-policy-extents.out <<EOF +detected misaligned count +detected misaligned offset +[512, 3] +[1, 0] +EOF + +# Check that unaligned requests still work when permitted (a user could trigger +# an assertion failure prior to 1.48, as a minor security flaw) +define script <<\EOF +def print_extents(context, offset, extents, err): + assert context == nbd.CONTEXT_BASE_ALLOCATION; + print(extents) + +h.set_strict_mode(0) +h.block_status(511, 512, print_extents) +h.block_status(512, 511, print_extents) +h.block_status(2, 32767, print_extents) +h.block_status(1, 32768, print_extents) +EOF +export script +nbdkit data "@32k 1" --filter=blocksize-policy \ + blocksize-minimum=512 blocksize-error-policy=allow \ + --run 'nbdsh --base-allocation -u "$uri" -c "$script"' \ + > blocksize-policy-extents.out +diff -u - blocksize-policy-extents.out <<EOF +[32256, 3] +[32257, 3] +[1, 3] +[1, 0] +EOF -- 2.52.0