Re: [Libguestfs] [libnbd PATCH v2 3/6] api: Sanitize sizes larger than INT64_MAX

Our stable API has always claimed that nbd_get_size() reports a non-negative value on success, and -1 on failure. While we know of no existing production server (such as nbdkit, qemu-nbd, nbd-server) that would advertise a size larger than off_t, the NBD spec has not yet committed to enforcing a hard maximum of an export size as a signed integer; at present, the protocol mandates merely: S: 64 bits, size of the export in bytes (unsigned) I've considered proposing a spec patch to upstream NBD to require a positive signed value, and can point to this patch to help justify such a patch - but that hasn't happened yet. Thus, it should be no surprise that our fuzzer was quickly able to emulate a theoretical server that claims a size larger than INT64_MAX - at which point, nbd_get_size() is returning a negative value that is not -1, and the API documentation was unclear whether the application should treat this as success or failure. However, as we did not crash, the fuzzer did not flag it as interesting in v1.16. I considered changing nbd_internal_set_size_and_flags() to move the state machine to DEAD for any server advertising something so large (that is, nbd_connect_*() would be unable to connect to such a server); but without the NBD spec mandating such a limit, that is an artificial constraint. Instead, this patch chooses to normalize all wire values that can't fit in the int64_t return type to an EOVERFLOW error, and clarifies the API documentation accordingly. Existing clients that depend on a known positive size and check for nbd_get_size()<0 are not impacted, while those that check for ==-1 will now reject such uncommon servers instead of potentially using a negative value in subsequent computations (the latter includes nbdinfo, which changes from reporting a negative size to instead printing an error message). Meanwhile, clients that never called nbd_get_size() (presumably because they infer the size from other means, or because they intend to access offsets above 2^63 despite not being able to reliably see that size from nbd_get_size) can still do so. Next, I audited all instances of 'exportsize', to see if libnbd itself has any arithmetic issues when a large size is stored. My results for v1.16 are as follows: lib/nbd-protocol.h and lib/internal.h both have uint64_t exportsize (so we are already treating size as unsigned both for wire and internal representation - no nasty surprises with sign extension). generator/states-{oldstyle,opt-{go,export-name}}.c grab exportsize off the wire, byteswap if needed, and pass it to nbd_internal_set_size_and_flags() without further inspection. lib/flags.c has nbd_internal_set_size_and_flags() which stores the wire value into the internal value, then nbd_unlocked_get_size() which reports the wire value as-is (prior to this patch adding EOVERFLOW normalization). nbd_internal_set_block_size() mentions exportsize in comments, but does not actually compare against it. lib/rw.c added LIBNBD_STRICT_BOUNDS checking in v1.6 via: if ((h->strict & LIBNBD_STRICT_BOUNDS) && (offset > h->exportsize || count > h->exportsize - offset)) { where offset and count are also unsigned values (count is 32-bit in 1.16, 64-bit in master); but the order of comparisons is robust against wraparound when using unsigned math. Earlier releases, or clients which use nbd_set_strict_mode(,0) to skip bounds checking, have been assuming the server will reject requests with a weird offset (most servers do, although the NBD spec only recommends that sever behavior, by stating that the burden is on the client to pass in-bounds requests in the first place). generator/states-reply-chunk.c added comparisons against exportsize for NBD_OPT_BLOCK_STATUS only after 1.16, and that's the subject of the next patch. No other direct uses of exportsize exist, so libnbd itself can internally handle sizes larger than 2^63 without misbehaving, outside of nbd_get_size(), even if such an offset was computed from taking the negative int64_t value of nbd_get_size() and turning it back into uint64_t offset parameter. I also audited our other APIs - everything else uses a parameter of type UInt64 in the generator for offsets, which is translated in C.ml to uint64_t; and we already know we are safe when C converts negative int64_t values into uint64_t. For other languages, Python.ml generates code for UInt64 by using 'PyArg_ParseValue("K")' with a detour through 'unsigned long long' in case 'uint64_t' is a different type rank despite being the same number of bits. The Python documentation states that "K" converts an arbitrary python integer value to a C uint64_t without overflow checking - so it is already possible to pass offset values larger than 2^63 in nbdsh; while values larger than 2^64 or negative values are effectively truncated as if with modulo math. Enhancing the language bindings to explicitly detect over/underflow is outside the scope of this patch (and could surprise users who were depending on the current truncation semantics). GoLang.ml generates UInt64 via native Go 'uint64' passed through 'C.uint64_t()', and Rust.ml generates UInt64 via native Rust 'u64' interpreted as C uint64_t. In both cases, while I am unsure whether those languages (which have tighter type rules than C) let you get away with directly assigning a negative value to the native type when you really want a positive value over 2^63; but since it is a direct map of an unsigned 64-bit value between the native type and C, there should be no surprises to people fluent in those languages. OCaml.ml is a bit different; as OCaml lacks a native unsigned 64-bit type, it generates UInt64 as native 'int64' converted to C via 'Int64_val()'. Thus, an OCaml client MUST pass a negative value if they want to access offsets beyond 2^63. But again, someone familiar with the language should be familiar with the limitations. Finally to demonstrate the difference in this patch, I temporarily applied this patch to qemu (here, on top of qemu commit 49076448): | diff --git i/nbd/server.c w/nbd/server.c | index b5f93a20c9c..228ce66ed2b 100644 | --- i/nbd/server.c | +++ w/nbd/server.c | @@ -691,7 +691,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) | myflags |= NBD_FLAG_SEND_DF; | } | trace_nbd_negotiate_new_style_size_flags(exp->size, myflags); | - stq_be_p(buf, exp->size); | + stq_be_p(buf, exp->size | 0x8000000000000000ULL); | stw_be_p(buf + 8, myflags); | rc = nbd_negotiate_send_info(client, NBD_INFO_EXPORT, | sizeof(buf), buf, errp); then with a just-built 'qemu-nbd -f raw -r file -t &' running, we have pre-patch: $ nbdsh --base -u nbd://localhost -c - <<\EOF > try: > print(h.get_size()) > except nbd.Error as ex: > print(ex.string) > EOF -9223372036854770176 vs. post-patch: $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF > try: > print(h.get_size()) > except nbd.Error as ex: > print(ex.string) > EOF nbd_get_size: server claims size 9223372036854781440 which does not fit in signed result: Value too large for defined data type A more complex patch to qemu to mask that bit back off from the offset parameter to NBD_CMD_READ/WRITE is also possible to explore behavior of passing large offsets over the wire, although I don't show it here. All stable releases of NBD have had this return value issue. We cannot guarantee whether clients may have their own arithmetic bugs (such as treating the size as signed, then entering an infinite loop when using a negative size as a loop bound), so we will be issuing a security notice in case client apps need to file their own CVEs. However, since all known production servers do not produces sizes that large, and our audit shows that all stable releases of libnbd gracefully handle large offsets even when a client convers a negative int64_t result of nbd_get_size() back into large uint64_t offset values in subsequent API calls, we did not deem it high enough risk to issue a CVE against libnbd proper at this time, although we have reached out to Red Hat's secalert team to see if revisiting that decision might be warranted. Based on recent IRC chatter, there is also a slight possibility that some future extension to the NBD protocol could specifically allow clients to opt in to an extension where the server reports an export size of 2^64-1 (all ones) for a unidirectional connection where offsets are ignored (either a read-only export of indefinite length, or an append-only export data sink - either way, basically turning NBD into a cross-system FIFO rather than a seekable device); if such an extension materializes, we'd probably add a named constant negative sentinel value distinct from -1 for actual return from nbd_get_size() at that time. Fixes: 40881fce75 ("lib: Expose flags and export size through the API.", v0.1) Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- generator/API.ml | 6 +++++- lib/flags.c | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/generator/API.ml b/generator/API.ml index 14988403..c4547615 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -2492,7 +2492,11 @@ "get_size", { permitted_states = [ Negotiating; Connected; Closed ]; shortdesc = "return the export size"; longdesc = "\ -Returns the size in bytes of the NBD export." +Returns the size in bytes of the NBD export. + +Note that this call fails with C<EOVERFLOW> for an unlikely +server that advertises a size which cannot fit in a 64-bit +signed integer." ^ non_blocking_test_call_description; see_also = [SectionLink "Size of the export"; Link "opt_info"]; example = Some "examples/get-size.c"; diff --git a/lib/flags.c b/lib/flags.c index 7e6ddedd..394abe87 100644 --- a/lib/flags.c +++ b/lib/flags.c @@ -253,6 +253,12 @@ nbd_unlocked_get_size (struct nbd_handle *h) return -1; } + if (h->exportsize > INT64_MAX) { + set_error (EOVERFLOW, "server claims size %" PRIu64 + " which does not fit in signed result", h->exportsize); + return -1; + } + return h->exportsize; }