Re: [PATCH 1/1] Fix permission checking by NFS client for open-create with mode 000

[Trond Myklebust]

On Wed, Jul 9, 2014 at 6:42 PM, Frank Filz <ffilzlnx@xxxxxxxxxxxxxx> wrote:
>> On Wed, Jul 9, 2014 at 5:54 PM, Frank S. Filz <ffilzlnx@xxxxxxxxxxxxxx>
>> wrote:
>> > From: "Frank S. Filz" <ffilzlnx@xxxxxxxxxxxxxx>
>> >
>> > The NFS v4 client sends a COMPOUND with an OPEN and an ACCESS.
>> >
>> > The ACCESS is required to verify an open for read is actually allowed
>> > because RFC 3530 indicates OPEN for read only must succeed for an
>> > execute only file.
>> >
>> > The old code expected to have read access if the requested access was
>> > O_RDWR.
>> >
>> > We can expect the OPEN to properly permission check as long as the
>> > open is O_WRONLY or O_RDWR.
>> >
>> > Signed-off-by: Frank S. Filz <ffilzlnx@xxxxxxxxxxxxxx>
>> > ---
>> >  fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++-----
>> >  1 file changed, 20 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index
>> > 4bf3d97..9742054 100644
>> > --- a/fs/nfs/nfs4proc.c
>> > +++ b/fs/nfs/nfs4proc.c
>> > @@ -1966,15 +1966,30 @@ static int nfs4_opendata_access(struct
>> rpc_cred *cred,
>> >                 return 0;
>> >
>> >         mask = 0;
>> > -       /* don't check MAY_WRITE - a newly created file may not have
>> > -        * write mode bits, but POSIX allows the creating process to write.
>> > -        * use openflags to check for exec, because fmode won't
>> > -        * always have FMODE_EXEC set when file open for exec. */
>> > +       /* Don't trust the permission check on OPEN if open for exec or for
>> > +        * read only. Since FMODE_EXEC doesn't go across the wire, the server
>> > +        * has no way to distinguish between an open to read an executable
>> file
>> > +        * and an open to read a readable file. Write access is properly checked
>> > +        * and permission SHOULD always be granted if the file was created as
>> a
>> > +        * result of this OPEN, no matter what mode the file was created with.
>> > +        *
>> > +        * NOTE: If the case of a OPEN CREATE READ-ONLY with a mode that
>> does
>> > +        *       not allow read access, this test will produce an incorrect
>> > +        *       EACCES error.
>> > +        */
>> >         if (openflags & __FMODE_EXEC) {
>> >                 /* ONLY check for exec rights */
>> >                 mask = MAY_EXEC;
>> > -       } else if (fmode & FMODE_READ)
>> > +       } else if (!(fmode & FMODE_WRITE)) {
>> > +               /* In case the file was execute only, check for read permission
>> > +                * ONLY if write access was not requested. It is expected that
>> > +                * an OPEN for write will fail if the file is execute only.
>> > +                * Note that if the file was newly created, the fmode SHOULD
>> > +                * include FMODE_WRITE, especially if the file will be created
>> > +                * with a restrictive mode.
>> > +                */
>> >                 mask = MAY_READ;
>> > +       }
>>
>> This looks wrong. AFAICS it will allow you to open an existing file which has -
>> wx permissions (i.e. no read permissions) for O_RDWR. That should not be
>> permitted under POSIX rules.
>
> The server permission checks the OPEN, this only affects the subsequent ACCESS.
>
> The server will fail the OPEN with NFS4_ERR_ACCESS if the open is for read/write and the file has write-execute permission.

RFC3530bis draft 33 (6.2.1.3.1.  Discussion of Mask Attributes) states
that for both the OPEN and the READ operations, "Servers SHOULD allow
a user the ability to read the data of the file when only the
ACE4_EXECUTE access mask bit is allowed". RFC5561 has the same
language.

To me that translates as saying that the server SHOULD accept an
OPEN(SHARE_ACCESS_READ|SHARE_ACCESS_WRITE) request in the above
situation.

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/