| Message ID | 20120809183557.GA10591@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote: > Sorry not to notice this before--the below causes a regression against > the Linux server; something like: > > # mount -osec=krb5i pip1:/exports /mnt/ > # echo "test" >/mnt/test > # umount /mnt/ > # mount -osec=krb5 pip1:/exports /mnt/ > # echo "test" >/mnt/test > bash: /mnt/test: Operation not permitted > > This fails after the below commit on the client, but not before, thanks > to the server rejecting the second setclientid with CLID_INUSE due to a > different security flavor. This was part of a series where the last few patches got dropped for other problems. Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature. One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support. That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases. > I haven't really thought through who's at fault here. The second > setclientid does lack some level of security that the former had, so I > think the server's within its rights to complain. I don't think the second _lacks_ a level of security: I think the second is using a _different_ security flavor with the same nfs_client_id4.id string. NFS4ERR_CLID_INUSE is the correct server response in this case. > --b. > > commit de734831224e74fcaf8917386e33644c4243db95 > Author: Chuck Lever <chuck.lever@oracle.com> > Date: Wed Jul 11 16:30:50 2012 -0400 > > NFS: Treat NFS4ERR_CLID_INUSE as a fatal error > > For NFSv4 minor version 0, currently the cl_id_uniquifier allows the > Linux client to generate a unique nfs_client_id4 string whenever a > server replies with NFS4ERR_CLID_INUSE. > > This implementation seems to be based on a flawed reading of RFC > 3530. NFS4ERR_CLID_INUSE actually means that the client has presented > this nfs_client_id4 string with a different principal at some time in > the past, and that lease is still in use on the server. > > For a Linux client this might be rather difficult to achieve: the > authentication flavor is named right in the nfs_client_id4.id > string. If we change flavors, we change strings automatically. > > So, practically speaking, NFS4ERR_CLID_INUSE means there is some other > client using our string. There is not much that can be done to > recover automatically. Let's make it a permanent error. > > Remove the recovery logic in nfs4_proc_setclientid(), and remove the > cl_id_uniquifier field from the nfs_client data structure. And, > remove the authentication flavor from the nfs_client_id4 string. > > Keeping the authentication flavor in the nfs_client_id4.id string > means that we could have a separate lease for each authentication > flavor used by mounts on the client. But we want just one lease for > all the mounts on this client. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 74dcd85..1148081 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -4029,42 +4029,28 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program, > .rpc_resp = res, > .rpc_cred = cred, > }; > - int loop = 0; > - int status; > > + /* nfs_client_id4 */ > nfs4_init_boot_verifier(clp, &sc_verifier); > - > - for(;;) { > - rcu_read_lock(); > - setclientid.sc_name_len = scnprintf(setclientid.sc_name, > - sizeof(setclientid.sc_name), "%s/%s %s %s %u", > - clp->cl_ipaddr, > - rpc_peeraddr2str(clp->cl_rpcclient, > - RPC_DISPLAY_ADDR), > - rpc_peeraddr2str(clp->cl_rpcclient, > - RPC_DISPLAY_PROTO), > - clp->cl_rpcclient->cl_auth->au_ops->au_name, > - clp->cl_id_uniquifier); > - setclientid.sc_netid_len = scnprintf(setclientid.sc_netid, > + rcu_read_lock(); > + setclientid.sc_name_len = scnprintf(setclientid.sc_name, > + sizeof(setclientid.sc_name), "%s/%s %s", > + clp->cl_ipaddr, > + rpc_peeraddr2str(clp->cl_rpcclient, > + RPC_DISPLAY_ADDR), > + rpc_peeraddr2str(clp->cl_rpcclient, > + RPC_DISPLAY_PROTO)); > + /* cb_client4 */ > + setclientid.sc_netid_len = scnprintf(setclientid.sc_netid, > sizeof(setclientid.sc_netid), > rpc_peeraddr2str(clp->cl_rpcclient, > RPC_DISPLAY_NETID)); > - setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr, > + rcu_read_unlock(); > + setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr, > sizeof(setclientid.sc_uaddr), "%s.%u.%u", > clp->cl_ipaddr, port >> 8, port & 255); > - rcu_read_unlock(); > > - status = rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT); > - if (status != -NFS4ERR_CLID_INUSE) > - break; > - if (loop != 0) { > - ++clp->cl_id_uniquifier; > - break; > - } > - ++loop; > - ssleep(clp->cl_lease_time / HZ + 1); > - } > - return status; > + return rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT); > } > > int nfs4_proc_setclientid_confirm(struct nfs_client *clp, > @@ -5262,10 +5248,9 @@ int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred) > nfs4_init_boot_verifier(clp, &verifier); > > args.id_len = scnprintf(args.id, sizeof(args.id), > - "%s/%s/%u", > + "%s/%s", > clp->cl_ipaddr, > - clp->cl_rpcclient->cl_nodename, > - clp->cl_rpcclient->cl_auth->au_flavor); > + clp->cl_rpcclient->cl_nodename); > > res.server_owner = kzalloc(sizeof(struct nfs41_server_owner), > GFP_NOFS); > diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c > index 1cfc460..81eabcd 100644 > --- a/fs/nfs/nfs4state.c > +++ b/fs/nfs/nfs4state.c > @@ -1606,10 +1606,15 @@ static int nfs4_handle_reclaim_lease_error(struct nfs_client *clp, int status) > return -ESERVERFAULT; > /* Lease confirmation error: retry after purging the lease */ > ssleep(1); > - case -NFS4ERR_CLID_INUSE: > case -NFS4ERR_STALE_CLIENTID: > clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state); > break; > + case -NFS4ERR_CLID_INUSE: > + pr_err("NFS: Server %s reports our clientid is in use\n", > + clp->cl_hostname); > + nfs_mark_client_ready(clp, -EPERM); > + clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state); > + return -EPERM; > case -EACCES: > if (clp->cl_machine_cred == NULL) > return -EACCES; > diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h > index f58325a..6532765 100644 > --- a/include/linux/nfs_fs_sb.h > +++ b/include/linux/nfs_fs_sb.h > @@ -69,10 +69,9 @@ struct nfs_client { > struct idmap * cl_idmap; > > /* Our own IP address, as a null-terminated string. > - * This is used to generate the clientid, and the callback address. > + * This is used to generate the mv0 callback address. > */ > char cl_ipaddr[48]; > - unsigned char cl_id_uniquifier; > u32 cl_cb_ident; /* v4.0 callback identifier */ > const struct nfs4_minor_version_ops *cl_mvops; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote: > > On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote: > > > Sorry not to notice this before--the below causes a regression against > > the Linux server; something like: > > > > # mount -osec=krb5i pip1:/exports /mnt/ > > # echo "test" >/mnt/test > > # umount /mnt/ > > # mount -osec=krb5 pip1:/exports /mnt/ > > # echo "test" >/mnt/test > > bash: /mnt/test: Operation not permitted > > > > This fails after the below commit on the client, but not before, thanks > > to the server rejecting the second setclientid with CLID_INUSE due to a > > different security flavor. > > This was part of a series where the last few patches got dropped for other problems. Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature. > > One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support. That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases. That should work. > > I haven't really thought through who's at fault here. The second > > setclientid does lack some level of security that the former had, so I > > think the server's within its rights to complain. > > I don't think the second _lacks_ a level of security: I think the second is using a _different_ security flavor with the same nfs_client_id4.id string. NFS4ERR_CLID_INUSE is the correct server response in this case. OK. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote: > On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote: > > > > On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote: > > > > > Sorry not to notice this before--the below causes a regression against > > > the Linux server; something like: > > > > > > # mount -osec=krb5i pip1:/exports /mnt/ > > > # echo "test" >/mnt/test > > > # umount /mnt/ > > > # mount -osec=krb5 pip1:/exports /mnt/ > > > # echo "test" >/mnt/test > > > bash: /mnt/test: Operation not permitted > > > > > > This fails after the below commit on the client, but not before, thanks > > > to the server rejecting the second setclientid with CLID_INUSE due to a > > > different security flavor. > > > > This was part of a series where the last few patches got dropped for other problems. Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature. > > > > One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support. That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases. > > That should work. Whoops, no, au_name is just "RPCSEC_GSS" in both cases. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 9, 2012, at 4:38 PM, J. Bruce Fields wrote: > On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote: >> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote: >>> >>> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote: >>> >>>> Sorry not to notice this before--the below causes a regression against >>>> the Linux server; something like: >>>> >>>> # mount -osec=krb5i pip1:/exports /mnt/ >>>> # echo "test" >/mnt/test >>>> # umount /mnt/ >>>> # mount -osec=krb5 pip1:/exports /mnt/ >>>> # echo "test" >/mnt/test >>>> bash: /mnt/test: Operation not permitted >>>> >>>> This fails after the below commit on the client, but not before, thanks >>>> to the server rejecting the second setclientid with CLID_INUSE due to a >>>> different security flavor. >>> >>> This was part of a series where the last few patches got dropped for other problems. Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature. >>> >>> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support. That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases. >> >> That should work. > > Whoops, no, au_name is just "RPCSEC_GSS" in both cases. Could you confirm that before that commit, the client had to send an additional SETCLIENTID with a new cl_id_uniquifier? We'll need to distinguish the pseudoflavor as well for GSS, apparently.
On Aug 9, 2012, at 4:46 PM, Chuck Lever wrote: > > On Aug 9, 2012, at 4:38 PM, J. Bruce Fields wrote: > >> On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote: >>> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote: >>>> >>>> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote: >>>> >>>>> Sorry not to notice this before--the below causes a regression against >>>>> the Linux server; something like: >>>>> >>>>> # mount -osec=krb5i pip1:/exports /mnt/ >>>>> # echo "test" >/mnt/test >>>>> # umount /mnt/ >>>>> # mount -osec=krb5 pip1:/exports /mnt/ >>>>> # echo "test" >/mnt/test >>>>> bash: /mnt/test: Operation not permitted >>>>> >>>>> This fails after the below commit on the client, but not before, thanks >>>>> to the server rejecting the second setclientid with CLID_INUSE due to a >>>>> different security flavor. >>>> >>>> This was part of a series where the last few patches got dropped for other problems. Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature. >>>> >>>> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support. That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases. >>> >>> That should work. >> >> Whoops, no, au_name is just "RPCSEC_GSS" in both cases. > > Could you confirm that before that commit, the client had to send an additional SETCLIENTID with a new cl_id_uniquifier? > > We'll need to distinguish the pseudoflavor as well for GSS, apparently. The client should present the same principal on both SETCLIENTID requests, shouldn't it? If the principal is the same, that's all that RFC 3530bis requires. Maybe I don't understand how GSS principals work.
On Thu, Aug 09, 2012 at 04:52:07PM -0400, Chuck Lever wrote: > > On Aug 9, 2012, at 4:46 PM, Chuck Lever wrote: > > > > > On Aug 9, 2012, at 4:38 PM, J. Bruce Fields wrote: > > > >> On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote: > >>> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote: > >>>> > >>>> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote: > >>>> > >>>>> Sorry not to notice this before--the below causes a regression against > >>>>> the Linux server; something like: > >>>>> > >>>>> # mount -osec=krb5i pip1:/exports /mnt/ > >>>>> # echo "test" >/mnt/test > >>>>> # umount /mnt/ > >>>>> # mount -osec=krb5 pip1:/exports /mnt/ > >>>>> # echo "test" >/mnt/test > >>>>> bash: /mnt/test: Operation not permitted > >>>>> > >>>>> This fails after the below commit on the client, but not before, thanks > >>>>> to the server rejecting the second setclientid with CLID_INUSE due to a > >>>>> different security flavor. > >>>> > >>>> This was part of a series where the last few patches got dropped for other problems. Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature. > >>>> > >>>> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support. That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases. > >>> > >>> That should work. > >> > >> Whoops, no, au_name is just "RPCSEC_GSS" in both cases. > > > > Could you confirm that before that commit, the client had to send an additional SETCLIENTID with a new cl_id_uniquifier? Confirmed. > > We'll need to distinguish the pseudoflavor as well for GSS, > > apparently. > > The client should present the same principal on both SETCLIENTID > requests, shouldn't it? If the principal is the same, that's all that > RFC 3530bis requires. Maybe I don't understand how GSS principals > work. Which rfc3530bis language are you looking at? I remember it being a bit vague. Looking at the spec.... "a deliberate change of the principal owner of the id string (such as the case of a client that changes security flavors, and under the new flavor, there is no mapping to the previous owner) will in rare cases result in NFS4ERR_CLID_INUSE." Which makes it sound like the server can arbitrarily decide how to map principals sent with different flavors--which doesn't offer much guidance about what to do. The server could just compare principal strings (and ignore pseudoflavors) in the gss case. If the intention is to ensure that a clientid can't be "hijacked" by someone malicious, then you don't want to allow a krb5 setclientid to blow away a clientid established with krb5i. (If sending the setclientid with krb5i indicates the client wants protection against attacks which replace the body of the rpc, then a later krb5 setclientid should be rejected, since it could be the product of such an attack.) --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote: > On Thu, Aug 09, 2012 at 04:52:07PM -0400, Chuck Lever wrote: >> >> On Aug 9, 2012, at 4:46 PM, Chuck Lever wrote: >> >>> >>> On Aug 9, 2012, at 4:38 PM, J. Bruce Fields wrote: >>> >>>> On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote: >>>>> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote: >>>>>> >>>>>> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote: >>>>>> >>>>>>> Sorry not to notice this before--the below causes a regression against >>>>>>> the Linux server; something like: >>>>>>> >>>>>>> # mount -osec=krb5i pip1:/exports /mnt/ >>>>>>> # echo "test" >/mnt/test >>>>>>> # umount /mnt/ >>>>>>> # mount -osec=krb5 pip1:/exports /mnt/ >>>>>>> # echo "test" >/mnt/test >>>>>>> bash: /mnt/test: Operation not permitted >>>>>>> >>>>>>> This fails after the below commit on the client, but not before, thanks >>>>>>> to the server rejecting the second setclientid with CLID_INUSE due to a >>>>>>> different security flavor. >>>>>> >>>>>> This was part of a series where the last few patches got dropped for other problems. Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature. >>>>>> >>>>>> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support. That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases. >>>>> >>>>> That should work. >>>> >>>> Whoops, no, au_name is just "RPCSEC_GSS" in both cases. >>> >>> Could you confirm that before that commit, the client had to send an additional SETCLIENTID with a new cl_id_uniquifier? > > Confirmed. > >>> We'll need to distinguish the pseudoflavor as well for GSS, >>> apparently. >> >> The client should present the same principal on both SETCLIENTID >> requests, shouldn't it? If the principal is the same, that's all that >> RFC 3530bis requires. Maybe I don't understand how GSS principals >> work. > > Which rfc3530bis language are you looking at? > > I remember it being a bit vague. Looking at the spec.... > > "a deliberate change of the principal owner of the id string > (such as the case of a client that changes security flavors, and > under the new flavor, there is no mapping to the previous owner) > will in rare cases result in NFS4ERR_CLID_INUSE." > > Which makes it sound like the server can arbitrarily decide how to map > principals sent with different flavors--which doesn't offer much > guidance about what to do. Not sure it's that liberal. We'd have to ask for some consensus about the interpretation of that paragraph. But... The authenticating principal is what matters, I'd think. Kerberos itself is supposed to provide just strong authentication here, so krb5 == krb5i == krb5p. I don't see how the same principal presented with krb5 and with krb5i could represent two different entities, but my understanding of this stuff is shallow. > The server could just compare principal strings (and ignore > pseudoflavors) in the gss case. Barring a correction about how GSS Kerberos principals work, I think that's the correct approach. > If the intention is to ensure that a clientid can't be "hijacked" by > someone malicious, then you don't want to allow a krb5 setclientid to > blow away a clientid established with krb5i. (If sending the > setclientid with krb5i indicates the client wants protection against > attacks which replace the body of the rpc, then a later krb5 setclientid > should be rejected, since it could be the product of such an attack.) If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis). The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string. A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic. I can't think of a good reason it should do this.
On Thu, Aug 09, 2012 at 05:29:47PM -0400, Chuck Lever wrote: > > On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote: > > The server could just compare principal strings (and ignore > > pseudoflavors) in the gss case. > > Barring a correction about how GSS Kerberos principals work, I think that's the correct approach. I also don't know if principals produced by other mechanisms are going to be comparable. Of course that's a bit academic given that kerberos is the only mechanism for the forseeable future. > > If the intention is to ensure that a clientid can't be "hijacked" by > > someone malicious, then you don't want to allow a krb5 setclientid to > > blow away a clientid established with krb5i. (If sending the > > setclientid with krb5i indicates the client wants protection against > > attacks which replace the body of the rpc, then a later krb5 setclientid > > should be rejected, since it could be the product of such an attack.) > > If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis). > > The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string. A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic. I can't think of a good reason it should do this. You don't know it's the same client any more. The attack is: client sends setclientid with krb5i. client later sends some other rpc with krb5. attacker intercepts that operation, takes the rpc header, strips out the body, and replaces the body by a setclientid operation destroying the client's state. Since krb5 only protects the header, the server can't tell that's what's happened. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 9, 2012, at 5:38 PM, J. Bruce Fields wrote: > On Thu, Aug 09, 2012 at 05:29:47PM -0400, Chuck Lever wrote: >> >> On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote: >>> The server could just compare principal strings (and ignore >>> pseudoflavors) in the gss case. >> >> Barring a correction about how GSS Kerberos principals work, I think that's the correct approach. > > I also don't know if principals produced by other mechanisms are going > to be comparable. Of course that's a bit academic given that kerberos > is the only mechanism for the forseeable future. Agreed, I would expect this to work only for pseudoflavors that share the same type of principal. Let me be sure I understand... I think the text of RFC 3530bis allows this implementation choice, but you are suggesting below that such an implementation may allow an attack. Does this concern warrant a change to RFC 3530bis or 5661? Should we bring this up on nfsv4@ietf.org? > >>> If the intention is to ensure that a clientid can't be "hijacked" by >>> someone malicious, then you don't want to allow a krb5 setclientid to >>> blow away a clientid established with krb5i. (If sending the >>> setclientid with krb5i indicates the client wants protection against >>> attacks which replace the body of the rpc, then a later krb5 setclientid >>> should be rejected, since it could be the product of such an attack.) >> >> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis). >> >> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string. A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic. I can't think of a good reason it should do this. > > You don't know it's the same client any more. The attack is: > > client sends setclientid with krb5i. > > client later sends some other rpc with krb5. > > attacker intercepts that operation, takes the rpc header, strips > out the body, and replaces the body by a setclientid operation > destroying the client's state. > > Since krb5 only protects the header, the server can't tell that's what's > happened.
On Fri, Aug 10, 2012 at 03:38:30PM -0400, Chuck Lever wrote: > > On Aug 9, 2012, at 5:38 PM, J. Bruce Fields wrote: > > > On Thu, Aug 09, 2012 at 05:29:47PM -0400, Chuck Lever wrote: > >> > >> On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote: > >>> The server could just compare principal strings (and ignore > >>> pseudoflavors) in the gss case. > >> > >> Barring a correction about how GSS Kerberos principals work, I think that's the correct approach. > > > > I also don't know if principals produced by other mechanisms are going > > to be comparable. Of course that's a bit academic given that kerberos > > is the only mechanism for the forseeable future. > > Agreed, I would expect this to work only for pseudoflavors that share the same type of principal. > > Let me be sure I understand... I think the text of RFC 3530bis allows this implementation choice, but you are suggesting below that such an implementation may allow an attack. Does this concern warrant a change to RFC 3530bis or 5661? Should we bring this up on nfsv4@ietf.org? This should be all completely settled in 4.1. It's 4.0 that I'm confused by. What do you need the client to be able to do? Does it need to be able to perform setclientid's on the same client with different security flavors? In this particular case (mount/umount with one flavor, then mount/umount with another), maybe the client should destroy the client state (with an "I rebooted" setclientid) on the first umount? (I thought you had patches that did that?) --b. > > > > >>> If the intention is to ensure that a clientid can't be "hijacked" by > >>> someone malicious, then you don't want to allow a krb5 setclientid to > >>> blow away a clientid established with krb5i. (If sending the > >>> setclientid with krb5i indicates the client wants protection against > >>> attacks which replace the body of the rpc, then a later krb5 setclientid > >>> should be rejected, since it could be the product of such an attack.) > >> > >> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis). > >> > >> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string. A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic. I can't think of a good reason it should do this. > > > > You don't know it's the same client any more. The attack is: > > > > client sends setclientid with krb5i. > > > > client later sends some other rpc with krb5. > > > > attacker intercepts that operation, takes the rpc header, strips > > out the body, and replaces the body by a setclientid operation > > destroying the client's state. > > > > Since krb5 only protects the header, the server can't tell that's what's > > happened. > > -- > Chuck Lever > chuck[dot]lever[at]oracle[dot]com > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 10, 2012, at 4:13 PM, J. Bruce Fields wrote: > On Fri, Aug 10, 2012 at 03:38:30PM -0400, Chuck Lever wrote: >> >> On Aug 9, 2012, at 5:38 PM, J. Bruce Fields wrote: >> >>> On Thu, Aug 09, 2012 at 05:29:47PM -0400, Chuck Lever wrote: >>>> >>>> On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote: >>>>> The server could just compare principal strings (and ignore >>>>> pseudoflavors) in the gss case. >>>> >>>> Barring a correction about how GSS Kerberos principals work, I think that's the correct approach. >>> >>> I also don't know if principals produced by other mechanisms are going >>> to be comparable. Of course that's a bit academic given that kerberos >>> is the only mechanism for the forseeable future. >> >> Agreed, I would expect this to work only for pseudoflavors that share the same type of principal. >> >> Let me be sure I understand... I think the text of RFC 3530bis allows this implementation choice, but you are suggesting below that such an implementation may allow an attack. Does this concern warrant a change to RFC 3530bis or 5661? Should we bring this up on nfsv4@ietf.org? > > This should be all completely settled in 4.1. It's 4.0 that I'm > confused by. Then 3530bis only. > What do you need the client to be able to do? Does it need to be able > to perform setclientid's on the same client with different security > flavors? The problem is what client implementations really do, not what they "should" do. They should use a single SETCLIENTID with one flavor. Linux currently doesn't, and I'm guessing others are in the same boat. Since clients use different id strings for each flavor, the problem is avoided, but that isn't desirable client behavior. My concern above is with the spec. If it allows insecure implementations, maybe we should fix the spec. > In this particular case (mount/umount with one flavor, then mount/umount > with another), maybe the client should destroy the client state (with an > "I rebooted" setclientid) on the first umount? (I thought you had > patches that did that?) OK, I didn't notice that you were unmounting between the tests. 3.5 and before did update the boot verifier after the last mount of a server goes away, but that's also not desirable behavior. So I guess 3.6 also has my patch which makes the NFS client use the same boot verifier until it reboots, as God and the authors of 3530 intended? I think we agree that, whether the Linux server is compliant with what's currently in 3530bis or not, it's the NFS client changes in 3.6 that "introduced the regression" and should be fixed. If you change the client to add the flavor and pseudoflavor name to the nfs_client_id4.id string, does that fix the regression satisfactorily? Maybe we should just revert those two and try again in 3.7. > --b. > >> >>> >>>>> If the intention is to ensure that a clientid can't be "hijacked" by >>>>> someone malicious, then you don't want to allow a krb5 setclientid to >>>>> blow away a clientid established with krb5i. (If sending the >>>>> setclientid with krb5i indicates the client wants protection against >>>>> attacks which replace the body of the rpc, then a later krb5 setclientid >>>>> should be rejected, since it could be the product of such an attack.) >>>> >>>> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis). >>>> >>>> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string. A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic. I can't think of a good reason it should do this. >>> >>> You don't know it's the same client any more. The attack is: >>> >>> client sends setclientid with krb5i. >>> >>> client later sends some other rpc with krb5. >>> >>> attacker intercepts that operation, takes the rpc header, strips >>> out the body, and replaces the body by a setclientid operation >>> destroying the client's state. >>> >>> Since krb5 only protects the header, the server can't tell that's what's >>> happened. >> >> -- >> Chuck Lever >> chuck[dot]lever[at]oracle[dot]com >> >> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 10, 2012 at 04:33:46PM -0400, Chuck Lever wrote: > OK, I didn't notice that you were unmounting between the tests. 3.5 and before did update the boot verifier after the last mount of a server goes away, but that's also not desirable behavior. So I guess 3.6 also has my patch which makes the NFS client use the same boot verifier until it reboots, as God and the authors of 3530 intended? I don't know. (Why is that the right thing to do? Telling the server when you're not using the client any more seems reasonable to me.) > I think we agree that, whether the Linux server is compliant with what's currently in 3530bis or not, it's the NFS client changes in 3.6 that "introduced the regression" and should be fixed. If you change the client to add the flavor and pseudoflavor name to the nfs_client_id4.id string, does that fix the regression satisfactorily? I haven't tested it, but yes, that should do the job. (Though note that's not just a revert, since previously only the flavor (not the pseudoflavor) was factored into the clientid string.) --b. > > Maybe we should just revert those two and try again in 3.7. > > > --b. > > > >> > >>> > >>>>> If the intention is to ensure that a clientid can't be "hijacked" by > >>>>> someone malicious, then you don't want to allow a krb5 setclientid to > >>>>> blow away a clientid established with krb5i. (If sending the > >>>>> setclientid with krb5i indicates the client wants protection against > >>>>> attacks which replace the body of the rpc, then a later krb5 setclientid > >>>>> should be rejected, since it could be the product of such an attack.) > >>>> > >>>> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis). > >>>> > >>>> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string. A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic. I can't think of a good reason it should do this. > >>> > >>> You don't know it's the same client any more. The attack is: > >>> > >>> client sends setclientid with krb5i. > >>> > >>> client later sends some other rpc with krb5. > >>> > >>> attacker intercepts that operation, takes the rpc header, strips > >>> out the body, and replaces the body by a setclientid operation > >>> destroying the client's state. > >>> > >>> Since krb5 only protects the header, the server can't tell that's what's > >>> happened. > >> > >> -- > >> Chuck Lever > >> chuck[dot]lever[at]oracle[dot]com > >> > >> > >> > >> > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Chuck Lever > chuck[dot]lever[at]oracle[dot]com > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 10, 2012 at 04:39:30PM -0400, J. Bruce Fields wrote: > On Fri, Aug 10, 2012 at 04:33:46PM -0400, Chuck Lever wrote: > > OK, I didn't notice that you were unmounting between the tests. 3.5 and before did update the boot verifier after the last mount of a server goes away, but that's also not desirable behavior. So I guess 3.6 also has my patch which makes the NFS client use the same boot verifier until it reboots, as God and the authors of 3530 intended? > > I don't know. (Why is that the right thing to do? Telling the server > when you're not using the client any more seems reasonable to me.) And anyway can't the trick described in 2c820d9a97f07b273b2c8a5960bd52b1b5864c68 "NFS: Force server to drop NFSv4 state" be used to do that without changing the boot verifier? --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 10, 2012, at 4:39 PM, J. Bruce Fields wrote: > On Fri, Aug 10, 2012 at 04:33:46PM -0400, Chuck Lever wrote: >> OK, I didn't notice that you were unmounting between the tests. 3.5 and before did update the boot verifier after the last mount of a server goes away, but that's also not desirable behavior. So I guess 3.6 also has my patch which makes the NFS client use the same boot verifier until it reboots, as God and the authors of 3530 intended? > > I don't know. (Why is that the right thing to do? Telling the server > when you're not using the client any more seems reasonable to me.) The pre-3.6 Linux NFS client uses a different nfs_client_id4.id string for every server address, even server addresses that happen to be endpoints for the same server instance (multi-homed servers). That means the id and verifier are associated with a struct nfs_client. The id, because it gets the server's presentation address from the nfs_client; and the verifier, because the verifier is stored in the nfs_client itself. That struct nfs_client goes away when the client unmounts the last share on a server address. The next mount of that server address will get a new nfs_client and thus a fresh verifier. The id string stays the same, but is unique for that server address, so the apparent reboot should not interfere with other leases on that server. If the client uses the same id string for all server addresses (and I explain below why we want to go there) this changes things. If the client completely unmounts and remounts one of the server's addresses, the new nfs_client will contain a fresh boot verifier, but the id string is the same as the one the client uses on all the other server addresses. And that will result in the client's next SETCLIENTID canceling its own leases on the other server addresses for that server. So, if the client uses one id string for all servers, it must also use one boot verifier for all servers, to avoid clobbering its own leases on multi-homed servers. Having a single atomic client instance <id, verifier> is critical for making Transparent State Migration work properly. A destination server can not recognize migrated leased state for a client if the id string and boot verifier can't be matched to it because the client uses a different boot verifier and id string for every server it talks to. Can you explain why you think its a better idea for the client to remove its lease? For 4.0, it's never had to do that before. The case you've hit worked before by establishing an additional lease rather than by zapping the old one, for example. Moreover, it shouldn't matter much that a client doesn't tell a server when it's done, since the client's leases will quickly expire. Generally the client cleans up everything but the lease and clientid4 when it unmounts, unless the client crashes, which is hopefully rare (and couldn't result in an "I'm done" notification anyway). So, I don't see how leaving a lease could be a significant server-side resource issue, or how it might interfere with the operation of other clients. > >> I think we agree that, whether the Linux server is compliant with what's currently in 3530bis or not, it's the NFS client changes in 3.6 that "introduced the regression" and should be fixed. If you change the client to add the flavor and pseudoflavor name to the nfs_client_id4.id string, does that fix the regression satisfactorily? > > I haven't tested it, but yes, that should do the job. > > (Though note that's not just a revert, since previously only the flavor > (not the pseudoflavor) was factored into the clientid string.) Correct, that fix is not a revert.
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 74dcd85..1148081 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -4029,42 +4029,28 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program, .rpc_resp = res, .rpc_cred = cred, }; - int loop = 0; - int status; + /* nfs_client_id4 */ nfs4_init_boot_verifier(clp, &sc_verifier); - - for(;;) { - rcu_read_lock(); - setclientid.sc_name_len = scnprintf(setclientid.sc_name, - sizeof(setclientid.sc_name), "%s/%s %s %s %u", - clp->cl_ipaddr, - rpc_peeraddr2str(clp->cl_rpcclient, - RPC_DISPLAY_ADDR), - rpc_peeraddr2str(clp->cl_rpcclient, - RPC_DISPLAY_PROTO), - clp->cl_rpcclient->cl_auth->au_ops->au_name, - clp->cl_id_uniquifier); - setclientid.sc_netid_len = scnprintf(setclientid.sc_netid, + rcu_read_lock(); + setclientid.sc_name_len = scnprintf(setclientid.sc_name, + sizeof(setclientid.sc_name), "%s/%s %s", + clp->cl_ipaddr, + rpc_peeraddr2str(clp->cl_rpcclient, + RPC_DISPLAY_ADDR), + rpc_peeraddr2str(clp->cl_rpcclient, + RPC_DISPLAY_PROTO)); + /* cb_client4 */ + setclientid.sc_netid_len = scnprintf(setclientid.sc_netid, sizeof(setclientid.sc_netid), rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_NETID)); - setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr, + rcu_read_unlock(); + setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr, sizeof(setclientid.sc_uaddr), "%s.%u.%u", clp->cl_ipaddr, port >> 8, port & 255); - rcu_read_unlock(); - status = rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT); - if (status != -NFS4ERR_CLID_INUSE) - break; - if (loop != 0) { - ++clp->cl_id_uniquifier; - break; - } - ++loop; - ssleep(clp->cl_lease_time / HZ + 1); - } - return status; + return rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT); } int nfs4_proc_setclientid_confirm(struct nfs_client *clp, @@ -5262,10 +5248,9 @@ int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred) nfs4_init_boot_verifier(clp, &verifier); args.id_len = scnprintf(args.id, sizeof(args.id), - "%s/%s/%u", + "%s/%s", clp->cl_ipaddr, - clp->cl_rpcclient->cl_nodename, - clp->cl_rpcclient->cl_auth->au_flavor); + clp->cl_rpcclient->cl_nodename); res.server_owner = kzalloc(sizeof(struct nfs41_server_owner), GFP_NOFS); diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index 1cfc460..81eabcd 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -1606,10 +1606,15 @@ static int nfs4_handle_reclaim_lease_error(struct nfs_client *clp, int status) return -ESERVERFAULT; /* Lease confirmation error: retry after purging the lease */ ssleep(1); - case -NFS4ERR_CLID_INUSE: case -NFS4ERR_STALE_CLIENTID: clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state); break; + case -NFS4ERR_CLID_INUSE: + pr_err("NFS: Server %s reports our clientid is in use\n", + clp->cl_hostname); + nfs_mark_client_ready(clp, -EPERM); + clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state); + return -EPERM; case -EACCES: if (clp->cl_machine_cred == NULL) return -EACCES; diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h index f58325a..6532765 100644 --- a/include/linux/nfs_fs_sb.h +++ b/include/linux/nfs_fs_sb.h @@ -69,10 +69,9 @@ struct nfs_client { struct idmap * cl_idmap; /* Our own IP address, as a null-terminated string. - * This is used to generate the clientid, and the callback address. + * This is used to generate the mv0 callback address. */ char cl_ipaddr[48]; - unsigned char cl_id_uniquifier; u32 cl_cb_ident; /* v4.0 callback identifier */ const struct nfs4_minor_version_ops *cl_mvops;
Sorry not to notice this before--the below causes a regression against the Linux server; something like: # mount -osec=krb5i pip1:/exports /mnt/ # echo "test" >/mnt/test # umount /mnt/ # mount -osec=krb5 pip1:/exports /mnt/ # echo "test" >/mnt/test bash: /mnt/test: Operation not permitted This fails after the below commit on the client, but not before, thanks to the server rejecting the second setclientid with CLID_INUSE due to a different security flavor. I haven't really thought through who's at fault here. The second setclientid does lack some level of security that the former had, so I think the server's within its rights to complain. --b. commit de734831224e74fcaf8917386e33644c4243db95 Author: Chuck Lever <chuck.lever@oracle.com> Date: Wed Jul 11 16:30:50 2012 -0400 NFS: Treat NFS4ERR_CLID_INUSE as a fatal error For NFSv4 minor version 0, currently the cl_id_uniquifier allows the Linux client to generate a unique nfs_client_id4 string whenever a server replies with NFS4ERR_CLID_INUSE. This implementation seems to be based on a flawed reading of RFC 3530. NFS4ERR_CLID_INUSE actually means that the client has presented this nfs_client_id4 string with a different principal at some time in the past, and that lease is still in use on the server. For a Linux client this might be rather difficult to achieve: the authentication flavor is named right in the nfs_client_id4.id string. If we change flavors, we change strings automatically. So, practically speaking, NFS4ERR_CLID_INUSE means there is some other client using our string. There is not much that can be done to recover automatically. Let's make it a permanent error. Remove the recovery logic in nfs4_proc_setclientid(), and remove the cl_id_uniquifier field from the nfs_client data structure. And, remove the authentication flavor from the nfs_client_id4 string. Keeping the authentication flavor in the nfs_client_id4.id string means that we could have a separate lease for each authentication flavor used by mounts on the client. But we want just one lease for all the mounts on this client. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html