Support automatic attachment of export devices

[rmol]

Adds udev rules and scripts to sys-usb. Modifies the qubes.USB RPC policy and adds qubes.USBAttach.

Testing

• Check out this branch in your development Qube.
• In your cloned repo in dom0:
  • make clone
  • make clean && make all
  • Check that /etc/qubes-rpc/policy/qubes.USB contains:

    ### BEGIN securedrop-workstation ###
    sd-export-usb sys-usb allow
    @anyvm @tag:sd-workstation deny
    ### END securedrop-workstation ###
    @anyvm @anyvm ask
    

  • Check that /etc/qubes-rpc/policy/qubes.USBAttach contains:

    ### BEGIN securedrop-workstation ###
    sys-usb sd-export-usb allow,user=root
    @anyvm @tag:sd-workstation deny
    ### END securedrop-workstation ###
    @anyvm @anyvm ask
    

• In sys-usb, confirm the existence of:
  • /etc/udev/rules.d/99-sd-export-usb.rules
  • /usr/local/bin/sd-attach-export-disk
  • /usr/local/bin/sd-attach-export-printer

Plug in a valid LUKS export stick. It should be automatically attached to sd-export-usb, and an export from the client or manually via qvm-open-in-vm sd-export-usb "filename" should succeed.

[sssoleileraaa]

Plug in a valid LUKS export stick. It should be automatically attached to sd-export-usb, and an export from the client or manually via qvm-open-in-vm sd-export-usb "filename" should succeed.

it also looks like this attaches drives that are not encrypted right? just want to double check

[rmol]

That's right; it attaches any storage or printer device. The script invoked by the udev rule could check the partition table and label, but they're supposed to be quick. Also, I'm not sure how we'd surface any errors in a nice way. I think it's better to do any validation in sd-export-usb.

[emkll]

Thanks @rmol , I've experienced some issues in testing these changes: the files were actually copied in dom0 instead of sys-usb. I have a theory for a fix, see comment inline.

Perhaps adding tests similar to https://github.com/freedomofpress/securedrop-workstation/blob/307-udev-export-devices/tests/test_sd_export.py#L12 would help ensure that the files are present on the correct VMs and prevent regressions during development.

Another improvement we should consider before merging is having the ability to remove these udev rules, in the case where the workstation for development, by perhaps creating a make remove-sys-usb or equivalent target. What do you think?

[rmol]

Corrected the target in the sys-usb Salt config. Added tests to catch my slop; sorry for wasting your time.

Parroted the Qubes YubiKey configuration you found to get the udev rules to survive reboots.

I'm not sure yet how best to undo this config. In addition to just removing it from sys-usb, it would need to be cleaned out of /srv/salt in dom0 -- I ran into this after switching to another issue; I had to manually excise these changes. We almost need Salt migrations. 🙂 I don't like the idea of littering the Salt config with one-time removal of obsolete files there, but don't know of a better way yet.

[emkll]

I'm not sure yet how best to undo this config. In addition to just removing it from sys-usb, it would need to be cleaned out of /srv/salt in dom0

good point, hadn't thought of that. We may need to handle this through conditional logic, maybe in an env var or config.json?.

[conorsch]

We almost need Salt migrations. slightly_smiling_face I don't like the idea of littering the Salt config with one-time removal of obsolete files there, but don't know of a better way yet.

Also relevant:

• Remove sd-export-usb persistent USB device attachment #323 (review)
• Upgrades all sys-* VMs to Fedora 30 #331 (review)

So it sounds like we do need a more robust "clean" action. For instance, we leave the sys-firewall customizations (for the dom0 RPM repo) in place after make clean is run. Will open another issue.

[kushaldas]

In my mind this is still a lot of code + configuration to maintain for auto attach. Doing it via qubesadmin API (Python code) seems to be much more manageable for future.

Here we are adding a complete new qrexec service and we will have to manage the config etc.

Also, we have things depending on sys-usb, and automatic calling qrexec service based on udev rules, any bad usb device may do sudo calls in the USB vm to change config.

[conorsch]

Will open another issue.

See #337.

[emkll]

Thanks @rmol for the changes, this works as expected. Tested as follows using the following steps, based on your test plan:

• (make clean) make all and make test, all tests pass
• sys-usb reboots are handled correctly: the attachment still works
• When a device is connected, sys-usb starts sd-export-usb
• Drives are not automounted
• Non printer/storage devices aren't connected: a mouse or keyboard, for example are not autoattached
• export to usb works as expected

There is one edge case (that will likely not happen is real-life scenarios, since sys-usb is set to autostart): If sys-usb is rebooted with a USB device connected to it, the rule will not file as sys-usb is restarted, the device needs to be attached and detached. In those cases, The client requests the user to attach a device, Since the preflight checks return USB_NOT_CONNECTED, I don't think it's worth addressing as part of this PR.

