Re: [Libguestfs] [libnbd PATCH v2 3/6] api: Sanitize sizes larger than INT64_MAX
On 9/21/23 22:58, Eric Blake wrote:
> 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
Re: [Libguestfs] [libnbd PATCH v2 3/6] api: Sanitize sizes larger than INT64_MAX
On Thu, Sep 21, 2023 at 03:58:02PM -0500, Eric Blake wrote:
> 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
[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
