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

[Frank Filz]

> >> >> 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.
> >
> > I'm reading the "only" in the first sentence:
> >
> > "Servers SHOULD allow a user the ability to read the data of the file when
> only the ACE4_EXECUTE access mask bit is allowed."
> 
> How does that support your assertion that setting a SHARE_BOTH mode
> turns off the exception? There is no mention of share modes there. All it says
> is that you should grant read permissions when the execute bit is set in the
> ACL.
> 
> 
> > But to entertain the idea that I'm reading too much into that sentence, let's
> go back to the situation:
> >
> > File does not already exist
> > Application on client makes an open("/nfs4mnt/foo", O_CREAT | O_RDWR,
> > 0) system call
> >
> > What can we do between the server and client to assure success of that
> call. It works on local filesystems. It works over NFS v3. But it fails, at least
> with the Linux NFS v4 client.
> >
> > With the Linux NFS v4 client, the following does succeed (because actual
> read access was granted):
> >
> > open("/nfs4mnt/foo", O_CREAT | O_RDWR, 0400)
> >
> > And the client can write to the file.
> >
> > On the other hand, going back to my interpretation of the sentence, that is
> indeed the interpretation the Linux knfsd server is taking, because my little
> test case works as I expect it with the changes I proposed, in that and
> open("/nfs4mnt/foo", O_RDWR) fails even if the file has -wx------
> permissions (and looking at wireshark traces, I see that indeed the OPEN fails
> if read permission is not granted, but execute permission is granted and
> write access was requested.
> >
> > Ok, here is the relevant code from fs/nfsd/vfs.c:
> >
> >         /* Allow read access to binaries even when mode 111 */
> >         if (err == -EACCES && S_ISREG(inode->i_mode) &&
> >              (acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE) ||
> >               acc == (NFSD_MAY_READ | NFSD_MAY_READ_IF_EXEC)))
> >                 err = inode_permission(inode, MAY_EXEC);
> >
> > So if the caller had requested write access, it does not check for
> MAY_EXEC.
> 
> That's example code from one server implementation. It's not from
> authoritative source.
> 
> 
> Frank, the bottom line is that I'm not going to accept that patch as it stands,
> because it is wrong.
> The right fix here seems rather to be to put in an exception if the
> data->file_created flag is set. I'll write a patch.

Hmm, had not considered that, but that will be a solution. That should also then pass the open("foo", O_CREAT | O_RDONLY, 0) case which certainly is better.

I'll be happy to test your patch.

Ok, switching hats to server...

If I am misinterpreting that paragraph, then I'm wondering what permission check the server should be doing. As far as I can tell, both Ganesha and knfsd are doing a permission check that matches my interpretation of the paragraph. If that's wrong, I'm happy to change the Ganesha implementation.

Frank

--
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/