Device detection in export VMs is unreliable

[eloquence]

In freedomofpress/securedrop-workstation#307 (comment) and follow-up comments we discovered that the current method for detecting whether a storage device is detected is unreliable: sd-export-usb sometimes shows different bus IDs in lsusb output than sys-usb.

We've considered various methods to improve reliability, including the abandoned approach in #16 to use lsblk, which would only work for storage devices.

Our goal is to implement a method to detect connected USB devices reliably in a manner which will work for both print and storage devices, for example, by querying the connected device class.

[rmol]

My findings so far:

The bus and device IDs are useless, as reported in the other issue.

Fingerprinting USB devices would let us reliably identify devices that have serial numbers, but many flash drives don't, so they look the same. I've been unable to find any distinguishing attribute for some cheap SMI sticks I've got; they're all identical under /sys.

This is all OK, because the persistent attachment isn't what we want in the long run anyway. It complicates provisioning, keeps sd-export-usb from starting if the specified device is not attached, and wouldn't work well in a situation where the workstation is used by multiple journalists with their own devices.

The Qubes core admin API makes it possible to write an extension loaded by qubesd that can react to events like VM startup and shutdown, and changes to the list of devices in the sys-usb VM, so in theory we could watch for those events, run a script in sys-usb to enumerate viable export devices, and automatically attach them to sd-export-usb. But if a journalist is using Qubes as more than a SecureDrop appliance, and wants to plug in a stick or printer to use with a non-SD VM, that might not be appreciated. We might be able to run something to ask their preference now, but that might not be the case in Qubes 4.1.

So we're left with having the script run in sd-export-usb to open an export archive look for viable storage and print devices. If only one is found that matches the current export method (disk or print), then export can just proceed. Otherwise we can prompt the user to select one, or plug one in if none are found. They'd simply use the Qubes device manager for this. The only problem with that is if the devices GUI flakes out on them.

There is a way we could eliminate the need for manual attachment, at least for storage devices. If we control the disk format of export sticks, we could add a marker (like a partition with a certain label) that would let the enumeration script run in sys-usb by the extension reliably identify dedicated SecureDrop export devices, so they could be automatically attached to sd-export-usb when plugged in.

[eloquence]

Thanks for the recap, John! I've nominated this for discussion in our Monday client check-in (at 12:30 PM EDT):
https://github.com/freedomofpress/securedrop-client/wiki/Monday-Meetings

[redshiftzero]

Regarding printer detection, from discussion from @rmol privately that with the printers we have tested so far that do appear to be fingerprintable (they include a distinct serial ID number) - if that is broadly true (needs investigation since we've only tested with two) then we wouldn't need similar logic for printer attachment. We'd keep some data store of allowed printers in dom0 and then attach them as they are seen.

[sssoleileraaa]

Recap

To recap today's quick client sync, we discussed this issue and what it would take to auto-attach a new device to sd-export-usb without persistent attachment. Here are some highlights:

• Ditching persistent attachment means that sd-export-usb will be able to start without a device plugged in. This improves UX because the VM can auto-start when the client loads rather than making the user wait a long time while the VM boots up when they're ready to export.

• A user will then be able to manually attach usb devices to the sd-export-vm, but we can still run a script to auto-attach devices for them.

• To make sure that we are auto-attaching the correct device to sd-export-usb, we would write a script that'll listen and respond whenever there is a change to the list of devices in sys-usb and check it against a fingerprint database (if a character device) or check that it has a partition with a special SecureDrop-formatted partition name (if a block device).

• Remove ERROR_USB_CONFIGURATION code from Export and Client code since it would no longer be relevant because we no longer need to use dom0 config.json to specify device id.

• Rewrite USB_CONNECTED and USB_NOT_CONNECTED code to take into account that there is no longer a dedicated slot and multiple devices can be attached to sd-export-usb, and that it needs to work for both printers and thumb drives.

Open questions

• How should this fingerprint collection for verified printers be maintained?

  I think we may want to require that printer devices be plugged in when sd-export-usb is provisioned so that we can grab the serial number and store it for the sys-usb script to use. Or we could have the admin add the serial numbers manually to config.json. Or something else?

• How should we handle the case when there are multiple usb devices attached to sd-export-usb?

  Let's say there are two block devices attached to sd-export-usb, how do we know which one the user wants to transfer documents to?

  The usb-test could check for the SecureDrop-formatted partition name, but there could also potentially be multiple devices with the SecureDrop-formatted partition name. It seems like we would need a way to support the scenario where a user has to choose which one of the devices they intend to transfer documents to. One way to address this multiple-attached-devices case is to use a new status code, something like MULTIPLE_DEVICES_CONNECTED_ERROR and ask the user to detach all other non-export devices from the sd-export-usb VM. The user would have to use the Qubes USB attachment GUI (which is what we've been trying to avoid) in order to detach devices.

  Now what if the user ends up accidentally removing the wrong device from the GUI? Read on...

• How should we handle the case when the device attached to sd-export-usb is not a SecureDrop transfer device or verfied printer when the user tries to export? To break this down in case it helps: If the only device attached to sd-export-usb after the user clicks on "Print" is a character device that has a serial ID that was not in the config.json file, should we still print to it? If the only device attached to sd-export-usb after the user clicks on "Export" is a block device that does not use the special SecureDrop-formatted partition name, should we still export to it?

• How should we handle the case when the device attached to sd-export-usb is either a) a character device when the user is trying to export to disk, or b) a block device when the user is trying to print a file?

  usb-test is supposed to be generically checking that a USB device is attached to the dedicated slot, but now we don't have a dedicated slot. If we just check that any device is connected then we'll enounter unexpected errors like ERROR_PRINTER_NOT_FOUND when we try to export to a printer or USB_ENCRYPTION_NOT_SUPPORTED when we try to print to a thumb drive.

  It may make the most sense to rewrite usb-test to be printer-check and disk-check which would get called based on what the user clicks on: "Print" or "Export".

