Test suite for running SELinux ontop of overlayfs.

[rhvgoyal]

cc @rhvgoyal

[rhvgoyal]

cc @stephensmalley

[stephensmalley]

Can we squash these into a single commit? Also, before merging this to master, we'll need to wrap it with a test of kernel version so that it doesn't trigger failures on older kernels.

[stephensmalley]

We get some error output from the test scripts during the run that should be redirected; just use 2>&1 as appropriate and it will be hidden when running the testsuite via make test but show up if we run the script by hand. overlay/append: line 2: overlay/container1/merged/null_noaccess: Permission denied overlay/append: line 2: overlay/container1/merged/null_noaccess: Permission denied

[stephensmalley]

Can you fix the commit message to include the overall description; when you squashed them I think you only kept the first one.

[rhatdan]

@stephensmalley Almost all of the commit messages were just fixes for the original two messages. I tried to rewrite it to describe the types. I can expand on it if you want.

I tried to remove all of the relabel permissions, turns out then when mounter mounts with context=, it needs relabelfrom on fs_t, and it needs relabelfrom/relabelto on test_overlay_files_rwx_t.

I fixed the error messages and removed a bogus relabel command for the client.

[stephensmalley]

The current commit message says "SELinux policy for overlayfs tests" and only describes the policy but the actual commit is adding the tests too. Don't remember if you previously had a commit message describing the tests. The whiteout type description in the commit message seems to be wrong - if it were for the whiteout device nodes, then the client wouldn't need access to it and would not use it for the regular files named "whiteout" that it creates. mounter only needs relabelfrom/relabelto class filesystem, not file, so that part of the policy is fine. client does need relabelfrom/relabelto IIUC because you have tests that do things like this: sub test_35 { print "Attempting to chcon writefile, should succeed.\n"; $result = system ("runcon -t test_overlay_client_t -l s0:c10,c20 chcon -t test_overlay_files_rwx_t $basedir/container1/merged/writefile >/dev/null"); ok($result eq 0); return; } The fact that the test passes without allowing that is confusing to me and worries me that something is broken in your test script.

[stephensmalley]

Oh, and we do not want to do > /dev/null or 2>/dev/null. We want error messages to show up if running the test script by hand. When running make test, the perl test harness will silence anything sent to stdout and only show what goes to stderr, so as long as you have 2>&1, it will silence the output under make test but leave it available for manual inspection when debugging failures.

[rhatdan]

We are only doing the 2>&1 when we are running expected failures. If the script is not expected to fail we allow stederr to go to the screen. If you run the tests manually and see lots of permission denied, people think it is failing.

[rhatdan]

My tests are passing without client_t having any relabel perms.

[stephensmalley]

Ok, so ask yourself: how can the following tests be passing if the client does not have any relabel permissions: Attempting to chcon noaccessfile, should fail. ok 32 Attempting to chcon readfile, should fail. ok 33 Attempting to chcon writefile, should succeed. ok 34

The first two make sense because they expect to fail anyway, but that implies that you are getting denied on TE and I assume you wanted to test MCS? The last one is the puzzler. Either a bug in the test, the policy, or the kernel somewhere...

[rhatdan]

I think the chcon was covering up and not actually relabeling if the label was correct. I changed the tests to change the level of the file as well as the type, and now I am seeing the mounter and the client need to relabelto/from for rwx_t. I left it as a separate patch so you could review it easier.

[stephensmalley]

So that explains why the last one was passing, but then I'd expect the first two to have triggered test failures because chcon would have done nothing, the result would have been zero, and they are supposed to have a non-zero result. So I'm still confused...

[rhatdan]

No the first two the types are different so chcon would attempt to change the labels, triggering the relabel check. test_33 the file is labeled test_overlay_files_noaccess_t test_34 file is labeled test_overlay_files_ro_t test_35 file is labeled test_overlay_files_rwx_t

[stephensmalley]

Ok, then I think we just need to: 1) Add a test of kernel version to either the tests/Makefile or to overlay/test to skip these tests entirely if the kernel doesn't include the overlay selinux support (I guess that will be >= 4.9.0 for upstream kernels; not sure if there is a shell equivalent to strversmp like we use in tests/ioctl/test_noioctl.c to adapt the test based on kernel version), 2) Squash the commits into one.

But might want to wait and see what Paul says.

