Bug 218809 - NFS client is able to relabel files when the NFS share is exported as root_squash
Summary: NFS client is able to relabel files when the NFS share is exported as root_sq...
Status: RESOLVED CODE_FIX
Alias: None
Product: File System
Classification: Unclassified
Component: NFS (show other bugs)
Hardware: All Linux
: P3 high
Assignee: Chuck Lever
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-04 23:40 UTC by marek.gresko
Modified: 2024-09-24 18:28 UTC (History)
4 users (show)

See Also:
Kernel Version: 6.8.8
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description marek.gresko 2024-05-04 23:40:48 UTC
Hello,

I have exported home directories on a NFS server with root_squash option.

I am able to relabel the user home directories using fixfiles -v -F relabel from the NFS client as root user. This should not be possible.

I am able to reproduce using kernel-6.8.7-300.fc40.x86_64 and kernel-6.8.8-300.fc40.x86_64 package.

Thanks

Marek
Comment 1 Trond Myklebust 2024-05-04 23:58:38 UTC
I'm assuming that the server is knfsd, so assigning to Chuck.
Comment 2 Chuck Lever 2024-05-05 14:44:30 UTC
Marek, can you reproduce without root_squash (and perhaps with an upstream kernel)?
Comment 3 Chuck Lever 2024-05-05 14:54:55 UTC
Also, "fixfiles relabel" seems to operate only on /tmp. I'm not sure what you mean when you say you are relabeling user home directories. Can you describe how your client is mounting the server, or provide a more narrow reproducer?
Comment 4 marek.gresko 2024-05-05 18:47:19 UTC
Hello Chuck,

I am unsure about expectations on disabling root_squash? I would expect this would be allowed then and probably this would be normal. With root_squash I would expect it to be blocked and it is not. I am pretty sure it was blocked in previous versions.

The fixfiles -v -F relabel operates on whole filesystem.

I have NFS home directories in /home/domain directory and my selinux policy on the server states the file types of user home directories /home/domain/$user are labele user_home_dir_t. The NFS client i not aware of this policy and when you run the same command on the client it relabels the directories to user_home_t.

My experts file look like this:

/home/domain client.fqdn(rw,root_squash,anonuid=65534,anongid=65534,security_label,sec=krb5)

The fstab entry on the client looks like this:

server.fqdn:/home/domain /home/domain nfs4    _netdev,defaults,sec=krb5,soft,intr,x-systemd.automount    0 0

Is this enough information to reproduce? I think the kerberos is not really needed to reproduce. Also the fixfiles is not needed to reproduce. I reproduced it by using restorecon also. I just tried restorecon -v /home/domain/$user on the client and the directory got relabeled. The I run it on the server and it was relabeled back. I would expect permission denied on the client when it tries to do it. The restorecon command is much faster way to reproduce, so probably you could try this.

I am thinking of possible workarounds. Maybe moving the home directories to some directory not included in the clients policy would solve my problems, but the security issue would stay there. I do not think client could be allowed to relabel files on the server when root_squash is used.

Thanks

Marek
Comment 5 marek.gresko 2024-05-05 19:19:14 UTC
Hello,

I made another test.

I created directories /home/domain/Music and /home/domain/Music/test on the server and their labels were user_home_dir_t and user_home_t respectively.

Then I run restorecon -R -v /home/domain/Music on the client and it relabeled the directories to audio_home_t.

Then I run restorecon -R -v /home/domain/Music on the server to get the labels back and changed owner and permissions on the /home/domain/Music directory to apache:apache 0750.

Then I retested the restorecon command. It relabeled the Music directory, and the I received the permission denied on the Music directory, so it could not relabel the contents. Apparently the client is able to relabel any files and directories on the server it has read permissions to. I expect changing to no_root_squash would allow me to relabel the contents of the Music directory in the second test, because as a root on the client I would get the permissions to enter the Music directory.

It seems to be clear some security test got lost somewhere in time when changing the selinux labels.

Marek
Comment 6 Chuck Lever 2024-05-05 19:20:12 UTC
> I am pretty sure it was blocked in previous versions.

It would be very useful to know the kernel version where this might have changed. If I had to take a stab, I would say v6.0 might have broken this.

> The fixfiles -v -F relabel operates on whole filesystem.

Thanks for the restorecon recipe. On my system, fixfiles seems to touch only /tmp.

Unfortunately I don't know much about SELinux. It would be good to have an expert join us here, if you know of one. Or, since you are reporting the problem against Fedora kernels, you could file a bug with them and let them diagnose it. If there is an actual NFSD bug, it will get back to me.
Comment 7 Chuck Lever 2024-05-05 19:22:25 UTC
> It seems to be clear some security test got lost somewhere in time when
> changing the selinux labels.

