11:53 a.m.
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>
---
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