[rh7] netfilter: Allow xt_owner in any user namespace

Submitted by Andrei Vagin on Oct. 13, 2017, 11:20 p.m.

Details

Message ID 20171013232017.30227-1-avagin@openvz.org
State New
Series "netfilter: Allow xt_owner in any user namespace"
Headers show

Commit Message

Andrei Vagin Oct. 13, 2017, 11:20 p.m.
From: "Eric W. Biederman" <ebiederm@xmission.com>

ML: 9847371a84b0be330f4bc4aaa98904101ee8573d
https://jira.sw.ru/browse/PSBM-69409?

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.

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

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
---
 net/netfilter/xt_owner.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
index 31dec4a..1744f78 100644
--- a/net/netfilter/xt_owner.c
+++ b/net/netfilter/xt_owner.c
@@ -80,11 +80,39 @@  owner_mt6_v0(const struct sk_buff *skb, struct xt_action_param *par)
 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_initial())
+	    (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;
 }
 
@@ -93,6 +121,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 net *net = dev_net(par->in ? par->in : par->out);
 
 	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
 		return (info->match ^ info->invert) == 0;
@@ -109,8 +138,8 @@  owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
 
 	if (info->match & XT_OWNER_UID) {
-		kuid_t uid_min = make_kuid(ve_init_user_ns(), info->uid_min);
-		kuid_t uid_max = make_kuid(ve_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))
@@ -118,8 +147,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(ve_init_user_ns(), info->gid_min);
-		kgid_t gid_max = make_kgid(ve_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))

Comments

Konstantin Khorenko Oct. 16, 2017, 10:33 a.m.
Stas, please review the patch.

Andrey, why do we need to support deeper user namespaces at all?
Someone app tries to create a new userns inside a vz7 CT and use ipt_owner inside it?

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 10/14/2017 02:20 AM, Andrei Vagin wrote:
> From: "Eric W. Biederman" <ebiederm@xmission.com>
>
> ML: 9847371a84b0be330f4bc4aaa98904101ee8573d
> https://jira.sw.ru/browse/PSBM-69409?
>
> 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.
>
> Similar to ping_group_range, this code does not try to detect
> noncontiguous UID/GID ranges.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> ---
>  net/netfilter/xt_owner.c | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
> index 31dec4a..1744f78 100644
> --- a/net/netfilter/xt_owner.c
> +++ b/net/netfilter/xt_owner.c
> @@ -80,11 +80,39 @@ owner_mt6_v0(const struct sk_buff *skb, struct xt_action_param *par)
>  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_initial())
> +	    (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;
>  }
>
> @@ -93,6 +121,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 net *net = dev_net(par->in ? par->in : par->out);
>
>  	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
>  		return (info->match ^ info->invert) == 0;
> @@ -109,8 +138,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
>
>  	if (info->match & XT_OWNER_UID) {
> -		kuid_t uid_min = make_kuid(ve_init_user_ns(), info->uid_min);
> -		kuid_t uid_max = make_kuid(ve_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))
> @@ -118,8 +147,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(ve_init_user_ns(), info->gid_min);
> -		kgid_t gid_max = make_kgid(ve_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))
>
Stanislav Kinsburskiy Oct. 16, 2017, 3:50 p.m.
Well, patch looks ok.
But shouldn't all the ve_init_user_ns() replaced by the par->net?

14.10.2017 01:20, Andrei Vagin пишет:
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> ML: 9847371a84b0be330f4bc4aaa98904101ee8573d
> https://jira.sw.ru/browse/PSBM-69409?
> 
> 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.
> 
> Similar to ping_group_range, this code does not try to detect
> noncontiguous UID/GID ranges.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> ---
>  net/netfilter/xt_owner.c | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
> index 31dec4a..1744f78 100644
> --- a/net/netfilter/xt_owner.c
> +++ b/net/netfilter/xt_owner.c
> @@ -80,11 +80,39 @@ owner_mt6_v0(const struct sk_buff *skb, struct xt_action_param *par)
>  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_initial())
> +	    (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;
>  }
>  
> @@ -93,6 +121,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 net *net = dev_net(par->in ? par->in : par->out);
>  
>  	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
>  		return (info->match ^ info->invert) == 0;
> @@ -109,8 +138,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
>  
>  	if (info->match & XT_OWNER_UID) {
> -		kuid_t uid_min = make_kuid(ve_init_user_ns(), info->uid_min);
> -		kuid_t uid_max = make_kuid(ve_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))
> @@ -118,8 +147,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(ve_init_user_ns(), info->gid_min);
> -		kgid_t gid_max = make_kgid(ve_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))
>
Andrey Vagin Oct. 17, 2017, 6:49 a.m.
On Mon, Oct 16, 2017 at 01:33:12PM +0300, Konstantin Khorenko wrote:
> Stas, please review the patch.
> 
> Andrey, why do we need to support deeper user namespaces at all?
> Someone app tries to create a new userns inside a vz7 CT and use ipt_owner inside it?

