permission() bug?

permission() bug?

[Andreas Gruenbacher]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

Hello Andrew,

the fix for permission() that makes it compliant with POSIX.1-2001
apparently was lost. Here is the patch I sent before. (The relevant
lines from the standard text are cited in
http://www.ussg.iu.edu/hypermail/linux/kernel/0310.2/0286.html. The fix
proposed in that posting did not handle directories without execute
permissions correctly.)

Could you please apply the attached patch to mainline? Thanks.


Regards,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG

[-- Attachment #2: permission.diff --]
[-- Type: text/plain, Size: 3358 bytes --]

Make permission check conform to POSIX.1-2001

The access(2) function does not conform to POSIX.1-2001: For root
and a file with no permissions, access(file, MAY_READ|MAY_EXEC)
returns 0 (it should return -1).

Index: linux-2.6.0+fix/fs/jfs/acl.c
===================================================================
--- linux-2.6.0+fix.orig/fs/jfs/acl.c	2003-12-18 03:58:07.000000000 +0100
+++ linux-2.6.0+fix/fs/jfs/acl.c	2003-12-27 02:01:56.000000000 +0100
@@ -191,7 +191,8 @@ check_capabilities:
 	 * Read/write DACs are always overridable.
 	 * Executable DACs are overridable if at least one exec bit is set.
 	 */
-	if ((mask & (MAY_READ|MAY_WRITE)) || (inode->i_mode & S_IXUGO))
+	if (!(mask & MAY_EXEC) ||
+	    (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode))
 		if (capable(CAP_DAC_OVERRIDE))
 			return 0;
 
Index: linux-2.6.0+fix/fs/xfs/xfs_inode.c
===================================================================
--- linux-2.6.0+fix.orig/fs/xfs/xfs_inode.c	2003-12-18 03:59:45.000000000 +0100
+++ linux-2.6.0+fix/fs/xfs/xfs_inode.c	2003-12-27 02:05:00.000000000 +0100
@@ -3722,7 +3722,8 @@ xfs_iaccess(
 	 * Read/write DACs are always overridable.
 	 * Executable DACs are overridable if at least one exec bit is set.
 	 */
-	if ((orgmode & (S_IRUSR|S_IWUSR)) || (inode->i_mode & S_IXUGO))
+	if (!(orgmode & S_IXUSR) || (inode->i_mode & S_IXUGO) ||
+	    (ip->i_d.di_mode & S_IFMT) == S_IFDIR)
 		if (capable_cred(cr, CAP_DAC_OVERRIDE))
 			return 0;
 
Index: linux-2.6.0+fix/fs/ext2/acl.c
===================================================================
--- linux-2.6.0+fix.orig/fs/ext2/acl.c	2003-12-18 03:59:18.000000000 +0100
+++ linux-2.6.0+fix/fs/ext2/acl.c	2003-12-27 02:05:46.000000000 +0100
@@ -322,7 +322,8 @@ check_groups:
 
 check_capabilities:
 	/* Allowed to override Discretionary Access Control? */
-	if ((mask & (MAY_READ|MAY_WRITE)) || (inode->i_mode & S_IXUGO))
+	if (!(mask & MAY_EXEC) ||
+	    (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode))
 		if (capable(CAP_DAC_OVERRIDE))
 			return 0;
 	/* Read and search granted if capable(CAP_DAC_READ_SEARCH) */
Index: linux-2.6.0+fix/fs/ext3/acl.c
===================================================================
--- linux-2.6.0+fix.orig/fs/ext3/acl.c	2003-12-18 03:58:39.000000000 +0100
+++ linux-2.6.0+fix/fs/ext3/acl.c	2003-12-27 02:07:02.000000000 +0100
@@ -327,7 +327,8 @@ check_groups:
 
 check_capabilities:
 	/* Allowed to override Discretionary Access Control? */
-	if ((mask & (MAY_READ|MAY_WRITE)) || (inode->i_mode & S_IXUGO))
+	if (!(mask & MAY_EXEC) ||
+	    (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode))
 		if (capable(CAP_DAC_OVERRIDE))
 			return 0;
 	/* Read and search granted if capable(CAP_DAC_READ_SEARCH) */
Index: linux-2.6.0+fix/fs/namei.c
===================================================================
--- linux-2.6.0+fix.orig/fs/namei.c	2003-12-18 03:58:40.000000000 +0100
+++ linux-2.6.0+fix/fs/namei.c	2003-12-27 01:58:23.000000000 +0100
@@ -190,7 +190,8 @@ int vfs_permission(struct inode * inode,
 	 * Read/write DACs are always overridable.
 	 * Executable DACs are overridable if at least one exec bit is set.
 	 */
-	if ((mask & (MAY_READ|MAY_WRITE)) || (inode->i_mode & S_IXUGO))
+	if (!(mask & MAY_EXEC) ||
+	    (inode->i_mode & S_IXUGO) || S_ISDIR(inode->i_mode))
 		if (capable(CAP_DAC_OVERRIDE))
 			return 0;
 

Re: permission() bug?

[Andrew Morton]

Andreas Gruenbacher <agruen@suse.de> wrote:
>
>  the fix for permission() that makes it compliant with POSIX.1-2001
>  apparently was lost. Here is the patch I sent before. (The relevant
>  lines from the standard text are cited in
>  http://www.ussg.iu.edu/hypermail/linux/kernel/0310.2/0286.html. The fix
>  proposed in that posting did not handle directories without execute
>  permissions correctly.)

Question is: should we fix it?  I'm not aware of any bug reports against
this behaviour, and there is the possibility that changing it now will
break some applications.

Yes, those applications are presumably broken on other OS's but that's
different.

Given that this has been a longstanding misbehaviour in Linux (yes?) maybe
the most prudent path is to remain bug-compatible?

I'll add the patch to -mm so we can pick up any obvious userspace breakage,
but it is likely that such problems will take a long time to emerge.

> The access(2) function does not conform to POSIX.1-2001: For root
> and a file with no permissions, access(file, MAY_READ|MAY_EXEC)
> returns 0 (it should return -1).

So are you saying that in this case access() is, in effect, returning

	access(file, MAY_READ) || access(file, MAY_EXEC)

whereas it should be returning

	access(file, MAY_READ) && access(file, MAY_EXEC)

?

Re: permission() bug?

[Andries Brouwer]

On Sun, Feb 01, 2004 at 01:14:57PM -0800, Andrew Morton wrote:

> >  the fix for permission() that makes it compliant with POSIX.1-2001
> 
> Question is: should we fix it?  I'm not aware of any bug reports against
> this behaviour, and there is the possibility that changing it now will
> break some applications.

Quite apart from this particular case, the general answer to such questions
should be Yes.

It must not be the case that Linux is roughly speaking POSIX-conforming
but deviates in a thousand obscure ways.

When a deviation is noticed, changing to be POSIX-conforming should
be the default action. Of course, some POSIX requirements are rather
unfortunate, and in individual cases there can be a good reason
not to change. Such individual cases should be discussed and well documented.

In a case like this, where it is clear that Linux is buggy, the bug
should just be fixed.  Of course it is your call to choose between
fixing a bug and keeping a stable interface.  If you choose the latter
this must be fixed in 2.7.

(By the way - 2.0.34 and 2.2.19 do not have this bug, 2.4.18 has.)


Andries

Re: permission() bug?

[Andreas Gruenbacher]

On Sun, 2004-02-01 at 22:14, Andrew Morton wrote:
> Andreas Gruenbacher <agruen@suse.de> wrote:
> >
> >  the fix for permission() that makes it compliant with POSIX.1-2001
> >  apparently was lost. Here is the patch I sent before. (The relevant
> >  lines from the standard text are cited in
> >  http://www.ussg.iu.edu/hypermail/linux/kernel/0310.2/0286.html. The fix
> >  proposed in that posting did not handle directories without execute
> >  permissions correctly.)
> 
> Question is: should we fix it?  I'm not aware of any bug reports against
> this behaviour, and there is the possibility that changing it now will
> break some applications.

We certainly should fix this bug. Michael Kerrisk has compared other
UNIXes in
http://linux.derkeiler.com/Mailing-Lists/Kernel/2003-10/6030.html; this
shows slightly better what is wrong. That message does not cover the
directory case though; for directories, execute access is always granted
to privileged users.

I don't expect any applications to break. We have had the patch I
attached to the previous message (not the one in the old messages!) in
the SUSE 2.6 kernel since roughtly two months now; it has undergone a
lot of testing.

> Yes, those applications are presumably broken on other OS's but that's
> different.
> 
> Given that this has been a longstanding misbehaviour in Linux (yes?) maybe
> the most prudent path is to remain bug-compatible?

Andries has already answered this. (Thank you!)

> I'll add the patch to -mm so we can pick up any obvious userspace breakage,
> but it is likely that such problems will take a long time to emerge.
> file, MAY_EXEC)

I'm fine with that as well, but I think the patch should go straight to
mainline.


Cheers,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG

permission() bug?

[Andreas Gruenbacher]

Hello,

I think there is a bug in fs/namei.c:vfs_permission(). The function
contains:


    int vfs_permission(struct inode * inode, int mask)
    {
	[...]
        /*
         * Read/write DACs are always overridable.
         * Executable DACs are overridable if at least one exec
         * bit is set.
         */
        if ((mask & (MAY_READ|MAY_WRITE)) || (inode->i_mode & S_IXUGO))
                if (capable(CAP_DAC_OVERRIDE))
                        return 0;
	[...]
	return -EACCES;
    }


The comment makes sense; the code doesn't quite implement what the
comment says. Consider the case of an inode with  "--" permissions. We
get the following results:

	permission(inode, MAY_READ)		= 0
	permission(inode, MAY_EXEC)		= -EACCESS
	permission(inode, MAY_READ|MAY_EXEC	= 0

The last result seems wrong; I would expect -EACCESS instead. So IMHO
the code in permission (and in the file system specific copies) should
read:

        if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
                if (capable(CAP_DAC_OVERRIDE))
                        return 0;


Regards,
-- 
Andreas Gruenbacher <agruen@suse.de>
SuSE Labs, SuSE Linux AG <http://www.suse.de/>

Re: permission() bug?

[Michael Kerrisk]

Andreas Gruenbacher wrote:

> I think there is a bug in fs/namei.c:vfs_permission(). The function
> contains:
> 
> 
>     int vfs_permission(struct inode * inode, int mask)
>     {
> [...]
>         /*
>          * Read/write DACs are always overridable.
>          * Executable DACs are overridable if at least one exec
>          * bit is set.
>          */
>         if ((mask & (MAY_READ|MAY_WRITE)) || (inode->i_mode & S_IXUGO))
>                 if (capable(CAP_DAC_OVERRIDE))
>                         return 0;
> [...]
> return -EACCES;
>     }
> 
> 
> The comment makes sense; the code doesn't quite implement what the
> comment says. Consider the case of an inode with  "--" permissions. We
> get the following results:
> 
> permission(inode, MAY_READ) = 0
> permission(inode, MAY_EXEC) = -EACCESS
> permission(inode, MAY_READ|MAY_EXEC = 0
> 
> The last result seems wrong; I would expect -EACCESS instead. 

Some pieces from SUSv3 that look relevant to me:

[[
<unistd.h>

    The constants F_OK, R_OK, W_OK, and X_OK and the expressions 
    R_OK|W_OK, R_OK|X_OK, and R_OK|W_OK|X_OK shall all have 
    distinct values.

access()
    If any access permissions are checked, each shall be checked 
    individually, as described in the Base Definitions volume of 
    IEEE Std 1003.1-2001, Chapter 3, Definitions. If the process 
    has appropriate privileges, an implementation may indicate 
    success for X_OK even if none of the execute file permission 
    bits are set.

    ERRORS
    [EACCES] Permission bits of the file mode do not 
    permit the requested access, or search permission is 
    denied on a component of the path prefix. 
]]

The implication of all this is that it is bits/masks that are
relevant for the check.  In this interpretation it is nonsensical 
that access() on a file with no perms should return:

0 if mask is R_OK | X_OK

but

-1/EACCES if mask is just X_OK.

I'd say that in this case, both of these calls should fail 
(-1/EACCES), though in my reading of the following,
SUSv3 allows (but discourages) the possibility that both 
would succeed.  (What is bizarre is the current scenario 
where one of the above calls succeeds and the other fails...)

[[
SUSv3 rationale to access():
    In early proposals, some inadequacies in the access() 
    function led to the creation of an eaccess() function 
    because:

    1. Historical implementations of access() do not test 
    file access correctly when the process' real user 
    ID is superuser. In particular, they always return
    zero when testing execute permissions without regard 
    to whether the file is executable.

    2. The superuser has complete access to all files on a 
    system. As a consequence, programs started by the 
    superuser and switched to the effective user ID with 
    lesser privileges cannot use access() to test their 
    file access permissions.

    However, the historical model of eaccess() does not 
    resolve problem (1), so this volume of IEEE Std 
    1003.1-2001 now allows access() to behave in the desired 
    way because several implementations have corrected the 
    problem. It was also argued that problem (2) is more 
    easily solved by using open(), chdir(), or one of the
    exec functions as appropriate and responding to the 
    error, rather than creating a new function that would 
    not be as reliable. Therefore, eaccess() is not included 
    in this volume of IEEE Std 1003.1-2001.

    The sentence concerning appropriate privileges and 
    execute permission bits reflects the two possibilities 
    implemented by historical implementations when checking
    superuser access for X_OK.

    New implementations are discouraged from returning X_OK 
    unless at least one execution permission bit is set.
]]

> So IMHO
> the code in permission (and in the file system specific copies) should
> read:
> 
>         if (!(mask & MAY_EXEC) || (inode->i_mode & S_IXUGO))
>                 if (capable(CAP_DAC_OVERRIDE))
>                         return 0;

The above is consistent with how I interpret SUSv3.

Cheers,

Michael