* (no subject)
@ 2022-02-13 22:40 Ronnie Sahlberg
2022-02-13 22:40 ` [PATCH] cifs: modefromsids must add an ACE for authenticated users Ronnie Sahlberg
2022-02-14 7:52 ` ronnie sahlberg
0 siblings, 2 replies; 6+ messages in thread
From: Ronnie Sahlberg @ 2022-02-13 22:40 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Steve, List,
Here is a small patch htat fixes an issue with modefromsid where
it would strip off and remove all the ACEs that grants us access to the file.
It fixes this by restoring the "allow AuthenticatedUsers access" ACE that is stripped in
set_chmod_dacl():
/* If it's any one of the ACE we're replacing, skip! */
if (((compare_sids(&pntace->sid, &sid_unix_NFS_mode) == 0) ||
(compare_sids(&pntace->sid, pownersid) == 0) ||
(compare_sids(&pntace->sid, pgrpsid) == 0) ||
(compare_sids(&pntace->sid, &sid_everyone) == 0) ||
(compare_sids(&pntace->sid, &sid_authusers) == 0))) {
goto next_ace;
}
This part is confusing since form many of these cases we are NOT replacing
all these ACEs but only some of them but the code unconditionally removes
all of them, contrary to what the comment suggests.
I think some of my confusion here is that afaik we don't have good documentation
of how modefromsid, and idsfromsid, are supposed to work, what the
restrictions are or the expected semantics.
We need to document both modefromsid and idsfromsid and what the expected
semantics are for when either of them or both of them are enabled.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cifs: modefromsids must add an ACE for authenticated users
2022-02-13 22:40 Ronnie Sahlberg
@ 2022-02-13 22:40 ` Ronnie Sahlberg
2022-02-13 23:46 ` Steve French
2022-02-14 0:13 ` Steve French
2022-02-14 7:52 ` ronnie sahlberg
1 sibling, 2 replies; 6+ messages in thread
From: Ronnie Sahlberg @ 2022-02-13 22:40 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Ronnie Sahlberg
When we create a file with modefromsids we set an ACL that
has one ACE for the magic modefromsid as well as a second ACE that
grants full access to all authenticated users.
When later we chante the mode on the file we strip away this, and other,
ACE for authenticated users in set_chmod_dacl() and then just add back/update
the modefromsid ACE.
Thus leaving the file with a single ACE that is for the mode and no ACE
to grant any user any rights to access the file.
Fix this by always adding back also the modefromsid ACE so that we do not
drop the rights to access the file.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/cifsacl.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index ee3aab3dd4ac..40cda87ce384 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -949,6 +949,9 @@ static void populate_new_aces(char *nacl_base,
pnntace = (struct cifs_ace *) (nacl_base + nsize);
nsize += setup_special_mode_ACE(pnntace, nmode);
num_aces++;
+ pnntace = (struct cifs_ace *) (nacl_base + nsize);
+ nsize += setup_authusers_ACE(pnntace);
+ num_aces++;
goto set_size;
}
@@ -1613,7 +1616,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
nsecdesclen = secdesclen;
if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
if (mode_from_sid)
- nsecdesclen += sizeof(struct cifs_ace);
+ nsecdesclen += 2 * sizeof(struct cifs_ace);
else /* cifsacl */
nsecdesclen += 5 * sizeof(struct cifs_ace);
} else { /* chown */
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: modefromsids must add an ACE for authenticated users
2022-02-13 22:40 ` [PATCH] cifs: modefromsids must add an ACE for authenticated users Ronnie Sahlberg
@ 2022-02-13 23:46 ` Steve French
2022-02-14 13:02 ` Shyam Prasad N
2022-02-14 0:13 ` Steve French
1 sibling, 1 reply; 6+ messages in thread
From: Steve French @ 2022-02-13 23:46 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs
Should I add:
cc:Stable # 5.12+
Thoughts?
On Sun, Feb 13, 2022 at 4:41 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> When we create a file with modefromsids we set an ACL that
> has one ACE for the magic modefromsid as well as a second ACE that
> grants full access to all authenticated users.
>
> When later we chante the mode on the file we strip away this, and other,
> ACE for authenticated users in set_chmod_dacl() and then just add back/update
> the modefromsid ACE.
> Thus leaving the file with a single ACE that is for the mode and no ACE
> to grant any user any rights to access the file.
> Fix this by always adding back also the modefromsid ACE so that we do not
> drop the rights to access the file.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/cifsacl.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index ee3aab3dd4ac..40cda87ce384 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -949,6 +949,9 @@ static void populate_new_aces(char *nacl_base,
> pnntace = (struct cifs_ace *) (nacl_base + nsize);
> nsize += setup_special_mode_ACE(pnntace, nmode);
> num_aces++;
> + pnntace = (struct cifs_ace *) (nacl_base + nsize);
> + nsize += setup_authusers_ACE(pnntace);
> + num_aces++;
> goto set_size;
> }
>
> @@ -1613,7 +1616,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
> nsecdesclen = secdesclen;
> if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
> if (mode_from_sid)
> - nsecdesclen += sizeof(struct cifs_ace);
> + nsecdesclen += 2 * sizeof(struct cifs_ace);
> else /* cifsacl */
> nsecdesclen += 5 * sizeof(struct cifs_ace);
> } else { /* chown */
> --
> 2.30.2
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: modefromsids must add an ACE for authenticated users
2022-02-13 22:40 ` [PATCH] cifs: modefromsids must add an ACE for authenticated users Ronnie Sahlberg
2022-02-13 23:46 ` Steve French
@ 2022-02-14 0:13 ` Steve French
1 sibling, 0 replies; 6+ messages in thread
From: Steve French @ 2022-02-14 0:13 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs
merged into cifs-2.6.git for-next pending review and more testing
Ronnie,
Maybe we should add a small test for create file with modefromsid,
chmod the file, and then getcifsacl?
On Sun, Feb 13, 2022 at 4:41 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> When we create a file with modefromsids we set an ACL that
> has one ACE for the magic modefromsid as well as a second ACE that
> grants full access to all authenticated users.
>
> When later we chante the mode on the file we strip away this, and other,
> ACE for authenticated users in set_chmod_dacl() and then just add back/update
> the modefromsid ACE.
> Thus leaving the file with a single ACE that is for the mode and no ACE
> to grant any user any rights to access the file.
> Fix this by always adding back also the modefromsid ACE so that we do not
> drop the rights to access the file.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> fs/cifs/cifsacl.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index ee3aab3dd4ac..40cda87ce384 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -949,6 +949,9 @@ static void populate_new_aces(char *nacl_base,
> pnntace = (struct cifs_ace *) (nacl_base + nsize);
> nsize += setup_special_mode_ACE(pnntace, nmode);
> num_aces++;
> + pnntace = (struct cifs_ace *) (nacl_base + nsize);
> + nsize += setup_authusers_ACE(pnntace);
> + num_aces++;
> goto set_size;
> }
>
> @@ -1613,7 +1616,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
> nsecdesclen = secdesclen;
> if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
> if (mode_from_sid)
> - nsecdesclen += sizeof(struct cifs_ace);
> + nsecdesclen += 2 * sizeof(struct cifs_ace);
> else /* cifsacl */
> nsecdesclen += 5 * sizeof(struct cifs_ace);
> } else { /* chown */
> --
> 2.30.2
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:
2022-02-13 22:40 Ronnie Sahlberg
2022-02-13 22:40 ` [PATCH] cifs: modefromsids must add an ACE for authenticated users Ronnie Sahlberg
@ 2022-02-14 7:52 ` ronnie sahlberg
1 sibling, 0 replies; 6+ messages in thread
From: ronnie sahlberg @ 2022-02-14 7:52 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French
Steve,
I have added a test to the buildbot to verify the fix.
I.e. two ACEs when file are created, one for mode and one for AuthenticatedUsers
and that after chmod we still have two ACEs but the one for mode has
been updated.
The test is cifs/107
and it can also show how we can now modify the mountoptions we need on
a test by test
basis by using -o remount, ... wooohooo new mount api :-)
On Mon, Feb 14, 2022 at 9:47 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> Steve, List,
>
> Here is a small patch htat fixes an issue with modefromsid where
> it would strip off and remove all the ACEs that grants us access to the file.
> It fixes this by restoring the "allow AuthenticatedUsers access" ACE that is stripped in
>
> set_chmod_dacl():
> /* If it's any one of the ACE we're replacing, skip! */
> if (((compare_sids(&pntace->sid, &sid_unix_NFS_mode) == 0) ||
> (compare_sids(&pntace->sid, pownersid) == 0) ||
> (compare_sids(&pntace->sid, pgrpsid) == 0) ||
> (compare_sids(&pntace->sid, &sid_everyone) == 0) ||
> (compare_sids(&pntace->sid, &sid_authusers) == 0))) {
> goto next_ace;
> }
>
> This part is confusing since form many of these cases we are NOT replacing
> all these ACEs but only some of them but the code unconditionally removes
> all of them, contrary to what the comment suggests.
>
> I think some of my confusion here is that afaik we don't have good documentation
> of how modefromsid, and idsfromsid, are supposed to work, what the
> restrictions are or the expected semantics.
> We need to document both modefromsid and idsfromsid and what the expected
> semantics are for when either of them or both of them are enabled.
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: modefromsids must add an ACE for authenticated users
2022-02-13 23:46 ` Steve French
@ 2022-02-14 13:02 ` Shyam Prasad N
0 siblings, 0 replies; 6+ messages in thread
From: Shyam Prasad N @ 2022-02-14 13:02 UTC (permalink / raw)
To: Steve French; +Cc: Ronnie Sahlberg, linux-cifs
On Mon, Feb 14, 2022 at 3:45 PM Steve French <smfrench@gmail.com> wrote:
>
> Should I add:
> cc:Stable # 5.12+
>
> Thoughts?
>
> On Sun, Feb 13, 2022 at 4:41 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> >
> > When we create a file with modefromsids we set an ACL that
> > has one ACE for the magic modefromsid as well as a second ACE that
> > grants full access to all authenticated users.
> >
> > When later we chante the mode on the file we strip away this, and other,
> > ACE for authenticated users in set_chmod_dacl() and then just add back/update
> > the modefromsid ACE.
> > Thus leaving the file with a single ACE that is for the mode and no ACE
> > to grant any user any rights to access the file.
> > Fix this by always adding back also the modefromsid ACE so that we do not
> > drop the rights to access the file.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> > fs/cifs/cifsacl.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> > index ee3aab3dd4ac..40cda87ce384 100644
> > --- a/fs/cifs/cifsacl.c
> > +++ b/fs/cifs/cifsacl.c
> > @@ -949,6 +949,9 @@ static void populate_new_aces(char *nacl_base,
> > pnntace = (struct cifs_ace *) (nacl_base + nsize);
> > nsize += setup_special_mode_ACE(pnntace, nmode);
> > num_aces++;
> > + pnntace = (struct cifs_ace *) (nacl_base + nsize);
> > + nsize += setup_authusers_ACE(pnntace);
> > + num_aces++;
> > goto set_size;
> > }
> >
> > @@ -1613,7 +1616,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
> > nsecdesclen = secdesclen;
> > if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
> > if (mode_from_sid)
> > - nsecdesclen += sizeof(struct cifs_ace);
> > + nsecdesclen += 2 * sizeof(struct cifs_ace);
> > else /* cifsacl */
> > nsecdesclen += 5 * sizeof(struct cifs_ace);
> > } else { /* chown */
> > --
> > 2.30.2
> >
>
>
> --
> Thanks,
>
> Steve
Good catch. Changes look good.
Please run the tests with memory sanitizers enabled for this one.
Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-14 13:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-13 22:40 Ronnie Sahlberg
2022-02-13 22:40 ` [PATCH] cifs: modefromsids must add an ACE for authenticated users Ronnie Sahlberg
2022-02-13 23:46 ` Steve French
2022-02-14 13:02 ` Shyam Prasad N
2022-02-14 0:13 ` Steve French
2022-02-14 7:52 ` ronnie sahlberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.