Message-ID: <20111020142407.GA2118@hallyn.com>
Date:	Thu, 20 Oct 2011 14:24:07 +0000
From:	"Serge E. Hallyn" <serge@...lyn.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	oleg@...hat.com, richard@....at, mikevs@...all.net,
	segoon@...nwall.com, gregkh@...e.de, dhowells@...hat.com,
	eparis@...hat.com, "Serge E. Hallyn" <serge.hallyn@...onical.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace
 aware

Quoting Eric W. Biederman (ebiederm@...ssion.com):
> "Serge E. Hallyn" <serge@...lyn.com> writes:
> 
> > Quoting Eric W. Biederman (ebiederm@...ssion.com):
> >> Serge Hallyn <serge@...lyn.com> writes:
> >> 
> >> > From: "Serge E. Hallyn" <serge.hallyn@...onical.com>
> >> >
> >> > Currently uids are compared without regard for the user namespace.
> >> > Fix that to prevent tasks in a different user namespace from
> >> > wrongly matching on SCM_CREDENTIALS.
> >> >
> >> > In the past, either your uids had to match, or you had to have
> >> > CAP_SETXID.  In a namespaced world, you must either (both be in the
> >> > same user namespace and have your uids match), or you must have
> >> > CAP_SETXID targeted at the other user namespace.  The latter can
> >> > happen for instance if uid 500 created a new user namespace and
> >> > now interacts with uid 0 in it.
> >> 
> >> Serge this approach is wrong.
> >
> > Thanks for looking, Eric.
> >
> >> Because we pass the cred and the pid through the socket socket itself
> >> is just a conduit and should be ignored in this context.
> >
> > Ok, that makes sense, but
> >
> >> The only interesting test should be are you allowed to impersonate other
> >> users in your current userk namespace.
> >
> > Why in your current user namespace?  Shouldn't it be in the
> > target user ns?  I understand it could be wrong to tie the
> > user ns owning the socket to the target userns (though I still
> > kind of like it), but just because I have CAP_SETUID in my
> > own user_ns doesn't mean I should be able to pose as another
> > uid in your user_ns.
> 
> First and foremost it is important that you be able if you have the
> capability to impersonate other users in your current user namespace.
> That is what the capability actually controls.
> 
> None of this allows you to impersonate any user in any other user
> namespace.  The translation between users prevents that.
> 
> > (Now I also see that cred_to_ucred() translates to the current
> > user_ns, so that should have been a hint to me before about
> > your intent, but I'm not convinced I agree with your intent).
> >
> > And you do the same with the pid.  Why is that a valid assumption?
> 
> Yes.  Basically all the code is allow you to impersonate people you
> would have been able to impersonate before.  If your target is in
> another namespace you can not fool them.
> 
> With pids the logic should be a lot clearer.  Pretend to be a pid you can
> see in your current pid namespace.  Lookup and convert to struct pid aka
> the namespace agnostic object.  On output return the pid value that
> the target process will know you as.
> 
> Ultimately I think we need a ns_capable for the current user namespace
> instead of a global one.  But I don't see any rush to introduce
> ns_capable here.

I think I agree - I was mistakenly thinking that without this patch
there is an opportunity for a less privileged task in child user ns
to impersonate, but that's not possible, so let's drop this patch
for now!

thanks,
-serge
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists