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 client
*client,
#define CHOWN(i,t) (((i)->Mask & AFS_SETOWNER) &&((i)->Owner !=
(t)->disk.owner))
#define CHGRP(i,t) (((i)->Mask & AFS_SETGROUP) &&((i)->Group !=
(t)->disk.group))
+#define CHECK_RELAXED_CHGRP(c,i,t) \
+ (relaxchgrp \
+ && !CHOWN((i), (t)) \
+ && VolumeOwner((c),(t)) \
+ && (((t)->disk.modeBits & (S_ISUID | S_ISGID)) == 0))
+
if (CallingRoutine & CHK_FETCH) {
if (CallingRoutine == CHK_FETCHDATA || VanillaUser(client)) {
if (targetptr->disk.type == vDirectory
@@ -1087,7 +1093,8 @@ Check_PermissionRights(Vnode * targetptr, struct client
*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 client
*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 != CHK_STOREACL)
> && (targetptr->disk.type == 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;
pgp9K3DybjpYR.pgp
Description: PGP signature
