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 <ebl...@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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
