Support VeraCrypt export devices #126

rocodes · 2023-10-05T01:16:57Z

Refs freedomofpress/securedrop-workstation#265

Add basic detection/of already-unlocked veracrypt drives (Releasing these changes will deprecate the legacy files and should be done in coordination with corresponding client changes)
Closes [securedrop-export] combine usb-test and disk-test into a single disk-test securedrop-client#1733: Update "preflight check" logic to combine usb preflight checks (disk-test and usb-test). This has other benefits, such as allowing for detection of already-unlocked drives rather than re-prompting for the passphrase.
Note: in order to accommodate different filesystems that could be inside Veracrypt containers re:mounting permissions, this PR introduces a change from using sudo mount to using disksctl to handle mounting and unmounting. However, I'd appreciate a review re: file system mounting permissions and if we're OK with the changes. It looks like its possible to configure additional restrictions (eg fmask/dmask) and mount options in a configuration file. (see https://wiki.archlinux.org/title/Udisks#Mount_to_/media_(udisks2))
Another note: I have WIP work that would simplify a lot of the code and logic in sd-export, and would also bring a few other benefits (For example, being able to support multi-partitioned drives as long as they have one correctly-formatted encrypted partition). Here it is.

Test Plan

Provision 2 export USBs: one one-partition USB with LUKS, and one one-partition USB with Veracrypt. Veracrypt provisioning instructions here.
The most basic test plan would be to just build a .deb from the tip of this branch, install it in any Debian-based VM, and experiment with running send-to-usb /path/to/export/tarball.sd_export (with a USB plugged in, unplugged, etc). The script should always exit 0 and should always print a string result to stdout. There are minimal sample tarballs included in securedrop_export/tests/files for convenience.
This doesn't test inter-VM communications, but starting with that test plan and ensuring results are as expected should allow for testing in conjunction with sd-app (test plan covers multi-vm testing).

Expected results

No USBs connected: NO_DEVICE_DETECTED
> 1 USBs connected: MULTI_DEVICE_DETECTED
Unsupported device connected (could be: device has 2 or more partitions, device has unrecognized encryption scheme, device is a locked VeraCrypt drive, which we won't support for now due to difficulty with error handling, or malformed device): INVALID_DEVICE_DETECTED
1 LUKS-formatted USB connected and it's locked: DEVICE_LOCKED
1 LUKS-formatted device (partition or whole device encrypted) connected and it's unlocked: DEVICE_WRITABLE (and device will be mounted in /media/user), or SUCCESS_EXPORT (depending on the command in metadata.json).
1 Veracrypt-formatted device (parition or whole device encrypted) and it's unlocked: same as above
Other error statuses are fairly self explanatory (ERROR_MOUNT, ERROR_CLEANUP, etc)

…eraCrypt drive and unlocking VC drive, using cryptsetup.

…either bad VC passphrase or corrupted drive). Add logic to detect and use VeraCrypt drives. Use new Status and Service for exporting files. Add method to cli.py to check all volumes and return most detailed information about each candidate Volume, including mounting if the volume is already unlocked.

…c test coverage. Changes to VC unlock logic and service.py to mount LUKS drive immediately after unlocking.

cfm · 2023-12-01T02:59:41Z

Per handoff today: @legoktm will finalize tests next week, and I will review the week following.

mypy noted a theoretical case in which `udisksctl mount` exits with a 0 status code but doesn't return our expected output. In this case just raise an exception as we couldn't figure out the mount point. While we're at it, add a test case to verify the mountpoint is parsed correctly.

I think this was just a mistake because the volume is already unmounted before the volume is closed.

This should hopefully fix CI and running in containers that don't have an active rsyslog daemon.

legoktm · 2023-12-05T18:31:31Z

CI is passing! Flipping to ready for review now :)

legoktm · 2023-12-05T18:53:35Z

README.md

@@ -64,9 +64,6 @@ Metadata contains three possible keys which may contain several possible values:
 `device`


