[MIR] nbd-client

[Dave Jones]

[ Availability ]

The nbd source package is already in main, but only the nbd-server binary package (produced by nbd) is in main; the nbd-client binary package is in universe. We would like to move the client package into main too. The nbd-client package builds for all relevant architectures.

https://launchpad.net/ubuntu/+source/nbd

[ Rationale ]

We would like to include the nbd-client package in the Ubuntu Raspberry Pi images to support netbooting. Traditionally, netbooting on the Pi has been performed with TFTP+NFS. However, NFS has a number of drawbacks, one of the most significant being that snapd does not work when the root is on NFS. We would like to support a simple means of netbooting with a block device as root, and TFTP+NBD is the simplest way to achieve this. As noted above, the nbd source package is already in main; we simply need the nbd-client binary package (produced by the nbd package) moved into main too.

[ Security ]

Related searches:

https://ubuntu.com/security/cves?q=&package=nbd&priority=&version=&status=
https://security-tracker.debian.org/tracker/source-package/nbd

There are a couple of CVEs related to the nbd package in the last few years, specifically:

CVE-2022-26495 -- integer overflow in nbd-server
CVE-2022-26496 -- stack-based buffer overflow in nbd-server

Related links:

https://lists.debian.org/debian-security-announce/2022/msg00067.html
https://launchpad.net/ubuntu/+source/nbd/1:3.20-1ubuntu0.1
https://launchpad.net/ubuntu/+source/nbd/1:3.23-3ubuntu1

However, I could find none related to the nbd-client package specifically (those related to the nbd-server package have, naturally, been dealt with by the security team already).

* The package does include the /sbin/nbd-client executable, but this is not a suid/sgid binary, and is included in /sbin as it forms part of the boot process when the rootfs is an NBD share
* The package installs no services, timers, or recurring jobs
* The package does not open privileged ports
* The package exposes no external endpoints
* The package is capable of using TLS, but as a client

[ Quality Assurance ]

The package works well after install, and has been tested in several netboot scenarios on the Raspberry Pi (all successfully). Configuration is typically required server-side to ensure that a given Pi boots the right image (associating an OS image with a Pi via its serial number), but no client-side configuration is typically required.

The package has no "important" open bugs, and only one security bug which details that the aforementioned CVEs are currently unpatched in two ESM releases (trusty and xenial), which the security team are aware of and tracking accordingly.

The package does not currently have a DEP-8 test-suite, but does have a comprehensive test suite run as part of the build process (which probably could be run as DEP-8 tests too, though from a brief look the code may need adjusting to run the installed binaries instead of the built ones).