The usual first step is to track down where the behavior changed using "git bisect".
Comment 8 marek.gresko 2024-05-05 19:33:02 UTC
I think the bug is not so old. I di not notice the bug in the Fedora 39, but cannot confirm it was not there. I may be just did not notice because I did not get selinux denials because of that. I am going to file a bug in Fedora's bugzilla. It was a good point.

Marek
Comment 9 bcodding 2024-08-22 17:21:01 UTC
We just noticed this on https://bugzilla.redhat.com/show_bug.cgi?id=2279179, coming back here to work on it.

I don't think this is a regression, but the problem's always been there.  The reason is knfsd bypasses normal permission checking to set the label (I believe because we don't want the server's local policy getting in the way).  So, instead of having vfs_setxattr() return a permission error while fsuid is squashed, it just succeeds.

Scott Mayhew suggests just having the server add MAY_WRITE for setting the security label, so the inode_permission() check would gate the change:

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 29b1f3613800..aabc51201029 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -517,6 +517,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
                        accmode |= NFSD_MAY_WRITE;
        }
 
+       if (attr->na_seclabel && attr->na_seclabel->len)
+               accmode |= NFSD_MAY_WRITE;
+
        /* Callers that do fh_verify should do the fh_want_write: */
        get_write_count = !fhp->fh_dentry;
 

... 

Another option might be to just use the normal vfs_setxattr after fh_verify(), but I don't think we can acquire the correct process contexts (yet).   I'll ask Scott to send his suggestion to the list, and or punt it along, or look for feedback here.
Comment 10 Chuck Lever 2024-08-22 18:23:13 UTC
> I don't think this is a regression, but the problem's always been there.

The reporter suggests it appeared after an upgrade, but it's also possible that a recent patch unobscured a latent bug. Upstream we still might need to nail this down because we will need appropriate Fixes: and Cc: stable information in the fix. So I would still like to get bisect results, if at all possible.


> I'll ask Scott to send his suggestion to the list, and or punt it along,
> or look for feedback here.

In particular, review from the SELinux community would help us choose one of the alternatives you have proposed here.

Thanks for looking into it.
Comment 11 Scott Mayhew 2024-08-22 18:45:48 UTC
(In reply to Chuck Lever from comment #10)
> > I don't think this is a regression, but the problem's always been there.
> 
> The reporter suggests it appeared after an upgrade, but it's also possible
> that a recent patch unobscured a latent bug. Upstream we still might need to
> nail this down because we will need appropriate Fixes: and Cc: stable
> information in the fix. So I would still like to get bisect results, if at
> all possible.

I didn't bisect, but I went back to RHEL7 and the behavior is there too.  
Going all the way back to 1ee65e37e904, the inode_setsecctx hooks have always called  __vfs_setxattr_noperm(), so I think this behavior has always been there.

> 
> 
> > I'll ask Scott to send his suggestion to the list, and or punt it along,
> > or look for feedback here.

Unfortunately my suggestion was no good.  It would fix the issue of root being able to set labels on files on root-squashed exports, but it would also make it so users would have to have write access on a file in order to change the label, i.e.

$ chcon -t etc_t /mnt/file
$ chmod 444 /mnt/file
$ ls -lZ /mnt/file
-r--r--r--. 1 smayhew smayhew unconfined_u:object_r:etc_t:s0 0 Aug 22 14:35 /mnt/file
$ chcon -t usr_t /mnt/file
chcon: failed to change context of '/mnt/file' to ‘unconfined_u:object_r:usr_t:s0’: Permission denied

(previously the last line would succeed, but w/ my patch it would fail)

> 
> In particular, review from the SELinux community would help us choose one of
> the alternatives you have proposed here.
> 
> Thanks for looking into it.
Comment 12 marek.gresko 2024-08-22 19:43:09 UTC
Hello,

I cannot confirm the behavior was not there before, but was latent as you suggested. Either the labels were for the chance the same, so it was not visible or there was some configuration to skip remote directories for relabeling? Not sure.

Thanks for looking into the problem and finding the root cause. I hope the proper solution will be found soon.

Marek
Comment 13 Chuck Lever 2024-08-28 15:08:56 UTC
Bisecting seems unlikely, so let's assume the problem has been there since labeled NFS support was merged. I'll use "Cc: stable" in the fix we choose.
Comment 14 Scott Mayhew 2024-08-28 19:53:31 UTC
I posted one possible fix:
https://lore.kernel.org/linux-nfs/20240828195129.223395-1-smayhew@redhat.com/T/#t
Comment 15 Chuck Lever 2024-09-03 15:00:52 UTC
Paul Moore took Scott's above fix for v6.12.
Comment 16 marek.gresko 2024-09-24 18:28:59 UTC
Hello,

I am not able to reproduce with kernel 6.10.10-200.fc40.x86_64. Was the fix backported there from version 6.11?

Thanks

Marek

Note You need to log in before you can comment on or make changes to this bug.