Normally, deleting a file requires MAY_WRITE access to the parent
directory. With richacls, a file may be deleted with MAY_DELETE_CHILD access
to the parent directory or with MAY_DELETE_SELF access to the file.
To support that, pass the MAY_DELETE_CHILD mask flag to inode_permission()
when checking for delete access inside a directory, and MAY_DELETE_SELF
when checking for delete access to a file itself.
The MAY_DELETE_SELF permission overrides the sticky directory check.
Signed-off-by: Andreas Gruenbacher <[email protected]>
Reviewed-by: J. Bruce Fields <[email protected]>
Reviewed-by: Steve French <steve.french-7I+n7zu2hftEKMMhf/[email protected]>
Reviewed-by: Jeff Layton <[email protected]>
---
fs/namei.c | 20 +++++++++++++-------
include/linux/fs.h | 2 ++
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 7290dea..c8bc9fd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -463,9 +463,9 @@ static int sb_permission(struct super_block *sb, struct inode *inode, int mask)
* this, letting us set arbitrary permissions for filesystem access without
* changing the "normal" UIDs which are used for other things.
*
- * MAY_WRITE must be set in @mask whenever MAY_APPEND, MAY_CREATE_FILE, or
- * MAY_CREATE_DIR are set. That way, file systems that don't support these
- * permissions will check for MAY_WRITE instead.
+ * MAY_WRITE must be set in @mask whenever MAY_APPEND, MAY_CREATE_FILE,
+ * MAY_CREATE_DIR, or MAY_DELETE_CHILD are set. That way, file systems that
+ * don't support these permissions will check for MAY_WRITE instead.
*/
int inode_permission(struct inode *inode, int mask)
{
@@ -2780,14 +2780,20 @@ static int may_delete_or_replace(struct inode *dir, struct dentry *victim,
BUG_ON(victim->d_parent->d_inode != dir);
audit_inode_child(dir, victim, AUDIT_TYPE_CHILD_DELETE);
- error = inode_permission(dir, mask);
+ error = inode_permission(dir, mask | MAY_WRITE | MAY_DELETE_CHILD);
+ if (!error && check_sticky(dir, inode))
+ error = -EPERM;
+ if (error && IS_RICHACL(inode) &&
+ inode_permission(inode, MAY_DELETE_SELF) == 0 &&
+ inode_permission(dir, mask) == 0)
+ error = 0;
if (error)
return error;
if (IS_APPEND(dir))
return -EPERM;
- if (check_sticky(dir, inode) || IS_APPEND(inode) ||
- IS_IMMUTABLE(inode) || IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode))
+ if (IS_APPEND(inode) || IS_IMMUTABLE(inode) ||
+ IS_SWAPFILE(inode) || HAS_UNMAPPED_ID(inode))
return -EPERM;
if (isdir) {
if (!d_is_dir(victim))
@@ -2805,7 +2811,7 @@ static int may_delete_or_replace(struct inode *dir, struct dentry *victim,
static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
{
- return may_delete_or_replace(dir, victim, isdir, MAY_WRITE | MAY_EXEC);
+ return may_delete_or_replace(dir, victim, isdir, MAY_EXEC);
}
static int may_replace(struct inode *dir, struct dentry *victim, bool isdir)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 26455c6..1d6d920 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -86,6 +86,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define MAY_NOT_BLOCK 0x00000080
#define MAY_CREATE_FILE 0x00000100
#define MAY_CREATE_DIR 0x00000200
+#define MAY_DELETE_CHILD 0x00000400
+#define MAY_DELETE_SELF 0x00000800
/*
* flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
--
2.7.4
On Tue, Oct 11, 2016 at 2:50 PM, Andreas Gruenbacher
<[email protected]> wrote:
> Normally, deleting a file requires MAY_WRITE access to the parent
> directory. With richacls, a file may be deleted with MAY_DELETE_CHILD access
> to the parent directory or with MAY_DELETE_SELF access to the file.
>
> To support that, pass the MAY_DELETE_CHILD mask flag to inode_permission()
> when checking for delete access inside a directory, and MAY_DELETE_SELF
> when checking for delete access to a file itself.
>
> The MAY_DELETE_SELF permission overrides the sticky directory check.
And MAY_DELETE_SELF seems totally inappropriate to any kind of rename,
since from the point of view of the inode we are not doing anything at
all. The modifications are all in the parent(s), and that's where the
permission checks need to be.
> @@ -2780,14 +2780,20 @@ static int may_delete_or_replace(struct inode *dir, struct dentry *victim,
> BUG_ON(victim->d_parent->d_inode != dir);
> audit_inode_child(dir, victim, AUDIT_TYPE_CHILD_DELETE);
>
> - error = inode_permission(dir, mask);
> + error = inode_permission(dir, mask | MAY_WRITE | MAY_DELETE_CHILD);
> + if (!error && check_sticky(dir, inode))
> + error = -EPERM;
> + if (error && IS_RICHACL(inode) &&
> + inode_permission(inode, MAY_DELETE_SELF) == 0 &&
> + inode_permission(dir, mask) == 0)
> + error = 0;
Why is MAY_WRITE missing here? Everything not aware of
MAY_DELETE_SELF (e.g. LSMs) will still need MAY_WRITE otherwise this
is going to be a loophole.
Thanks,
Miklos
On Fri, Dec 02, 2016 at 10:57:42AM +0100, Miklos Szeredi wrote:
> On Tue, Oct 11, 2016 at 2:50 PM, Andreas Gruenbacher
> <[email protected]> wrote:
> > Normally, deleting a file requires MAY_WRITE access to the parent
> > directory. With richacls, a file may be deleted with MAY_DELETE_CHILD access
> > to the parent directory or with MAY_DELETE_SELF access to the file.
> >
> > To support that, pass the MAY_DELETE_CHILD mask flag to inode_permission()
> > when checking for delete access inside a directory, and MAY_DELETE_SELF
> > when checking for delete access to a file itself.
> >
> > The MAY_DELETE_SELF permission overrides the sticky directory check.
>
> And MAY_DELETE_SELF seems totally inappropriate to any kind of rename,
> since from the point of view of the inode we are not doing anything at
> all. The modifications are all in the parent(s), and that's where the
> permission checks need to be.
I'm having a hard time finding an authoritative reference here (Samba
people might be able to help), but my understanding is that Windows
gives this a meaning something like "may I delete a link to this file".
(And not even "may I delete the *last* link to this file", which might
also sound more logical.)
--b.
>
> > @@ -2780,14 +2780,20 @@ static int may_delete_or_replace(struct inode *dir, struct dentry *victim,
> > BUG_ON(victim->d_parent->d_inode != dir);
> > audit_inode_child(dir, victim, AUDIT_TYPE_CHILD_DELETE);
> >
> > - error = inode_permission(dir, mask);
> > + error = inode_permission(dir, mask | MAY_WRITE | MAY_DELETE_CHILD);
> > + if (!error && check_sticky(dir, inode))
> > + error = -EPERM;
> > + if (error && IS_RICHACL(inode) &&
> > + inode_permission(inode, MAY_DELETE_SELF) == 0 &&
> > + inode_permission(dir, mask) == 0)
> > + error = 0;
>
> Why is MAY_WRITE missing here? Everything not aware of
> MAY_DELETE_SELF (e.g. LSMs) will still need MAY_WRITE otherwise this
> is going to be a loophole.
>
> Thanks,
> Miklos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 6, 2016 at 10:13 PM, Jeremy Allison <[email protected]> wrote:
> On Tue, Dec 06, 2016 at 03:15:29PM -0500, J. Bruce Fields wrote:
>> On Fri, Dec 02, 2016 at 10:57:42AM +0100, Miklos Szeredi wrote:
>> > On Tue, Oct 11, 2016 at 2:50 PM, Andreas Gruenbacher
>> > <[email protected]> wrote:
>> > > Normally, deleting a file requires MAY_WRITE access to the parent
>> > > directory. With richacls, a file may be deleted with MAY_DELETE_CHILD access
>> > > to the parent directory or with MAY_DELETE_SELF access to the file.
>> > >
>> > > To support that, pass the MAY_DELETE_CHILD mask flag to inode_permission()
>> > > when checking for delete access inside a directory, and MAY_DELETE_SELF
>> > > when checking for delete access to a file itself.
>> > >
>> > > The MAY_DELETE_SELF permission overrides the sticky directory check.
>> >
>> > And MAY_DELETE_SELF seems totally inappropriate to any kind of rename,
>> > since from the point of view of the inode we are not doing anything at
>> > all. The modifications are all in the parent(s), and that's where the
>> > permission checks need to be.
>>
>> I'm having a hard time finding an authoritative reference here (Samba
>> people might be able to help), but my understanding is that Windows
>> gives this a meaning something like "may I delete a link to this file".
>>
>> (And not even "may I delete the *last* link to this file", which might
>> also sound more logical.)
>
> I just did a recent patch here. In Samba we now check for
> SEC_DIR_ADD_FILE/SEC_DIR_ADD_SUBDIR on the target directory
> (depending on if the object being moved is a file or dir).
And MAY_DELETE_SELF as well, for rename? That's really counterintuitive for me.
Thanks,
Miklos
On Tue, Dec 06, 2016 at 03:15:29PM -0500, J. Bruce Fields wrote:
> On Fri, Dec 02, 2016 at 10:57:42AM +0100, Miklos Szeredi wrote:
> > On Tue, Oct 11, 2016 at 2:50 PM, Andreas Gruenbacher
> > <[email protected]> wrote:
> > > Normally, deleting a file requires MAY_WRITE access to the parent
> > > directory. With richacls, a file may be deleted with MAY_DELETE_CHILD access
> > > to the parent directory or with MAY_DELETE_SELF access to the file.
> > >
> > > To support that, pass the MAY_DELETE_CHILD mask flag to inode_permission()
> > > when checking for delete access inside a directory, and MAY_DELETE_SELF
> > > when checking for delete access to a file itself.
> > >
> > > The MAY_DELETE_SELF permission overrides the sticky directory check.
> >
> > And MAY_DELETE_SELF seems totally inappropriate to any kind of rename,
> > since from the point of view of the inode we are not doing anything at
> > all. The modifications are all in the parent(s), and that's where the
> > permission checks need to be.
>
> I'm having a hard time finding an authoritative reference here (Samba
> people might be able to help), but my understanding is that Windows
> gives this a meaning something like "may I delete a link to this file".
>
> (And not even "may I delete the *last* link to this file", which might
> also sound more logical.)
I just did a recent patch here. In Samba we now check for
SEC_DIR_ADD_FILE/SEC_DIR_ADD_SUBDIR on the target directory
(depending on if the object being moved is a file or dir).
On Tue, Dec 06, 2016 at 10:25:22PM +0100, Miklos Szeredi wrote:
> On Tue, Dec 6, 2016 at 10:13 PM, Jeremy Allison <[email protected]> wrote:
> > On Tue, Dec 06, 2016 at 03:15:29PM -0500, J. Bruce Fields wrote:
> >> On Fri, Dec 02, 2016 at 10:57:42AM +0100, Miklos Szeredi wrote:
> >> > On Tue, Oct 11, 2016 at 2:50 PM, Andreas Gruenbacher
> >> > <[email protected]> wrote:
> >> > > Normally, deleting a file requires MAY_WRITE access to the parent
> >> > > directory. With richacls, a file may be deleted with MAY_DELETE_CHILD access
> >> > > to the parent directory or with MAY_DELETE_SELF access to the file.
> >> > >
> >> > > To support that, pass the MAY_DELETE_CHILD mask flag to inode_permission()
> >> > > when checking for delete access inside a directory, and MAY_DELETE_SELF
> >> > > when checking for delete access to a file itself.
> >> > >
> >> > > The MAY_DELETE_SELF permission overrides the sticky directory check.
> >> >
> >> > And MAY_DELETE_SELF seems totally inappropriate to any kind of rename,
> >> > since from the point of view of the inode we are not doing anything at
> >> > all. The modifications are all in the parent(s), and that's where the
> >> > permission checks need to be.
> >>
> >> I'm having a hard time finding an authoritative reference here (Samba
> >> people might be able to help), but my understanding is that Windows
> >> gives this a meaning something like "may I delete a link to this file".
> >>
> >> (And not even "may I delete the *last* link to this file", which might
> >> also sound more logical.)
> >
> > I just did a recent patch here. In Samba we now check for
> > SEC_DIR_ADD_FILE/SEC_DIR_ADD_SUBDIR on the target directory
> > (depending on if the object being moved is a file or dir).
>
> And MAY_DELETE_SELF as well, for rename? That's really counterintuitive for me.
Yeah on the source handle we insist on DELETE_ACCESS|FILE_WRITE_ATTRIBUTES
permissions also.
On Tue, Dec 6, 2016 at 10:25 PM, Miklos Szeredi <[email protected]> wrote:
> On Tue, Dec 6, 2016 at 10:13 PM, Jeremy Allison <[email protected]> wrote:
>> On Tue, Dec 06, 2016 at 03:15:29PM -0500, J. Bruce Fields wrote:
>>> On Fri, Dec 02, 2016 at 10:57:42AM +0100, Miklos Szeredi wrote:
>>> > On Tue, Oct 11, 2016 at 2:50 PM, Andreas Gruenbacher
>>> > <[email protected]> wrote:
>>> > > Normally, deleting a file requires MAY_WRITE access to the parent
>>> > > directory. With richacls, a file may be deleted with MAY_DELETE_CHILD access
>>> > > to the parent directory or with MAY_DELETE_SELF access to the file.
>>> > >
>>> > > To support that, pass the MAY_DELETE_CHILD mask flag to inode_permission()
>>> > > when checking for delete access inside a directory, and MAY_DELETE_SELF
>>> > > when checking for delete access to a file itself.
>>> > >
>>> > > The MAY_DELETE_SELF permission overrides the sticky directory check.
>>> >
>>> > And MAY_DELETE_SELF seems totally inappropriate to any kind of rename,
>>> > since from the point of view of the inode we are not doing anything at
>>> > all. The modifications are all in the parent(s), and that's where the
>>> > permission checks need to be.
>>>
>>> I'm having a hard time finding an authoritative reference here (Samba
>>> people might be able to help), but my understanding is that Windows
>>> gives this a meaning something like "may I delete a link to this file".
>>>
>>> (And not even "may I delete the *last* link to this file", which might
>>> also sound more logical.)
>>
>> I just did a recent patch here. In Samba we now check for
>> SEC_DIR_ADD_FILE/SEC_DIR_ADD_SUBDIR on the target directory
>> (depending on if the object being moved is a file or dir).
>
> And MAY_DELETE_SELF as well, for rename? That's really counterintuitive for me.
Yes, MAY_DELETE_SELF applies to delete as well as rename; otherwise
rename() would behave different from link() + unlink(); when a user
has the appropriate permissions, the result should be the same though.
Thanks,
Andreas
On Fri, Dec 2, 2016 at 10:57 AM, Miklos Szeredi <[email protected]> wrote:
> On Tue, Oct 11, 2016 at 2:50 PM, Andreas Gruenbacher
> <[email protected]> wrote:
>> Normally, deleting a file requires MAY_WRITE access to the parent
>> directory. With richacls, a file may be deleted with MAY_DELETE_CHILD access
>> to the parent directory or with MAY_DELETE_SELF access to the file.
>>
>> To support that, pass the MAY_DELETE_CHILD mask flag to inode_permission()
>> when checking for delete access inside a directory, and MAY_DELETE_SELF
>> when checking for delete access to a file itself.
>>
>> The MAY_DELETE_SELF permission overrides the sticky directory check.
>
> And MAY_DELETE_SELF seems totally inappropriate to any kind of rename,
> since from the point of view of the inode we are not doing anything at
> all. The modifications are all in the parent(s), and that's where the
> permission checks need to be.
>
>> @@ -2780,14 +2780,20 @@ static int may_delete_or_replace(struct inode *dir, struct dentry *victim,
>> BUG_ON(victim->d_parent->d_inode != dir);
>> audit_inode_child(dir, victim, AUDIT_TYPE_CHILD_DELETE);
>>
>> - error = inode_permission(dir, mask);
>> + error = inode_permission(dir, mask | MAY_WRITE | MAY_DELETE_CHILD);
>> + if (!error && check_sticky(dir, inode))
>> + error = -EPERM;
>> + if (error && IS_RICHACL(inode) &&
>> + inode_permission(inode, MAY_DELETE_SELF) == 0 &&
>> + inode_permission(dir, mask) == 0)
>> + error = 0;
>
> Why is MAY_WRITE missing here? Everything not aware of
> MAY_DELETE_SELF (e.g. LSMs) will still need MAY_WRITE otherwise this
> is going to be a loophole.
Hmm, this has indeed slipped me. Should be fixed in the version I've
just posted.
Many thanks for the review.
Andreas