diff mbox series

cifs: modefromsids must add an ACE for authenticated users

Message ID 20220213224052.3387192-2-lsahlber@redhat.com (mailing list archive)
State New, archived
Headers show
Series cifs: modefromsids must add an ACE for authenticated users | expand

Commit Message

Ronnie Sahlberg Feb. 13, 2022, 10:40 p.m. UTC
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(-)

Comments

Steve French Feb. 13, 2022, 11:46 p.m. UTC | #1
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
>
Steve French Feb. 14, 2022, 12:13 a.m. UTC | #2
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
>
Shyam Prasad N Feb. 14, 2022, 1:02 p.m. UTC | #3
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>
diff mbox series

Patch

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 */