BUG: selinux-testsuite overlayfs failures on v4.19-rc1

[pcmoore]

Running the selinux-testsuite results in the following failure:

# uname -r
4.19.0-0.rc1.git0.1.1.secnext.fc30.x86_64
# make test
{ ...snip... }
Running as user root with context unconfined_u:unconfined_r:unconfined_t

domain_trans/test ........... ok   
entrypoint/test ............. ok   
execshare/test .............. ok   
exectrace/test .............. ok   
execute_no_trans/test ....... ok   
fdreceive/test .............. ok   
inherit/test ................ ok   
link/test ................... ok   
mkdir/test .................. ok   
msg/test .................... ok     
open/test ................... ok   
ptrace/test ................. ok   
readlink/test ............... ok   
relabel/test ................ ok   
rename/test ................. ok   
rxdir/test .................. ok   
sem/test .................... ok     
setattr/test ................ ok   
setnice/test ................ ok   
shm/test .................... ok     
sigkill/test ................ ok     
stat/test ................... ok   
sysctl/test ................. ok   
task_create/test ............ ok   
task_setnice/test ........... ok   
task_setscheduler/test ...... ok   
task_getscheduler/test ...... ok   
task_getsid/test ............ ok   
task_getpgid/test ........... ok   
task_setpgid/test ........... ok   
file/test ................... ok     
ioctl/test .................. ok   
capable_file/test ........... ok     
capable_net/test ............ ok   
capable_sys/test ............ ok   
dyntrans/test ............... ok   
dyntrace/test ............... ok   
bounds/test ................. ok     
nnp_nosuid/test ............. ok     
mmap/test ................... ok     
unix_socket/test ............ ok   
inet_socket/test ............ ok     
overlay/test ................ 63/121 
#   Failed test at overlay/test line 592.
# Looks like you failed 1 test of 121.
overlay/test ................ Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/121 subtests 
checkreqprot/test ........... ok   
mqueue/test ................. ok     
mac_admin/test .............. ok   
atsecure/test ............... ok   
cap_userns/test ............. ok   
extended_socket_class/test .. ok     
sctp/test ................... ok     
netlink_socket/test ......... ok   
prlimit/test ................ ok   
binder/test ................. ok   
infiniband_endport/test ..... ok   
infiniband_pkey/test ........ ok   

Test Summary Report
-------------------
overlay/test              (Wstat: 256 Tests: 121 Failed: 1)
  Failed test:  111
  Non-zero exit status: 1
Files=55, Tests=628, 140 wallclock secs ( 0.30 usr  0.09 sys + 12.64 cusr 13.57 csys = 26.60 CPU)
Result: FAIL
Failed 1/55 test programs. 1/628 subtests failed.

[pcmoore]

Observations:

• When running the failing test the kernel prints the following to the console: overlayfs: failed to get metacopy (-13).
• Based on the git log, it appears that v4.19 enables a metadata only copy up feature, this may be the source of the problem.

[rhvgoyal]

Yes, I think this is related. So during dentry lookup we also check if metacopy xattr is present or not. So kernel makes a vfs_getxattr() call on underlying file. This test is not giving any access on the file, so getxattr fails.

I think we should modify selinux policy for test and atleast give getattr access to underlying file and that should fix the issue.

Another alternative is to not check for metacopy xattr if metacopy=on is not passed. But this will create problems if somebody mounted overlayfs with metacopy=on and later remounted without metacopy=on. In that case we don't want to create any new metacopy indoes but there are already existing metacopy inodes in the fs and we want to make sure that these are handled without any issues.

Unfortunately we don't have any mechanism in overlayfs to determine that what options were used with overlayfs mount and what options are now incompatible for mounting. There are some proposals but nothing has made upstream yet.

[pcmoore]

I think we should modify selinux policy for test and atleast give getattr access to underlying file and that should fix the issue.

I dislike the requirement to change the SELinux policy for the simple reason that it implies a breaking change in kernel behavior. Changing the selinux-testsuite policy is easy enough, but what about all of the distribution, third-party, and custom SELinux policies? I think we need to find a way to make this work with the existing policy/tests.

[stephensmalley]

Should overlayfs be using __vfs_getxattr() to avoid permission checking on these kinds of internal operations?

[stephensmalley]

vfs_getxattr() vs __vfs_getxattr(): The former does permission checking and also transparently redirects requests for the security.* attributes to the security module for handling (to deal with filesystems that do not natively support security xattrs and to allow the security module to provide the canonical value). The latter just invokes the xattr handler to fetch the value from the filesystem. For non-security attributes, unless we want permission checking here, we should just use __vfs_getxattr().

[rhvgoyal]

Current overlayfs model is to switch to mounter's creds for operations on lower or upper files. so bypassing checks on getxattr() is just one operation and it does not fit into overlayfs's current security model.

