Re: Security issue in NFS localio

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Jul 04, 2024 at 08:24:44AM +1000, NeilBrown wrote:
> 
> I've been pondering security questions with localio - particularly
> wondering what questions I need to ask.  I've found three focal points
> which overlap but help me organise my thoughts:
> 1- the LOCALIO RPC protocol
> 2- the 'auth_domain' that nfsd uses to authorise access
> 3- the credential that is used to access the file

I'm missing some details that are critical to giving your hypothetical
attack vector teeth.  But I do think no matter what we need to be
precise with documenting localio's security in localio.rst.

> 1/ It occurs to me that I could find out the UUID reported by a given
> local server (just ask it over the RPC connection), find out the
> filehandle for some file that I don't have write access to (not too
> hard), and create a private NFS server (hacking nfs-ganasha?) which
> reports the same uuid and reports that I have access to a file with
> that filehandle.  If I then mount from that server inside a private
> container on the same host that is running the local server, I would get
> localio access to the target file.

Even if nfs-ganasha were modified to allow the nfs client's
nfs_init_localioclient() to establish access to the 'struct net'
associated with the knfsd (rpc_bind_new_program -> GETUUID ->
nfsd_uuid_is_local): what does that _really_ give the nfs client?

The nfs client still needs to call into nfsd with nfsd_open_local_fh()
which requires rpc_clnt->cl_auth to allow access to the server's
files.

In an earlier email Chuck asked why localio's svcauth_unix_set_client()
would ever fail, and he correctly answered his question with:
"Wouldn't it only be because the local application is trying to open a
file it doesn't have permission to?"

Yes, if a client never actually negotiated with the NFS server that
provides access to protected files, then the client must not be
granted access.  _This_ is why having a "fake" svc_rqst is important.
[Also "fake" isn't a great name considering it is still meant to
enforce required established NFS credentials.]

> I might not be able to write to it because of credential checking, but I
> think that is getting a lot closer to unauthorised access than I would
> like.

Kind of hand-wavy on the finale... but I'm also relieved ;)

That said, I appreciate the desire to avoid the "fake" svc_rqst based
access control.  So I still agree it is worthwhile to carry your
nfsd_file_acquire_local() series through to completion.

> I would much prefer it if there was no credible way to subvert the
> LOCALIO protocol.
>
> My current idea goes like this:
>  - NFS client tells nfs_common it is going to probe for localio
>    and gets back a nonce.  nfs_common records that this probe is happening
>  - NFS client sends the nonce to the server over LOCALIO.
>  - server tells nfs_common "I just got this nonce - does it mean
>    anything?".  If it does, the server gets connected with the client
>    through nfs_common.  The server reports success over LOCALIO.
>    If it doesn't the server reports failure of LOCALIO.
>  - NFS client gets the reply and tells nfs_common that it has a reply
>    so the nonce is invalidated.  If the reply was success and nfs_local
>    confirms there is a connection, then the two stay connected. 
> I think that having a nonce (single-use uuid) is better than using the
> same uuid for the life of the server, and I think that sending it
> proactively by client rather than reactively by the server is also
> safer.

Inverting and tweaking the localio protocol like this is clever, but
it still strikes me as unnecessary (given above).

Having it be less widely accessible is a good idea in general though.

> 2/ The localio access should use exactly the same auth_domain as the
>    network access uses.  This ensure the credentials implied by
>    rootsquash and allsquash are used correctly.  I think the current
>    code has the client guessing what IP address the server will see and
>    finding an auth_domain based on that.  I'm not comfortable with that.

nfsd_local_fakerqst_create() isn't guessing.  rpc_peeraddr() returns the
IP address of the server because the rpc_clnt is the same as
established for traditional network access.

>    In the new LOCALIO protocol I suggest above, the server registers
>    with nfs_common at the moment it receives an RPC request.  At that
>    moment it knows the characteristics of the connection - remote IP?
>    krb5?  tls?  - and can determine an auth_domain and give it to
>    nfs_common and so make it available to the client.
> 
>    Jeff wondered about an export option to explicitly enable LOCALIO.  I
>    had wondered about that too.  But I think that if we firmly tie the
>    localio auth_domain to the connection as above, that shouldn't be needed.

I do have concerns that your approach to use "exactly the same
auth_domain" isn't so much better than what the current localio code
does.  But I'll concede it sounds better than the hackish "fake"
svc_rqst based security of the current localio code.

Worth doing just to see how it all shakes out in benchmarks, and on a
"better approach" level, but I do currently have "speed kills" concerns.

> 3/ The current code uses the 'struct cred' of the application to look up
>    the file in the server code.  When a request goes over the wire the
>    credential is translated to uid/gid (or krb identity) and this is
>    mapped back to a credential on the server which might be in a
>    different uid name space (might it?  Does that even work for nfsd?)
>
>    I think that if rootsquash or allsquash is in effect the correct
>    server-side credential is used but otherwise the client-side
>    credential is used.  That is likely correct in many cases but I'd
>    like to be convinced that it is correct in all case.  Maybe it is
>    time to get a deeper understanding of uid name spaces.

You just made me feel slightly better about not knowing enough about
user namespaces.
 
> Have I missed anything?  Any other thoughts?

Here is the branch again that I'd like to use as a base for continued
_incremental_ localio development (make code evolution clearer, if you
want to rip and replace code, just remove in a separate commit.. I
can rebase to cleanup when the dust settles):
https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=nfs-localio-for-next

Thanks for spending time on all of this localio stuff!

It's now 4th of July for me, so I'm with Jeff: I need a drink! ;)

Mike