Netfilter owner matching inside user namespace

Netfilter owner matching inside user namespace

[Alin Dobre]

Hello,

I am trying to run the following command inside an image using user
namespaces via contain [1], a very simplistic implementation of linux
containers:
  contain /path/to/image /bin/bash

Although the host kernel does have support for owner matching and it
works with no errors, running the following iptables command inside the
container:
  iptables -A OUTPUT -m owner --uid-owner root -j ACCEPT
returns the error "Invalid argument".

The last commit for the netfilter xt_owner module is exactly Eric's
basic support for user namespaces, but there might be some other recent
changes either in the namespaces area or netfilter in general, which
brought the module in an unusable state inside containers - at least for
the above command usage.

I can try to send the image I used for testing to anyone who desires,
but a handy shortcut should be "deboostrap trusty /path/to/image" and
"chroot /path/to/image apt-get install iptables".

The host kernel is 3.14.4, iptables version on the host is 1.4.15 and
inside the Ubuntu container is 1.4.18. I have tried with Ubuntu 13.* and
Ubuntu 14.04, but I don't think the userspace has anything to do with this.

I can provide with any additional information needed.

Any insights on this?

Cheers,
Alin.

[1] https://github.com/arachsys/containers

Re: Netfilter owner matching inside user namespace

[Eric W. Biederman]

Alin Dobre <alin.dobre@elastichosts.com> writes:

> Hello,
>
> I am trying to run the following command inside an image using user
> namespaces via contain [1], a very simplistic implementation of linux
> containers:
>   contain /path/to/image /bin/bash
>
> Although the host kernel does have support for owner matching and it
> works with no errors, running the following iptables command inside the
> container:
>   iptables -A OUTPUT -m owner --uid-owner root -j ACCEPT
> returns the error "Invalid argument".
>
> The last commit for the netfilter xt_owner module is exactly Eric's
> basic support for user namespaces, but there might be some other recent
> changes either in the namespaces area or netfilter in general, which
> brought the module in an unusable state inside containers - at least for
> the above command usage.

The code says.

static int owner_check(const struct xt_mtchk_param *par)
{
	struct xt_owner_match_info *info = par->matchinfo;

	/* For now only allow adding matches from the initial user namespace */
	if ((info->match & (XT_OWNER_UID|XT_OWNER_GID)) &&
	    (current_user_ns() != &init_user_ns))
		return -EINVAL;
	return 0;
}

So it is not expected to work in user namespaces by design.

> I can try to send the image I used for testing to anyone who desires,
> but a handy shortcut should be "deboostrap trusty /path/to/image" and
> "chroot /path/to/image apt-get install iptables".
>
> The host kernel is 3.14.4, iptables version on the host is 1.4.15 and
> inside the Ubuntu container is 1.4.18. I have tried with Ubuntu 13.* and
> Ubuntu 14.04, but I don't think the userspace has anything to do with this.
>
> I can provide with any additional information needed.
>
> Any insights on this?

As I recall this code largely matches directly on the values passed in
from userspace.  Which in this case is a problem because I would like to
store kuids and kgids in the data structures and compare those.

I believe it would take some careful refactoring to allow massaging the
data from the form the user supplied to something more appropriate
before we perform the match.

That is making this work is tricky so I punted and did not support it
from inside a user namespace.

Eric

Re: Netfilter owner matching inside user namespace

