similar to: LIBNBD SECURITY: Remote code execution vulnerability

We discovered a possible Downgrade Attack in libnbd. Lifecycle --------- Reported: 2019-09-14 Fixed: 2019-09-16 Published: 2019-09-16 There is no CVE number assigned for this issue yet, but the bug is being categorized and processed by Red Hat's security team which may result in a CVE being published later. Description ----------- Libnbd includes the method nbd_set_tls(h,

This is the same, but now we have a symbol for it. --- docs/libnbd.pod | 6 ++++-- interop/interop.c | 2 +- lib/connect.c | 2 +- tests/aio-parallel-load.c | 2 +- tests/aio-parallel.c | 2 +- tests/connect-tls.c | 2 +- tests/synch-parallel.c | 2 +- 7 files changed, 10 insertions(+), 8 deletions(-) diff --git a/docs/libnbd.pod b/docs/libnbd.pod

This neutral refactoring adds -DTLS_MODE. We can in future change the requested TLS mode, but not in this commit. It also checks that nbd_get_tls_negotiated returns true after connecting, when the requested mode was set to LIBNBD_TLS_REQUIRE. --- interop/Makefile.am | 4 ++++ interop/interop.c | 26 ++++++++++++++++++++------ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git

When we moved to an enum instead of raw int for nbd_set_tls(), we should have also updated our code to prefer the enum values. While at it, improve the grammar of error messages (confusing since 632196ec, and copy-and-pasted into more locations since then). Fixes: 4488cf2a Thanks: Rich Jones --- Rich noticed this while reviewing the patch for today's CVE fix. It's not a show-stopper if

When LIBNBD_TLS_ALLOW is used we don't have a way to find out if TLS was really negotiated. This adds a flag and a way to read it back. Unfortunately there is no test yet, because LIBNBD_TLS_ALLOW is not tested -- it really should be but requires quite a complicated set of tests because ideally we'd like to find out whether it falls back correctly for all supported servers. --- TODO

In the generated wrapper code this adds checks for all Enum parameters. Since only nbd_set_tls uses an Enum parameter, the only extra code generated by this change is: int nbd_set_tls (struct nbd_handle *h, int tls) { // ... switch (tls) { case LIBNBD_TLS_DISABLE: case LIBNBD_TLS_ALLOW: case LIBNBD_TLS_REQUIRE: break; default: set_error (EINVAL, "%s:

On Wed, Sep 11, 2019 at 08:48:28AM -0500, Eric Blake wrote: > On 9/11/19 5:40 AM, Richard W.M. Jones wrote: > > Very much a work in progress as there are still many tests using > > qemu-io which are candidates for conversion. > > > > You'll notice at the end of test-full.sh that the new test has some > > duplicated code which looks as if it ought to be

The 0.9.8 release breaks API, requiring a number of changes: - Use symbolic constants instead of magic numbers/open-coded strings (well, the string for "base:allocation" was present before this libnbd bump) - Change callbacks to drop the valid_flag parameter - Add _is to nbd_read_only call - Drop the _callback suffix on nbd_aio_FOO calls - Add a struct for managing callback/user_data

Automake insists that any file present in a Makefile.am 'include' must be present, but it has no idea about the GNU make extension 'sinclude'. Use this to our advantage: as long as Makefile.inc has nothing other than variable modifications (done in the previous patch), and our rules properly generate it when needed (thanks to generator_built), then running 'make' will

This is slightly redundant with just trying nbd_set_tls(nbd, 2) then checking for failure; however, this function does not set errors and looks more similar to nbd_supports_uri. --- This is borderline enough that I figured I'd post it to check if we want it. generator/generator | 45 ++++++++++++++++++++++++++++++++++++++------- interop/interop.c | 4 ++++ lib/handle.c | 12

Previously nbd_set_tls had an integer argument which was 0 for disable, 1 for allow and 2 for require. This commit adds a proper enumerated type to describe this, defining LIBNBD_TLS_DISABLE = 0, LIBNBD_TLS_ALLOW = 1 and LIBNBD_TLS_REQUIRE = 2. (Note the C API doesn't change). In C the enumerated type is still defined and passed as an int (not as an enum). While we could define an enum

‘make clean && make’ was not rebuilding the docs/*.3 files. The reason is obscure: - docs/Makefile has rules: MANS = $(man_MANS) all: all-am all-am: Makefile $(MANS) - sinclude docs/Makefile.inc happened long after MANS is defined, so MANS held the earlier version of $(man_MANS) without the api-built man pages listed. This was confirmed by looking at the output of

The next strict knob: allow the user to pass unknown flags across the wire (this is different than passing a known flag at the wrong time). It is interesting to note that NBD only permits 16 bits of flags, but we have a signature that takes uint32_t; if we wanted, we could pack libnbd-specific flags in the upper bits that the NBD protocol would never see. --- generator/API.ml | 46

Test both the TLS enabled and fallback paths. nbd-server doesn't appear to support TLS at all, and qemu-nbd is known not to allow fallback to unencrypted, and therefore it only makes sense to test nbdkit at the moment. --- .gitignore | 4 ++++ TODO | 3 --- interop/Makefile.am | 54 +++++++++++++++++++++++++++++++++++++++++++++ interop/interop.c | 30

Document and make it easier for applications to determine which bitmask values were supported at compile time. Also, generating bitmask values in hex tends to be more legible than in decimal, as it makes it obvious that the values are intended for bitwise operations. And it doesn't hurt that this reduces churn in some of the tests if new bits are added. --- docs/libnbd.pod

On Fri, Sep 11, 2020 at 04:49:55PM -0500, Eric Blake wrote: > The next strict knob: allow the user to pass unknown flags across the > wire (this is different than passing a known flag at the wrong time). > > It is interesting to note that NBD only permits 16 bits of flags, but > we have a signature that takes uint32_t; if we wanted, we could pack > libnbd-specific flags in the

Copies heavily after a similar addition recently made in nbdkit. --- docs/Makefile.am | 7 +++++++ docs/libnbd-security.pod | 32 ++++++++++++++++++++++++++++++++ docs/libnbd.pod | 1 + Makefile.am | 1 + .gitignore | 3 ++- SECURITY | 14 ++++++++++++++ 6 files changed, 57 insertions(+), 1 deletion(-) create mode 100644

Let's check for a quite a few more errors. Among other things, this adds some coverage for a few things I've patched recently. --- And these enhancements set me up for my next fix: making NBD_CMD_DISC prevent future commands. tests/errors.c | 167 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 154 insertions(+), 13 deletions(-) diff --git a/tests/errors.c

I'm easily able to provoke NBD_REP_ERR_TLS_REQD (use nbd_set_tls(0) to talk to a server that requires encryption) and NBD_REP_ERR_UNKNOWN (forget to use nbd_set_export_name for qemu-nbd); it's nice to display a useful error for these rather than "unknown reply from NBD_OPT_GO: 0x80000005" or similar. Other errors are less common, but as long as we're decoding things, it

Right now, libnbd has refused to issue a command not advertised as supported by a server, including unknown flags, mainly because the NBD protocol does not guarantee what the server will do, and libnbd would rather stay in sync with the server than drop the connection. However, for integration purposes, it can be handy to coerce libnbd into sending something to see how the server will react