[PATCH] fix permission checks for executables

[PATCH] fix permission checks for executables

[Christoph Hellwig]

Hi Linux,

vfs_permission in the Linux 2.4 series tries to check for
CAP_DAC_OVERRIDE if the modes didn't match.  This means even
for an file without executable bits set at all, root will
be reported that it is.   I've actually found one apllication
(scomail under linux-abi) that fails because of this, besides
not matching my reading of Posix 1003.1e.

Of the operating systems with capabilty-like features at least
OpenUNIX gets it right, of the others at least OpenServer and
4.4BSD, but these semantics seem natural to me anyway..

Please aplly the attached patch.

	Christoph

-- 
Whip me.  Beat me.  Make me maintain AIX.


--- linux.really_plain/fs/namei.c	Wed Aug  8 17:56:58 2001
+++ linux.plain/fs/namei.c	Wed Aug  8 18:13:22 2001
@@ -163,9 +163,13 @@
 	else if (in_group_p(inode->i_gid))
 		mode >>= 3;
 
-	if (((mode & mask & S_IRWXO) == mask) || capable(CAP_DAC_OVERRIDE))
+	if (((mode & mask & S_IRWXO) == mask))
 		return 0;
 
+	if (!(mask & S_IXOTH) || S_ISDIR(inode->i_mode))
+		if (capable(CAP_DAC_OVERRIDE))
+			return 0;
+
 	/* read and search access */
 	if ((mask == S_IROTH) ||
 	    (S_ISDIR(inode->i_mode)  && !(mask & ~(S_IROTH | S_IXOTH))))

Re: [Linux-privs-discuss] [PATCH] fix permission checks for executables

[Christoph Hellwig]

On Wed, Aug 08, 2001 at 06:22:19PM +0200, Christoph Hellwig wrote:
> Hi Linux,
> 
> vfs_permission in the Linux 2.4 series tries to check for
> CAP_DAC_OVERRIDE if the modes didn't match.  This means even
> for an file without executable bits set at all, root will
> be reported that it is.   I've actually found one apllication
> (scomail under linux-abi) that fails because of this, besides
> not matching my reading of Posix 1003.1e.
> 
> Of the operating systems with capabilty-like features at least
> OpenUNIX gets it right, of the others at least OpenServer and
> 4.4BSD, but these semantics seem natural to me anyway..

Andreas Gruenbacher pointed out that it is much leaner to use
MAY_READ¸MAY_EXEC and MAY_WRITE instead of abusing the stat-macros.

Here is a patch that changes the complete function to use these
and also cleans up and clarifies the comments.

	Christoph


--- linux.really_plain/fs/namei.c	Wed Aug  8 17:56:58 2001
+++ linux.plain/fs/namei.c	Wed Aug  8 19:27:19 2001
@@ -140,7 +140,7 @@
 }
 
 /*
- *	permission()
+ *	vfs_permission()
  *
  * is used to check for read/write/execute permissions on a file.
  * We use "fsuid" for this, letting us set arbitrary permissions
@@ -151,24 +151,40 @@
 {
 	int mode = inode->i_mode;
 
-	if ((mask & S_IWOTH) && IS_RDONLY(inode) &&
-		 (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
-		return -EROFS; /* Nobody gets write access to a read-only fs */
-
-	if ((mask & S_IWOTH) && IS_IMMUTABLE(inode))
-		return -EACCES; /* Nobody gets write access to an immutable file */
+	if (mask & MAY_WRITE) {
+		/*
+		 * Nobody gets write access to a read-only fs.
+		 */
+		if (IS_RDONLY(inode) &&
+		    (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
+			return -EROFS;
+
+		/*
+		 * Nobody gets write access to an immutable file.
+		 */
+		if (IS_IMMUTABLE(inode))
+			return -EACCES;
+	}
 
 	if (current->fsuid == inode->i_uid)
 		mode >>= 6;
 	else if (in_group_p(inode->i_gid))
 		mode >>= 3;
 
-	if (((mode & mask & S_IRWXO) == mask) || capable(CAP_DAC_OVERRIDE))
+	if (((mode & mask & (MAY_READ|MAY_WRITE|MAY_EXEC)) == mask))
 		return 0;
 
-	/* read and search access */
-	if ((mask == S_IROTH) ||
-	    (S_ISDIR(inode->i_mode)  && !(mask & ~(S_IROTH | S_IXOTH))))
+	/*
+	 * Only read/write DACs are overridable.
+	 */
+	if ((mask & (MAY_READ|MAY_WRITE)) || S_ISDIR(inode->i_mode))
+		if (capable(CAP_DAC_OVERRIDE))
+			return 0;
+
+	/*
+	 * Searching includes executable on directories, else just read.
+	 */
+	if (mask == MAY_READ || (S_ISDIR(inode->i_mode) && !(mask & MAY_WRITE)))
 		if (capable(CAP_DAC_READ_SEARCH))
 			return 0;