Apologies if this has been answered before, however...
In nfsd_write() we have:
if (file) {
err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE);
if (err)
goto out;
err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen, cnt,
stablep);
} else {
So if a 'file' is already available - because the request came via NFSv4 and
there was a valid state id, and a 'struct file' was associated with that
state - we still call nfsd_permission().
Is that really needed? The permission check will have been performed at open
- it shouldn't be needed again now.
With NFSv3 we have to check permission at each IO, and this is slightly
different from POSIX semantics. We shouldn't have to with NFSv4... should we?
The particular issue that brought this to my attention is that "chattr +i" -
to make a file immutable - is not supposed to affect current opens, only
future opens. But a current open over NFSv4 is affected.
Is there some reason that we cannot just remove that nfsd_permission() check?
Thanks,
NeilBrown
On Thu, May 21, 2015 at 02:40:29PM +1000, NeilBrown wrote:
>
>
> Apologies if this has been answered before, however...
>
> In nfsd_write() we have:
>
> if (file) {
> err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE);
> if (err)
> goto out;
> err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen, cnt,
> stablep);
> } else {
>
> So if a 'file' is already available - because the request came via NFSv4 and
> there was a valid state id, and a 'struct file' was associated with that
> state - we still call nfsd_permission().
>
> Is that really needed? The permission check will have been performed at open
> - it shouldn't be needed again now.
>
> With NFSv3 we have to check permission at each IO, and this is slightly
> different from POSIX semantics. We shouldn't have to with NFSv4... should we?
>
> The particular issue that brought this to my attention is that "chattr +i" -
> to make a file immutable - is not supposed to affect current opens, only
> future opens. But a current open over NFSv4 is affected.
>
> Is there some reason that we cannot just remove that nfsd_permission() check?
The only proof that this write is part of the open is a stateid provided
as part of the write arguments. Anyone could sniff or guess that
stateid.
We could try to make that work by checking the stateid against the
principal from the rpc header. Unfortunately that turns out to be more
complicated than "is this the same principal as did the open"; among
other things I think it's possible the stateid resulted from opens done
by different principals, so we'd need to keep a list. If we added that
kind of check, could we drop the per-operation check? It's not obvious
to me.
--b.
On 05/21/2015 04:07 PM, J. Bruce Fields wrote:
> On Thu, May 21, 2015 at 02:40:29PM +1000, NeilBrown wrote:
>>
>>
>> Apologies if this has been answered before, however...
>>
>> In nfsd_write() we have:
>>
>> if (file) {
>> err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
>> NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE);
>> if (err)
>> goto out;
>> err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen, cnt,
>> stablep);
>> } else {
>>
>> So if a 'file' is already available - because the request came via NFSv4 and
>> there was a valid state id, and a 'struct file' was associated with that
>> state - we still call nfsd_permission().
>>
>> Is that really needed? The permission check will have been performed at open
>> - it shouldn't be needed again now.
>>
>> With NFSv3 we have to check permission at each IO, and this is slightly
>> different from POSIX semantics. We shouldn't have to with NFSv4... should we?
>>
>> The particular issue that brought this to my attention is that "chattr +i" -
>> to make a file immutable - is not supposed to affect current opens, only
>> future opens. But a current open over NFSv4 is affected.
>>
>> Is there some reason that we cannot just remove that nfsd_permission() check?
>
> The only proof that this write is part of the open is a stateid provided
> as part of the write arguments. Anyone could sniff or guess that
> stateid.
>
Perhaps like in OSD take a few bits in the state ID for a crypto
sig on the stateid+internal-server-counter this way it will be
impossible to fake. (The crypto key is only known to the server
and is volatile) (Yes you'll need to fake something with the DSs
unless you want to communicate public keys for verification)
(No it is not implemented in open-osd, is just provisioned for
in the STD)
Just a thought
Boaz
> We could try to make that work by checking the stateid against the
> principal from the rpc header. Unfortunately that turns out to be more
> complicated than "is this the same principal as did the open"; among
> other things I think it's possible the stateid resulted from opens done
> by different principals, so we'd need to keep a list. If we added that
> kind of check, could we drop the per-operation check? It's not obvious
> to me.
>
> --b.
On Thu, 21 May 2015 09:07:35 -0400 "J. Bruce Fields" <[email protected]>
wrote:
> On Thu, May 21, 2015 at 02:40:29PM +1000, NeilBrown wrote:
> >
> >
> > Apologies if this has been answered before, however...
> >
> > In nfsd_write() we have:
> >
> > if (file) {
> > err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> > NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE);
> > if (err)
> > goto out;
> > err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen, cnt,
> > stablep);
> > } else {
> >
> > So if a 'file' is already available - because the request came via NFSv4 and
> > there was a valid state id, and a 'struct file' was associated with that
> > state - we still call nfsd_permission().
> >
> > Is that really needed? The permission check will have been performed at open
> > - it shouldn't be needed again now.
> >
> > With NFSv3 we have to check permission at each IO, and this is slightly
> > different from POSIX semantics. We shouldn't have to with NFSv4... should we?
> >
> > The particular issue that brought this to my attention is that "chattr +i" -
> > to make a file immutable - is not supposed to affect current opens, only
> > future opens. But a current open over NFSv4 is affected.
> >
> > Is there some reason that we cannot just remove that nfsd_permission() check?
>
> The only proof that this write is part of the open is a stateid provided
> as part of the write arguments. Anyone could sniff or guess that
> stateid.
But a stateid is tied to a clientid and the clientid is tied to server
credentials....??
I guess it is harder than I at first imagined.
>
> We could try to make that work by checking the stateid against the
> principal from the rpc header. Unfortunately that turns out to be more
> complicated than "is this the same principal as did the open"; among
> other things I think it's possible the stateid resulted from opens done
> by different principals, so we'd need to keep a list. If we added that
> kind of check, could we drop the per-operation check? It's not obvious
> to me.
Delegations would certainly make that interesting. Who exactly does
authentic writes when a delegation is flushed ... I don't remember.
Sounds like this belongs in the too-hard basket.
Thanks,
NeilBrown
On Fri, May 22, 2015 at 09:01:27AM +1000, NeilBrown wrote:
> On Thu, 21 May 2015 09:07:35 -0400 "J. Bruce Fields" <[email protected]>
> wrote:
>
> > On Thu, May 21, 2015 at 02:40:29PM +1000, NeilBrown wrote:
> > >
> > >
> > > Apologies if this has been answered before, however...
> > >
> > > In nfsd_write() we have:
> > >
> > > if (file) {
> > > err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> > > NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE);
> > > if (err)
> > > goto out;
> > > err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen, cnt,
> > > stablep);
> > > } else {
> > >
> > > So if a 'file' is already available - because the request came via NFSv4 and
> > > there was a valid state id, and a 'struct file' was associated with that
> > > state - we still call nfsd_permission().
> > >
> > > Is that really needed? The permission check will have been performed at open
> > > - it shouldn't be needed again now.
> > >
> > > With NFSv3 we have to check permission at each IO, and this is slightly
> > > different from POSIX semantics. We shouldn't have to with NFSv4... should we?
> > >
> > > The particular issue that brought this to my attention is that "chattr +i" -
> > > to make a file immutable - is not supposed to affect current opens, only
> > > future opens. But a current open over NFSv4 is affected.
> > >
> > > Is there some reason that we cannot just remove that nfsd_permission() check?
> >
> > The only proof that this write is part of the open is a stateid provided
> > as part of the write arguments. Anyone could sniff or guess that
> > stateid.
>
> But a stateid is tied to a clientid
The stateid does have sufficient information to allow the server to
determine which NFSv4 client it's associated with.
> and the clientid is tied to server credentials....??
I'm not sure what you're thinking of there. It could conceivably be
tied to some kind of client credentials, but it isn't really. A write
is sent as the user performing the write, but could come from any IP
address, from a different user than did the open or the setclientid,
etc.
4.1 server state protection does give us more of a client credential,
but I don't know that that helps here.
> I guess it is harder than I at first imagined.
>
> >
> > We could try to make that work by checking the stateid against the
> > principal from the rpc header. Unfortunately that turns out to be more
> > complicated than "is this the same principal as did the open"; among
> > other things I think it's possible the stateid resulted from opens done
> > by different principals, so we'd need to keep a list. If we added that
> > kind of check, could we drop the per-operation check? It's not obvious
> > to me.
>
> Delegations would certainly make that interesting. Who exactly does
> authentic writes when a delegation is flushed ... I don't remember.
>
> Sounds like this belongs in the too-hard basket.
That's been my conclusion so far.
--b.
On Thu, 21 May 2015 14:40:29 +1000
NeilBrown <[email protected]> wrote:
>
>
> Apologies if this has been answered before, however...
>
> In nfsd_write() we have:
>
> if (file) {
> err = nfsd_permission(rqstp, fhp->fh_export, fhp->fh_dentry,
> NFSD_MAY_WRITE|NFSD_MAY_OWNER_OVERRIDE);
> if (err)
> goto out;
> err = nfsd_vfs_write(rqstp, fhp, file, offset, vec, vlen, cnt,
> stablep);
> } else {
>
> So if a 'file' is already available - because the request came via NFSv4 and
> there was a valid state id, and a 'struct file' was associated with that
> state - we still call nfsd_permission().
>
> Is that really needed? The permission check will have been performed at open
> - it shouldn't be needed again now.
>
> With NFSv3 we have to check permission at each IO, and this is slightly
> different from POSIX semantics. We shouldn't have to with NFSv4... should we?
>
> The particular issue that brought this to my attention is that "chattr +i" -
> to make a file immutable - is not supposed to affect current opens, only
> future opens. But a current open over NFSv4 is affected.
>
> Is there some reason that we cannot just remove that nfsd_permission() check?
>
AIUI (maybe wrong), the stateid is mainly provided in WRITE for ordering
purposes (in case of another OPEN or OPEN_DOWNGRADE, or some locking is
also going on). From RFC5661:
Except when special stateids are used, the stateid value for a WRITE
request represents a value returned from a previous byte-range LOCK
or OPEN request or the stateid associated with a delegation. The
stateid identifies the associated owners if any and is used by the
server to verify that the associated locks are still valid (e.g.,
have not been revoked).
That last bit is the significant part here...
The actual info for the write itself is supposed to be given in other
fields.
While, it probably is a good idea for the server to verify that the
current_fh and the fh associated with the stateid actually match, you
can't really do that with the creds.
In short, READs and WRITEs are really still quite NFSv3-like in NFSv4...
--
Jeff Layton <[email protected]>