[redshiftzero]

Some thoughts on the above:

How should we handle the case when the device attached to sd-export-usb is not a SecureDrop transfer device or verfied printer when the user tries to export?

if the USB drive is not a valid transfer device we should return an error code - ERROR_USB_CONFIGURATION seems appropriate?

Regarding printers, unless there's some security advantage that I'm not seeing to disallowing printing to a printer the user manually attaches, we should not block that action.

How should we handle the case when the device attached to sd-export-usb is either a) a character device when the user is trying to export to disk, or b) a block device when the user is trying to print a file?

in these cases I think we should return USB_NOT_CONNECTED and the corresponding code for printing in the print scenario

[sssoleileraaa]

if the USB drive is not a valid transfer device we should return an error code - ERROR_USB_CONFIGURATION seems appropriate?

Currently ERROR_USB_CONFIGURATION is done in usb-test and used for both printers and drives so it might be better if we check that the attached block device is valid (does "valid" also mean it contains the SecureDrop-formatted partition name?) in disk-test, which currently checks if the disk is luks encrypted.

Regarding printers, unless there's some security advantage that I'm not seeing to disallowing printing to a printer the user manually attaches, we should not block that action.

Yeah, I aslo don't see a security advantage. I guess I was building off of the proposal of only auto-attaching verified printers (printers that have a serial number in our maintained printer list). If we allow any printer to be attached and printed to then it might make sense to just auto-attach any printer device to sd-export-usb, rather than just the ones in the list?

[redshiftzero]

If we allow any printer to be attached and printed to then it might make sense to just auto-attach any printer device to sd-export-usb, rather than just the ones in the list?

I think the three scenarios to consider are:

1. User wants to use Printer A for SD
2. User has another printer device B they use for ... let's say scanning in another VM. We don't want to hijack their printer and attach it to an SD export VM (that is pretty aggressive and not necessary if we can fingerprint printers).
3. User gets annoyed trying to print something, asks admin to assist, who uses qvm-usb attach, which while not the preferred method we shouldn't disable (if we disable I think it will annoy users).

[sssoleileraaa]

So let's assume we're able to get rid of persistent attachment, then there's no need for us to spend time on trying to make the current usb-test generic for both drives and printers. Here's what I propose:

delete usb-test

It should be deleted here and here

update printer

For printers, the client would not make a usb-test request, it would only make a printer request, which would:

• return PRINTER_NOT_FOUND if there are no printers attached (we're already doing this)
• return MULTIPLE_DEVICES_CONNECTED_ERROR if there is more than one printer device attached

To get this working, we would need to update this code so that we don't always assume that the first printer result from lpinfo -v is the one we want.

update disk-test

For drives, the client would not make a usb-test request, it would only make a disk-test request (as it's already doing in the preflight check), which would:

• return DRIVE_NOT_FOUND (instead of the generic USB_NOT_CONNECTED) if no block device is attached or none of the attached block devices have a partition label of securedrop-transfer-device (or whatever we decide to call it)
• return MULTIPLE_DEVICES_CONNECTED_ERROR if there is more than one attached device with the securedrop-transfer-device partition label
• return USB_NO_SUPPORTED_ENCRYPTION if the attached SecureDrop-Export device is not luks encrypted (we're already doing this by running sudo cryptsetup isLuks /dev/sda1)

To get this working, I suggest we reopen my bus-id-workaround PR with an update to append -o PARTLABEL securedrop-transfer-device to the end of this command. I already tested this on my dev machine and it works great after assinging a partition label of securedrop-transfer-device to my transfer device using the Disks application. We also need to account for the MULTIPLE_DEVICES_CONNECTED_ERROR case.

And that's it!

This would leave the provision part of this issue to be worked on in parallel to what I proposed above.

---

Update

One adjustment to the proposal above: remove checking for the partition label of securedrop-transfer-device since we should allow users to export to any luks-encrypted device (also you technically could add the securedrop-transfer-device label to any kind of partition, luks encrypted or not, so it really tells us nothing special (it's also a way for us to just narrow down the types of devices with auto-attach -- a user might attach a drive manually afterall)

[redshiftzero]

thanks @creviera your proposal makes a lot of sense

return DRIVE_NOT_FOUND (instead of the generic USB_NOT_CONNECTED) if no block device is attached

is this because there might be a USB connected, but it's not got the correct partition name?

return MULTIPLE_DEVICES_CONNECTED

What do you think having instead of one message we have two error messages i.e. one for MULTIPLE_PRINTERS_CONNECTED and MULTIPLE_DRIVES_CONNECTED or similar? I'm wondering this because if on the client-side we are going to emit an exception in a signal to update the GUI, when we do the print integration we could more easily distinguish a print and export failure if the errors are distinct between the two cases. This would be for a case where there is a concurrent export to USB and print actions occurring.

[sssoleileraaa]

is this because there might be a USB connected, but it's not got the correct partition name?

No, it's because I think we should just delete this whole method and simplify the code and number of back and forth calls between the client and export vm by consolidating usb-test and disk-test into one preflight request: disk-test, which means we don't need the status code USB_CONNECTED. Instead, the export script would just immediately continue to check validity of the device, i.e. that it's luks encrypted, rather than tell the client caller that it can now make a request to check the validity of the device. If the device is not found, then the script would not continue the preflight check, instead it would return DEVICE_NOT_FOUND, which I think is more descriptive than USB_NOT_CONNECTED and also more consistent with how we use PRINTER_NOT_FOUND for printers.

What do you think having instead of one message we have two error messages i.e. one for MULTIPLE_PRINTERS_CONNECTED and MULTIPLE_DRIVES_CONNECTED or similar?

Sounds good to me. It's better since it's not really an error rather an unsupported feature. We will eventually need to provide the user with a way to select a device from a menu, and it would make most sense to do this from the Export VM rather than the client. I'll open up another discussion ticket for this to elaborate and to ask questions. If we continue to try to steer the user away from using the OS's USB attachment menu, we'll have to rewrite a lot of what Qubes is already doing, which we could just be improving. Also we don't plan to block the user from using the Qubes menu afaik so it might be more confusing with multiple ways of doing things.

[eloquence]

We've agreed to use the lsblk approach after all for storage devices. The printer code already has its own equivalent to lsblk, i.e.lpinfo; @creviera may propose some follow-up code changes regarding the organization of the printer code as a separate issue. This specific issue will therefore be resolved by PR #16.

Other changes to the USB-related logic are tracked in the securedrop-workstation repo because they relate to code that is not in this repo (securedrop-export):

• Prototyping of auto-attachment of USB devices will be tracked in Investigate inconsistent ordering of PCI bus IDs for USB devices securedrop-workstation#307 for now
• Removing persistent attachments is tracked in Remove persistent attachment to sd-export-usb VM securedrop-workstation#320.