Re: Corrupted RPC_GSS_PROC_DESTROY packets coming from Linux servers.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 6249 bytes --]

On Wed, 30 Jan 2013 23:19:20 +0000 "Myklebust, Trond"
<Trond.Myklebust@netapp.com> wrote:

> > -----Original Message-----
> > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs-
> > owner@vger.kernel.org] On Behalf Of Chuck Lever
> > Sent: Thursday, January 24, 2013 11:14 AM
> > To: NeilBrown
> > Cc: Kevin Coffman; J.Bruce Fields; Steve Dickson; NFS
> > Subject: Re: Corrupted RPC_GSS_PROC_DESTROY packets coming from Linux
> > servers.
> > 
> > 
> > On Jan 23, 2013, at 9:02 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> > >
> > > Hi peoples,
> > >
> > > this issue has appeared on the mailing list before (particularly
> > > around July
> > > 2011) but hasn't been resolved yet and it just bit me again so I
> > > figure it is time it got fixed.
> > >
> > >
> > > If you tcpdump the network connection while mounting an NFS filesystem
> > > using kerberos - or while the client is establishing a new context
> > > because e.g. the server rebooted - you will see a NULL RPC with an
> > > RPC_GSS_PROC_DESTROY credential but no verifier.  The lack of a
> > > verifier makes the packet corrupt so the server ignores it, but people
> > > see it and think something is wrong.
> > >
> > > It is good that the server ignores it as it really shouldn't be there.
> > > What happens is that the NFS client calls up to rpc.gssd to request a
> > > credential.  rpc.gssd then establishes a connection directly with the
> > > server, including the establishment of the security context.  Then it
> > > gathers the context details and passed them down to the kernel.
> > > Then it closes the connection part of which involves calling
> > > AUTH_DESTROY(auth) - necessary to free up data structures and not leak
> > > memory.
> > > This AUTH_DESTROY tries to destroy the context completely, including
> > > telling the server that it has been destroyed! But it hasn't, it has
> > > just been passed down to the kernel for use on a different connection.
> > >
> > > So there are two issues here:
> > >  - why is the GSS_PROC_DESTROY packet missing a verifier
> > >  - how can we get AUTH_DESTROY to *not* try to destroy the context on
> > the
> > >    server - as that would be a bad thing.
> > >
> > > The first I cannot completely answer.  I do  know that in libtirpc, in
> > > auth_gss.c, in authgss_marshal(), gss_get_mic is failing because it doesn't
> > > think it has a valid context.   I don't know why it thinks that, and I don't
> > > really care.
> > >
> > >
> > > The second question is more interesting and I see two possible options.
> > >
> > > 1/ If we knew why gss_get_mic failed and had good reason to believe it
> > > would keep on failing, we could consider changing clnt_vc_call to
> > > respond to an error from AUTH_MARSHALL not by sending a truncated
> > > packet, but by purging the current message and not sending it at all.
> > > This should be possible but might be messy.
> > >
> > > 2/ Make libtirpc behave more like librpcsecgss.
> > >  In libtirpc, the authgss_get_private_data() function just hands over
> > > a  pointer to the private data, but keeps its own pointer so it can
> > > free it  when the client is finally destroyed.
> > >
> > >  In librpcsecgss, since commit
> > > 07fce317cac267509b944a8191cafa8e49b5e328
> > >  (thanks Kevin), authgss_get_private_data() hands the data over to the
> > > caller and doesn't keep it's own reference to it.  So the caller has
> > > to call
> > >  authgss_free_private_data() when it has finished with the data.
> > >  As the library no longer has the credential, it doesn't even bother
> > > trying  to send a GSS_PROC_DESTROY request.
> > >
> > >  When Chuck noticed this difference between the two libraries, he
> > > resolved  it - in commit 336f8bca825416082d62ef38314f3e0b7e8f5cc2 as
> > follow:
> > >
> > >        if (token.value)
> > >                free(token.value);
> > > +#ifndef HAVE_LIBTIRPC
> > >        if (pd.pd_ctx_hndl.length != 0)
> > >                authgss_free_private_data(&pd);
> > > +#endif
> > >
> > >  Clearly to significance of this difference was not obvious, and this
> > > was  the easiest fix.
> > >
> > >  If we were to "fix" this properly, we would need to add a commit like
> > > the  one from Kevin to libtirpc, and remove that #ifndef from nfs-utils.
> > >  co-ordinating this might be tricking.  nfs-utils could presumably
> > > test if  libtirpc provided the function (at configure time) and call
> > > it if it does,
> > 
> > This seems to me like the best approach for 2.
> > 
> > >  However is someone updates libtirpc without updating or recompiling
> > > nfs-utils they would get a memory leak.  May it would be slow enough
> > > not to  be serious, and if anyone noticed that could just upgrade and get a
> > fix.
> > 
> > Telling people to upgrade for a fix is what we do for a living.  In all
> > seriousness, though, in the common case, people will be using nfs-utils and
> > libtirpc built by distributions, and we expect the distros will get the fix
> > dependency right over time.
> > 
> > >  Does this seem reasonable?   How is maintaining libtirpc these days?
> > >  Could we get the fix into 0.2.3, or would we need a minor version
> > > bump to  0.3.0??
> > 
> > A minor version bump shouldn't be necessary if we're not changing the
> > synopsis of a published API, nor are we removing a published API.
> > 
> > > 3/ there is actually a third option.  We could change
> > > authgss_get_private_data() to set gc.gc_ctx.length to 0, but not free
> > > the buffer.  Then aithgss_destroy_context() could notice that the
> > > length is zero and the buffer is not NULL, and could free the buffer but not
> > try to send
> > > the context_destroy request.   It's an ugly hack though and I think I'd
> > > rather not.
> 
> 4/ Have authgss_get_private_data() consume the 'auth' argument.
> 
> Reusing the auth in an RPC call after we've transferred the context to the kernel is in any case a bug, so why allow it at all?
> 

This is exactly the same as '2' - though stated much more succinctly.  It
looks like it is the approach that everyone prefers.
I'll send some patches.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]