On Thu, Jul 10, 2014 at 12:26 AM, Frank Filz <ffilz...@mindspring.com> wrote:
>> On Wed, Jul 9, 2014 at 7:06 PM, Trond Myklebust
>> <trond.mykleb...@primarydata.com> wrote:
>> > On Wed, Jul 9, 2014 at 6:42 PM, Frank Filz <ffilz...@mindspring.com>
>> wrote:
>> >>> On Wed, Jul 9, 2014 at 5:54 PM, Frank S. Filz
>> >>> <ffilz...@mindspring.com>
>> >>> wrote:
>> >>> > From: "Frank S. Filz" <ffilz...@mindspring.com>
>> >>> >
>> >>> > 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 <ffilz...@mindspring.com>
>> >>> > ---
>> >>> > 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.
>>
>> Oops. Sorry, the correct sub-sub-sub-sub-....paragraph is this one:
>>
>> Permission to execute a file.
>>
>> Servers SHOULD allow a user the ability to read the data of the
>> file when only the ACE4_EXECUTE access mask bit is allowed.
>> This is because there is no way to execute a file without
>> reading the contents. Though a server may treat ACE4_EXECUTE
>> and ACE4_READ_DATA bits identically when deciding to permit a
>> READ operation, it SHOULD still allow the two bits to be set
>> independently in ACLs, and MUST distinguish between them when
>> replying to ACCESS operations. In particular, servers SHOULD
>> NOT silently turn on one of the two bits when the other is set,
>> as that would make it impossible for the client to correctly
>> enforce the distinction between read and execute permissions.
>>
>>
>> > To me that translates as saying that the server SHOULD accept an
>> > OPEN(SHARE_ACCESS_READ|SHARE_ACCESS_WRITE) request in the above
>> > situation.
>>
>> Same conclusion, though....
>
> When I read that paragraph, my interpretation is that OPEN (and READ) should
> be permission checked normally, however, if ONLY execute permission is
> granted, and the OPEN is read only (and READ of course is read only) then
> permission would be granted for the purpose of execution. But if any other
> combination of bits was allowed, then the paragraph doesn't apply. Thus an
> OPEN SHARE_ACCESS_READ | SHARE_ACCESS_WRITE must fail since write access was
> not granted (if it was, the exception doesn't apply to my reading).
>
Where does that paragraph say anything about SHARE_WRITE, or even
mention the word "only"?
All it says is that as far as the OPEN and READ operations are
concerned, ACE4_EXECUTE == ACE4_READ_DATA, whereas for the ACCESS
operation, they differ.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.mykleb...@primarydata.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
