Skip to content

Commit

Permalink
nfsd: allow owner_override only for regular files
Browse files Browse the repository at this point in the history
We normally allow the owner of a file to override permissions checks on
IO operations, since:
	- the client will take responsibility for doing an access check
	  on open;
	- the permission checks offer no protection against malicious
	  clients--if they can authenticate as the file's owner then
	  they can always just change its permissions;
	- checking permission on each IO operation breaks the usual
	  posix rule that permission is checked only on open.

However, we've never allowed the owner to override permissions on
readdir operations, even though the above logic would also apply to
directories.  I've never heard of this causing a problem, probably
because a) simultaneously opening and creating a directory (with
restricted mode) isn't possible, and b) opening a directory, then
chmod'ing it, is rare.

Our disallowal of owner-override on directories appears to be an
accident, though--the readdir itself succeeds, and then we fail just
because lookup_one_len() calls in our filldir methods fail.

I'm not sure what the easiest fix for that would be.  For now, just make
this behavior obvious by denying the override right at the start.

This also fixes some odd v4 behavior: with the rdattr_error attribute
requested, it would perform the readdir but return an ACCES error with
each entry.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
  • Loading branch information
J. Bruce Fields committed Jul 10, 2012
1 parent 74dbafa commit d91d0b5
Showing 1 changed file with 9 additions and 1 deletion.
10 changes: 9 additions & 1 deletion fs/nfsd/vfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -757,8 +757,16 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
* If we get here, then the client has already done an "open",
* and (hopefully) checked permission - so allow OWNER_OVERRIDE
* in case a chmod has now revoked permission.
*
* Arguably we should also allow the owner override for
* directories, but we never have and it doesn't seem to have
* caused anyone a problem. If we were to change this, note
* also that our filldir callbacks would need a variant of
* lookup_one_len that doesn't check permissions.
*/
err = fh_verify(rqstp, fhp, type, may_flags | NFSD_MAY_OWNER_OVERRIDE);
if (type == S_IFREG)
may_flags |= NFSD_MAY_OWNER_OVERRIDE;
err = fh_verify(rqstp, fhp, type, may_flags);
if (err)
goto out;

Expand Down

0 comments on commit d91d0b5

Please sign in to comment.