Need to change the line above that says "We do not yet support drives that use full-disk encryption with VeraCrypt." ;-)

cfm

This is looking good. I've left a few observations and nits inline, plus these questions:

This feels very state-mechanistic especially now that there is varying precision in what we can know about LUKS versus VeraCrypt volumes. We don't have to worry about that here, since send-to-usb is a one-shot action at a particular time, but I'll raise this again in securedrop-client.

Re: #126 (comment):

in order to accommodate different filesystems that could be inside Veracrypt containers re:mounting permissions, this PR introduces a change from using sudo mount to using disksctl to handle mounting and unmounting. However, I'd appreciate a review re: file system mounting permissions and if we're OK with the changes.

I'd like to understand better (before releasing if not as part of reviewing this feature) what you view as the changes in permissions and your concerns about them. For a LUKS volume, at least, the permissions appear unchanged:

user@sd-app:~/QubesIncoming/sd-dev$ ls -al /media/user/
total 12
drwxr-x---+  3 root root 4096 Dec 13 15:48  .
drwxr-xr-x   3 root root 4096 Dec 13 11:54  ..
drwx------  24 user user 4096 Dec 13 15:05 'QA export_transf'

And losing sudo to un/mount devices seems at face value like a good thing, even if sudo is a passwordless operation in a Qubes VM anyway.

Test plan