[rhatdan]

@stephensmalley PTAL I merged into a separate pull request and added a check to test to make sure you are running a 4.9 kernel. I had to split out the tests into overlaytest since putting the version check into the same script was causing errors.

[stephensmalley]

Doesn't seem to work from 'make test', see output below. You need something akin to what we have in tests/inet_socket/test, where it does plan skip_all if it isn't a supported kernel, so that the test harness knows that the tests were intentionally skipped. Also it seems like the kernel version comparison doesn't work because it isn't strictly numeric? Don't know if there is an equivalent to strverscmp() in C that we can readily leverage in perl or sh.

overlay/test ............ Argument "4.6.4-301.fc24.x86_64" isn't numeric in numeric lt (<) at overlay/test line 5. overlay/test ............ No subtests run

Test Summary Report

overlay/test (Wstat: 0 Tests: 0 Failed: 0) Parse errors: No plan found in TAP output Files=44, Tests=306, 40 wallclock secs ( 0.15 usr 0.03 sys + 0.45 cusr 0.63 csys = 1.26 CPU) Result: FAIL Failed 1/44 test programs. 0/306 subtests failed.

[rhatdan]

How does this look, basically I am grepping though /proc/kallsyms for security_inode_copy_up and if this is there then I figure overlay has SELinux support.

[rhatdan]

@rhvgoyal PTAL

[rhatdan]

@stephensmalley PTAL

[stephensmalley]

Looks like you accidentally changed inet_socket/test too?

[rhatdan]

Fixed all of the issues.

[stephensmalley]

+1, LGTM, all tests pass on rawhide and tests are properly skipped on fedora 24. Will wait for Paul to ACK before merging.

[stephensmalley]