[Marian Marinov]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/22/2014 12:04 AM, Eric W. Biederman wrote:
> Alin Dobre <alin.dobre-1hSFou9RDDldEee+Cai+ZQ@public.gmane.org> writes:
> 
>> Hello,
>> 
>> I am trying to run the following command inside an image using user namespaces via contain [1], a very simplistic
>> implementation of linux containers: contain /path/to/image /bin/bash
>> 
>> Although the host kernel does have support for owner matching and it works with no errors, running the following
>> iptables command inside the container: iptables -A OUTPUT -m owner --uid-owner root -j ACCEPT returns the error
>> "Invalid argument".
>> 
>> The last commit for the netfilter xt_owner module is exactly Eric's basic support for user namespaces, but there
>> might be some other recent changes either in the namespaces area or netfilter in general, which brought the
>> module in an unusable state inside containers - at least for the above command usage.
> 
> The code says.
> 
> static int owner_check(const struct xt_mtchk_param *par) { struct xt_owner_match_info *info = par->matchinfo;
> 
> /* For now only allow adding matches from the initial user namespace */ if ((info->match &
> (XT_OWNER_UID|XT_OWNER_GID)) && (current_user_ns() != &init_user_ns)) return -EINVAL; return 0; }
> 
> So it is not expected to work in user namespaces by design.
> 
>> I can try to send the image I used for testing to anyone who desires, but a handy shortcut should be "deboostrap
>> trusty /path/to/image" and "chroot /path/to/image apt-get install iptables".
>> 
>> The host kernel is 3.14.4, iptables version on the host is 1.4.15 and inside the Ubuntu container is 1.4.18. I
>> have tried with Ubuntu 13.* and Ubuntu 14.04, but I don't think the userspace has anything to do with this.
>> 
>> I can provide with any additional information needed.
>> 
>> Any insights on this?
> 
> As I recall this code largely matches directly on the values passed in from userspace.  Which in this case is a
> problem because I would like to store kuids and kgids in the data structures and compare those.
> 
> I believe it would take some careful refactoring to allow massaging the data from the form the user supplied to
> something more appropriate before we perform the match.
> 
> That is making this work is tricky so I punted and did not support it from inside a user namespace.
> 

Erik I have proposition about that... Please, tell me if I'm on the right track:

diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
index ca2e577..3682825 100644
- --- a/net/netfilter/xt_owner.c
+++ b/net/netfilter/xt_owner.c
@@ -33,6 +27,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 user_namespace *ns = get_current_cred()->user_ns;

        if (skb->sk == NULL || skb->sk->sk_socket == NULL)
                return (info->match ^ info->invert) == 0;
@@ -49,8 +44,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(&init_user_ns, info->uid_min);
- -           kuid_t uid_max = make_kuid(&init_user_ns, info->uid_max);
+         kuid_t uid_min = make_kuid(ns, info->uid_min);
+         kuid_t uid_max = make_kuid(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))
@@ -58,8 +53,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(ns, info->gid_min);
+         kgid_t gid_max = make_kgid(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))

If the process has its own user and network namespaces, are the above changes enough?

Since the rule is added to its own network namespace is it still a problem?

Marian

> Eric _______________________________________________ Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org 
> https://lists.linuxfoundation.org/mailman/listinfo/containers
> 


- -- 
Marian Marinov
Founder & CEO of 1H Ltd.
Jabber/GTalk: hackman-/eSpBmjxGS4dnm+yROfE0A@public.gmane.org
ICQ: 7556201
Mobile: +359 886 660 270
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAlN9cG0ACgkQ4mt9JeIbjJTs8wCeJ5JwH8molS6pG4uh1qRt5bP3
u7gAoLrBEnPXQKo5ZMuZjDMuSlk22HRe
=EmqL
-----END PGP SIGNATURE-----

Re: Netfilter owner matching inside user namespace

[Eric W. Biederman]

Marian Marinov <mm@1h.com> writes:

