Feature #5315: Warn users when running in a non-free VM

Feature #5315

Warn users when running in a non-free VM

Added by Tails 2013-07-18 07:38:59 . Updated 2015-09-17 13:12:44 .

Status:

Resolved

Priority:

Low

Assignee:

Category:

Virtualization

Target version:

Tails_2.0

Start date:

Due date:

% Done:

100%

Feature Branch:

Type of work:

Code

Blueprint:

Starter:

Affected tool:

Deliverable for:

Description

When running inside a non-free VM, suggest that the user switches to VirtualBox or some other free software VM.

Among the "facts" reported by virt-what(1), these ones are free: kvm, qemu, uml, virtualbox, xen, xen-domU, xen-hvm. So, if one of the lines returned by virt-what is not in this set, then we should assume Tails is running inside a non-free virtualization environment.

The file where this logics should be implemented is /usr/local/bin/tails-virt-notify-user.

Files

updated-warning.txt (2520 B)

hybridwipe, 2015-08-10 13:04:33

Subtasks

History

#1 Updated by intrigeri 2013-09-04 03:27:55

Priority changed from Normal to Low
Starter set to No

#2 Updated by BitingBird 2014-06-09 12:08:56

Subject changed from warn users when running in a non-free VM to Warn users when running in a non-free VM

#3 Updated by sajolida 2014-11-16 11:22:40

Category set to Virtualization

#4 Updated by hybridwipe 2015-07-07 16:33:09

File <del>missing: 0001-tails-virt-notify-user-notify-user-if-they-re-runnin.patch</del> added

I’ve attached a patch to implement this. It also converts the patch to a (IMHO much more readable) shell script.

Please review and let me know of any issues. Thanks in advance.

#5 Updated by intrigeri 2015-07-07 17:13:01

Assignee set to intrigeri
Target version set to Tails_1.5
QA Check set to Ready for QA

> I’ve attached a patch to implement this.

Thanks! I’ll look at it shortly.

#6 Updated by BitingBird 2015-07-08 16:20:46

Status changed from Confirmed to In Progress
% Done changed from 0 to 20

#7 Updated by intrigeri 2015-07-09 01:41:10

Assignee changed from intrigeri to hybridwipe
QA Check changed from Ready for QA to Dev Needed

So, I’ve taken a look, and fundamentally it looks pretty good. Here are a few improvement suggestions.

I could personally be fine with the move to shell scripting, but the bad news is that this will break when porting to Jessie (~~Bug #7989~~); see the changes that were made to this script in our feature/jessie branch, that introduce notification actions to react when the user clicks the link. If you find a way to solve this problem in shell, fine (and actually it could be reused elsewhere, which would be awesome). Otherwise, I think we’ll have to go back to Perl or Python.
If we stick to shell, why use bash instead of a POSIX shell interpreter?
If we stick to shell, please consider adding set -u, and dropping set -x.
The i18n support was dropped while converting to shell. If we stick to shell, please reintroduce it (see examples listed in SHELL_PROGS) and update refresh-translations.

Thanks again! :)

#8 Updated by hybridwipe 2015-07-13 18:13:02

File <del>missing: tails-virt-nonfree-notify-user-add-a-script-service.txt</del> added

I attempted to add a second notification to the original tails-virt-notify-user script, but ran into concurrency issues; elected in the end to have a separate script, which looks much cleaner imo.

#9 Updated by intrigeri 2015-07-16 07:13:45

> I attempted to add a second notification to the original tails-virt-notify-user script, but ran into concurrency issues; elected in the end to have a separate script, which looks much cleaner imo.

Why not instead display a single notification when running in a VM, with a slightly different text depending on whether the VM technology being used is free or not?

#10 Updated by hybridwipe 2015-07-16 22:05:49

File <del>missing: tails-notify-nonfree.txt</del> added

Sure, good point.

#11 Updated by intrigeri 2015-07-17 01:45:05

QA Check changed from Dev Needed to Ready for QA

#12 Updated by intrigeri 2015-07-17 01:48:24

Assignee deleted (~~hybridwipe~~)

#13 Updated by intrigeri 2015-07-17 01:58:11

Assignee set to intrigeri

#14 Updated by intrigeri 2015-07-17 07:35:52

Assignee changed from intrigeri to hybridwipe
Target version changed from Tails_1.5 to Tails_2.0
QA Check changed from Ready for QA to Dev Needed

Here are a few comments:

Yay, getting closer to something we can merge. Congrats! :)
This is based on current feature/jessie, so I’m setting the Target version accordingly.
It’s a bit sad to run /usr/bin/systemd-detect-virt twice, once to get its exit code, and another time to get its stdout. You could instead do something like my $vm_type = capturex([0, 1], qw{/usr/bin/systemd-detect-virt});, and then exit 0 if $EXITVAL == 1; — see the doc for IPC::System::Simple for details.
The list of if/elsif for setting $whitelist could be replaced with an array containing the whitelist, and then instead of if ($whitelist == 1) we would check if $vm_type is in the whitelist.
We should call $notify->create only once, that is outside of the if ($whitelist == 1) block. This requires declaring my ($summary, body); outside of the if block, then setting $summary’s and $body’s value in the if/else block, and then calling $notify->create.

#15 Updated by hybridwipe 2015-07-28 23:47:33

File <del>missing: almost.txt</del> added
File <del>missing: fails.txt</del> added

intrigeri wrote:
> Here are a few comments:

Thanks for all the comments, perl is not my forte, but that helped immensely.

> * Yay, getting closer to something we can merge. Congrats! :)
> * This is based on current feature/jessie, so I’m setting the Target version accordingly.
> * It’s a bit sad to run /usr/bin/systemd-detect-virt twice, once to get its exit code, and another time to get its stdout. You could instead do something like my $vm_type = capturex([0, 1], qw{/usr/bin/systemd-detect-virt});, and then exit 0 if $EXITVAL == 1; — see the doc for IPC::System::Simple for details.
> * The list of if/elsif for setting $whitelist could be replaced with an array containing the whitelist, and then instead of if ($whitelist == 1) we would check if $vm_type is in the whitelist.

Done.

> * We should call $notify->create only once, that is outside of the if ($whitelist == 1) block. This requires declaring my ($summary, body); outside of the if block, then setting $summary’s and $body’s value in the if/else block, and then calling $notify->create.

I wasn’t able to get that to work, not sure if I did something wrong. I’ve attached two patches (almost.txt / fails.txt). With $notify->create moved outside of the if block, the summary is missing. The body is still present, however..perhaps you can find some mistake I made, as I said, I’m not comfortable in perl, so may have missed something obvious.

Thanks again for the review/tips.

#16 Updated by hybridwipe 2015-07-29 00:45:23

File deleted (~~fails.txt~~)

#17 Updated by hybridwipe 2015-07-29 00:45:32

File deleted (~~almost.txt~~)

#18 Updated by hybridwipe 2015-07-29 00:45:34

File deleted (~~tails-notify-nonfree.txt~~)

#19 Updated by hybridwipe 2015-07-29 00:45:38

File deleted (~~tails-virt-nonfree-notify-user-add-a-script-service.txt~~)

#20 Updated by hybridwipe 2015-07-29 00:45:39

File deleted (~~0001-tails-virt-notify-user-notify-user-if-they-re-runnin.patch~~)

#21 Updated by hybridwipe 2015-07-29 00:46:14

File <del>missing: patch.txt</del> added

Found the problem in last patch, working patch attached for review.

#22 Updated by BitingBird 2015-07-29 09:35:55

Assignee changed from hybridwipe to intrigeri
QA Check changed from Dev Needed to Ready for QA

A patch was submitted for review -> setting as ready for QA.

#23 Updated by intrigeri 2015-08-07 02:33:06

Assignee changed from intrigeri to hybridwipe
% Done changed from 20 to 60
QA Check changed from Ready for QA to Dev Needed

Thanks!

I would add bochs to the whitelist. Would this be wrong?
Perhaps we could pass --vm to systemd-detect-virt? (I realize that it’s orthogonal to this ticket, but while we’re at it… it could make the call a bit faster and decrease a bit the attack surface.)
I can’t find any reference to xen-domU nor xen-hvm in the manpage, be it on Jessie or current sid. Are they really values that can be output by systemd-detect-virt?
May you please consult the tails-ux@boum.org (public mailing-list) for how best to phrase the new warning you’re introducing? It seems quite good already, but e.g. I’m not sure about the “for defects” part.

Except these few nitpicks, the last version of this patch looks perfect to me :)

How about getting yourself a Git repo by the way? This would ease reviewing. If interested, ask tails-sysadmins@boum.org.

#24 Updated by hybridwipe 2015-08-07 12:07:12

File <del>missing: foo.txt</del> added

intrigeri wrote:
> Thanks!
>
> * I would add bochs to the whitelist. Would this be wrong?

Agreed.

> * Perhaps we could pass --vm to systemd-detect-virt? (I realize that it’s orthogonal to this ticket, but while we’re at it… it could make the call a bit faster and decrease a bit the attack surface.)

Sure.

> * I can’t find any reference to xen-domU nor xen-hvm in the manpage, be it on Jessie or current sid. Are they really values that can be output by systemd-detect-virt?

That was from virt-what, see http://linux.die.net/man/1/virt-what and the original bug summary. systemd-detect-virt does not appear to support them, however, so I’ve removed them.

> * May you please consult the tails-ux@boum.org (public mailing-list) for how best to phrase the new warning you’re introducing? It seems quite good already, but e.g. I’m not sure about the “for defects” part.

Sent an RFC.