Skip to contentSkip to conversation

fix permission checks for executables

PATCH In review Christoph Hellwig 1 participant 2 messages Aug 8, 2001 VFS

lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 fix permission checks for executables

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

Signature · 20 lines
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))))
author
#1 lore raw
Quoted 9 lines · 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.

Quoted 3 lines

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

fs/namei.c +27 -11
@@ -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;