> On 05/22/2014 12:04 AM, Eric W. Biederman wrote:
>> Alin Dobre <alin.dobre@elastichosts.com> writes:
>> 
>>> Hello,
>>> 
>>> I am trying to run the following command inside an image using user namespaces via contain [1], a very simplistic
>>> implementation of linux containers: contain /path/to/image /bin/bash
>>> 
>>> Although the host kernel does have support for owner matching and it works with no errors, running the following
>>> iptables command inside the container: iptables -A OUTPUT -m owner --uid-owner root -j ACCEPT returns the error
>>> "Invalid argument".
>>> 
>>> The last commit for the netfilter xt_owner module is exactly Eric's basic support for user namespaces, but there
>>> might be some other recent changes either in the namespaces area or netfilter in general, which brought the
>>> module in an unusable state inside containers - at least for the above command usage.
>> 
>> The code says.
>> 
>> static int owner_check(const struct xt_mtchk_param *par) { struct xt_owner_match_info *info = par->matchinfo;
>> 
>> /* For now only allow adding matches from the initial user namespace */ if ((info->match &
>> (XT_OWNER_UID|XT_OWNER_GID)) && (current_user_ns() != &init_user_ns)) return -EINVAL; return 0; }
>> 
>> So it is not expected to work in user namespaces by design.
>> 
>>> I can try to send the image I used for testing to anyone who desires, but a handy shortcut should be "deboostrap
>>> trusty /path/to/image" and "chroot /path/to/image apt-get install iptables".
>>> 
>>> The host kernel is 3.14.4, iptables version on the host is 1.4.15 and inside the Ubuntu container is 1.4.18. I
>>> have tried with Ubuntu 13.* and Ubuntu 14.04, but I don't think the userspace has anything to do with this.
>>> 
>>> I can provide with any additional information needed.
>>> 
>>> Any insights on this?
>> 
>> As I recall this code largely matches directly on the values passed in from userspace.  Which in this case is a
>> problem because I would like to store kuids and kgids in the data structures and compare those.
>> 
>> I believe it would take some careful refactoring to allow massaging the data from the form the user supplied to
>> something more appropriate before we perform the match.
>> 
>> That is making this work is tricky so I punted and did not support it from inside a user namespace.
>> 
>
> Erik I have proposition about that... Please, tell me if I'm on the right track:
>
> diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
> index ca2e577..3682825 100644
> --- a/net/netfilter/xt_owner.c
> +++ b/net/netfilter/xt_owner.c
> @@ -33,6 +27,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 user_namespace *ns = get_current_cred()->user_ns;
>
>         if (skb->sk == NULL || skb->sk->sk_socket == NULL)
>                 return (info->match ^ info->invert) == 0;
> @@ -49,8 +44,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(&init_user_ns, info->uid_min);
> -           kuid_t uid_max = make_kuid(&init_user_ns, info->uid_max);
> +         kuid_t uid_min = make_kuid(ns, info->uid_min);
> +         kuid_t uid_max = make_kuid(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))
> @@ -58,8 +53,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(ns, info->gid_min);
> +         kgid_t gid_max = make_kgid(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))
>
> If the process has its own user and network namespaces, are the above changes enough?
>
> Since the rule is added to its own network namespace is it still a
> problem?


As I read the code the netfilter match does not need to be and I would
be highly surprised if it were called in the context of any specific
user process.   Which means we can not rely on current.

Check comes close to being the right place to change things, but check
doesn't change the data, only validates it :(

Eric

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

[Eric W. Biederman]

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-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 net/netfilter/xt_owner.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
index ca2e577ed8ac..8111d8a19113 100644
--- a/net/netfilter/xt_owner.c
+++ b/net/netfilter/xt_owner.c
@@ -20,11 +20,37 @@
 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_lte(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_lte(gid_max, gid_min)) {
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
@@ -33,6 +59,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;
+	const struct net *net;
 
 	if (skb->sk == NULL || skb->sk->sk_socket == NULL)
 		return (info->match ^ info->invert) == 0;
@@ -48,9 +75,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))
@@ -58,8 +86,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))
-- 
1.9.1

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

[Jan Engelhardt]

On Sunday 2014-05-25 09:39, Eric W. Biederman wrote:
>
>Making this work is a little tricky as it really isn't kosher to
>change the xt_owner_match_info in a check function.

It is ok if you set aside members for kernel internal use, though
that is going to require a new match revision. That may be justified
since it would alleviate repeated calls to make_kuid for each packet.

>@@ -48,9 +75,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))

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

[Alin Dobre]

Thanks, Eric. I'll give this a try and come back with some results if
interested.

Cheers,
Alin.

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

[Alin Dobre]

Unfortunately, applying this patch now makes iptables return EINVAL both
on host and container.

Cheers,
Alin.