The kernel grabs userns when we send START to the "state" file of a
container ve cgroup. But vzctl does this after restoring a container,
so we don't know a ve userns, when we are retoriung iptable rules.

CRIU can't dump nested userns, so if any app will create a new userns,
criu dump will return an error.

> 
> --
> Best regards,
> 
> Konstantin Khorenko,
> Virtuozzo Linux Kernel Team
> 
> On 10/14/2017 02:20 AM, Andrei Vagin wrote:
> > From: "Eric W. Biederman" <ebiederm@xmission.com>
> > 
> > ML: 9847371a84b0be330f4bc4aaa98904101ee8573d
> > https://jira.sw.ru/browse/PSBM-69409?
> > 
> > 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.
> > 
> > Similar to ping_group_range, this code does not try to detect
> > noncontiguous UID/GID ranges.
> > 
> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> > ---
> >  net/netfilter/xt_owner.c | 41 +++++++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
> > index 31dec4a..1744f78 100644
> > --- a/net/netfilter/xt_owner.c
> > +++ b/net/netfilter/xt_owner.c
> > @@ -80,11 +80,39 @@ owner_mt6_v0(const struct sk_buff *skb, struct xt_action_param *par)
> >  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_initial())
> > +	    (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;
> >  }
> > 
> > @@ -93,6 +121,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 net *net = dev_net(par->in ? par->in : par->out);
> > 
> >  	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
> >  		return (info->match ^ info->invert) == 0;
> > @@ -109,8 +138,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
> > 
> >  	if (info->match & XT_OWNER_UID) {
> > -		kuid_t uid_min = make_kuid(ve_init_user_ns(), info->uid_min);
> > -		kuid_t uid_max = make_kuid(ve_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))
> > @@ -118,8 +147,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(ve_init_user_ns(), info->gid_min);
> > -		kgid_t gid_max = make_kgid(ve_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))
> >
Andrey Vagin Oct. 17, 2017, 6:53 a.m.
On Mon, Oct 16, 2017 at 05:50:38PM +0200, Stanislav Kinsburskiy wrote:
> Well, patch looks ok.
> But shouldn't all the ve_init_user_ns() replaced by the par->net?

This patch does this.

> 
> 14.10.2017 01:20, Andrei Vagin пишет:
> > From: "Eric W. Biederman" <ebiederm@xmission.com>
> > 
> > ML: 9847371a84b0be330f4bc4aaa98904101ee8573d
> > https://jira.sw.ru/browse/PSBM-69409?
> > 
> > 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.
> > 
> > Similar to ping_group_range, this code does not try to detect
> > noncontiguous UID/GID ranges.
> > 
> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> > ---
> >  net/netfilter/xt_owner.c | 41 +++++++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
> > index 31dec4a..1744f78 100644
> > --- a/net/netfilter/xt_owner.c
> > +++ b/net/netfilter/xt_owner.c
> > @@ -80,11 +80,39 @@ owner_mt6_v0(const struct sk_buff *skb, struct xt_action_param *par)
> >  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_initial())
> > +	    (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;
> >  }
> >  
> > @@ -93,6 +121,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 net *net = dev_net(par->in ? par->in : par->out);
> >  
> >  	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
> >  		return (info->match ^ info->invert) == 0;
> > @@ -109,8 +138,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
> >  
> >  	if (info->match & XT_OWNER_UID) {
> > -		kuid_t uid_min = make_kuid(ve_init_user_ns(), info->uid_min);
> > -		kuid_t uid_max = make_kuid(ve_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))
> > @@ -118,8 +147,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(ve_init_user_ns(), info->gid_min);
> > -		kgid_t gid_max = make_kgid(ve_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))
> > 
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
Stanislav Kinsburskiy Oct. 17, 2017, 9:08 a.m.
17.10.2017 08:53, Andrei Vagin пишет:
> On Mon, Oct 16, 2017 at 05:50:38PM +0200, Stanislav Kinsburskiy wrote:
>> Well, patch looks ok.
>> But shouldn't all the ve_init_user_ns() replaced by the par->net?
> 
> This patch does this.
> 

Yes, but not everywhere.
Say, there are owner_mt_ve0 and owner_mt6_ve0.
Shouldn't there functions also patched?

