[PATCH V2] net: Allow xt_owner in any user namespace

[PATCH V2] net: Allow xt_owner in any user namespace

[Kevin Cernekee]

From: "Eric W. Biederman" <ebiederm@xmission.com>

Making this work is a little tricky as it really isn't kosher to
change the xt_owner_match_info in a check function.

Without changing xt_owner_match_info we need to know the user
namespace the uids and gids are specified in.  In the common case
net->user_ns == current_user_ns().  Verify net->user_ns ==
current_user_ns() in owner_check so we can later assume it in
owner_mt.

In owner_check also verify that all of the uids and gids specified are
in net->user_ns and that the expected min/max relationship exists
between the uids and gids in xt_owner_match_info.

In owner_mt get the network namespace from the outgoing socket, as this
must be the same network namespace as the netfilter rules, and use that
network namespace to find the user namespace the uids and gids in
xt_match_owner_info are encoded in.  Then convert from their encoded
from into the kernel internal format for uids and gids and perform the
owner match.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
---


Original post:
https://lists.linuxfoundation.org/pipermail/containers/2014-May/034514.html

V1->V2:

 - Change (max <= min) check to (max < min), because it is legal for
min == max.  I suspect this was the root cause of this error:

https://lists.linuxfoundation.org/pipermail/containers/2014-June/034661.html

 - Fix checkpatch warnings.


This was tested on a system running Linux 3.14, then compile-tested on
Linus' master branch.

Similar to ping_group_range, this code does not try to detect
noncontiguous UID/GID ranges.


 net/netfilter/xt_owner.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
index 1302b475abcb..e1c1f31ef1e9 100644
--- a/net/netfilter/xt_owner.c
+++ b/net/netfilter/xt_owner.c
@@ -21,11 +21,39 @@
 static int owner_check(const struct xt_mtchk_param *par)
 {
 	struct xt_owner_match_info *info = par->matchinfo;
+	struct net *net = par->net;
 
-	/* For now only allow adding matches from the initial user namespace */
+	/* Only allow the common case where the userns of the writer
+	 * matches the userns of the network namespace.
+	 */
 	if ((info->match & (XT_OWNER_UID|XT_OWNER_GID)) &&
-	    (current_user_ns() != &init_user_ns))
+	    (current_user_ns() != net->user_ns))
 		return -EINVAL;
