On Wednesday 12 February 2025 17:47:19 Steve French wrote:
> On Wed, Feb 12, 2025 at 4:58 PM Paulo Alcantara <pc@xxxxxxxxxxxxx> wrote:
> >
> > Pali Rohár <pali@xxxxxxxxxx> writes:
> >
> > > On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
> > >> Pali Rohár <pali@xxxxxxxxxx> writes:
> > >>
> > >> > On Wednesday 12 February 2025 17:49:31 Paulo Alcantara wrote:
> > >> >> Steve,
> > >> >>
> > >> >> The commit 438e2116d7bd ("cifs: Change translation of
> > >> >> STATUS_PRIVILEGE_NOT_HELD to -EPERM") regressed getcifsacl(1) because it
> > >> >> expects -EIO to be returned from getxattr(2) when the client can't read
> > >> >> system.cifs_ntsd_full attribute and then fall back to system.cifs_acl
> > >> >> attribute. Either -EIO or -EPERM is wrong for getxattr(2), but that's a
> > >> >> different problem, though.
> > >> >>
> > >> >> Reproduced against samba-4.22 server.
> > >> >
> > >> > That is bad. I can prepare a fix for cifs.ko getxattr syscall to
> > >> > translate -EPERM to -EIO. This will ensure that getcifsacl will work as
> > >> > before as it would still see -EIO error.
> > >>
> > >> Sounds good.
> > >>
> > >> > But as discussed before, we need to distinguish between
> > >> > privilege/permission error and other generic errors (access/io).
> > >> > So I think that we need 438e2116d7bd commit.
> > >>
> > >> OK.
> > >>
> > >> > Based on linux-fsdevel discussion it is a good idea to distinguish
> > >> > between errors by mapping status codes to appropriate posix errno, and
> > >> > then updating linux syscall manpages.
> > >>
> > >> Either way, we shouldn't be leaking -EIO or -EPERM to userland from
> > >> getxattr(2). By looking at the man pages, -ENODATA seems to be the
> > >> appropriate error to return instead.
> > >
> > > It looks like there are missing error codes for getxattr. Because any
> > > path based syscall can return -EACCES if trying to open path to which
> > > calling process does not have access.
> > >
> > > And EACCES is not mentioned nor documented in getxattr(2). Same applies
> > > for listxattr(2). Now I have tried listxattr() and it really returns
> > > EACCES for /root/file called by nobody.
> >
> > Both man pages have this:
> >
> > > In addition, the errors documented in stat(2) can also occur.
> >
> > and stat(2) actually documents EACCES.
> >
> > > -EIO is generic I/O error. And I think that this error code could be
> > > returned by any I/O syscall when unknown I/O error occurs.
> >
> > Makes sense.
> >
> > > Returning -ENODATA for generic or unknown I/O error is a bad idea
> > > because ENODATA (= ENOATTR) has already specific meaning when attribute
> > > does not exists at all (or process does not have access to it).
> >
> > You are right.
> >
> > > For me it makes sense to return -EIO and -EPERM by those syscalls. But
> > > for getxattr() we cannot do it due that backward compatibility needed by
> > > getcifsacl application.
> >
> > -EACCES seems the correct one. But yeah, we can't do it due to
> > getcifsacl(1) relying on -EIO.
>
> Since EIO is incorrect, we probably should fix getcifsacl ASAP so we
> can start returning something more correct for this call e.g. -EACCESS
> or -EPERM
>
> Since updating cifs-utils for newer kernels is relatively easy (and
> the next version of cifs-utils has some security fixes so will be
> easier to rollout), why don't we also change getcifsacl ASAP to handle
> the correct rc to give us more freedom for cifs.ko to return the
> correct error on newer kernels. Thoughts about this change to
> getcifsacl() function which would work with both old and newer kernels
> with the rc mapping change? Change to fix the cifs.ko mapping to EIO
> could be delayed as well so cifs-utils with the updated check is
> rolled out?!
That should work too.
Anyway, if I'm looking correctly at that getcifsacl.c code, it contains
fallback from fetching SACL+DACL attribute (ATTRNAME_NTSD_FULL) to
DACL-only attribute.
And if the user does not have permission to access SACL then
STATUS_PRIVILEGE_NOT_HELD is returned by the SMB server.
STATUS_PRIVILEGE_NOT_HELD is being mapped to EPERM.
So EACCES should not be needed there.
If SMB server returns STATUS_ACCESS_DENIED (EACCES) then it means that
user does not have access to path or DACL, and so fallback from
SACL+DACL (ATTRNAME_NTSD_FULL) to DACL-only attribute is useless.
> diff --git a/getcifsacl.c b/getcifsacl.c
> index 123d11e..3c12789 100644
> --- a/getcifsacl.c
> +++ b/getcifsacl.c
> @@ -447,7 +447,8 @@ getxattr:
> free(attrval);
> bufsize += BUFSIZE;
> goto cifsacl;
> - } else if (errno == EIO && !(strcmp(attrname,
> ATTRNAME_NTSD_FULL))) {
> + } else if (((errno == EIO) || (errno == EPERM) ||
> (errno == EACCES)) &&
> + !(strcmp(attrname, ATTRNAME_NTSD_FULL))) {
> /*
> * attempt to fetch SACL in addition to owner
> and DACL via
> * ATTRNAME_NTSD_FULL, fall back to owner/DACL via
>
>
>
> Thanks,
>
> Steve
