[OpenAFS-devel] GIDs and file server permission checks
Nathaniel W Filardo
nwf@cs.jhu.edu
Sat, 25 Oct 2014 20:30:12 -0400
--5eP1bVg2JUWsLUv1
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Sat, Oct 25, 2014 at 07:07:10PM -0400, Jeffrey Altman wrote:
> On 10/25/2014 3:35 PM, Nathaniel W Filardo wrote:
> > Hello all,
> >
> > At present, GIDs seem a little funny -- it takes system:administrator
> > membership to change them, but files can be created with arbitrary GID
> > simply by operating a client with a UNIX credential whose primary GID is
> > whatever is desired.
>
> The actual requirements for setting the ownership and/or group of a
> vnode are
>
> 1a. Insert permission on the directory
> 1b. Ownership of the file
>
> or
>
> 2. Member of system:administrator group
>
> or
>
> 3a. Owner can be changed if the volume owner is the object owner
> 3b. Group can be changed if the volume group is the object group
>
> In addition, the volume owner has implicit Administrator permission and
> can grant themselves Insert permission for the purpose of changing the
> owner or group on any object.
Is this written down somewhere authoritative that I don't know about or have
forgotten? (i.e. is there a specification document for the "operation X
rights X UNIX permissions on special files" table)
In any case, something seems fishy about that story, though I would like it
to be true, so what am I doing wrong (other than misreading the code,
apparently)? (All of this is tested, by the by, with a Debian 1.6.9-2
client and 1.6.9-1 server; spot-checking against my home server running
master seems to confirm the results, too, but I did that less carefully.)
> pf% kdestroy
> pf% unlog
> pf% cd /afs/acm.jhu.edu/user/nwf
> pf% kinit nwf@ACM.JHU.EDU
> Password for nwf@ACM.JHU.EDU:
> pf% aklog .
> pf% tokens
>
> Tokens held by the Cache Manager:
>
> User's (AFS ID 1063) tokens for afs@acm.jhu.edu [Expires Oct 26 05:26]
> --End of list--
> pf% fs la .
> Access list for . is
> Normal rights:
>[...]
> nwf:me rlidwka
>[...]
> pf% pts memb nwf:me -cell acm.jhu.edu
> Members of nwf:me (id: -208) are:
> nwf
>[...]
So you can see I (nwf, ID 1063) have full permissions on this directory and
own the file, and cannot change the GID of a file therein:
> pf% touch test
> pf% ls -l test
> -rw-r--r-- 1 1063 nwf 0 Oct 25 19:27 test
> pf% chgrp 100 test
> chgrp: changing group of =E2=80=98test=E2=80=99: Operation not permitted
The volume root directory is
> pf% ls -lad .
> drwxrwxrwx 56 1063 root 6144 Oct 25 19:29 .
> pf% chgrp 0 test
> chgrp: changing group of =E2=80=98test=E2=80=99: Operation not permitted
And I cannot change the GID to match that of the volume root. Just in case
this is something special about 0, tho', using my admin hat I changed the
volume root directory GID to be 100 ("users"), and even then, my non-admin
hat cannot change the GID to match:
> pf% ls -lad .
> drwxrwxrwx 56 1063 users 6144 Oct 25 19:30 .
> pf% chgrp users test
> chgrp: changing group of =E2=80=98test=E2=80=99: Operation not permitted
And even if I make the test file's GID match the volume root using my admin
hat...
> pf% ls -l test
> -rw-r--r-- 1 1063 users 0 Oct 25 19:30 test
> pf% chgrp 0 test
> chgrp: changing group of =E2=80=98test=E2=80=99: Operation not permitted
> pf% chgrp 1063 test
> chgrp: changing group of =E2=80=98test=E2=80=99: Operation not permitted
I can't change the GID. Even if I make my PTS UID the GID of the volume, it
still doesn't work:
> pf% ls -lad .
> drwxrwxrwx 56 1063 1063 6144 Oct 25 19:30 .
> ls -l test
> -rw-r--r-- 1 1063 users 0 Oct 25 19:30 test
> chgrp 1063 test
> chgrp: changing group of =E2=80=98test=E2=80=99: Operation not permitted
It's possible, however, that I've misunderstood your criterion 3b.
> > I would like to propose a flag for file servers which permits the volume
> > owner to change the UNIX GID of objects on that volume, so long as the
> > setuid and setgid bits are not set.
>
> Since a volume owner already has implicit Admin permission, is this
> necessary?
I would love it if it were not. In light of the above, do you think
something is actually wrong and my patch serves no purpose once that bug is
fixed? (I hope so!)
> > 1) Check_PermissionRights contains the comment
> >
> >> /* bypass protection checks on first store after a create
> >> * for the creator; also prevent chowns during this time
> >> * unless you are a system administrator */
> >
> > But there's nothing temporal about the test being done above it:
> >
> >> if ((rights & PRSFS_INSERT) && OWNSp(client, targetptr)
> >> && (CallingRoutine !=3D CHK_STOREACL)
> >> && (targetptr->disk.type =3D=3D vFile)) {
> >
> > So I think the upshot is that having PRSFS_INSERT rights in a given
> > directory is enough to confer write permissions to any file owned by you
> > in that directory?
>
> There are no descriptors or handles in AFS protocol to maintain server
> side state for an "open" or "create". The server grants the permissions
> based upon Insert and the cache managers take them away as required by the
> local OS behavior.
Well, cache managers can only be trusted to take away permissions if they're
trusted; since they're running on arbitrary machines and contacting our
servers, that's surely not the case. So I think I stand by my statement,
even if it would take a patched AFS kernel module or speaking RX by hand to
exercise that permission. It's fine; I don't think it's wrong that Insert
is "Write to things you own in this directory", and man fs_setacl even says
so; I jut think the comment's imprecise.
That said, I think this "also prevent chowns" check is in disagreement with
your criteria 1a and 1b above, as it reads:
> if (CHOWN(InStatus, targetptr) || CHGRP(InStatus, targetptr)) {
> if (readonlyServer)
> return (VREADONLY);
> else if (VanillaUser(client))
> return (EPERM); /* Was EACCES */
>[...]
and so a VanillaUser executing a CHGRP with INSERT rights to an OWNSp'd file
should see EPERM, as I seem to be. Varying only the vFile to be a
vDirectory hits the same check much later in the code, below a comment "/*
watch for chowns and chgrps */".
> > 2) The first chunk of code commented out with #ifdef USE_GROUP_PERMS
> > (that symbol seems to never be defined anywhere) overlooks cases
> > considered in the #else branch. Defining it would eliminate the ability
> > ("kludge") of a user to read her own files which lacked UNIX owner read
> > permission. The second chunk looks fine. Should the first chunk be
> > adjusted?
>
> Mode bits are stored for the benefit of UNIX they are not an AFS
> capability. Not all operating systems use mode bits. Sites can
> activate that code if they are running a UNIX only environment but it
> would not be appropriate for a call with non-UNIX clients.
Well, "sites can activate that code" if they recompile and ship their own
servers, sure. But that wasn't really my question -- I was asking if
> #ifdef USE_GROUP_PERMS
> if (!OWNSp(client, targetptr)
> && !client_HasAsMember(client, targetptr->disk.owner)=
) {
> errorCode =3D
> (((GROUPREAD | GROUPEXEC) &
> targetptr->disk.modeBits)
> ? 0 : EACCES);
> } else {
> errorCode =3D
> (((OWNERREAD | OWNEREXEC) &
> targetptr->disk.modeBits)
> ? 0 : EACCES);
> }
> #else
> /*
> * The check with the ownership below is a kludge to allow
> * reading of files created with no read permission. The =
owner
> * of the file is always allowed to read it.
> */
> if (OWNSp(client, targetptr)
> && VanillaUser(client))
> errorCode =3D
> (((OWNERREAD | OWNEREXEC) & targetptr->disk.
> modeBits) ? 0 : EACCES);
> #endif
should be something else. In particular, in the OWNSp && !VanillaUser case,
the latter chunk will leave errorCode set to 0, while the former will check
against targetptr->disk.modeBits, and I do not see an earlier exception for
!VanillaUser in Check_PermissionRights (though maybe it's in the callers;
haven't checked that carefully).
> > 3) CREATE_SGUID_ADMIN_ONLY is defined twice in the file, once up top and
> > once mid-way through. Is this a kind of trap for would-be code mutators
> > like yours truly and the third test is somehow dramatically more
> > important than the first two, or is it just lingering junk? ;)
>
> The first one was added by 30433f36a953187f27b5db9fb432f3b7dce91e6b when
> the second one already existed later in the file. The second should have
> been removed as part of 30433f36a953187f27b5db9fb432f3b7dce91e6b.
Patch sent to gerrit.
Thanks.
--nwf;
--5eP1bVg2JUWsLUv1
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
iEYEARECAAYFAlRMQJQACgkQTeQabvr9Tc/A3wCdHK1oqaV1fL3AueD6SB5Fgr1K
vlwAnj6tdJiPegdsUt+YXPoA3jhh5zvn
=oCWT
-----END PGP SIGNATURE-----
--5eP1bVg2JUWsLUv1--