Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AppArmor rules for Qubes 4.1; update dispVM syntax #1485

Closed
wants to merge 2 commits into from

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented May 3, 2022

Description

  1. Adds /usr/bin/expr to the AppArmor profile for securedrop-client because it is now called by qvm-open-in-vm (see https://github.com/QubesOS/qubes-core-agent-linux/blame/master/qubes-rpc/qvm-open-in-vm). This causes attempts to open files in disposable VMs to fail. See Cannot open files in disposable VM using SecureDrop Client securedrop-workstation#766 (comment) for details.
  2. Uses @ instead of $ syntax for qvm-open-in-vm call per https://github.com/QubesOS/qubes-secpack/blob/master/QSBs/qsb-038-2018.txt

Resolves freedomofpress/securedrop-workstation#766

Test Plan

For all test cases:

  • Build a package based on this branch and install it in sd-small-buster-template (this is necessary because the AppArmor change is installed in /etc/apparmor.d in the template); then restart the TemplateVM and the AppVM to pick up the changes
  • Ensure that downloadable files are present on the server

Test case 1: No breakage on Qubes 4.0

  1. Run the client you installed via the custom package
  2. Download a file
  • Observe that you are able to open files in disposable VMs by clicking on them in SecureDrop Client
  • Observe no AppArmor-related messages in /var/log/syslog on sd-app

Test case 2: Fixes regression on Qubes 4.1 with Qubes 4.1 packages in AppmVM

  1. Ensure your sd-small-buster-template uses the Qubes 4.1 packages. On my system, /etc/apt/sources.list.d/qubes-r4.list contains only the following uncommented line:
deb [arch=amd64] https://deb.qubes-os.org/r4.1/vm buster main
  1. After changing it, run sudo apt update and sudo apt dist-upgrade on sd-small-buster-template to switch to 4.1 packages, and restart sd-small-buster-template and sd-app.
  2. Run the client you installed via the custom package
  3. Download a file

@eaon
Copy link
Contributor

eaon commented May 4, 2022

Works for me in 4.1, haven't tested in 4.0

@eloquence eloquence marked this pull request as ready for review May 9, 2022 23:25
@eloquence eloquence requested a review from a team as a code owner May 9, 2022 23:25
@eloquence
Copy link
Member Author

I've added a test plan for 4.0 and 4.1. @eaon, can you confirm that your test procedure matches the test plan for 4.1? If so I think we can consider that part tested.

I've omitted a test case for "4.1 with 4.0 repos" on the assumption that this is not a state we'll ship to production users.

@eloquence
Copy link
Member Author

Ah, because of the AppArmor rules living in /etc/apparmor.d/usr.bin.securedrop-client, tester needs to either manually patch that or build a package, then reload AppArmor. Updating test plan to fix.

@eloquence
Copy link
Member Author

Updated test plan (best to actually build a package from this branch).

@eaon
Copy link
Contributor

eaon commented May 10, 2022

Yes, test plan for 4.1 pretty much aligns with my experience, the core difference though:

  1. After changing it, run sudo apt update and sudo apt dist-upgrade on sd-small-buster-template to switch to 4.1 packages, and restart sd-small-buster-template and sd-app.

This failed for me because xen-utils-guest and xen-utils-common both want to write to /lib/systemd/system/xendriverdomain.service - I manually resolved this by running dpkg --force-overwrite -i /var/cache/apt/archives/xen-utils-guest_4.14.4-+deb10u1_amd64.deb and apt --fix-broken install.

After that, the rest worked as expected:

@sssoleileraaa
Copy link
Contributor

@eaon are you able to test this on a 4.0 system and approve if all works as expected?

sssoleileraaa
sssoleileraaa previously approved these changes May 11, 2022
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test Plan

For all test cases:

  • Build a package based on this branch and install it in sd-small-buster-template (this is necessary because the AppArmor change is installed in /etc/apparmor.d in the template); then restart the TemplateVM and the AppVM to pick up the changes
  • Ensure that downloadable files are present on the server

Test case 1: No breakage on Qubes 4.0

  1. Run the client you installed via the custom package
  2. Download a file
  • Observe that you are able to open files in disposable VMs by clicking on them in SecureDrop Client
  • Observe no AppArmor-related messages in /var/log/syslog on sd-app

Test case 2: Fixes regression on Qubes 4.1 with Qubes 4.1 packages in AppmVM

@eaon confirmed this test case

@sssoleileraaa
Copy link
Contributor

This PR in particular seems to be stuck in a state that gives me no option to merge, so I'll try a rebase push to see if I can kick it....

@sssoleileraaa
Copy link
Contributor

looks like the rebase empty push trick worked

sssoleileraaa
sssoleileraaa previously approved these changes May 11, 2022
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sssoleileraaa
Copy link
Contributor

approving it puts it back in the same un-mergable state (see below). about to head out so i'll try again tomorrow.
Screenshot from 2022-05-10 19-15-00

@eloquence
Copy link
Member Author

I'm seeing an actual error 500 from GitHub so hopefully intermittent issues, but will shoot off a note to their support just in case.

@sssoleileraaa
Copy link
Contributor

@eloquence it's only happening with this PR, so could you try opening another PR for now?

@eloquence
Copy link
Member Author

Due to GitHub's borked status, I force-pushed a git --amend which dismissed Allie's approval. There's no content change, so should be OK to approve/merge by any maintainer if CI goes through.

@sssoleileraaa
Copy link
Contributor

Due to GitHub's borked status, I force-pushed a git --amend which dismissed Allie's approval. There's no content change, so should be OK to approve/merge by any maintainer if CI goes through.

I did this last week and, after approval, the issue occurred again. But let's give another try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot open files in disposable VM using SecureDrop Client
3 participants