linux-kernel - Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to rpsec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130305231001.GJ15816@fieldses.org>
Date:	Tue, 5 Mar 2013 18:10:01 -0500
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	linux-fsdevel@...r.kernel.org,
	Linux Containers <containers@...ts.linux-foundation.org>,
	linux-kernel@...r.kernel.org, "Serge E. Hallyn" <serge@...lyn.com>,
	Trond Myklebust <Trond.Myklebust@...app.com>
Subject: Re: [PATCH review 49/85] sunrpc: Update svcgss xdr handle to
 rpsec_contect cache

On Mon, Mar 04, 2013 at 09:11:16AM -0800, Eric W. Biederman wrote:
> "J. Bruce Fields" <bfields@...ldses.org> writes:
> 
> 
> > krb5 mounts started failing for me as of this patch (upstream as
> > 683428fae8c73d7d7da0fa2e0b6beb4d8df4e808),
> 
> Ouch!
> 
> >  and I believe the problem is
> > these uid/gid_valid checks: if I recall correctly, gssd uses -1 uid/gid
> > values to indicate "authentication succeeded, but treat this user as
> > anonymous".  We delay mapping -1 id's to the (configurable-per-export)
> > anonymous id's till fs/nfsd/auth.c:nfsd_setuser():
> >
> >         if (uid_eq(new->fsuid, INVALID_UID))
> > 		new->fsuid = exp->ex_anon_uid;
> > 	if (gid_eq(new->fsgid, INVALID_GID))
> > 		new->fsgid = exp->ex_anon_gid;
> >
> > (We wouldn't be able to do that earlier because we don't know the export
> > yet.)
> >
> > Confirmed that the following fixes the regression.
> >
> > Eric, any objection to removing those checks?
> 
> Not strongly.  As long as we have the uid ang gid valid checks in
> there somewhere before we start using these uids and gids much.
> This does seems to be the case of using out of range uids and gids
> to mean out of range uids and gids.
> 
> In the description of my original patch before I split it up, I
> noted that one of the small dangers might be that I had added
> some possibly unneceessary uid and gid valid checks.  So it seems I had
> even considered this slight possibility once and noted that there was a
> slight chance we might have to remove a few of these.

Ah, I should have seen that and checked that, my bad.

> It would be nice to have a comment to say that we expect this to happen
> so people don't start wondering why there are missing checks on this
> path.

OK--something like the appended?

> There is also a gid_valid check in the secondary uids.  It looks like we
> should keep that as we don't have any checks for invalid gids in
> nfsd_setuser.  Is that possibly an oversight in nfsd_setuser?

Honestly I don't think anyone's thought that through....

But it would seem odd to pass down an "anonymous" supplementary gid--why
not just leave it off the list instead?

(BTW, dumb question: is it legal for userspace to use a uid -1?  And was
it illegal before the user namespace patches?)

> Also looking towards a future in which all of these things don't reside
> in the initial user namespace.  Is mapping any out of range uid to
> INVALID_UID and then into ex_anon_uid the always the right thing to do?
> 
> Or is -1 a very special case in this instance.  In which case we
> probably want checks that look like:

svcgssd should pass down either -1 or a valid uid, so I don't think
we're committed to any particular handling of invalid id's other than
-1--whatever's easiest.

--b.

commit d78f5cd062edb400190521e9540b042b4805875b
Author: J. Bruce Fields <bfields@...hat.com>
Date:   Mon Mar 4 08:44:01 2013 -0500

    nfsd: fix krb5 handling of anonymous principals
    
    krb5 mounts started failing as of
    683428fae8c73d7d7da0fa2e0b6beb4d8df4e808 "sunrpc: Update svcgss xdr
    handle to rpsec_contect cache".
    
    The problem is that mounts are usually done with some host principal
    which isn't normally mapped to any user, in which case svcgssd passes
    down uid -1, which the kernel is then expected to map to the
    export-specific anonymous uid or gid.
    
    The new uid_valid/gid_valid checks were therefore causing that downcall
    to fail.
    
    (Note the regression may not have been seen with older userspace that
    tended to map unknown principals to an anonymous id on their own rather
    than leaving it to the kernel.)
    
    Signed-off-by: J. Bruce Fields <bfields@...hat.com>

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index f7d34e7..c2ab26f 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -447,17 +447,21 @@ static int rsc_parse(struct cache_detail *cd,
 	else {
 		int N, i;
 
+		/*
+		 * NOTE: we don't check uid_valid() or gid_valid(): instead,
+		 * -1 id's are later mapped to the (export-specific)
+		 * anonymous id by nfsd_setuser.
+		 *
+		 * (But supplementary gid's get no such special
+		 * treatment so are checked for validity here.)
+		 */
 		/* uid */
 		rsci.cred.cr_uid = make_kuid(&init_user_ns, id);
-		if (!uid_valid(rsci.cred.cr_uid))
-			goto out;
 
 		/* gid */
 		if (get_int(&mesg, &id))
 			goto out;
 		rsci.cred.cr_gid = make_kgid(&init_user_ns, id);
-		if (!gid_valid(rsci.cred.cr_gid))
-			goto out;
 
 		/* number of additional gid's */
 		if (get_int(&mesg, &N))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/