Libguestfs - Jul 2023 - [libnbd PATCH] api: Fix block status assertion under set_strict bypass

A compliant server should not send NBD_REPLY_TYPE_BLOCK_STATUS unless
we successfully negotiated a meta context.  And our default strictness
settings refuse to let us send NBD_CMD_BLOCK_STATUS unless we
negotiated a meta context.  But when you mix non-default settings
(using nbd_set_strict to disable STRICT_COMMANDS) to send a block
status without having negotiated it, coupled with a non-compliant
server that responds with status anyways, we can then hit the
assertion failure where h->meta_valid is not set during the
REPLY.CHUNK_REPLY.RECV_BS_ENTRIES state.

Demonstration of the assertion failure can be done with a quick patch
to nbdkit (here, on top of v1.35.6):

| diff --git i/server/protocol.c w/server/protocol.c
| index cb530e65..6b115d99 100644
| --- i/server/protocol.c
| +++ w/server/protocol.c
| @@ -190,7 +190,7 @@ validate_request (uint16_t cmd, uint16_t flags, uint64_t
offset, uint32_t count,
|    }
|
|    /* Block status allowed? */
| -  if (cmd == NBD_CMD_BLOCK_STATUS) {
| +  if (cmd == NBD_CMD_BLOCK_STATUS && 0) {
|      if (!conn->structured_replies) {
|        nbdkit_error ("invalid request: "
|                      "%s: structured replies was not negotiated",
| @@ -536,7 +536,7 @@ send_structured_reply_block_status (uint64_t cookie,
|    size_t i;
|    int r;
|
| -  assert (conn->meta_context_base_allocation);
| +  //  assert (conn->meta_context_base_allocation);
|    assert (cmd == NBD_CMD_BLOCK_STATUS);
|
|    blocks = extents_to_block_descriptors (extents, flags, count, offset,

plus this sequence:

$ patched/nbdkit memory 1M
$ ./run nbdsh --opt-mode -u nbd://localhost
nbd> h.set_request_meta_context(False)
nbd> h.set_export_name('a')
nbd> def c(arg):
...   pass
...
nbd> h.opt_set_meta_context_queries(['base:allocation'], c)
1
nbd> h.set_export_name('')
nbd> h.opt_go()
nbd> h.set_strict_mode(0)
nbd> h.block_status(1024*1024, 0, c)
nbdsh: generator/states-reply-chunk.c:425:
enter_STATE_REPLY_CHUNK_REPLY_RECV_BS_ENTRIES: Assertion `h->meta_valid'
failed.
Aborted (core dumped)

As this is not a typical setup, and is trivially avoided by sticking
to default settings (or more safely, by using TLS connections to
trusted servers that don't reply to a spurious block status call),
this did not warrant a CVE.  However, since it is a case where a
server's reply can prompt a libnbd denial of service crash, I will be
sending a security announcement and upcoming patch be to the
libnbd-security man page once backports are in place.

Fixes: 55b09667 ("api: Fix nbd_can_meta_context if NBD_OPT_SET_META_CONTEXT
fails", v1.15.3)
Signed-off-by: Eric Blake <eblake at redhat.com>
---
 generator/states-reply-chunk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
index 02c65414..26b8a6b0 100644
--- a/generator/states-reply-chunk.c
+++ b/generator/states-reply-chunk.c
@@ -101,6 +101,7 @@  REPLY.CHUNK_REPLY.START:

   case NBD_REPLY_TYPE_BLOCK_STATUS:
     if (cmd->type != NBD_CMD_BLOCK_STATUS ||
+        !h->meta_valid || h->meta_contexts.len == 0 ||
         length < 12 || ((length-4) & 7) != 0)
       goto resync;
     assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent));
-- 
2.41.0

On Sat, Jul 15, 2023 at 08:49:51PM -0500, Eric Blake
wrote:
> A compliant server should not send NBD_REPLY_TYPE_BLOCK_STATUS unless
> we successfully negotiated a meta context.  And our default strictness
> settings refuse to let us send NBD_CMD_BLOCK_STATUS unless we
> negotiated a meta context.  But when you mix non-default settings
> (using nbd_set_strict to disable STRICT_COMMANDS) to send a block
> status without having negotiated it, coupled with a non-compliant
> server that responds with status anyways, we can then hit the
> assertion failure where h->meta_valid is not set during the
> REPLY.CHUNK_REPLY.RECV_BS_ENTRIES state.
> 
> Demonstration of the assertion failure can be done with a quick patch
> to nbdkit (here, on top of v1.35.6):
> 
> | diff --git i/server/protocol.c w/server/protocol.c
> | index cb530e65..6b115d99 100644
> | --- i/server/protocol.c
> | +++ w/server/protocol.c
> | @@ -190,7 +190,7 @@ validate_request (uint16_t cmd, uint16_t flags,
uint64_t offset, uint32_t count,
> |    }
> |
> |    /* Block status allowed? */
> | -  if (cmd == NBD_CMD_BLOCK_STATUS) {
> | +  if (cmd == NBD_CMD_BLOCK_STATUS && 0) {
> |      if (!conn->structured_replies) {
> |        nbdkit_error ("invalid request: "
> |                      "%s: structured replies was not
negotiated",
> | @@ -536,7 +536,7 @@ send_structured_reply_block_status (uint64_t cookie,
> |    size_t i;
> |    int r;
> |
> | -  assert (conn->meta_context_base_allocation);
> | +  //  assert (conn->meta_context_base_allocation);
> |    assert (cmd == NBD_CMD_BLOCK_STATUS);
> |
> |    blocks = extents_to_block_descriptors (extents, flags, count, offset,
> 
> plus this sequence:
> 
> $ patched/nbdkit memory 1M
> $ ./run nbdsh --opt-mode -u nbd://localhost
> nbd> h.set_request_meta_context(False)
> nbd> h.set_export_name('a')
> nbd> def c(arg):
> ...   pass
> ...
> nbd> h.opt_set_meta_context_queries(['base:allocation'], c)
> 1
> nbd> h.set_export_name('')
> nbd> h.opt_go()
> nbd> h.set_strict_mode(0)
> nbd> h.block_status(1024*1024, 0, c)
> nbdsh: generator/states-reply-chunk.c:425:
enter_STATE_REPLY_CHUNK_REPLY_RECV_BS_ENTRIES: Assertion `h->meta_valid'
failed.
> Aborted (core dumped)
> 
> As this is not a typical setup, and is trivially avoided by sticking
> to default settings (or more safely, by using TLS connections to
> trusted servers that don't reply to a spurious block status call),
> this did not warrant a CVE.  However, since it is a case where a
> server's reply can prompt a libnbd denial of service crash, I will be
> sending a security announcement and upcoming patch be to the
> libnbd-security man page once backports are in place.
> 
> Fixes: 55b09667 ("api: Fix nbd_can_meta_context if
NBD_OPT_SET_META_CONTEXT fails", v1.15.3)
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  generator/states-reply-chunk.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/generator/states-reply-chunk.c
b/generator/states-reply-chunk.c
> index 02c65414..26b8a6b0 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -101,6 +101,7 @@  REPLY.CHUNK_REPLY.START:
> 
>    case NBD_REPLY_TYPE_BLOCK_STATUS:
>      if (cmd->type != NBD_CMD_BLOCK_STATUS ||
> +        !h->meta_valid || h->meta_contexts.len == 0 ||
>          length < 12 || ((length-4) & 7) != 0)
>        goto resync;
>      assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent));
> -- 
> 2.41.0
Thanks.  I think this is worth a note in docs/libnbd-security.pod too.

The pipeline failed after you pushed this:

https://gitlab.com/nbdkit/libnbd/-/pipelines/932589424

but I think it's an unrelated OCaml failure.  I'll take a proper look
at it tomorrow.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top