restoring original perms should be conditional to having changed them

[jpouellet]

is_untrusted_xattr() always tries to restore orig_perms, regardless of whether they were changed (to be able to read xattrs) or not. This is not the correct behavior when we did not change the perms in the first place.

Note the chmod in the following:

$ strace -y qvm_file_trust.py /tmp/foo 2>&1 | grep foo
execve("qubesfiletrust/qvm_file_trust.py", ["qubesfiletrust/qvm_file_trust.py", "/tmp/foo"], [/* 47 vars */]) = 0
stat("/tmp/foo", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
stat("/tmp/foo", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
stat("/tmp/foo", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
open("/tmp/foo", O_RDONLY|O_CLOEXEC)    = 3</tmp/foo>
fstat(3</tmp/foo>, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
ioctl(3</tmp/foo>, TCGETS, 0x7ffc1f90aef0) = -1 ENOTTY (Inappropriate ioctl for device)
fstat(3</tmp/foo>, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
lseek(3</tmp/foo>, 0, SEEK_CUR)         = 0
ioctl(3</tmp/foo>, TCGETS, 0x7ffc1f90ae50) = -1 ENOTTY (Inappropriate ioctl for device)
lseek(3</tmp/foo>, 0, SEEK_CUR)         = 0
close(3</tmp/foo>)                      = 0
listxattr("/tmp/foo", NULL, 0)          = 0
chmod("/tmp/foo", 0100664)              = 0

This manifests itself as odd behavior such as:

$ qvm_file_trust.py /dev/null  
Error: Unable to set original permissions of /dev/null

[jpouellet]

Also:

https://github.com/anoadragon453/qubes-mime-types/blob/0b8b5a85522499462e48e00dc4915246a73ec790/qubesfiletrust/qvm_file_trust.py#L289-L291

Not that it matters a ton in Qubes, but 644 is too wide. You should only need 400 in check_file() and 200 (maybe 600) in change_file().

[jpouellet]

In fact, I wonder if we should ever change the original perms to begin with. Just treating unreadable files as untrusted sounds like it has a safer failure mode. The unix permissions model is somewhat less relevant in Qubes, but users may still have completely valid reasons for wanting a file to remain unreadable and wouldn't want:

1. a window in which it becomes readable while qvm-file-trust operates normally
2. a potential failure mode resulting in a net increase in permissions in the event that qvm-file-trust after increasing and before restoring permissions

idk... @marmarek @rootkovska thoughts?

[jpouellet]

Also, the way qvm-open-trust-based is currently implemented (without locking), it's possible for two concurrent invocations of it to have the net-effect of making an unreadable file readable. One can imagine other such bugs not being caught.

[anoadragon453]

We'll still need to change the permissions of files to change between trusted/untrusted, but I've made it so we now assume untrusted on inability to read.

I assume this still means that if a file is readable, yet we also find our attribute, then it's still considered untrusted.

[anoadragon453]

@jpouellet I've made files have 0200 permission (w) upon being set trusted.