* [PATCH 07/44] nfsd41: create_session check replay first
@ 2009-06-16 1:19 Benny Halevy
2009-06-16 20:47 ` J. Bruce Fields
0 siblings, 1 reply; 7+ messages in thread
From: Benny Halevy @ 2009-06-16 1:19 UTC (permalink / raw)
To: bfields; +Cc: pnfs, linux-nfs
From: Andy Adamson <andros@netapp.com>
Replay processing needs to preceed other error processing.
Signed-off-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
fs/nfsd/nfs4state.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index bfc808b..5aef525 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
}
conf->cl_slot.sl_seqid++;
} else if (unconf) {
- if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
- (ip_addr != unconf->cl_addr)) {
- status = nfserr_clid_inuse;
- goto out_cache;
- }
-
slot = &unconf->cl_slot;
status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
if (status) {
@@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
goto out;
}
+ if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
+ (ip_addr != unconf->cl_addr)) {
+ status = nfserr_clid_inuse;
+ goto out_cache;
+ }
+
slot->sl_seqid++; /* from 0 to 1 */
move_to_confirmed(unconf);
--
1.6.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 07/44] nfsd41: create_session check replay first
2009-06-16 1:19 [PATCH 07/44] nfsd41: create_session check replay first Benny Halevy
@ 2009-06-16 20:47 ` J. Bruce Fields
2009-06-16 21:16 ` J. Bruce Fields
2009-06-17 1:15 ` William A. (Andy) Adamson
0 siblings, 2 replies; 7+ messages in thread
From: J. Bruce Fields @ 2009-06-16 20:47 UTC (permalink / raw)
To: Benny Halevy; +Cc: pnfs, linux-nfs, andros
On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote:
> From: Andy Adamson <andros@netapp.com>
>
> Replay processing needs to preceed other error processing.
Why?
--b.
>
> Signed-off-by: Andy Adamson <andros@netapp.com>
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
> fs/nfsd/nfs4state.c | 12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index bfc808b..5aef525 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
> }
> conf->cl_slot.sl_seqid++;
> } else if (unconf) {
> - if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
> - (ip_addr != unconf->cl_addr)) {
> - status = nfserr_clid_inuse;
> - goto out_cache;
> - }
> -
> slot = &unconf->cl_slot;
> status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
> if (status) {
> @@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
> goto out;
> }
>
> + if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
> + (ip_addr != unconf->cl_addr)) {
> + status = nfserr_clid_inuse;
> + goto out_cache;
> + }
> +
> slot->sl_seqid++; /* from 0 to 1 */
> move_to_confirmed(unconf);
>
> --
> 1.6.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 07/44] nfsd41: create_session check replay first
2009-06-16 20:47 ` J. Bruce Fields
@ 2009-06-16 21:16 ` J. Bruce Fields
2009-06-17 1:22 ` [pnfs] " William A. (Andy) Adamson
2009-06-17 1:15 ` William A. (Andy) Adamson
1 sibling, 1 reply; 7+ messages in thread
From: J. Bruce Fields @ 2009-06-16 21:16 UTC (permalink / raw)
To: Benny Halevy; +Cc: pnfs, linux-nfs, andros
On Tue, Jun 16, 2009 at 04:47:34PM -0400, bfields wrote:
> On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote:
> > From: Andy Adamson <andros@netapp.com>
> >
> > Replay processing needs to preceed other error processing.
>
> Why?
Note: there's a slight leak of information here, which I don't think is
really important, but still by default would rather avoid: if a replay
is sent by someone other than the sender of the original create_session,
then sending back the cached result means they get to see a
create_session result that otherwise would have required them to sniff
the network (or decrypt a packet sent with the original user's creds, if
krb5p was in effect).
I doubt that's a big deal: I don't see anything that looks
security-critical in the create_session results. But absent any real
reason to do otherwise, I'd rather check the creds before checking for
replay.
(Note: this is more critical in the case of the sessions DRC: e.g. if
the cached result includes read data, then we could end up exposing
filesystem data to someone who wouldn't otherwise be able to see it.)
--b.
>
> --b.
>
> >
> > Signed-off-by: Andy Adamson <andros@netapp.com>
> > Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> > ---
> > fs/nfsd/nfs4state.c | 12 ++++++------
> > 1 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index bfc808b..5aef525 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
> > }
> > conf->cl_slot.sl_seqid++;
> > } else if (unconf) {
> > - if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
> > - (ip_addr != unconf->cl_addr)) {
> > - status = nfserr_clid_inuse;
> > - goto out_cache;
> > - }
> > -
> > slot = &unconf->cl_slot;
> > status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
> > if (status) {
> > @@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
> > goto out;
> > }
> >
> > + if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
> > + (ip_addr != unconf->cl_addr)) {
> > + status = nfserr_clid_inuse;
> > + goto out_cache;
> > + }
> > +
> > slot->sl_seqid++; /* from 0 to 1 */
> > move_to_confirmed(unconf);
> >
> > --
> > 1.6.3
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first
2009-06-16 20:47 ` J. Bruce Fields
2009-06-16 21:16 ` J. Bruce Fields
@ 2009-06-17 1:15 ` William A. (Andy) Adamson
[not found] ` <89c397150906161815v6136e000t338f4fce10ceff23-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 7+ messages in thread
From: William A. (Andy) Adamson @ 2009-06-17 1:15 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Benny Halevy, linux-nfs, pnfs
On Tue, Jun 16, 2009 at 4:47 PM, J. Bruce Fields<bfields@fieldses.org> wrote:
> On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Replay processing needs to preceed other error processing.
>
> Why?
>
Section 18.36.4. (CREATE_SESSION implementation section) states the
ordering of clientid confirmation processing as
1) client id record lookup
2) sequence id processing
3) client id confirmation
rpc cred processing is done in #3.
->Andy
> --b.
>
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>> fs/nfsd/nfs4state.c | 12 ++++++------
>> 1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index bfc808b..5aef525 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> }
>> conf->cl_slot.sl_seqid++;
>> } else if (unconf) {
>> - if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> - (ip_addr != unconf->cl_addr)) {
>> - status = nfserr_clid_inuse;
>> - goto out_cache;
>> - }
>> -
>> slot = &unconf->cl_slot;
>> status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
>> if (status) {
>> @@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> goto out;
>> }
>>
>> + if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> + (ip_addr != unconf->cl_addr)) {
>> + status = nfserr_clid_inuse;
>> + goto out_cache;
>> + }
>> +
>> slot->sl_seqid++; /* from 0 to 1 */
>> move_to_confirmed(unconf);
>>
>> --
>> 1.6.3
>>
> _______________________________________________
> pNFS mailing list
> pNFS@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first
2009-06-16 21:16 ` J. Bruce Fields
@ 2009-06-17 1:22 ` William A. (Andy) Adamson
0 siblings, 0 replies; 7+ messages in thread
From: William A. (Andy) Adamson @ 2009-06-17 1:22 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Benny Halevy, linux-nfs, pnfs
On Tue, Jun 16, 2009 at 5:16 PM, J. Bruce Fields<bfields@fieldses.org> wrote:
> On Tue, Jun 16, 2009 at 04:47:34PM -0400, bfields wrote:
>> On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote:
>> > From: Andy Adamson <andros@netapp.com>
>> >
>> > Replay processing needs to preceed other error processing.
>>
>> Why?
>
> Note: there's a slight leak of information here, which I don't think is
> really important, but still by default would rather avoid: if a replay
> is sent by someone other than the sender of the original create_session,
> then sending back the cached result means they get to see a
> create_session result that otherwise would have required them to sniff
> the network (or decrypt a packet sent with the original user's creds, if
> krb5p was in effect).
>
> I doubt that's a big deal: I don't see anything that looks
> security-critical in the create_session results. But absent any real
> reason to do otherwise, I'd rather check the creds before checking for
> replay.
>
> (Note: this is more critical in the case of the sessions DRC: e.g. if
> the cached result includes read data, then we could end up exposing
> filesystem data to someone who wouldn't otherwise be able to see it.)
We should figure out credential checking for both create session and
sequence drc...
-->Andy
>
> --b.
>
>>
>> --b.
>>
>> >
>> > Signed-off-by: Andy Adamson <andros@netapp.com>
>> > Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> > ---
>> > fs/nfsd/nfs4state.c | 12 ++++++------
>> > 1 files changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> > index bfc808b..5aef525 100644
>> > --- a/fs/nfsd/nfs4state.c
>> > +++ b/fs/nfsd/nfs4state.c
>> > @@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> > }
>> > conf->cl_slot.sl_seqid++;
>> > } else if (unconf) {
>> > - if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> > - (ip_addr != unconf->cl_addr)) {
>> > - status = nfserr_clid_inuse;
>> > - goto out_cache;
>> > - }
>> > -
>> > slot = &unconf->cl_slot;
>> > status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
>> > if (status) {
>> > @@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> > goto out;
>> > }
>> >
>> > + if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> > + (ip_addr != unconf->cl_addr)) {
>> > + status = nfserr_clid_inuse;
>> > + goto out_cache;
>> > + }
>> > +
>> > slot->sl_seqid++; /* from 0 to 1 */
>> > move_to_confirmed(unconf);
>> >
>> > --
>> > 1.6.3
>> >
> _______________________________________________
> pNFS mailing list
> pNFS@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first
[not found] ` <89c397150906161815v6136e000t338f4fce10ceff23-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2009-06-17 1:25 ` J. Bruce Fields
2009-06-17 1:39 ` William A. (Andy) Adamson
0 siblings, 1 reply; 7+ messages in thread
From: J. Bruce Fields @ 2009-06-17 1:25 UTC (permalink / raw)
To: William A. (Andy) Adamson; +Cc: Benny Halevy, linux-nfs, pnfs
On Tue, Jun 16, 2009 at 09:15:32PM -0400, William A. (Andy) Adamson wrote:
> On Tue, Jun 16, 2009 at 4:47 PM, J. Bruce Fields<bfields@fieldses.org> wrote:
> > On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote:
> >> From: Andy Adamson <andros@netapp.com>
> >>
> >> Replay processing needs to preceed other error processing.
> >
> > Why?
> >
>
> Section 18.36.4. (CREATE_SESSION implementation section) states the
> ordering of clientid confirmation processing as
> 1) client id record lookup
> 2) sequence id processing
> 3) client id confirmation
>
> rpc cred processing is done in #3.
I don't see any reason we can't check the cred earlier--let's just leave
this as is and drop this patch.
--b.
>
> ->Andy
>
> > --b.
> >
> >>
> >> Signed-off-by: Andy Adamson <andros@netapp.com>
> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> >> ---
> >> fs/nfsd/nfs4state.c | 12 ++++++------
> >> 1 files changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index bfc808b..5aef525 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
> >> }
> >> conf->cl_slot.sl_seqid++;
> >> } else if (unconf) {
> >> - if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
> >> - (ip_addr != unconf->cl_addr)) {
> >> - status = nfserr_clid_inuse;
> >> - goto out_cache;
> >> - }
> >> -
> >> slot = &unconf->cl_slot;
> >> status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
> >> if (status) {
> >> @@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
> >> goto out;
> >> }
> >>
> >> + if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
> >> + (ip_addr != unconf->cl_addr)) {
> >> + status = nfserr_clid_inuse;
> >> + goto out_cache;
> >> + }
> >> +
> >> slot->sl_seqid++; /* from 0 to 1 */
> >> move_to_confirmed(unconf);
> >>
> >> --
> >> 1.6.3
> >>
> > _______________________________________________
> > pNFS mailing list
> > pNFS@linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pnfs] [PATCH 07/44] nfsd41: create_session check replay first
2009-06-17 1:25 ` J. Bruce Fields
@ 2009-06-17 1:39 ` William A. (Andy) Adamson
0 siblings, 0 replies; 7+ messages in thread
From: William A. (Andy) Adamson @ 2009-06-17 1:39 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Benny Halevy, linux-nfs, pnfs
On Tue, Jun 16, 2009 at 9:25 PM, J. Bruce Fields<bfields@fieldses.org> wrote:
> On Tue, Jun 16, 2009 at 09:15:32PM -0400, William A. (Andy) Adamson wrote:
>> On Tue, Jun 16, 2009 at 4:47 PM, J. Bruce Fields<bfields@fieldses.org> wrote:
>> > On Tue, Jun 16, 2009 at 04:19:32AM +0300, Benny Halevy wrote:
>> >> From: Andy Adamson <andros@netapp.com>
>> >>
>> >> Replay processing needs to preceed other error processing.
>> >
>> > Why?
>> >
>>
>> Section 18.36.4. (CREATE_SESSION implementation section) states the
>> ordering of clientid confirmation processing as
>> 1) client id record lookup
>> 2) sequence id processing
>> 3) client id confirmation
>>
>> rpc cred processing is done in #3.
>
> I don't see any reason we can't check the cred earlier--let's just leave
> this as is and drop this patch.
you want to replace the cache with this error? I think at the very
least you need to goto out instead of out_cache.
note that a replay of a CREATE_SESSION on an unconfirmed clientid
returns NFS4ERR_SEQMISORDERED. So it doesn't matter at all what the
rpc_cred is.
The choice is between an NFS4ERR_SEQMISORDERED and NFS4ERR_CLIDINUSE.
I think the patch should stay.
-->Andy
>
> --b.
>
>>
>> ->Andy
>>
>> > --b.
>> >
>> >>
>> >> Signed-off-by: Andy Adamson <andros@netapp.com>
>> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> >> ---
>> >> fs/nfsd/nfs4state.c | 12 ++++++------
>> >> 1 files changed, 6 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> >> index bfc808b..5aef525 100644
>> >> --- a/fs/nfsd/nfs4state.c
>> >> +++ b/fs/nfsd/nfs4state.c
>> >> @@ -1378,12 +1378,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> >> }
>> >> conf->cl_slot.sl_seqid++;
>> >> } else if (unconf) {
>> >> - if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> >> - (ip_addr != unconf->cl_addr)) {
>> >> - status = nfserr_clid_inuse;
>> >> - goto out_cache;
>> >> - }
>> >> -
>> >> slot = &unconf->cl_slot;
>> >> status = check_slot_seqid(cr_ses->seqid, slot->sl_seqid, 0);
>> >> if (status) {
>> >> @@ -1392,6 +1386,12 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>> >> goto out;
>> >> }
>> >>
>> >> + if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
>> >> + (ip_addr != unconf->cl_addr)) {
>> >> + status = nfserr_clid_inuse;
>> >> + goto out_cache;
>> >> + }
>> >> +
>> >> slot->sl_seqid++; /* from 0 to 1 */
>> >> move_to_confirmed(unconf);
>> >>
>> >> --
>> >> 1.6.3
>> >>
>> > _______________________________________________
>> > pNFS mailing list
>> > pNFS@linux-nfs.org
>> > http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-06-17 1:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-06-16 1:19 [PATCH 07/44] nfsd41: create_session check replay first Benny Halevy
2009-06-16 20:47 ` J. Bruce Fields
2009-06-16 21:16 ` J. Bruce Fields
2009-06-17 1:22 ` [pnfs] " William A. (Andy) Adamson
2009-06-17 1:15 ` William A. (Andy) Adamson
[not found] ` <89c397150906161815v6136e000t338f4fce10ceff23-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-06-17 1:25 ` J. Bruce Fields
2009-06-17 1:39 ` William A. (Andy) Adamson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox