[OpenAFS-devel] posix chown again

[Michael Meffie]

Hello,

I have been looking at Derrick's patch from last spring
to implement POSIX style chown, with the hope it can be
included in OpenAFS 1.5. Since there are security implications
to this change, I am posting it here for further review
and comment.

The idea is to allow regular users to disown files when the
C acl bit is set. The setuid and setguid mode bits are cleared
when ownership is changed by regular users. (To prevent, for
example, someone from doing a 'chown root' on a script that
has the suid bit set.)

Since the C acl is documented as having no default meaning,
this is conditionally compiled into the fileserver with
the --enable-posix-chown option (disabled by default).

I have been running this patch against the following tests,

  1. Regular users may change ownership of owned files and directories
     when the w and C ACLs are set.

  2. Regular users may not steal ownership of files or directories
     (regardless of acls).

  3. Regular users my change group ownership of owned files when the w
     and C ACLs are set.

  4. Setuid and Setguid mode bits are cleared when a regular user
     changes ownership or group ownership of a file.


Finally, there is an oddity in the current Update_TargetVnodeStatus()
function.  It seems original intent of the code was to disallow non-admin
users to set the suid/sgid bits. However the current implementation
also clears the sticky bit as well.  Is that a code error, or was
disallowing setting of the sticky bit intentional?

Mike --





diff --git a/acinclude.m4 b/acinclude.m4
index 17b2996..bd3da1a 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -1483,6 +1483,10 @@ fi
  AC_SUBST(XBSA_CFLAGS)
  AC_SUBST(XBSA_XLIBS)

+if test "$enable_posix_chown" = "yes"; then
+	AC_DEFINE(ACL_POSIX_CHOWN, 1, [define if you want posix chown])
+fi
+
  dnl checks for header files.
  AC_HEADER_STDC
  AC_HEADER_SYS_WAIT
diff --git a/src/viced/afsfileprocs.c b/src/viced/afsfileprocs.c
index d61f179..940f25e 100644
--- a/src/viced/afsfileprocs.c
+++ b/src/viced/afsfileprocs.c
@@ -923,7 +923,11 @@ 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)
+#ifdef ACL_POSIX_CHOWN
+		    && !(rights & PRSFS_USR2) /* ownership already checked */
+#endif
+                )
  		    return (EPERM);	/* Was EACCES */
  		else
  		    osi_audit(PrivilegeEvent, 0, AUD_ID,
@@ -949,7 +953,11 @@ Check_PermissionRights(Vnode * targetptr, struct client *client,
  			|| CHGRP(InStatus, targetptr)) {
  			if (readonlyServer)
  			    return (VREADONLY);
-			else if (VanillaUser(client))
+			else if (VanillaUser(client)
+#ifdef ACL_POSIX_CHOWN
+			    && (!OWNSp(client,targetptr) || !(rights & PRSFS_USR2))
+#endif
+                        )
  			    return (EPERM);	/* Was EACCES */
  			else
  			    osi_audit(PrivilegeEvent, 0, AUD_ID,
@@ -1536,10 +1544,9 @@ Update_TargetVnodeStatus(Vnode * targetptr, afs_uint32 Caller,
      if (InStatus->Mask & AFS_SETOWNER) {
  	/* admin is allowed to do chmod, chown as well as chown, chmod. */
  	if (VanillaUser(client)) {
-	    targetptr->disk.modeBits &= ~04000;	/* turn off suid for file. */
-#ifdef CREATE_SGUID_ADMIN_ONLY
-	    targetptr->disk.modeBits &= ~02000;	/* turn off sgid for file. */
-#endif
+            /* posix chown; clear suid/sgid mode bits to prevent privilege escalation */
+	    targetptr->disk.modeBits &= ~S_ISUID;	/* turn off suid for file. */
+	    targetptr->disk.modeBits &= ~S_ISGID;	/* turn off sgid for file. */
  	}
  	targetptr->disk.owner = InStatus->Owner;
  	if (VolumeRootVnode(targetptr)) {
@@ -1551,9 +1558,10 @@ Update_TargetVnodeStatus(Vnode * targetptr, afs_uint32 Caller,
      }
      if (InStatus->Mask & AFS_SETMODE) {
  	int modebits = InStatus->UnixModeBits;
-#define	CREATE_SGUID_ADMIN_ONLY 1
-#ifdef CREATE_SGUID_ADMIN_ONLY
  	if (VanillaUser(client))
+#ifdef ACL_POSIX_CHOWN
+            modebits &= ~(S_ISUID|S_ISGID); /* only admin can set suid/sgid */
+#else
  	    modebits = modebits & 0777;
  #endif
  	if (VanillaUser(client)) {
@@ -1576,8 +1584,14 @@ Update_TargetVnodeStatus(Vnode * targetptr, afs_uint32 Caller,
  	}
      }
      targetptr->disk.serverModifyTime = FT_ApproxTime();
-    if (InStatus->Mask & AFS_SETGROUP)
+    if (InStatus->Mask & AFS_SETGROUP) {
+	if (VanillaUser(client)) {
+            /* posix chown; clear suid/sgid mode bits to prevent privilege escalation */
+	    targetptr->disk.modeBits &= ~S_ISUID;	/* turn off suid for file. */
+	    targetptr->disk.modeBits &= ~S_ISGID;	/* turn off sgid for file. */
+	}
  	targetptr->disk.group = InStatus->Group;
+    }
      /* vnode changed : to be written back by VPutVnode */
      targetptr->changed_newTime = 1;