| Message ID | 20200220153234.152426-1-richard_c_haines@btinternet.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | selinux: Add xfs quota command types | expand |
On Thu, Feb 20, 2020 at 03:32:33PM +0000, Richard Haines wrote: > Added these quota command types for SELinux checks on XFS quotas. I picked > those I thought useful. The selinux-testsuite will have tests for these > permission checks on XFS. > > One point to note: XFS does not call dquot_quota_on() to trigger > security_quota_on(), therefore the 'file quotaon' permission is not tested > for SELinux Is that a feature or a bug? :) (It sounds like a bug to me, but let's see if anyone complains...) --D > Richard Haines (1): > selinux: Add xfs quota command types > > security/selinux/hooks.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > -- > 2.24.1 >
On Thu, Feb 20, 2020 at 07:57:31AM -0800, Darrick J. Wong wrote: > On Thu, Feb 20, 2020 at 03:32:33PM +0000, Richard Haines wrote: > > Added these quota command types for SELinux checks on XFS quotas. I picked > > those I thought useful. The selinux-testsuite will have tests for these > > permission checks on XFS. > > > > One point to note: XFS does not call dquot_quota_on() to trigger > > security_quota_on(), therefore the 'file quotaon' permission is not tested > > for SELinux > > Is that a feature or a bug? :) > > (It sounds like a bug to me, but let's see if anyone complains...) The dquot_* routines are not generic quota code, but a specific implementation used by most non-XFS file systems. So if there is a bug it is that the security call is not in the generic dispatch code.
On Thu, Feb 20, 2020 at 11:06:10AM -0500, Stephen Smalley wrote: > > The dquot_* routines are not generic quota code, but a specific > > implementation used by most non-XFS file systems. So if there is a bug > > it is that the security call is not in the generic dispatch code. > > Hmm...any reason the security hook call couldn't be taken to > quota_quotaon()? I haven't touched the quota code for a while, but yes, the existing calls should move to the quota_* routines in fs/quota/quota.c. Note that you still need to add checks, e.g. for Q_XSETQLIM.
On Thu, Feb 20, 2020 at 11:08 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Feb 20, 2020 at 11:06:10AM -0500, Stephen Smalley wrote: > > > The dquot_* routines are not generic quota code, but a specific > > > implementation used by most non-XFS file systems. So if there is a bug > > > it is that the security call is not in the generic dispatch code. > > > > Hmm...any reason the security hook call couldn't be taken to > > quota_quotaon()? > > I haven't touched the quota code for a while, but yes, the existing > calls should move to the quota_* routines in fs/quota/quota.c. Note > that you still need to add checks, e.g. for Q_XSETQLIM. Who wanted to submit a patch for this? Christoph were you planning on fixing this? If not, Richard, do you want to give it a try?
On Sat, 2020-02-22 at 14:49 -0500, Paul Moore wrote: > On Thu, Feb 20, 2020 at 11:08 AM Christoph Hellwig <hch@infradead.org > > wrote: > > On Thu, Feb 20, 2020 at 11:06:10AM -0500, Stephen Smalley wrote: > > > > The dquot_* routines are not generic quota code, but a specific > > > > implementation used by most non-XFS file systems. So if there > > > > is a bug > > > > it is that the security call is not in the generic dispatch > > > > code. > > > > > > Hmm...any reason the security hook call couldn't be taken to > > > quota_quotaon()? > > > > I haven't touched the quota code for a while, but yes, the existing > > calls should move to the quota_* routines in > > fs/quota/quota.c. Note > > that you still need to add checks, e.g. for Q_XSETQLIM. > > Who wanted to submit a patch for this? Christoph were you planning > on > fixing this? If not, Richard, do you want to give it a try? > I've had a go at this and found I can (almost) get it working. I've attached a sample patch just in case anyone is interested. However if the calls do need to move to fs/quota/quota.c, then I think the xfs team are best placed to do this (I've had my playtime).
On Mon, Feb 24, 2020 at 04:41:17PM +0000, Richard Haines wrote: > On Sat, 2020-02-22 at 14:49 -0500, Paul Moore wrote: > > On Thu, Feb 20, 2020 at 11:08 AM Christoph Hellwig <hch@infradead.org > > > wrote: > > > On Thu, Feb 20, 2020 at 11:06:10AM -0500, Stephen Smalley wrote: > > > > > The dquot_* routines are not generic quota code, but a specific > > > > > implementation used by most non-XFS file systems. So if there > > > > > is a bug > > > > > it is that the security call is not in the generic dispatch > > > > > code. > > > > > > > > Hmm...any reason the security hook call couldn't be taken to > > > > quota_quotaon()? > > > > > > I haven't touched the quota code for a while, but yes, the existing > > > calls should move to the quota_* routines in > > > fs/quota/quota.c. Note > > > that you still need to add checks, e.g. for Q_XSETQLIM. > > > > Who wanted to submit a patch for this? Christoph were you planning > > on > > fixing this? If not, Richard, do you want to give it a try? > > > > I've had a go at this and found I can (almost) get it working. I've > attached a sample patch just in case anyone is interested. Almost? Are you talking about the weird looking dentry? > However if the calls do need to move to fs/quota/quota.c, then I think > the xfs team are best placed to do this (I've had my playtime). > > > diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c > index 38669e827..c02cf9a63 100644 > --- a/fs/xfs/xfs_quotaops.c > +++ b/fs/xfs/xfs_quotaops.c > @@ -14,7 +14,7 @@ > #include "xfs_trans.h" > #include "xfs_icache.h" > #include "xfs_qm.h" > - > +#include <linux/security.h> > > static void > xfs_qm_fill_state( > @@ -161,11 +161,15 @@ xfs_quota_enable( > unsigned int uflags) > { > struct xfs_mount *mp = XFS_M(sb); > + struct dentry *dentry = mp->m_super->s_root; > > if (sb_rdonly(sb)) > return -EROFS; > if (!XFS_IS_QUOTA_RUNNING(mp)) > return -ENOSYS; > + /* Emulates dquot_quota_on() Labeled correctly */ > + if (!security_quota_on(dentry)) > + return -EACCES; > > return xfs_qm_scall_quotaon(mp, xfs_quota_flags(uflags)); > } > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 2094386af..59855d060 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -39,6 +39,7 @@ > #include <linux/magic.h> > #include <linux/fs_context.h> > #include <linux/fs_parser.h> > +#include <linux/security.h> > > static const struct super_operations xfs_super_operations; > > @@ -1507,6 +1508,22 @@ xfs_fc_fill_super( > goto out_unmount; > } > > + /* > + * Emulates dquot_quota_on_mount() and works, however the dentry > + * is unlabeled: > + * allow test_filesystem_t unlabeled_t:dir { quotaon }; > + */ > + struct xfs_mount *mpx = XFS_M(sb); > + struct dentry *dentry = mpx->m_super->s_root; > + if (XFS_IS_QUOTA_RUNNING(mp)) { > + error = security_quota_on(dentry); Er... I'm not sure if this is doing what you really want it to? Mostly because I don't know what the quota security hooks are supposed to activate against? :) The other three filesystems (ext4/f2fs/reiserfs) keep their quota files linked off the root directory, and dquot_quota_on_mount looks up the dentry for a root directory quota file and feeds that into security_quota_on. This patch calls it on XFS' root directory itself (quota inodes are not linked in the directory tree at all on xfs), which I guess is the best you can do, but might not be what the security hooks is expecting. <shrug> Apologies for my unfamiliarity. --D > + if (error) { > + dput(sb->s_root); > + sb->s_root = NULL; > + goto out_unmount; > + } > + } > + > return 0; > > out_filestream_unmount: > diff --git a/security/security.c b/security/security.c > index db7b574c9..ac0cc9872 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -770,6 +770,7 @@ int security_quota_on(struct dentry *dentry) > { > return call_int_hook(quota_on, 0, dentry); > } > +EXPORT_SYMBOL(security_quota_on); > > int security_syslog(int type) > {
On Mon, 2020-02-24 at 20:13 -0800, Darrick J. Wong wrote: > On Mon, Feb 24, 2020 at 04:41:17PM +0000, Richard Haines wrote: > > On Sat, 2020-02-22 at 14:49 -0500, Paul Moore wrote: > > > On Thu, Feb 20, 2020 at 11:08 AM Christoph Hellwig < > > > hch@infradead.org > > > > wrote: > > > > On Thu, Feb 20, 2020 at 11:06:10AM -0500, Stephen Smalley > > > > wrote: > > > > > > The dquot_* routines are not generic quota code, but a > > > > > > specific > > > > > > implementation used by most non-XFS file systems. So if > > > > > > there > > > > > > is a bug > > > > > > it is that the security call is not in the generic dispatch > > > > > > code. > > > > > > > > > > Hmm...any reason the security hook call couldn't be taken to > > > > > quota_quotaon()? > > > > > > > > I haven't touched the quota code for a while, but yes, the > > > > existing > > > > calls should move to the quota_* routines in > > > > fs/quota/quota.c. Note > > > > that you still need to add checks, e.g. for Q_XSETQLIM. > > > > > > Who wanted to submit a patch for this? Christoph were you > > > planning > > > on > > > fixing this? If not, Richard, do you want to give it a try? > > > > > > > I've had a go at this and found I can (almost) get it working. I've > > attached a sample patch just in case anyone is interested. > > Almost? Are you talking about the weird looking dentry? > > > However if the calls do need to move to fs/quota/quota.c, then I > > think > > the xfs team are best placed to do this (I've had my playtime). > > > > > > diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c > > index 38669e827..c02cf9a63 100644 > > --- a/fs/xfs/xfs_quotaops.c > > +++ b/fs/xfs/xfs_quotaops.c > > @@ -14,7 +14,7 @@ > > #include "xfs_trans.h" > > #include "xfs_icache.h" > > #include "xfs_qm.h" > > - > > +#include <linux/security.h> > > > > static void > > xfs_qm_fill_state( > > @@ -161,11 +161,15 @@ xfs_quota_enable( > > unsigned int uflags) > > { > > struct xfs_mount *mp = XFS_M(sb); > > + struct dentry *dentry = mp->m_super->s_root; > > > > if (sb_rdonly(sb)) > > return -EROFS; > > if (!XFS_IS_QUOTA_RUNNING(mp)) > > return -ENOSYS; > > + /* Emulates dquot_quota_on() Labeled correctly */ > > + if (!security_quota_on(dentry)) > > + return -EACCES; > > > > return xfs_qm_scall_quotaon(mp, xfs_quota_flags(uflags)); > > } > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index 2094386af..59855d060 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > > @@ -39,6 +39,7 @@ > > #include <linux/magic.h> > > #include <linux/fs_context.h> > > #include <linux/fs_parser.h> > > +#include <linux/security.h> > > > > static const struct super_operations xfs_super_operations; > > > > @@ -1507,6 +1508,22 @@ xfs_fc_fill_super( > > goto out_unmount; > > } > > > > + /* > > + * Emulates dquot_quota_on_mount() and works, however the > > dentry > > + * is unlabeled: > > + * allow test_filesystem_t unlabeled_t:dir { quotaon }; > > + */ > > + struct xfs_mount *mpx = XFS_M(sb); > > + struct dentry *dentry = mpx->m_super->s_root; > > + if (XFS_IS_QUOTA_RUNNING(mp)) { > > + error = security_quota_on(dentry); > > Er... I'm not sure if this is doing what you really want it > to? Mostly > because I don't know what the quota security hooks are supposed to > activate against? :) > > The other three filesystems (ext4/f2fs/reiserfs) keep their quota > files > linked off the root directory, and dquot_quota_on_mount looks up the > dentry for a root directory quota file and feeds that into > security_quota_on. > > This patch calls it on XFS' root directory itself (quota inodes are > not > linked in the directory tree at all on xfs), which I guess is the > best > you can do, but might not be what the security hooks is expecting. > > <shrug> Apologies for my unfamiliarity. Thanks for your comments. What I was trying to achieve is obtain a dentry for the root directory to call the security hooks with. My reasoning was that as xfs quotas are 'internal' (i.e. not files as in ext4 etc.), then using this as a reference, I could check that the SELinux 'quotaon' permission was set on this to allow quotas to be used (if set as mount options), if not fail the mount. Once mounted with quotas enabled, I could then use quotactl(2) to turn them off/on, get stats etc. providing of course the mount directory also had the SELinux quotamod and quotaget permissions as well (hence the hook in xfs_quota_enable()). As I don't know the xfs code at all, I was just experimenting to test my theory, however the xfs dev team is better placed to resolve this as its been suggested that the calls are moved to fs/quota/quota.c. > > --D > > > + if (error) { > > + dput(sb->s_root); > > + sb->s_root = NULL; > > + goto out_unmount; > > + } > > + } > > + > > return 0; > > > > out_filestream_unmount: > > diff --git a/security/security.c b/security/security.c > > index db7b574c9..ac0cc9872 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -770,6 +770,7 @@ int security_quota_on(struct dentry *dentry) > > { > > return call_int_hook(quota_on, 0, dentry); > > } > > +EXPORT_SYMBOL(security_quota_on); > > > > int security_syslog(int type) > > {
On Tue, Feb 25, 2020 at 05:54:03PM +0000, Richard Haines wrote: > On Mon, 2020-02-24 at 20:13 -0800, Darrick J. Wong wrote: > > On Mon, Feb 24, 2020 at 04:41:17PM +0000, Richard Haines wrote: > > > On Sat, 2020-02-22 at 14:49 -0500, Paul Moore wrote: > > > > On Thu, Feb 20, 2020 at 11:08 AM Christoph Hellwig < > > > > hch@infradead.org > > > > > wrote: > > > > > On Thu, Feb 20, 2020 at 11:06:10AM -0500, Stephen Smalley > > > > > wrote: > > > > > > > The dquot_* routines are not generic quota code, but a > > > > > > > specific > > > > > > > implementation used by most non-XFS file systems. So if > > > > > > > there > > > > > > > is a bug > > > > > > > it is that the security call is not in the generic dispatch > > > > > > > code. > > > > > > > > > > > > Hmm...any reason the security hook call couldn't be taken to > > > > > > quota_quotaon()? > > > > > > > > > > I haven't touched the quota code for a while, but yes, the > > > > > existing > > > > > calls should move to the quota_* routines in > > > > > fs/quota/quota.c. Note > > > > > that you still need to add checks, e.g. for Q_XSETQLIM. > > > > > > > > Who wanted to submit a patch for this? Christoph were you > > > > planning > > > > on > > > > fixing this? If not, Richard, do you want to give it a try? > > > > > > > > > > I've had a go at this and found I can (almost) get it working. I've > > > attached a sample patch just in case anyone is interested. > > > > Almost? Are you talking about the weird looking dentry? > > > > > However if the calls do need to move to fs/quota/quota.c, then I > > > think > > > the xfs team are best placed to do this (I've had my playtime). > > > > > > > > > diff --git a/fs/xfs/xfs_quotaops.c b/fs/xfs/xfs_quotaops.c > > > index 38669e827..c02cf9a63 100644 > > > --- a/fs/xfs/xfs_quotaops.c > > > +++ b/fs/xfs/xfs_quotaops.c > > > @@ -14,7 +14,7 @@ > > > #include "xfs_trans.h" > > > #include "xfs_icache.h" > > > #include "xfs_qm.h" > > > - > > > +#include <linux/security.h> > > > > > > static void > > > xfs_qm_fill_state( > > > @@ -161,11 +161,15 @@ xfs_quota_enable( > > > unsigned int uflags) > > > { > > > struct xfs_mount *mp = XFS_M(sb); > > > + struct dentry *dentry = mp->m_super->s_root; > > > > > > if (sb_rdonly(sb)) > > > return -EROFS; > > > if (!XFS_IS_QUOTA_RUNNING(mp)) > > > return -ENOSYS; > > > + /* Emulates dquot_quota_on() Labeled correctly */ > > > + if (!security_quota_on(dentry)) > > > + return -EACCES; > > > > > > return xfs_qm_scall_quotaon(mp, xfs_quota_flags(uflags)); > > > } > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > > index 2094386af..59855d060 100644 > > > --- a/fs/xfs/xfs_super.c > > > +++ b/fs/xfs/xfs_super.c > > > @@ -39,6 +39,7 @@ > > > #include <linux/magic.h> > > > #include <linux/fs_context.h> > > > #include <linux/fs_parser.h> > > > +#include <linux/security.h> > > > > > > static const struct super_operations xfs_super_operations; > > > > > > @@ -1507,6 +1508,22 @@ xfs_fc_fill_super( > > > goto out_unmount; > > > } > > > > > > + /* > > > + * Emulates dquot_quota_on_mount() and works, however the > > > dentry > > > + * is unlabeled: > > > + * allow test_filesystem_t unlabeled_t:dir { quotaon }; > > > + */ > > > + struct xfs_mount *mpx = XFS_M(sb); > > > + struct dentry *dentry = mpx->m_super->s_root; > > > + if (XFS_IS_QUOTA_RUNNING(mp)) { > > > + error = security_quota_on(dentry); > > > > Er... I'm not sure if this is doing what you really want it > > to? Mostly > > because I don't know what the quota security hooks are supposed to > > activate against? :) > > > > The other three filesystems (ext4/f2fs/reiserfs) keep their quota > > files > > linked off the root directory, and dquot_quota_on_mount looks up the > > dentry for a root directory quota file and feeds that into > > security_quota_on. > > > > This patch calls it on XFS' root directory itself (quota inodes are > > not > > linked in the directory tree at all on xfs), which I guess is the > > best > > you can do, but might not be what the security hooks is expecting. > > > > <shrug> Apologies for my unfamiliarity. > > Thanks for your comments. > What I was trying to achieve is obtain a dentry for the root directory > to call the security hooks with. My reasoning was that as xfs quotas > are 'internal' (i.e. not files as in ext4 etc.), then using this as a > reference, I could check that the SELinux 'quotaon' permission was set > on this to allow quotas to be used (if set as mount options), if not > fail the mount. Hmm, maybe let's back up a step here. What's the purpose of being able to set quota_on privileges on a file? Is it so that sysadmins can designate a particular file as a "quota" file and thereby prevent the kernel from being tricked into loading some other file as the quota data file? I ask this because AFAICT in ext4, the "usrjquota=<path>" mount options are for the old quota system wherein quotas are files that are managed separately from the filesystem. The new ext4 quota system links the quota inode pointers only from the superblock, and it calls dquot_load_quota_inode, which doesn't seem to have a security callout. Seeing as there's no user-visible file for new style quotas, I don't see why you'd need a selinux hook either. I guess it could be generally useful to be able to be able to apply the same quota_on labels to root directories of filesystems that have new style quotas that one can apply to quota files on fses with old style quotas... but (a) I don't see that being the case right now, and (b) if you're trying to /add/ that functionality then yes it ought to be in fs/quota/... and yes xfs will have to play catch up once it's there. Is there already a precedent for setting quota_on labels to the root dir? It's entirely possible that I'm simply unaware of what's going on in fs/quota/ because xfs mostly does its own things wrt quota. --D > Once mounted with quotas enabled, I could then use quotactl(2) to turn > them off/on, get stats etc. providing of course the mount directory > also had the SELinux quotamod and quotaget permissions as well (hence > the hook in xfs_quota_enable()). > > As I don't know the xfs code at all, I was just experimenting to test > my theory, however the xfs dev team is better placed to resolve this as > its been suggested that the calls are moved to fs/quota/quota.c. > > > > > > --D > > > > > + if (error) { > > > + dput(sb->s_root); > > > + sb->s_root = NULL; > > > + goto out_unmount; > > > + } > > > + } > > > + > > > return 0; > > > > > > out_filestream_unmount: > > > diff --git a/security/security.c b/security/security.c > > > index db7b574c9..ac0cc9872 100644 > > > --- a/security/security.c > > > +++ b/security/security.c > > > @@ -770,6 +770,7 @@ int security_quota_on(struct dentry *dentry) > > > { > > > return call_int_hook(quota_on, 0, dentry); > > > } > > > +EXPORT_SYMBOL(security_quota_on); > > > > > > int security_syslog(int type) > > > { >
On Tue, Feb 25, 2020 at 4:36 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > Hmm, maybe let's back up a step here. What's the purpose of being able > to set quota_on privileges on a file? Is it so that sysadmins can > designate a particular file as a "quota" file and thereby prevent the > kernel from being tricked into loading some other file as the quota data > file? Yes. > I ask this because AFAICT in ext4, the "usrjquota=<path>" mount options > are for the old quota system wherein quotas are files that are managed > separately from the filesystem. > > The new ext4 quota system links the quota inode pointers only from the > superblock, and it calls dquot_load_quota_inode, which doesn't seem to > have a security callout. Seeing as there's no user-visible file for new > style quotas, I don't see why you'd need a selinux hook either. > > I guess it could be generally useful to be able to be able to apply the > same quota_on labels to root directories of filesystems that have new > style quotas that one can apply to quota files on fses with old style > quotas... but (a) I don't see that being the case right now, and (b) if > you're trying to /add/ that functionality then yes it ought to be in > fs/quota/... and yes xfs will have to play catch up once it's there. > > Is there already a precedent for setting quota_on labels to the root > dir? It's entirely possible that I'm simply unaware of what's going on > in fs/quota/ because xfs mostly does its own things wrt quota. Ok, in that case I'd say we don't need the hook to be called at all in the cases where the quota inode is private to the superblock and not directly exposed to userspace. So no change required to xfs or fs/quota and we can skip/ignore the test when not using usrquota=...