In our test we have noaccess file. I think we need to give atleast getattr() permission to mounter and then check would succeed.

If mounter can't even do getattr, file unlink will be denied. Is this something critical from user's point of view?

Also metacopy is just one xattr. There are other xattrs as well. May be it was by chance that we did not hit that path in the past. I remember having discussed this with dan walsh and he had agreed that noaccessfile should have atleast getattr permission. Or get rid of test.

CC @rhatdan

[rhvgoyal]

I was wondering why this test did not fail previously because we already have logic for checking XATTR_ORIGIN in ovl_lookup(). Looks like checking XATTR_ORIGIN kicks in only if there is upperdentry. Given there is no upper file in our test, I think that's why it succeeded. If we modify test to copy up file and then do unlink() it should fail same way.

IOW, current lookup in overlayfs has dependency that atleast mounter should be able to check xattr of the file.

[pcmoore]

As a quick test, I attempted a similar operation using an ext4 filesystem:

# uname -r
4.19.0-0.rc1.git0.1.1.secnext.fc30.x86_64
# getenforce
Enforcing
#
# mount -t ext4 -o loop /mnt/testfs.img /mnt/test
# ls -Z /mnt/test
    system_u:object_r:unlabeled_t:s0 lost+found
unconfined_u:object_r:unlabeled_t:s0 testfile
# umount /mnt/test
#
# mount -t ext4 -o 'context="unconfined_u:object_r:test_overlay_files_rwx_t:s0:c10,c20",loop' /mnt/testfs.img /mnt/test
# ls -Z /mnt/test
unconfined_u:object_r:test_overlay_files_rwx_t:s0:c10,c20 lost+found
unconfined_u:object_r:test_overlay_files_rwx_t:s0:c10,c20 testfile
# runcon -t test_overlay_client_t -l s0:c10,c20 -- ls -Z /mnt/test
unconfined_u:object_r:test_overlay_files_rwx_t:s0:c10,c20 lost+found
unconfined_u:object_r:test_overlay_files_rwx_t:s0:c10,c20 testfile
# runcon -t test_overlay_client_t -l s0:c10,c20 -- unlink /mnt/test/testfile 
# runcon -t test_overlay_client_t -l s0:c10,c20 -- ls -Z /mnt/test
unconfined_u:object_r:test_overlay_files_rwx_t:s0:c10,c20 lost+found
# umount /mnt/test

[pcmoore]

Based on the comments above I think the right answer is simply to expect the test to fail; basically we should replace the call to test_72_ctx with a call to test_72 in this particular case. Any last objections before I make the change?

[rhvgoyal]

@pcmoore, If we replace test_72_ctx() with test_72(), then it will start failing on older kernels.

So either we should not run test_72() at all during context mounts. Or if we want to run test with context mounts, then we need to give getattr permission on noaccess file.

I just experimented with giving getattr permission to mounter and created a PR.

SELinuxProject/selinux-testsuite#39

I am fine with whatever change you like.

[rhvgoyal]

Or may be trying unlink on noaccess_file_t is not the right thing to test. We should it on a file which does not have unlink permission to overlay_client_t. So unlink will fail without context. But it will succeed with context mount. And mounter should have getattr on this file type.

So may be we need to create a new type file_no_unlink_t?

[stephensmalley]

I am still unclear on why we are using vfs_getxattr() here rather than __vfs_getxattr(). In the case of fetching the metacopy xattr, we are not doing so in order to expose it to userspace, but rather to use it internally within the kernel code, right? At no point are we returning the xattr value to userspace in this code path? So omitting the permission check altogether when fetching overlayfs-internal xattrs does not risk allowing the mounter to gain access to unauthorized files through overlayfs context mounts, avoids needing to allow the mounter to directly perform getxattr(2) calls on it, and avoids the brittleness of making policy dependent on overlayfs internal implementation details.
In contrast, using vfs_getxattr() after switching creds in ovl_xattr_get() makes sense because there we are returning the xattr value to userspace and thus we want a permission check against the mounter's creds.
Does that make sense?

[stephensmalley]

This btw would apply to all of the overlay xattrs (metacopy, origin, redirect, nlink) - as long as it is purely kernel-internal, why do we need to perform a check here? Not sure what happens in the NFS case though where the server might apply a check.

[pcmoore]

@pcmoore, If we replace test_72_ctx() with test_72(), then it will start failing on older kernels.

My current opinion is that the selinux-testsuite should focus on testing for the correct behavior, even though this may cause test failures on older, unpatched kernels. The primary focus of the selinux-testsuite is to serve as a quick regression test for upstream development. Of course it can be used for other purposes as well, but those are secondary.

