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.

Sign up for GitHub

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

Permit whitelisting VMs for copy/paste & copying logs via tags #533

Merged
merged 7 commits into from
May 5, 2020
Merged

Permit whitelisting VMs for copy/paste & copying logs via tags #533

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
956cc10
Permit whitelisting VMs for copy/paste & copying logs via tags
eloquence Apr 10, 2020
846105e
Add @default rule so qvm-copy works
eloquence Apr 10, 2020
38dba62
Prefix all tags with sd-
eloquence Apr 14, 2020
3951038
Add script for removing SD tags from VMs
eloquence Apr 17, 2020
d23dd8c
Rescope clipboard permission to sd-app
eloquence Apr 21, 2020
49916a2
Run tag cleanup via Salt to avoid duplication
eloquence Apr 22, 2020
a04589a
Add require logic to clean-all state
eloquence Apr 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ clean: assert-dom0 prep-salt ## Destroys all SD VMs
$(MAKE) destroy-all
sudo qubesctl --show-output --skip-dom0 --targets whonix-gw-15 state.sls sd-clean-whonix
sudo qubesctl --show-output state.sls sd-clean-all
./scripts/remove-tags
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it would make sense to invoke this script/cleanup via sd-clean-all.sls? The challenge is that the path in dev environment will be different than the one in staging and production environments

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that seems like the better way to do it, will poke.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely fold into clean-salt, as @emkll suggests. If we use a separate script, then the RPM spec will need to be updated, as well. Consider using the less elegant, but more simply xargs-based approach outlined above.

Copy link
Member Author

Choose a reason for hiding this comment

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

The RPM spec already picks up the script by wildcard.

Copy link
Member Author

Choose a reason for hiding this comment

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