With device: "disk-test": No USBs connected: NO_DEVICE_DETECTED
With device: "disk-test": > 1 USBs connected: MULTI_DEVICE_DETECTED
With device: "disk-test": Unsupported device connected (could be: device has 2 or more partitions, device has unrecognized encryption scheme, device is a locked VeraCrypt drive, which we won't support for now due to difficulty with error handling, or malformed device): INVALID_DEVICE_DETECTED

No: UNKNOWN_DEVICE_DETECTED (see case [7] below).

With device: "disk-test": 1 LUKS-formatted USB connected and it's locked: DEVICE_LOCKED
With device: "disk-test": 1 LUKS-formatted device (partition or whole device encrypted) connected and it's unlocked: DEVICE_WRITABLE (and device will be mounted in /media/user), or SUCCESS_EXPORT (depending on the command in metadata.json).
With device: "disk":

user@sd-app:~/QubesIncoming/sd-dev$ lsblk /dev/sda
NAME           MAJ:MIN RM  SIZE RO TYPE  MOUNTPOINT
sda              8:0    1 14.3G  0 disk  
└─sda1           8:1    1 14.3G  0 part  
  └─luks-e538abe0-cd9e-4eab-b1d5-fb652acc235b
               254:0    0 14.3G  0 crypt /media/user/QA export_transf
user@sd-app:~/QubesIncoming/sd-dev$ send-to-usb sample_export.sd_export.cfm
Usage:
  udisksctl unmount [OPTION…]

Unmount a filesystem.

Options:
  -p, --object-path         Object to unmount
  -b, --block-device        Block device to unmount
  -f, --force               Force/lazy unmount
  --no-user-interaction     Do not authenticate the user if needed

DEVICE_ERROR

Fix proposed in #126 (comment).

1 Veracrypt-formatted device (parition or whole device encrypted) and it's unlocked: same as above

We shouldn't assume a single partition will be /sdX1:

user@sd-app:~/QubesIncoming/sd-dev$ lsblk
NAME    MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
sda       8:0    1 14.3G  0 disk 
└─sda2    8:2    1 14.1G  0 part 
xvda    202:0    0   10G  0 disk 
├─xvda1 202:1    0  200M  0 part 
├─xvda2 202:2    0    2M  0 part 
└─xvda3 202:3    0  9.8G  0 part /
xvdb    202:16   0   10G  0 disk /rw
xvdc    202:32   0   10G  0 disk 
├─xvdc1 202:33   0    1G  0 part [SWAP]
└─xvdc3 202:35   0    9G  0 part 
user@sd-app:~/QubesIncoming/sd-dev$ send-to-usb sample_export.sd_export
Device /dev/sda1 does not exist or access denied.
UNKNOWN_DEVICE_DETECTED

(Will this also be eased by rocodes#1?)

Other error statuses are fairly self explanatory (ERROR_MOUNT, ERROR_CLEANUP, etc)

securedrop_export/disk/cli.py

securedrop_export/disk/service.py

securedrop_export/archive.py

securedrop_export/disk/cli.py

cfm · 2023-12-14T00:03:20Z

(@legoktm, I will resubmit this review in securedrop-client once this pull request gets migrated there.)

cfm · 2023-12-14T20:07:39Z

Review copied to freedomofpress/securedrop-client#1669 (review).

rocodes force-pushed the 80-support-veracrypt branch from b2c4f28 to 68f24bc Compare October 5, 2023 01:22

rocodes mentioned this pull request Oct 6, 2023

Support VeraCrypt; additional export improvements freedomofpress/securedrop-client#1669

Closed

11 tasks

rocodes added 8 commits November 30, 2023 08:44

Add EncryptionScheme.VERACRYPT. Add methods for retrieving unlocked V…

131262c

…eraCrypt drive and unlocking VC drive, using cryptsetup.

Delete legacy_status and legacy_service

e5ed236

Update tests to use new service and status files and add additional v…

cad5655

…c test coverage. Changes to VC unlock logic and service.py to mount LUKS drive immediately after unlocking.

Add sample export tarballs and update test_main to use them

39cb393

Check /dev/mapper + use cryptsetup status to detect veracrypt drives.

6ce9855

Use udisksctl for USB mounting

e588a53

Error sooner when multiple drives attached

d867203

rocodes force-pushed the 80-support-veracrypt branch from ba3dd58 to d867203 Compare November 30, 2023 13:46

rocodes added 2 commits November 30, 2023 13:14

Remove encryption_method parameter.

1efe27f

Update README

4fdfa49

cfm self-assigned this Dec 1, 2023

cfm self-requested a review December 1, 2023 02:58

cfm assigned legoktm Dec 1, 2023

legoktm added 4 commits December 5, 2023 13:14

Apply black formatting

548c298

Fix type signature of CLI._close_veracrypt_volume()

15a8fa8

I think this was just a mistake because the volume is already unmounted before the volume is closed.

Disable logging when tests are run

3b7a34c

This should hopefully fix CI and running in containers that don't have an active rsyslog daemon.

legoktm marked this pull request as ready for review December 5, 2023 18:31

legoktm reviewed Dec 5, 2023

View reviewed changes

legoktm mentioned this pull request Dec 5, 2023

WIP: Use native Python functions instead of shelling out #129

Closed

cfm suggested changes Dec 13, 2023

View reviewed changes

cfm closed this Dec 14, 2023

rocodes mentioned this pull request Jan 23, 2024

Veracrypt support freedomofpress/securedrop-client#1777

Merged

34 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support VeraCrypt export devices #126

Support VeraCrypt export devices #126

rocodes commented Oct 5, 2023 •

edited by cfm

Loading

cfm commented Dec 1, 2023

legoktm commented Dec 5, 2023

legoktm Dec 5, 2023

cfm left a comment •

edited

Loading

cfm commented Dec 14, 2023

cfm commented Dec 14, 2023

		@@ -64,9 +64,6 @@ Metadata contains three possible keys which may contain several possible values:
		`device`

Support VeraCrypt export devices #126

Support VeraCrypt export devices #126

Conversation

rocodes commented Oct 5, 2023 • edited by cfm Loading

Test Plan

cfm commented Dec 1, 2023

legoktm commented Dec 5, 2023

legoktm Dec 5, 2023

Choose a reason for hiding this comment

cfm left a comment • edited Loading

Choose a reason for hiding this comment

Test plan

cfm commented Dec 14, 2023

cfm commented Dec 14, 2023

rocodes commented Oct 5, 2023 •

edited by cfm

Loading

cfm left a comment •

edited

Loading