>>
>> 14.10.2017 01:20, Andrei Vagin пишет:
>>> From: "Eric W. Biederman" <ebiederm@xmission.com>
>>>
>>> ML: 9847371a84b0be330f4bc4aaa98904101ee8573d
>>> https://jira.sw.ru/browse/PSBM-69409?
>>>
>>> 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.
>>>
>>> Similar to ping_group_range, this code does not try to detect
>>> noncontiguous UID/GID ranges.
>>>
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
>>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
>>> ---
>>>  net/netfilter/xt_owner.c | 41 +++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 35 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
>>> index 31dec4a..1744f78 100644
>>> --- a/net/netfilter/xt_owner.c
>>> +++ b/net/netfilter/xt_owner.c
>>> @@ -80,11 +80,39 @@ owner_mt6_v0(const struct sk_buff *skb, struct xt_action_param *par)
>>>  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_initial())
>>> +	    (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;
>>>  }
>>>  
>>> @@ -93,6 +121,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 net *net = dev_net(par->in ? par->in : par->out);
>>>  
>>>  	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
>>>  		return (info->match ^ info->invert) == 0;
>>> @@ -109,8 +138,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
>>>  
>>>  	if (info->match & XT_OWNER_UID) {
>>> -		kuid_t uid_min = make_kuid(ve_init_user_ns(), info->uid_min);
>>> -		kuid_t uid_max = make_kuid(ve_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))
>>> @@ -118,8 +147,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(ve_init_user_ns(), info->gid_min);
>>> -		kgid_t gid_max = make_kgid(ve_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))
>>>
>> _______________________________________________
>> Devel mailing list
>> Devel@openvz.org
>> https://lists.openvz.org/mailman/listinfo/devel
Konstantin Khorenko Oct. 17, 2017, 4:16 p.m.
On 10/17/2017 12:08 PM, Stanislav Kinsburskiy wrote:
>
>
> 17.10.2017 08:53, Andrei Vagin пишет:
>> On Mon, Oct 16, 2017 at 05:50:38PM +0200, Stanislav Kinsburskiy wrote:
>>> Well, patch looks ok.
>>> But shouldn't all the ve_init_user_ns() replaced by the par->net?
>>
>> This patch does this.
>>
>
> Yes, but not everywhere.
> Say, there are owner_mt_ve0 and owner_mt6_ve0.
> Shouldn't there functions also patched?

Seems so.
It's needed for old iptables userspace support (iptables-1.3.5 CentOS 5).

Andrey, please send an incremental patch for it.
(and owner_mt_check_v0()/owner_mt6_check_v0() are to be enhanced with uid validness check as well i suppose)


commit cf3bb0a2da9940cd534d486c778f9d1d6728e26c
Author: Kirill Tkhai <ktkhai@parallels.com>
Date:   Wed Oct 8 13:41:09 2014 +0400

     ve/netfilter: Restore revision 0 "owner" match support

     Rebasing and splitting netfilters sybsystem
     (port 66-diff-ve-net-netfilter-combined).
     Part 1.

     https://jira.sw.ru/browse/PSBM-18322

     Revision 0 is still used in our VE templates (iptables-1.3.5)
     so we just can't drop this kind of support. Bring them back.

     Restore version 0 of match "owner",
     which was deleted from mainstream.

     Signed-off-by: Kirill Tkhai <ktkhai@parallels.com>


>
>>>
>>> 14.10.2017 01:20, Andrei Vagin пишет:
>>>> From: "Eric W. Biederman" <ebiederm@xmission.com>
>>>>
>>>> ML: 9847371a84b0be330f4bc4aaa98904101ee8573d
>>>> https://jira.sw.ru/browse/PSBM-69409?
>>>>
>>>> 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.
>>>>
>>>> Similar to ping_group_range, this code does not try to detect
>>>> noncontiguous UID/GID ranges.
>>>>
>>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>>> Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
>>>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>>> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
>>>> ---
>>>>  net/netfilter/xt_owner.c | 41 +++++++++++++++++++++++++++++++++++------
>>>>  1 file changed, 35 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
>>>> index 31dec4a..1744f78 100644
>>>> --- a/net/netfilter/xt_owner.c
>>>> +++ b/net/netfilter/xt_owner.c
>>>> @@ -80,11 +80,39 @@ owner_mt6_v0(const struct sk_buff *skb, struct xt_action_param *par)
>>>>  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_initial())
>>>> +	    (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;
>>>>  }
>>>>
>>>> @@ -93,6 +121,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 net *net = dev_net(par->in ? par->in : par->out);
>>>>
>>>>  	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
>>>>  		return (info->match ^ info->invert) == 0;
>>>> @@ -109,8 +138,8 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>>>  		       (XT_OWNER_UID | XT_OWNER_GID)) == 0;
>>>>
>>>>  	if (info->match & XT_OWNER_UID) {
>>>> -		kuid_t uid_min = make_kuid(ve_init_user_ns(), info->uid_min);
>>>> -		kuid_t uid_max = make_kuid(ve_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))
>>>> @@ -118,8 +147,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(ve_init_user_ns(), info->gid_min);
>>>> -		kgid_t gid_max = make_kgid(ve_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))
>>>>
>>> _______________________________________________
>>> Devel mailing list
>>> Devel@openvz.org
>>> https://lists.openvz.org/mailman/listinfo/devel
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
>