[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@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@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
On Sun, Jul 16, 2023 at 04:10:56PM +0100, Richard W.M. Jones wrote:
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.
The error is: FAIL: test_140_explicit_close.bc ================================ libnbd: debug: nbd1: nbd_create: opening handle libnbd: debug: nbd1: nbd_set_debug: enter: debug=true libnbd: debug: nbd1: nbd_set_debug: leave: ret=0 libnbd: debug: nbd1: nbd_set_debug_callback: enter: debug=<fun> Fatal error: Fatal error during lock: Resource deadlock avoided FAIL test_140_explicit_close.bc (exit status: 134) FAIL: test_140_explicit_close.opt ================================= libnbd: debug: nbd1: nbd_create: opening handle libnbd: debug: nbd1: nbd_set_debug: enter: debug=true libnbd: debug: nbd1: nbd_set_debug: leave: ret=0 libnbd: debug: nbd1: nbd_set_debug_callback: enter: debug=<fun> Fatal error: Fatal error during lock: Resource deadlock avoided FAIL test_140_explicit_close.opt (exit status: 134) This is with OCaml 5.0 which was just added to Fedora Rawhide. This version of OCaml is much more stringent about checking that locks are used correctly. The thing is that we fixed this already ... https://gitlab.com/nbdkit/libnbd/-/commit/2c909bc3b8594943c4306ec78e8c13327c... I am not able to reproduce this locally (with the same version of OCaml), and the artifacts that we collect from the CI system do not include coredumps or stack traces. If anyone is able to reproduce this problem with the latest libnbd let me know, especially if you can collect a stack trace. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Sun, Jul 16, 2023 at 04:10:56PM +0100, Richard W.M. Jones wrote:
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.
Thanks. I think this is worth a note in docs/libnbd-security.pod too.
Done in 61f8cf661f
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.
My first reaction on seeing the failure message was to click the retry button in the UI; and that gave me a green checkmark before I had time to investigate what failed. We may be up against a pre-existing transient failure; I'll try and keep an eye out for a recurrence. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones