Re: [PATCH v2 1/5] fscrypt: fix loophole in one-encryption-policy-per-tree enforcement

[Theodore Ts'o <tytso@mit.edu>]

On Mon, Dec 19, 2016 at 02:20:12PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Filesystem encryption is designed to enforce that all files in an
> encrypted directory tree use the same encryption policy.  Operations
> that violate this constraint are supposed to fail with EPERM.  There was
> one case that was missed, however: the cross-rename operation (i.e.
> renameat2 with RENAME_EXCHANGE) allowed two files with different
> encryption policies to be exchanged, provided that neither encryption
> key was available.

I'm actually not sure this is the best way to address this issue.

What I think is better would be to forbid any renames if we are
missing the encryption key for the directory.  I had actually noticed
a problem here early when this worked:

root@kvm-xfstests:/# mount -o test_dummy_encryption /dev/vdc /vdc
root@kvm-xfstests:/# mkdir /vdc/test
root@kvm-xfstests:/# cp /etc/motd /vdc/test
root@kvm-xfstests:/# touch /vdc/test/empty
root@kvm-xfstests:/# umount /vdc
root@kvm-xfstests:/# mount /dev/vdc /vdc
root@kvm-xfstests:/# cd /vdc/test
root@kvm-xfstests:/vdc/test# ls -l
total 4
-rw-r--r-- 1 root root   0 Dec 27 22:35 6,LKNRJsp209FbXoSvJWzB
-rw-r--r-- 1 root root 286 Dec 27 22:35 uRJ5vJh9gE7vcomYMqTAyD
root@kvm-xfstests:/vdc/test# mv uRJ5vJh9gE7vcomYMqTAyD  6,LKNRJsp209FbXoSvJWzB
root@kvm-xfstests:/vdc/test# ls -l
total 4
-rw-r--r-- 1 root root 286 Dec 27 22:35 6,LKNRJsp209FbXoSvJWzB
root@kvm-xfstests:/vdc/test#

While there's no cryptographic reason why this can't be done
(obviously), and while a bad guy can always do something like this via
debugfs, I can't see any legitimate reason why we should allow this to
work.

It's one thing to allow a process without the encryption key
(generally with root privileges) to delete a file, but to rename two
files in an encrypted directory?  Or as you pointed out, allowing an
exchange of two files via RENAME_EXCHANGE?  Even if encryption
policies match, I don't think we should be allowing this at all.

	 	 	  	      	 - Ted