[OpenAFS-devel] GIDs and file server permission checks

[Nathaniel W Filardo]

--AnSJTMMZ92c40QA7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.  In particular:

  viced/afsfileprocs.c:1921 indicates that we default to inheriting group
  from the parent directory, but

  afs/VNOPS/afs_vnop_create.c:/^afs_create calls RXAFS_CreateFile with a
  InStatus including SETGROUP and group set to afs_cr_gid(acred).
  This succeeds because

  viced/afsfileprocs.c:/^SAFSS_CreateFile only calls CheckWriteMode to look
  at permissions before calling Alloc_NewVnode and Update_TargetVnodeStatus,
  which honors the client's request, including GID.

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.  The relevant changes, other than to
viced.c for command handling, are as follows:

diff --git a/src/viced/afsfileprocs.c b/src/viced/afsfileprocs.c
index 3d1d504..bd0bcb7 100644
--- a/src/viced/afsfileprocs.c
+++ b/src/viced/afsfileprocs.c
@@ -1027,6 +1027,12 @@ Check_PermissionRights(Vnode * targetptr, struct cli=
ent *client,
 #define CHOWN(i,t) (((i)->Mask & AFS_SETOWNER) &&((i)->Owner !=3D (t)->dis=
k.owner))
 #define CHGRP(i,t) (((i)->Mask & AFS_SETGROUP) &&((i)->Group !=3D (t)->dis=
k.group))
=20
+#define CHECK_RELAXED_CHGRP(c,i,t) \
+	(relaxchgrp \
+	 && !CHOWN((i), (t)) \
+	 && VolumeOwner((c),(t)) \
+	 && (((t)->disk.modeBits & (S_ISUID | S_ISGID)) =3D=3D 0))
+
     if (CallingRoutine & CHK_FETCH) {
 	if (CallingRoutine =3D=3D CHK_FETCHDATA || VanillaUser(client)) {
 	    if (targetptr->disk.type =3D=3D vDirectory
@@ -1087,7 +1093,8 @@ Check_PermissionRights(Vnode * targetptr, struct clie=
nt *client,
 	    if (CHOWN(InStatus, targetptr) || CHGRP(InStatus, targetptr)) {
 		if (readonlyServer)
 		    return (VREADONLY);
-		else if (VanillaUser(client))
+		else if (VanillaUser(client)
+			 && !CHECK_RELAXED_CHGRP(client, InStatus, targetptr))
 		    return (EPERM);	/* Was EACCES */
 		else
 		    osi_audit(PrivilegeEvent, 0, AUD_ID,
@@ -1113,7 +1120,8 @@ Check_PermissionRights(Vnode * targetptr, struct clie=
nt *client,
 			|| CHGRP(InStatus, targetptr)) {
 			if (readonlyServer)
 			    return (VREADONLY);
-			else if (VanillaUser(client))
+			else if (VanillaUser(client)
+				 && !CHECK_RELAXED_CHGRP(client, InStatus, targetptr))
 			    return (EPERM);	/* Was EACCES */
 			else
 			    osi_audit(PrivilegeEvent, 0, AUD_ID,

While here and while I've got your attention, I have some other questions...

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?

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?

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? ;)

Thanks much.
--nwf;

--AnSJTMMZ92c40QA7
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iEYEARECAAYFAlRL+3UACgkQTeQabvr9Tc/4IQCeMqf7iKU1cwCYsFDMnDLgvpQ6
5K8AnjjGIrmu3ZZPmNQimTf13RP7O0l7
=J/nS
-----END PGP SIGNATURE-----

--AnSJTMMZ92c40QA7--