The package does not have a d/watch file, however it is currently maintained in Debian by the primary upstream author. The package defines the correct Maintainer, and does not produce an overly worrying set of lintian warnings (there is one lintian error to do with debconf, but it's a false positive as explained in nbd-client.templates; that said it should probably be overridden).

The package has no obsolete dependencies and, though it does use debconf, will not bother the default user (the debconf questions are only used by nbd-server, and we're intending to seed nbd-client where the debconf configuration is now vestigial / unused).

The source packaging under debian/ is reasonably simple and modern (uses a fairly recent standards-version, and debhelper-compat version 13), but for one glaring facet: it's in source format 1.0 (an orig tar-ball plus one huge compressed diff file). Changing this would almost certainly require buy-in from the upstream author (and Debian maintainer), and evidently this wasn't sufficient impediment to prevent nbd-server from moving to main already.

[ UI Standards ]

Application is indirectly end-user facing (potentially provides their rootfs although there's little that could really be called a "user interface" as such). Translations are present via standard mechanisms (see debian/po).

[ Dependencies ]

All dependencies are in main (unsurprisingly, given status of the source package).

[ Standards Compliance ]

The package follows Debian Policy, although arguably its use of version 1.0 packaging is not "best practice". However, its standard compliance is fairly recent (4.1.3), its debhelper compatibility is fully up to date, and in most other respects the packaging is nice and simple.

[ Maintenance / Owner ]

The package is already maintained in main by the server team.

[ Background Information ]

Upstream name is simply "nbd". The project was formerly maintained on SourceForge, but has recently moved to GitHub for future releases. The primary author is also the Debian maintainer. Relevant links:

* https://nbd.sourceforge.io/ (former home)
* https://github.com/NetworkBlockDevice/ (new home)
* https://salsa.debian.org/wouter/nbd (salsa repo)

[Wouter Verhelst (wouter-debian)]

👋 Wouter here.

Some comments:

> However, I could find none related to the nbd-client package specifically (those related to the nbd-server package have, naturally, been dealt with by the security team already).

This is mostly because nbd-client itself is fairly simple. It sets up the connection to the server, performs the handshake, and then hands over to the kernel. The rest of the NBD communication happens in kernel space. This means that the attack surface for nbd-client specifically is obviously very small, because most of the work does not happen there.

> The package does not currently have a DEP-8 test-suite, but does have a comprehensive test suite run as part of the build process (which probably could be run as DEP-8 tests too, though from a brief look the code may need adjusting to run the installed binaries instead of the built ones).

A proper DEP-8 test would at least set up nbd-server, connect an NBD device, write to it and read from it again. Additionally, the initramfs his that are shipped would preferably be tested too, which requires a netboot setup.

This is not trivial to do in a DEP-8 context, and while I have a plan that could work, I have not yet had the time to implement this. If anyone is willing to help with this as a prerequisite to moving this to Ubuntu main, I'm happy to have a chat or something to discuss the details. This is absolutely something I really really really want to do, but the complexity of the problem is significant.

The test suite mainly tests the server, as that is where most things could go wrong. With one exception, none of the tests even use nbd-client, but instead use a specifically written test client which makes some assertions about server behavior. The one exception simply runs 'nbd-client -l', which requests the list of server exports before immediately exiting, making the test extremely superficial.

None the less, running the compile time test suite as a DEP-8 test is achievable and a reasonable way to test the server, which I will look into as a first step.

> The package follows Debian Policy, although arguably its use of version 1.0 packaging is not "best practice".

Migrating to the 3.0 formats has not been high on my priority list, mostly because the first few times I tried converting a package it insisted that I use individual patches, which I find to be too involved (if you want individual patches, use git, that's what it's for). I have since learned about adding single-debian-patch to debian/source/local-options, which solves the issue for me and which I've been slowly converting my packages to. I just haven't gotten around to the NBD package yet in this regard. If it helps, I'm happy to prioritize that.

[Dave Jones (waveform)]

Hi Wouter -- thanks for the notes! I don't think anything listed above is a huge impediment to getting nbd-client into main (particularly given the source package itself is already there), but it's all stuff I need to document for the process.

> The test suite mainly tests the server, as that is where most things could go wrong. With one exception, none of the tests even use nbd-client, but instead use a specifically written test client which makes some assertions about server behavior. The one exception simply runs 'nbd-client -l', which requests the list of server exports before immediately exiting, making the test extremely superficial.

I had a brief look at converting the existing DEP-8 suite for autopkgtest use. As you note, for the server side this isn't too hard (I think it's mostly a case of having it use the installed binaries rather than the built ones). I hadn't noticed that the client tests are basically superficial, but as you note the attack surface is small so I don't think it'll be an issue.

> Migrating to the 3.0 formats has not been high on my priority list, mostly because the first few times I tried converting a package it insisted that I use individual patches, which I find to be too involved (if you want individual patches, use git, that's what it's for).

Heh, getting used to the quilt format can take a bit of getting used to. gbp's patch-queue (pq) command makes it less painful, particularly if you're used to working in git (it'll convert between the patch-set of the package and a sequence of git commits and back again). The main concern here is that, while 1.0 format is fine for working in Debian, it makes it harder for us downstream to, for example, revert/edit an individual patch if necessary, whereas this is rather simpler in the quilt format.

Again, I doubt it's an impediment since it didn't prevent the source package from entering main already, and the security team have obviously worked with it backporting patches before; it's just one of those things I needed to document as a potential concern for the MIR reviewer.

> I have since learned about adding single-debian-patch to debian/source/local-options, which solves the issue for me and which I've been slowly converting my packages to. I just haven't gotten around to the NBD package yet in this regard. If it helps, I'm happy to prioritize that.

Interesting -- I hadn't come across that before. Anyway, the last thing I want to do is put work on your plate -- if the MIR review comes back with anything, I'm more than happy to have a crack at it and send patches upstream (though obviously I wouldn't try and foist a packaging format on you -- that's very much your choice!). I'll try and have a look at adding the server-side tests to autopkgtest though, as that seems not too complex and certainly worthwhile.

[Didier Roche-Tolomelli (didrocks)]

It's really nice to have upstream and the debian maintainer looking at this issue too, thanks for joining the conversation and your recent release! :)

Review for Source Package: nbd (already in main) with focused on promoting nbd-client

[Summary]
MIR team ACK under the constraint to resolve the list below.
This does not need a security review as -server package was already promoted previously and the
client doesn’t have a large attack surface.
List of specific binary packages to be promoted to main in addition to exising one: nbd-client

Notes:
I mainly focused my review on the -client binary package. However, we always use this opportunity to rereview the other parts of the code and packaging. Packages are modified, upstream source code too, so the blanket "this is already in main" doesn't prevent us to do another round of review (ideally, that would be done also outside of the MIR cycle), and I used that opportunity for this. Also, the MIR requirements evolves (hopefully in a good direction) with time, and this is the perfect opportunity to get the required TODO being fixed! For instance, I see the recent upload migrated to source 3.0 (quilt) format and this is excellent!

I really appreciated the detailed bug description. However, some elements were missing on the MIR template rules and checks, like the output of lintian pedantic running as per https://github.com/canonical/ubuntu-mir. Also, some information are not exact, you state: "* The package installs no services, timers, or recurring jobs". However, nbd-client installs /lib/systemd/system/nbd@.service (not nbd-client which is symlinked to /dev/null). I have then had to recheck and find the informations myself. Please try to follow the template thoughtfully next time so that the burden on the MIR team is lighter, and we can process MIR bugs quicker. :)

Required TODOs:
- I see that we have a really trivial autopkgtests as of now (I read the discussion above about it too). I think this one covers the real basics of the packages but it’s not comprehensive enough. Since the package was introduced in main, the MIR rules have changed and now required better testing story than when it was promoted. This include autopkgtests too. If non trivial autopkgtests is not feasable, we have then to fallback to a manual test plan, that is documented, updated, and run with any upload of the package. This will though not prevent from reverse dependency regression, meaning that the test plan has to be run manually at regular intervals too. As you can see, autopkgtest is a way more future-proof looking in term of maintainance :)
- We do run nbd-client as a root process for each nbd device block found via a systemd service. However, I see this one has no systemd confinement set. It would be great to have as much restriction on the systemd unit itself to limit the attack surface.

[Rationale, Duplication and Ownership]
The rationale given in the report seems valid and useful for Ubuntu

[Dependencies]
OK:
- no other Dependencies to MIR due to this
   - nbd checked with `check-mir`
- all dependencies can be found in `seeded-in-ubuntu` (already in main)
- none of the (potentially auto-generated) dependencie...

[Dave Jones (waveform)]

> I really appreciated the detailed bug description. However, some
> elements were missing on the MIR template rules and checks, like the
> output of lintian pedantic running as per
> https://github.com/canonical/ubuntu-mir. Also, some information are
> not exact, you state: "* The package installs no services, timers,
> or recurring jobs". However, nbd-client installs
> /lib/systemd/system/nbd@.service (not nbd-client which is symlinked
> to /dev/null). I have then had to recheck and find the informations
> myself. Please try to follow the template thoughtfully next time so
> that the burden on the MIR team is lighter, and we can process MIR
> bugs quicker. :)

Sorry about missing that -- the vast majority of my testing had been
in the netboot scenario and in those cases that service isn't used
(because no instances are enabled; it's all handled by the initrd).
Hence I'd seen the systemd server units derived from init.d (more on
that below) but completely missed the instanced client unit.

> I see that we have a really trivial autopkgtests as of now (I read
> the discussion above about it too). I think this one covers the real
> basics of the packages but it’s not comprehensive enough. Since the
> package was introduced in main, the MIR rules have changed and now
> required better testing story than when it was promoted. This
> include autopkgtests too. If non trivial autopkgtests is not
> feasable, we have then to fallback to a manual test plan, that is
> documented, updated, and run with any upload of the package. This
> will though not prevent from reverse dependency regression, meaning
> that the test plan has to be run manually at regular intervals too.
> As you can see, autopkgtest is a way more future-proof looking in
> term of maintainance :)