+
+	/* Ensure the uids are valid */
+	if (info->match & XT_OWNER_UID) {
+		kuid_t uid_min = make_kuid(net->user_ns, info->uid_min);
+		kuid_t uid_max = make_kuid(net->user_ns, info->uid_max);
+
+		if (!uid_valid(uid_min) || !uid_valid(uid_max) ||
+		    (info->uid_max < info->uid_min) ||
+		    uid_lt(uid_max, uid_min)) {
+			return -EINVAL;
+		}
+	}
+
+	/* Ensure the gids are valid */
+	if (info->match & XT_OWNER_GID) {
+		kgid_t gid_min = make_kgid(net->user_ns, info->gid_min);
+		kgid_t gid_max = make_kgid(net->user_ns, info->gid_max);
+
+		if (!gid_valid(gid_min) || !gid_valid(gid_max) ||
+		    (info->gid_max < info->gid_min) ||
+		    gid_lt(gid_max, gid_min)) {
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
@@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	const struct xt_owner_match_info *info = par->matchinfo;
 	const struct file *filp;
 	struct sock *sk = skb_to_full_sk(skb);
+	const struct net *net;
 
 	if (sk == NULL || sk->sk_socket == NULL)
 		return (info->match ^ info->invert) == 0;
@@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		return ((info->match ^ info->invert) &
 		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
 
+	net = sock_net(skb->sk);
 	if (info->match & XT_OWNER_UID) {
-		kuid_t uid_min = make_kuid(&init_user_ns, info->uid_min);
-		kuid_t uid_max = make_kuid(&init_user_ns, info->uid_max);
+		kuid_t uid_min = make_kuid(net->user_ns, info->uid_min);
+		kuid_t uid_max = make_kuid(net->user_ns, info->uid_max);
 		if ((uid_gte(filp->f_cred->fsuid, uid_min) &&
 		     uid_lte(filp->f_cred->fsuid, uid_max)) ^
 		    !(info->invert & XT_OWNER_UID))
@@ -60,8 +90,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	}
 
 	if (info->match & XT_OWNER_GID) {
-		kgid_t gid_min = make_kgid(&init_user_ns, info->gid_min);
-		kgid_t gid_max = make_kgid(&init_user_ns, info->gid_max);
+		kgid_t gid_min = make_kgid(net->user_ns, info->gid_min);
+		kgid_t gid_max = make_kgid(net->user_ns, info->gid_max);
 		if ((gid_gte(filp->f_cred->fsgid, gid_min) &&
 		     gid_lte(filp->f_cred->fsgid, gid_max)) ^
 		    !(info->invert & XT_OWNER_GID))
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH V2] net: Allow xt_owner in any user namespace

[Eric W. Biederman]

Kevin Cernekee <cernekee@chromium.org> writes:

> From: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Making this work is a little tricky as it really isn't kosher to
> change the xt_owner_match_info in a check function.
>
> Without changing xt_owner_match_info we need to know the user
> namespace the uids and gids are specified in.  In the common case
> net->user_ns == current_user_ns().  Verify net->user_ns ==
> current_user_ns() in owner_check so we can later assume it in
> owner_mt.
>
> In owner_check also verify that all of the uids and gids specified are
> in net->user_ns and that the expected min/max relationship exists
> between the uids and gids in xt_owner_match_info.
>
> In owner_mt get the network namespace from the outgoing socket, as this
> must be the same network namespace as the netfilter rules, and use that
> network namespace to find the user namespace the uids and gids in
> xt_match_owner_info are encoded in.  Then convert from their encoded
> from into the kernel internal format for uids and gids and perform the
> owner match.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> ---
>
>
> Original post:
> https://lists.linuxfoundation.org/pipermail/containers/2014-May/034514.html
>
> V1->V2:
>
>  - Change (max <= min) check to (max < min), because it is legal for
> min == max.  I suspect this was the root cause of this error:
>
> https://lists.linuxfoundation.org/pipermail/containers/2014-June/034661.html
>
>  - Fix checkpatch warnings.
>
>
> This was tested on a system running Linux 3.14, then compile-tested on
> Linus' master branch.
>
> Similar to ping_group_range, this code does not try to detect
> noncontiguous UID/GID ranges.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Would it be worth including this changelog in with the text?

>
>
>  net/netfilter/xt_owner.c | 42 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
> index 1302b475abcb..e1c1f31ef1e9 100644
> --- a/net/netfilter/xt_owner.c
> +++ b/net/netfilter/xt_owner.c
> @@ -21,11 +21,39 @@
>  static int owner_check(const struct xt_mtchk_param *par)
>  {
>  	struct xt_owner_match_info *info = par->matchinfo;
> +	struct net *net = par->net;
>  
> -	/* For now only allow adding matches from the initial user namespace */
> +	/* Only allow the common case where the userns of the writer
> +	 * matches the userns of the network namespace.
> +	 */
>  	if ((info->match & (XT_OWNER_UID|XT_OWNER_GID)) &&
> -	    (current_user_ns() != &init_user_ns))
> +	    (current_user_ns() != net->user_ns))
>  		return -EINVAL;
> +
> +	/* Ensure the uids are valid */
> +	if (info->match & XT_OWNER_UID) {
> +		kuid_t uid_min = make_kuid(net->user_ns, info->uid_min);
> +		kuid_t uid_max = make_kuid(net->user_ns, info->uid_max);
> +
> +		if (!uid_valid(uid_min) || !uid_valid(uid_max) ||
> +		    (info->uid_max < info->uid_min) ||
> +		    uid_lt(uid_max, uid_min)) {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Ensure the gids are valid */
> +	if (info->match & XT_OWNER_GID) {
> +		kgid_t gid_min = make_kgid(net->user_ns, info->gid_min);
> +		kgid_t gid_max = make_kgid(net->user_ns, info->gid_max);
> +
> +		if (!gid_valid(gid_min) || !gid_valid(gid_max) ||
> +		    (info->gid_max < info->gid_min) ||
> +		    gid_lt(gid_max, gid_min)) {
> +			return -EINVAL;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  	const struct xt_owner_match_info *info = par->matchinfo;
>  	const struct file *filp;
>  	struct sock *sk = skb_to_full_sk(skb);
> +	const struct net *net;
>  
>  	if (sk == NULL || sk->sk_socket == NULL)
>  		return (info->match ^ info->invert) == 0;
> @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  		return ((info->match ^ info->invert) &
>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
>  
> +	net = sock_net(skb->sk);
>  	if (info->match & XT_OWNER_UID) {
> -		kuid_t uid_min = make_kuid(&init_user_ns, info->uid_min);
> -		kuid_t uid_max = make_kuid(&init_user_ns, info->uid_max);
> +		kuid_t uid_min = make_kuid(net->user_ns, info->uid_min);
> +		kuid_t uid_max = make_kuid(net->user_ns, info->uid_max);
>  		if ((uid_gte(filp->f_cred->fsuid, uid_min) &&
>  		     uid_lte(filp->f_cred->fsuid, uid_max)) ^
>  		    !(info->invert & XT_OWNER_UID))
> @@ -60,8 +90,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  	}
>  
>  	if (info->match & XT_OWNER_GID) {
> -		kgid_t gid_min = make_kgid(&init_user_ns, info->gid_min);
> -		kgid_t gid_max = make_kgid(&init_user_ns, info->gid_max);
> +		kgid_t gid_min = make_kgid(net->user_ns, info->gid_min);
> +		kgid_t gid_max = make_kgid(net->user_ns, info->gid_max);
>  		if ((gid_gte(filp->f_cred->fsgid, gid_min) &&
>  		     gid_lte(filp->f_cred->fsgid, gid_max)) ^
>  		    !(info->invert & XT_OWNER_GID))

Re: [PATCH V2] net: Allow xt_owner in any user namespace

[Florian Westphal]

Kevin Cernekee <cernekee@chromium.org> wrote:
> @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  	const struct xt_owner_match_info *info = par->matchinfo;
>  	const struct file *filp;
>  	struct sock *sk = skb_to_full_sk(skb);
> +	const struct net *net;
>  
>  	if (sk == NULL || sk->sk_socket == NULL)
>  		return (info->match ^ info->invert) == 0;
> @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  		return ((info->match ^ info->invert) &
>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
>  
> +	net = sock_net(skb->sk);

I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something
other than skb->sk.

Re: [PATCH V2] net: Allow xt_owner in any user namespace

[Eric Dumazet]

On Tue, 2016-06-14 at 02:02 +0200, Florian Westphal wrote:
> Kevin Cernekee <cernekee@chromium.org> wrote:
> > @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >  	const struct xt_owner_match_info *info = par->matchinfo;
> >  	const struct file *filp;
> >  	struct sock *sk = skb_to_full_sk(skb);
> > +	const struct net *net;
> >  
> >  	if (sk == NULL || sk->sk_socket == NULL)
> >  		return (info->match ^ info->invert) == 0;
> > @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >  		return ((info->match ^ info->invert) &
> >  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
> >  
> > +	net = sock_net(skb->sk);
> 
> I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something
> other than skb->sk.

They should share same network namespace ?

Re: [PATCH V2] net: Allow xt_owner in any user namespace

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Kevin Cernekee <cernekee@chromium.org> wrote:
> > @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >  	const struct xt_owner_match_info *info = par->matchinfo;
> >  	const struct file *filp;
> >  	struct sock *sk = skb_to_full_sk(skb);
> > +	const struct net *net;
> >  
> >  	if (sk == NULL || sk->sk_socket == NULL)
> >  		return (info->match ^ info->invert) == 0;
> > @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >  		return ((info->match ^ info->invert) &
> >  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
> >  
> > +	net = sock_net(skb->sk);
> 
> I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something
> other than skb->sk.

I was worried about layout but sk_net is part of sock_common so its
fine; comment withdrawn.

Re: [PATCH V2] net: Allow xt_owner in any user namespace

[Eric W. Biederman]

Florian Westphal <fw@strlen.de> writes:

> Kevin Cernekee <cernekee@chromium.org> wrote:
>> @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>  	const struct xt_owner_match_info *info = par->matchinfo;
>>  	const struct file *filp;
>>  	struct sock *sk = skb_to_full_sk(skb);
>> +	const struct net *net;
>>  
>>  	if (sk == NULL || sk->sk_socket == NULL)
>>  		return (info->match ^ info->invert) == 0;
>> @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>  		return ((info->match ^ info->invert) &
>>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
>>  
>> +	net = sock_net(skb->sk);
>
> I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something
> other than skb->sk.

Actually this should be "par->net".  That did not exist a few years ago
when the patch was written but it does now, and that should simplify
things a little bit, and remove any guess work or uncertainty.

Eric

Re: [PATCH V2] net: Allow xt_owner in any user namespace

[Pablo Neira Ayuso]

On Mon, Jun 13, 2016 at 09:06:55PM -0500, Eric W. Biederman wrote:
> Florian Westphal <fw@strlen.de> writes:
> 
> > Kevin Cernekee <cernekee@chromium.org> wrote:
> >> @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >>  	const struct xt_owner_match_info *info = par->matchinfo;
> >>  	const struct file *filp;
> >>  	struct sock *sk = skb_to_full_sk(skb);
> >> +	const struct net *net;
> >>  
> >>  	if (sk == NULL || sk->sk_socket == NULL)
> >>  		return (info->match ^ info->invert) == 0;
> >> @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >>  		return ((info->match ^ info->invert) &
> >>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
> >>  
> >> +	net = sock_net(skb->sk);
> >
> > I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something
> > other than skb->sk.
> 
> Actually this should be "par->net".  That did not exist a few years ago
> when the patch was written but it does now, and that should simplify
> things a little bit, and remove any guess work or uncertainty.

Right.

BTW, could you also send a follow up patch to update
net/netfilter/nft_meta.c? We have similar support for socket owner in
nf_tables as well (actually it will be a more simple patch that this,
I would expect).

Thanks.

Re: [PATCH V2] net: Allow xt_owner in any user namespace

[Eric W. Biederman]

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> On Mon, Jun 13, 2016 at 09:06:55PM -0500, Eric W. Biederman wrote:
>> Florian Westphal <fw@strlen.de> writes:
>> 
>> > Kevin Cernekee <cernekee@chromium.org> wrote:
>> >> @@ -35,6 +63,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>> >>  	const struct xt_owner_match_info *info = par->matchinfo;
>> >>  	const struct file *filp;
>> >>  	struct sock *sk = skb_to_full_sk(skb);
>> >> +	const struct net *net;
>> >>  
>> >>  	if (sk == NULL || sk->sk_socket == NULL)
>> >>  		return (info->match ^ info->invert) == 0;
>> >> @@ -50,9 +79,10 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>> >>  		return ((info->match ^ info->invert) &
>> >>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
>> >>  
>> >> +	net = sock_net(skb->sk);
>> >
>> > I think you need to use sock_net(sk) as skb_to_full_sk(skb) can return something
>> > other than skb->sk.
>> 
>> Actually this should be "par->net".  That did not exist a few years ago
>> when the patch was written but it does now, and that should simplify
>> things a little bit, and remove any guess work or uncertainty.
>
> Right.
>
> BTW, could you also send a follow up patch to update
> net/netfilter/nft_meta.c? We have similar support for socket owner in
> nf_tables as well (actually it will be a more simple patch that this,
> I would expect).

That sounds worth doing.

At the very least we need a test that whoever sets the filter is
in the initial user namespace or else the filter is just nonsense.

I took a glance at the code and I completely don't remember the
context in which .init and .eval methods run in so I am temporarily at a
loss.

That said I suspect we want something like the patch below.  I am a
little uncertain if we want from_kuid or from_kuid_munged.  The
difference is how we handle unmapped uids and gids (in the rare case
they come up).  There are some nuances but in practice from_xxx_munged
returns (u16)-1 when things don't map, and from_xxx returns (u32)-1
when things don't map.

Eric

diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 16c50b0dd426..b0b832eddb0d 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -104,7 +104,7 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 			goto err;
 		}
 
-		*dest =	from_kuid_munged(&init_user_ns,
+		*dest =	from_kuid_munged(pkt->net->user_ns,
 				sk->sk_socket->file->f_cred->fsuid);
 		read_unlock_bh(&sk->sk_callback_lock);
 		break;
@@ -119,7 +119,7 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 			read_unlock_bh(&sk->sk_callback_lock);
 			goto err;
 		}
-		*dest =	from_kgid_munged(&init_user_ns,
+		*dest =	from_kgid_munged(pkt->net->user_ns,
 				 sk->sk_socket->file->f_cred->fsgid);
 		read_unlock_bh(&sk->sk_callback_lock);
 		break;
@@ -263,8 +263,6 @@ int nft_meta_get_init(const struct nft_ctx *ctx,
 	case NFT_META_MARK:
 	case NFT_META_IIF:
 	case NFT_META_OIF:
-	case NFT_META_SKUID:
-	case NFT_META_SKGID:
 #ifdef CONFIG_IP_ROUTE_CLASSID
 	case NFT_META_RTCLASSID:
 #endif
@@ -288,6 +286,12 @@ int nft_meta_get_init(const struct nft_ctx *ctx,
 		prandom_init_once(&nft_prandom_state);
 		len = sizeof(u32);
 		break;
+	case NFT_META_SKUID:
+	case NFT_META_SKGID:
+		if (current_user_ns() != net->user_ns)
+			return -EINVAL;
+		len = sizeof(u32);
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}



Eric