@kushaldas I brought up the issue you've raised in #159 (comment) , we can continue the discussion there and perhaps track any tests/code/results in that ticket from now on. Your findings and proof of concept will be especially helpful for the more general admin actions

[emkll]

There's also an issue with the Qubes USB Widget, that was raised in the November 14th engineering meeting (thanks @redshiftzero for flagging):

• When a device is auto-attached, it is not registered as attached by the USB GUI widget (bolded and shown as attached to sd-export-usb)
• Using this Qubes USB widget to attach a device to a given VM will result in an error:

ERROR
Attaching device <device> to <vm> Error:DeviceAlreadyAttached: "Device sys-usb:2-1 already attached to sd-export-usb"

Manually running qvm-usb detach sys-usb:2-1 resolves the issue (and a user may now attach that device to whichever Qube they choose), but requires a manual step.

Dismissing for now to investigate/discuss #324 (comment)

[rmol]

@emkll Thanks. I'll need to investigate the boot-time udev handling. The problem with the devices widget seems to be that it isn't receiving or responding to the device-list-changed:usb events fired by qubesusbproxy. I've confirmed that the changes to the database under /qubes-usb-devices are being noticed when the device is attached, so I think the problem lies only in the widget.

[emkll]

Another side effect of the changes introduced here that I forgot to note, is the increased likelihood of having multiple relevant (mass storage or printer) devices connected. We already do have an issue to track this: freedomofpress/securedrop-export#25

[redshiftzero]

I tested this today and the auto-attachment logic is working nicely for me. I can reproduce that the devices widget currently used in qubes does not reflect the auto-attachment of USBs to our sd-export-usb VM, but regarding the last issue, @rmol has some wip resolving the issue (this branch currently has some experimental UI updates) in the qubes devices widget which resolves for me (next step is he's going to make a PR upstream).

If anyone wants to test or make changes to the devices widget here's instructions on how to get started:

1. git clone your fork https://github.com/QubesOS/qubes-desktop-linux-manager in your sd-dev AppVM and check out the relevant branch
2. in dom0 stop the existing devices widget: systemctl --user stop qubes-widget@qui-devices
3. in dom0 assuming you have cloned the repo in your homedir in sd-dev: qvm-run --pass-io sd-dev 'tar -c -C /home/user qubes-desktop-linux-manager' | tar xvf -
4. in dom0 run python3 -m qui.tray.devices

[rmol]

PR for the devices widget: QubesOS/qubes-desktop-linux-manager#87

[emkll]

Due to changes in #387, this will need to be rebased on latest master. We should also deny-by-default outbound USB policies ( @tag:sd-workstation @anyvm deny) as part of this PR as well.

Upstream fix for the device widget will be released in qubes-usb-proxy 1.0.23 which is still currently in testing repos.

[rmol]

Rebased, outbound denial added to policies.

[emkll]

Apologies, @rmol this will need to once again be rebased on latest master: due to changes introduced #407, sd-export-usb was renamed to sd-devices.

[rmol]

I rebased, updated the naming, it seems to be working fine and in about a week the Qubes devices widget should be correctly showing the state of automatically attached devices.

[rmol]

The upstream fix has made it to stable.

[emkll]

I am pleased to report that usb autoattach is working as expected with up-to-date dom0 🎉 . Thank you for your patience on this. I have two small request before this is ready to merge, from my perspective:

• make clean currently does not remove the udev rules in sys-usb. I recommend we remove those configurations as part of the sd-cleanup-sys-firewall task here. This behavior was changed after this PR was initially opened.
• explicitly set permissions in rpm spec (see comment inline)

Two other (for discussion, let me know what you think)

1. The makefile target sys-usb sounds confusing to me, I would recommend usb-autoattach or something of the sort.
2. Should we have a remove- target? The current behavior, based on my local testing has not been annoying/invasive at all, but can you think of any situations where a user would want to disable autoattach?

[rmol]

Good catches and suggestions. I:

• renamed the sys-usb make target to add-usb-autoattach
• added a remove-usb-autoattach make target
• included the removal logic in sd-clean-all.sls, so now make clean cleans up sys-usb
• explicitly set permissions of autoattach files in RPM specfile

[emkll]

Thanks @rmol , changes look great!

I've tested with 4 devices:

• 2 usb flash drives, they autoattach
• 1 yubikey, does not attach
• 1 printer, it auto-attaches
• Export and print work as expected
• Device is marked as attached by the GUI tool
• They can be re-attached using the GUI tool
• make {add, remove}-usb-autoattach work as expected (see comments inline, shouldn't block merge, imo but let me know what you think)

[emkll]

Tested the existing install scenario:

• make clone on this branch
• make add-usb-autoattach
• make test

✔️ autoattach works
✔️ all tests pass

Great work @rmol !