Agreed. I've had a crack at enhancing the autopkgtests, and I'm
reasonably pleased with the results. Quoting Wouter:

> A proper DEP-8 test would at least set up nbd-server, connect an NBD
> device, write to it and read from it again.

In a branch up on salsa [1] I've now enhanced the existing regression
test (nbdtab-ipv4-address) to do just this; it's now a "read-write"
test that checks operations on a file-system match between local
access and nbd access (I'd have liked to add this as a separate
autopkgtest but it turned out that isolation between tests is not as
simple as I'd thought; LP: #2058040).

> Additionally, the initramfs his that are shipped would preferably be
> tested too, which requires a netboot setup.
>
> This is not trivial to do in a DEP-8 context, and while I have a
> plan that could work, I have not yet had the time to implement this.
> If anyone is willing to help with this as a prerequisite to moving
> this to Ubuntu main, I'm happy to have a chat or something to
> discuss the details. This is absolutely something I really really
> really want to do, but the complexity of the problem is significant.

While this wasn't trivial, this wasn't as hard as I was expecting
either, but this is thanks in large part to my being familiar with the
sterling efforts of others in sbuild's autopkgtests. These use the new
debvm tools to construct and run VMs. What's most impressive is that...

[Mark Esler (eslerm)]

Was -server code ever reviewed by a MIR?

The client contains many ioctl calls.

[Wouter Verhelst (wouter-debian)]

Not to the best of my knowledge, no. It was moved to main very early on; I think it might've been 15 years ago. Perhaps the MIR prices didn't exist yet back then? I wouldn't know, not having heard of the whole thing until I saw this one pass by 🙂

The client's job is to configure an NBD device in the kernel. It does that through the APIs that the kernel provides. There are two of these in existence; one is an older ioctl-based API, the other is a more recent netlink-based one. As these are the only days you can configure NBD device nodes, it makes sense that the are a lot of ioctl calls...

[Mark Esler (eslerm)]

Thanks Wouter

It appears nbd-client existed in main at some point http://old-releases.ubuntu.com/ubuntu/pool/main/n/nbd/ (thanks Seth).

Between this MIR and tree's LP#2056099 I am concerned that Security is being bypassed as NN approaches. That's not to say anything is wrong with how nbd-client uses ioctl, but we haven't looked. Security is not asking to review this for NN, just flagging for MIR Team discussion.

[Didier Roche-Tolomelli (didrocks)]

With this in light (but we have the wider "everything that is in main for a very long time in ubuntu, even being security reviewed and got multiple uploads), I would agree that -server could have another security/fresh look. Do you think it’s something the security team has the capacity to look?

Otherwise, we may not want to special case this case, as the problem is really linked to the pre-existing packages in main (even GNOME for instance in general, didn’t get a security review… and even if it did, GNOME has nothing looking like the one released in 2004).

[Lukas Märdian (slyon)]

cpaelzer> how about this - sarnold spends 20 minutes, and gives a shallow security review based on this being in main in the past kind of already
cpaelzer> if that outcome is good, get it into 24.04
mclemenceau_> thanks cpaelzer sarnold, I'm ok with the plan w.r.t netboot with a preference for addition in 24.04 :)

[Seth Arnold (seth-arnold)]

I gave the nbd-client.c file a very quick read and it looked moderately well-written to me. It feels like it's got nearly three decades of history to it -- solid, been around a while, and maybe you'd do things different if you were doing it again, but it exists today and solves problems, today.

So, ACK. :)

[Christian Ehrhardt  (paelzer)]

Thank you all,
with that I sum up the details here.

- MIR ack with a few requirements
- Security Ack
- From here
  1. add the tests you mentioned to the package
  2. continue to !try! isolation (2058040)
  3. ready for promotion in 24.10
  4. SRU the tests to 24.04
  5. we consider this even ok to promote for 24.04.1

Subscribing Dave to continue on that