@rhvgoyal it would also be good to address @stephensmalley's concern regarding the use of __vfs_getxattr(); while you made some comments about overlayfs' design, I don't think you really answered the question.

[rhvgoyal]

I think using vfs_getxattr() in the context of mounter creds works well with the current security model. Any operation on underlying layer happens with the creds of mounter and if mounter is not allowed to do certain operation it fails.

Bypassing checks during lookup for getxattr() seems like a band aid fix. There are other cases to consider.

For example, what if mounter does not have permission to search in lower dir. In that case also path resolution will fail (IIUC) and then unlink will fail. And it makes sense because that's how security model was designed.

Another case to consider is that unlink drops a whiteout if a file on upper directory is being removed. (So that lower file does not become visible). Now if mounter does not have permission to write to upper or create char device, this unlink operation will fail.

What I am trying to say is that getxattr() is just one operation which is done in with the mounter's creds on underlying file system. There are many operations which are done with mounter's creds and if policy is not configured right, it will break in all those cases as well.

While it is little difficult, I think we have to configure policy right and make sure mounter has right permissions. It might be little un-intuitive sometimes (as in this case. Why unlink is resulting in getxattr in lower filesystem), but that's overlayfs design. It tries to emulate certain things. Dropping whiteout on unlink is another example of operation which is not intuitive.

[rhvgoyal]

Why are we trying to run these test cases on noaccess_t files. We are trying to first test no-context vs context. So a file which does not have "unlink" permission without context, gets permission to unlink with-context. But that should assume that mounter has all the access to the file.

By creating a test where client does not have access as well as mounter can't do the operation on the underlying file is going to complicate things.

I think mounter's tests should be decoupled from client's test. When client is being tested, it should be assumed that mounter can do all all the operations.

And then there should probably be separate tests to make sure if mounter does not have certain access, client can't get those accesses as well and there is no privilege escalation.

Right now we seem to be mixing two things and assuming unlink() does not result in any other operation in overlayfs while trying to emulate it.

[rhvgoyal]

@pcmoore Assuming that mounter is going to need exactly same permission as client I think is a problem. And trying to change overlayfs design for that will just lead to more confusion. That is in what cases should we check mounter's creds and in what cases to bypass it.

A rather simpler model is that give mounter's policy all basic access on underlying files and make client's access restrictive and run the tests.

Assuming that because client does not require getattr permission to unlink a file and hence mounter will not require getattr as well on underlying file I think is going to be problematic. We will run into these kind of situations again and again as overlayfs design evolves.

[rhvgoyal]

For example, android folks are proposing another security model where checks on underlying filesystem also happen with the creds of the caller/client. (And not mounter). There is a good chance that those patches will be merged as well and user gets to choose security model they want. In that case, IIUC, ovl_lookup() will happen with the creds of caller. And getattr() will be called in the context of caller as well. And unlink() will need getattr permission for it to succeed. (As opposed to current needs).

Given overlayfs is stacked filesystem and does certain operations on lower to emulate top level operations, writing policy will be little un-intuitive. I gave another example of already, doing unlink of
upper file might have to drop a whiteout and it might be unintuitive that why unlink requires extra permissions.

[stephensmalley]

Ok, I accept your argument WRT to vfs_getxattr() and getattr permission.
Decoupling mounter vs client tests also seems like a good idea, as long as we retain some testing that mounter cannot use context mounts to gain unauthorized access to lower/upper/work dirs.
WRT keeping the tests working on older kernels, we have historically preserved backward compatibility for the testsuite either through conditional logic within the test code or scripts themselves or by omitting certain tests on older kernels/distros. Plenty of examples of that within the existing tests. Eventually it will need to pass on RHEL for use by the QA folks.

[rhvgoyal]

Ok, here is another PR to fix the issue.

SELinuxProject/selinux-testsuite#40

I got rid of test_72() and test_72_ctx(). IMO, test_70() and test_70_ctx() are doing the job we want to do. That is make sure a file can't be unlinked without context mount but same file can be unlinked with context mount.

And this should work with old kernel as well as new kernel.

test_72() and test_72_ctxt() did not make much sense to me. It was not clear to me what are we testing which test_70() is not already doing.

[pcmoore]

WRT keeping the tests working on older kernels, we have historically preserved backward compatibility for the testsuite either through conditional logic within the test code or scripts themselves or by omitting certain tests on older kernels/distros. Plenty of examples of that within the existing tests. Eventually it will need to pass on RHEL for use by the QA folks.

The test suite focus remains on making sure we are testing the proper behavior on current, upstream kernels. RHEL kernels are such an odd mix of backports and unusual behavior that the test suite often needs special patching to work on a given RHEL release.

Upstream should focus on upstream and let the distros worry about the distros.

[pcmoore]

Resolved in SELinuxProject/selinux-testsuite#40