Re: permission() bug?

["Michael Kerrisk" <mtk-lists@jambit.com>]

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