(to clarify, passes on rawhide with Paul's copr kernels)

[rhatdan]

@pcmoore PTAL

[stephensmalley]

I went ahead and merged. Passes for me on rawhide, skips the test properly on f24. Paul, if you have an issue with it, let me know.

[rhvgoyal]

:+1: :+1: :+1:

[pcmoore]

Sorry for the late reply, I've been in almost a constant state of travel since the last week of July.

I just ran the tests that Stephen merged against 4.8.0-0.rc4.git2.1.1.secnext.fc26.x86_64 and everything went okay (thanks again everyone!) but I noticed a few of the following in dmesg:

SELinux: inode_doinit_with_dentry:  getxattr returned 13 for dev=overlay ino=26253

I'm guessing that these are due to some negative tests and are to be expected. However, how often are we likely to see this in the kernel log? I ask because I'm sure that I'm going to see at least one problem report about this in the future. How critical is this message @rhvgoyal @rhatdan @stephensmalley ? Can we squelch it?

[stephensmalley]

I assume these correspond to these denials triggered by the tests: avc: denied { getattr } for pid=3165 comm="touch" name="noaccessfile" dev="dm-0" ino=1434256 scontext=unconfined_u:unconfined_r:test_overlay_mounter_t:s0-s0:c0.c1023 tcontext=unconfined_u:object_r:test_overlay_files_noaccess_t:s0 tclass=file permissive=0 When the mounter lacks getattr permission to the underlying file, then SELinux won't be able to fetch the inode label and we'll get those errors. We could probably reduce that to a KERN_DEBUG or something.

[stephensmalley]

Actually, should we be allowing getattr in the test policy for the mounter? Otherwise, we might be getting denied for the wrong reason? With regard to your original question though, this error is unlikely to occur in real policies because the mounter will generally be allowed getattr to the underlying inodes...

[pcmoore]

Actually, should we be allowing getattr in the test policy for the mounter? Otherwise, we might be getting denied for the wrong reason?

@rhatdan ?

With regard to your original question though, this error is unlikely to occur in real policies because the mounter will generally be allowed getattr to the underlying inodes...

Okay, I'll buy that explanation. Let's leave the kernel message as-is for right now, if we get complaints we can demote it to KERN_DEBUG.

[stephensmalley]

Ok, I see - if I allow mounter to getattr on the noaccess file, then we'll still pass the no-context-mount case but we'll fail the context mount case. I guess it is a question of what it is that you intend to test here. For the noaccess ones, if we don't allow the mounter to getattr, then the client check will always be against the unlabeled context.

[rhatdan]

The type of the file was noaccess_file_t, so preventing access for the mounter. We could add another type which the mounter has access to but the client does not. But I think we really cover this with some of the readonly types which get copied up when they are mounted with the context mount.

[stephensmalley]

AFAICT, both mounter and client have read-only permissions to test_overlay_files_ro_t, and rwx permissions to test_overlay_files_rwx_t, and no acces to test_overlay_files_noaccess_t. So you are never independently testing preventing either one of them from accessing the file when the other is allowed, unless I missed it. For the current noaccess test, you are only testing that the mounter cannot access test_overlay_files_noaccess_t; the client is just failing on unlabeled_t due to the getattr failure.

[rhvgoyal]

we discussed this warning during upstream posting and nobody had objected to the idea of converting this to a KERN-DEBUG instead of KERN_WARN.

https://lkml.org/lkml/2016/7/7/660

So if real configurations start hitting it and they complain, we will have to change this in kernel.

[rhatdan]

@stephensmalley I am checking the noaccess_t access on the client in the NON context= checks.

[stephensmalley]

@rhatdan If the mounter lacks getattr permission, then the overlay inode is assigned the unlabeled context, and the check is then between the client and the unlabeled type, not the client and the noaccess type. Look at the AVC denials that occur during the test. I do see one getattr denial between the client and the noaccess type (which I don't quite understand how that can happen), but the rest are between the client and the unlabeled type or between the mounter and the noaccess type.

[rhatdan]

I believe the mounter only comes in and does a check if there is a context=LABEL mount, otherwize the check is between the client and the underlying lower label. @rhvgoyal correct?

[stephensmalley]

No, operations on the underlying filesystem are always done in the mounter's context now. And operations on the overlay inode are done in the current/client's context. So the checks are between the client and the overlay (which will either be the context mount label if using context= OR the label copied from the lower file OR unlabeled if the mounter could not getattr the lower file), and between the mounter and the lower file.

[rhvgoyal]

@stephensmalley right. Mounter's permission checks are always done on underlying inode (lower/upper). And client/taks's credentials are checked against overlay inode.

[rhvgoyal]

@stephensmalley You mentioned that you noticed one one getattr denial between client and noaccess type. I ran tests and I did can't find anything such denial. Are you able to reproduce it.

[stephensmalley]

I see it every time I run the selinux-testsuite. With kernel-4.8.0-0.rc4.git0.2.1.secnext.fc26.x86_64 (Paul's kernel). I'll attach my AVCs. avc.txt

[stephensmalley]

This is the only one between the client and noaccess_t: type=AVC msg=audit(1472824544.086:507): avc: denied { getattr } for pid=3336 comm="touch" path="/noaccessfile" dev="dm-0" ino=1434266 scontext=unconfined_u:unconfined_r:test_overlay_client_t:s0:c10,c20 tcontext=unconfined_u:object_r:test_overlay_files_noaccess_t:s0 tclass=file permissive=0

[stephensmalley]

It also manifests with the latest rawhide kernel (not Paul's): 4.8.0-0.rc4.git1.1.fc26.x86_64 I guess the overlayfs selinux support is in that kernel too since the tests run and pass.

[rhvgoyal]

This one is really strange. I have cloned paul's git tree and built a kernel from that and I did not notice it there. Something is strange. This avc should not be there.

This suggests we have a path somewhere where client's credentials are checked against the underlying inode label and AFAIK, that should not be happening.

[rhatdan]

@stephensmalley Yes you need to follow me on twitter.

https://twitter.com/rhatdan/status/771346472533168128?lang=en

[rhatdan]

Now that we have this in Rawhide and Fedora 25, next step is to get it into Fedora 24 and then RHEL7.

[rhvgoyal]

Looks like I can reproduce it too this time. Not sure what happened last time. Let me try to get to bottom of it.

type=AVC msg=audit(1472826175.533:437): avc: denied { getattr } for pid=3363 comm="touch" path="/noaccessfile" dev="dm-0" ino=2508742 scontext=unconfined_u:unconfined_r:test_overlay_client_t:s0:c10,c20 tcontext=unconfined_u:object_r:test_overlay_files_noaccess_t:s0 tclass=file permissive=0

[rhvgoyal]

This seems to happen when test_2() runs with context= mount.

[rhatdan]

Just added https://github.com/SELinuxProject/selinux-testsuite/pull/4 And I think we might have a bug.