What I would propose is this:

  1. Move remove-tags from scripts to dom0
  2. Add it to RPM (I think that's what you meant, Conor?)
  3. Call it from sd-clean-all.sls as we do for update-xfce-settings resets.
  4. Remove it from securedrop-admin and Makefile.

Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

@eloquence Yes, those steps make sense. Moving the invocation to the salt clean-all state is sound at this point.

Ideally we'd call it from the scripts dir in /usr/share/, but before we can easily do that, we'll need to use the RPM in all environments, including make-dev, as described in #505 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 49916a2, not tested yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested now, seems to work as intended (this is via securedrop-admin):

          ID: remove-rpc-policy-tags
    Function: cmd.script
        Name: salt://remove-tags
      Result: True
     Comment: Command 'salt://remove-tags' run
     Started: 17:49:47.430389
    Duration: 267.274 ms
     Changes:   
              ----------
              pid:
                  29831
              retcode:
                  0
              stderr:
              stdout:
                  Removing tag 'sd-send-app-clipboard' from VM 'work'.
                  Removing tag 'sd-receive-app-clipboard' from VM 'work'.
----------

sudo dnf -y -q remove securedrop-workstation-dom0-config 2>/dev/null || true
$(MAKE) clean-salt

Expand Down
4 changes: 4 additions & 0 deletions dom0/sd-dom0-qvm-rpc.sls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ dom0-rpc-qubes.ClipboardPaste:
- marker_start: "### BEGIN securedrop-workstation ###"
- marker_end: "### END securedrop-workstation ###"
- content: |
@tag:sd-send-clipboard @tag:sd-workstation ask
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an sd-client tag that we assign to sd-app, to allow gpg operations. Perhaps we can use that one here instead?

#526 states the three use cases we are looking to enable are as follows:

* pasting passphrases into the login window from a vault VM
* copying messages from and to the conversation view (e.g., to another secure messaging app)
* copying logs and error messages

With those requirements in mind, a policy scoped to @tag:sd-client and the ability to copy logs via qvm-copy and centralized logging, what use case have we documented that require enabling copy paste to all workstation VMs?

Copy link
Member Author

@eloquence eloquence Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking about this as well a bit over the weekend. One use case is copying text out of disposable VMs used for viewing submissions. Right now this just works (with these changes) because the dispVMs have the sd-workstation tag. If we want to narrow the policies and still support that use case, we could perhaps use sd-client (two-directional copy/paste in and out) and sd-viewer-vm (one-directional copy/paste out).

Copy link
Contributor

Choose a reason for hiding this comment

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

Echoing @emkll, I recommend we scope to @tag:sd-client (i.e. exclude disposable VMs), see this note in the qubes documentation discouraging copy/paste from less trusted to more trusted domains:

one should keep in mind that performing a copy and paste operation from less trusted to more trusted domain can always be potentially insecure, because the data that we insert might potentially try to exploit some hypothetical bug in the destination VM (e.g. the seemingly innocent link that we copy from untrusted domain, might turn out to be, in fact, a large buffer of junk that, when pasted into the destination VM’s word processor could exploit a hypothetical bug in the undo buffer). This is a general problem and applies to any data transfer between less trusted to more trusted domains.... So, you should always copy clipboard and data only from more trusted to less trusted domains.

However, if we are finding (or otherwise think that) data transfer from disposable VMs into trusted VMs is an important workflow to support, we can file a followup issue for further investigation to either determine how to decrease risk when copy/pasting data from sd-viewer to sd-client or to provide an alternate method to handle that use case (some of the workflows we need to support for removing identifying metadata could handle this use case, e.g. adding an OCR step after we convert documents to trusted PDFs).

Copy link
Member Author

@eloquence eloquence Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good; for now I'll call the permission sd-send-app-clipboard and sd-receive-app-clipboard to reflect its narrow scope.

Copy link
Member Author

@eloquence eloquence Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is done in d23dd8c (I used sd-app rather than a tag for the SecureDrop VM since that seems the best way to ensure we have at most a many-to-one/one-to-many relationship here). The test plan has been updated; I've not re-tested yet.

@tag:sd-workstation @tag:sd-receive-clipboard ask
@anyvm @tag:sd-workstation deny
@tag:sd-workstation @anyvm deny
dom0-rpc-qubes.FeaturesRequest:
Expand All @@ -35,6 +37,8 @@ dom0-rpc-qubes.Filecopy:
- marker_start: "### BEGIN securedrop-workstation ###"
- marker_end: "### END securedrop-workstation ###"
- content: |
sd-log @default ask
sd-log @tag:sd-receive-logs ask
sd-proxy @tag:sd-client allow
@anyvm @tag:sd-workstation deny
@tag:sd-workstation @anyvm deny
Expand Down
32 changes: 32 additions & 0 deletions scripts/remove-tags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#!/usr/bin/env python3
"""
Removes tags used for exempting VMs from default SecureDrop Workstation
RPC policies from all VMs (including non-SecureDrop ones).
"""
import qubesadmin

q = qubesadmin.Qubes()

TAGS_TO_REMOVE = ["sd-send-clipboard", "sd-receive-clipboard", "sd-receive-logs"]


def main():
tags_removed = False
for vm in q.domains:
for tag in TAGS_TO_REMOVE:
if tag in q.domains[vm].tags:
print("Removing tag '{}' from VM '{}'.".format(tag, vm))
try:
q.domains[vm].tags.remove(tag)
except Exception as error:
print("Error removing tag: '{}'".format(error))
print("Aborting.")
exit(1)
tags_removed = True

if tags_removed is False:
print("Tags {} not set on any VMs, nothing removed.".format(TAGS_TO_REMOVE))


if __name__ == "__main__":
main()
5 changes: 4 additions & 1 deletion scripts/securedrop-admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ def perform_uninstall():
subprocess.check_call(
["sudo", "dnf", "-y", "-q", "remove", "qubes-template-securedrop-workstation-buster"]
)
print("Removing SecureDrop tags from remaining VMs")
subprocess.check_call([os.path.join(SCRIPTS_PATH, "scripts/remove-tags")])
print("Uninstalling dom0 config package")
subprocess.check_call(
["sudo", "dnf", "-y", "-q", "remove", "securedrop-workstation-dom0-config"]
Expand All @@ -132,7 +134,8 @@ def main():
elif args.uninstall:
print(
"Uninstalling will remove all packages and destroy all VMs associated\n"
"with SecureDrop Workstation."
"with SecureDrop Workstation. It will also remove all SecureDrop tags\n"
"from other VMs on the system."
)
response = input("Are you sure you want to uninstall (y/N)? ")
if response.lower() != 'y':
Expand Down
4 changes: 4 additions & 0 deletions tests/vars/qubes-rpc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
- policy: qubes.ClipboardPaste
starts_with: |-
### BEGIN securedrop-workstation ###
@tag:sd-send-clipboard @tag:sd-workstation ask
@tag:sd-workstation @tag:sd-receive-clipboard ask
@anyvm @tag:sd-workstation deny
@tag:sd-workstation @anyvm deny
### END securedrop-workstation ###
Expand All @@ -15,6 +17,8 @@
- policy: qubes.Filecopy
starts_with: |-
### BEGIN securedrop-workstation ###
sd-log @default ask
sd-log @tag:sd-receive-logs ask
sd-proxy @tag:sd-client allow
@anyvm @tag:sd-workstation deny
@tag:sd-workstation @anyvm deny
Expand Down