[PATCH] Expose x_tables /proc entries as 0444 not 0440

[PATCH] Expose x_tables /proc entries as 0444 not 0440

[Philip Whineray]

Reading these files is impossible in an unprivileged user namespace,
interfering with various firewall tools. For instance, iptables-save
relies on reading /proc/net/ip_tables_names to dump only loaded tables.

Hiding the contents from non-root users does not achieve anything
practical. Possible values are well-known and the specifics can
be inferred from a list of loaded modules on most systems.

Signed-off-by: Philip Whineray <phil@firehol.org>
---
An alternate might be to change the ownership of the files within the
namespace when it is created:

https://lists.linuxcontainers.org/pipermail/lxc-users/2014-November/008110.html

I do not see that there is much advantage to this, it just ties the
ability to read the files to the ability to create an unprivileged
namespace.

 net/netfilter/x_tables.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 9b42b5e..c05adde 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1236,21 +1236,21 @@ int xt_proto_init(struct net *net, u_int8_t af)
 #ifdef CONFIG_PROC_FS
 	strlcpy(buf, xt_prefix[af], sizeof(buf));
 	strlcat(buf, FORMAT_TABLES, sizeof(buf));
-	proc = proc_create_data(buf, 0440, net->proc_net, &xt_table_ops,
+	proc = proc_create_data(buf, 0444, net->proc_net, &xt_table_ops,
 				(void *)(unsigned long)af);
 	if (!proc)
 		goto out;
 
 	strlcpy(buf, xt_prefix[af], sizeof(buf));
 	strlcat(buf, FORMAT_MATCHES, sizeof(buf));
-	proc = proc_create_data(buf, 0440, net->proc_net, &xt_match_ops,
+	proc = proc_create_data(buf, 0444, net->proc_net, &xt_match_ops,
 				(void *)(unsigned long)af);
 	if (!proc)
 		goto out_remove_tables;
 
 	strlcpy(buf, xt_prefix[af], sizeof(buf));
 	strlcat(buf, FORMAT_TARGETS, sizeof(buf));
-	proc = proc_create_data(buf, 0440, net->proc_net, &xt_target_ops,
+	proc = proc_create_data(buf, 0444, net->proc_net, &xt_target_ops,
 				(void *)(unsigned long)af);
 	if (!proc)
 		goto out_remove_matches;
-- 
2.1.4

Re: [PATCH] Expose x_tables /proc entries as 0444 not 0440

[Pablo Neira Ayuso]

On Sat, Nov 07, 2015 at 07:49:39AM +0000, Philip Whineray wrote:
> Reading these files is impossible in an unprivileged user namespace,
> interfering with various firewall tools. For instance, iptables-save
> relies on reading /proc/net/ip_tables_names to dump only loaded tables.
> 
> Hiding the contents from non-root users does not achieve anything
> practical. Possible values are well-known and the specifics can
> be inferred from a list of loaded modules on most systems.
> 
> Signed-off-by: Philip Whineray <phil@firehol.org>
> ---
> An alternate might be to change the ownership of the files within the
> namespace when it is created:
> 
> https://lists.linuxcontainers.org/pipermail/lxc-users/2014-November/008110.html
> 
> I do not see that there is much advantage to this, it just ties the
> ability to read the files to the ability to create an unprivileged
> namespace.

So I understood this correctly, this approach would set the ownership
of the /proc entry to the corresponding root uid mapping from the
unpriviledged namespace, right? If so, I would prefer that approach.

This is partially leaking the filtering policy to non-root users as it
contains what modules are being used, so you can at least infer how
complex your ruleset is.

And I guess it will not be long time until someone else will follow up
with a similar patch later on to expose the content of
/proc/net/nf_conntrack to get this working on unpriviledged namespaces
too.

Thanks.

Re: [PATCH] Expose x_tables /proc entries as 0444 not 0440

[Jozsef Kadlecsik]

On Wed, 11 Nov 2015, Pablo Neira Ayuso wrote:

> On Sat, Nov 07, 2015 at 07:49:39AM +0000, Philip Whineray wrote:
> > Reading these files is impossible in an unprivileged user namespace,
> > interfering with various firewall tools. For instance, iptables-save
> > relies on reading /proc/net/ip_tables_names to dump only loaded tables.
> > 
> > Hiding the contents from non-root users does not achieve anything
> > practical. Possible values are well-known and the specifics can
> > be inferred from a list of loaded modules on most systems.
> > 
> > Signed-off-by: Philip Whineray <phil@firehol.org>
> > ---
> > An alternate might be to change the ownership of the files within the
> > namespace when it is created:
> > 
> > https://lists.linuxcontainers.org/pipermail/lxc-users/2014-November/008110.html
> > 
> > I do not see that there is much advantage to this, it just ties the
> > ability to read the files to the ability to create an unprivileged
> > namespace.
> 
> So I understood this correctly, this approach would set the ownership
> of the /proc entry to the corresponding root uid mapping from the
> unpriviledged namespace, right? If so, I would prefer that approach.
> 
> This is partially leaking the filtering policy to non-root users as it
> contains what modules are being used, so you can at least infer how
> complex your ruleset is.
> 
> And I guess it will not be long time until someone else will follow up
> with a similar patch later on to expose the content of
> /proc/net/nf_conntrack to get this working on unpriviledged namespaces
> too.

Can't it be worked out by using CAP_NET_ADMIN? I don't really like the 
idea to expose the files to non root users or users with not sufficient 
capabilities.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] Expose x_tables /proc entries as 0444 not 0440

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Hiding the contents from non-root users does not achieve anything
> > practical. Possible values are well-known and the specifics can
> > be inferred from a list of loaded modules on most systems.

I agree, thus
Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH] Expose x_tables /proc entries as 0444 not 0440

[Jan Engelhardt]

On Wednesday 2015-11-11 19:40, Florian Westphal wrote:
>Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > Hiding the contents from non-root users does not achieve anything
>> > practical. Possible values are well-known and the specifics can
>> > be inferred from a list of loaded modules on most systems.

Conversely, an administrator could just load all modules to give a false 
impression. Since the adversary can in turn expect it, he knows as 
little as before. In particular, containerized environments will have it 
such that many modules are loaded, but each container still has their 
own ruleset.
So yeah, hiding the contents is not going to achieve anything - nor is 
showing. (I am concurring here with the other respondents.)

Re: [PATCH] Expose x_tables /proc entries as 0444 not 0440

[Phil Whineray]

On Wed, Nov 11, 2015 at 07:48:11PM +0100, Jan Engelhardt wrote:
> On Wednesday 2015-11-11 19:40, Florian Westphal wrote:
> >Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > Hiding the contents from non-root users does not achieve anything
> >> > practical. Possible values are well-known and the specifics can
> >> > be inferred from a list of loaded modules on most systems.
> 
> Conversely, an administrator could just load all modules to give a false 
> impression. Since the adversary can in turn expect it, he knows as 
> little as before. In particular, containerized environments will have it 
> such that many modules are loaded, but each container still has their 
> own ruleset.
> So yeah, hiding the contents is not going to achieve anything - nor is 
> showing. (I am concurring here with the other respondents.)

Sorry - I've gotten confused about who thinks what exactly. I hope
the below isn't wasting everyone's time.

Unhiding /proc/ip_tables_names content achieves something specific: it
is used by iptables-save to determine what it should write out (in
the absence of a specific table being asked for, which I believe is
the norm).

I currently have a workaround using a horrific series of bind mounts
which substitutes in a normal file with the values iptables-save expects.

Two alternates on the table are:

- change ownership of the file in the namespace (wherein a user runs
  "unshare -U -r -n cat /proc/net/ip_tables_names" to do the same as
  "cat /proc/net/ip_tables_names" with 0444 perms, so not adding much
  unless unprivileged namespaces are disabled.
  
- try to do something with capabilities (I'd need to research this)

Given /proc/modules is 0444 and the observation that loaded != used
I don't think any significant information is leaked by the patch. I
don't mind having a stab at an alternate implementation but is it worth
it?

Re: [PATCH] Expose x_tables /proc entries as 0444 not 0440

[Jozsef Kadlecsik]

On Wed, 11 Nov 2015, Phil Whineray wrote:

> On Wed, Nov 11, 2015 at 07:48:11PM +0100, Jan Engelhardt wrote:
> > On Wednesday 2015-11-11 19:40, Florian Westphal wrote:
> > >Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >> > Hiding the contents from non-root users does not achieve anything
> > >> > practical. Possible values are well-known and the specifics can
> > >> > be inferred from a list of loaded modules on most systems.
> > 
> > Conversely, an administrator could just load all modules to give a false 
> > impression. Since the adversary can in turn expect it, he knows as 
> > little as before. In particular, containerized environments will have it 
> > such that many modules are loaded, but each container still has their 
> > own ruleset.
> > So yeah, hiding the contents is not going to achieve anything - nor is 
> > showing. (I am concurring here with the other respondents.)
> 
> Sorry - I've gotten confused about who thinks what exactly. I hope
> the below isn't wasting everyone's time.

My opinion is that the files should not be exposed. Whenever it does not 
clash something vital and cannot be resolved I run kernels with grsec, 
where normal user cannot list the loaded in kernel modules. By exposing 
these files, some part of that data would then be leaked out.

> Unhiding /proc/ip_tables_names content achieves something specific: it
> is used by iptables-save to determine what it should write out (in
> the absence of a specific table being asked for, which I believe is
> the norm).
> 
> I currently have a workaround using a horrific series of bind mounts
> which substitutes in a normal file with the values iptables-save expects.
> 
> Two alternates on the table are:
> 
> - change ownership of the file in the namespace (wherein a user runs
>   "unshare -U -r -n cat /proc/net/ip_tables_names" to do the same as
>   "cat /proc/net/ip_tables_names" with 0444 perms, so not adding much
>   unless unprivileged namespaces are disabled.

I don't quite understand this: if the ownership of the files are changed, 
then why do you need to change the access right to 0444?
   
> - try to do something with capabilities (I'd need to research this)
> 
> Given /proc/modules is 0444 and the observation that loaded != used
> I don't think any significant information is leaked by the patch. I
> don't mind having a stab at an alternate implementation but is it worth
> it?

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] Expose x_tables /proc entries as 0444 not 0440

[Phil Whineray]

On Wed, Nov 11, 2015 at 09:10:13PM +0100, Jozsef Kadlecsik wrote:
> On Wed, 11 Nov 2015, Phil Whineray wrote:
> 
> > On Wed, Nov 11, 2015 at 07:48:11PM +0100, Jan Engelhardt wrote:
> > > On Wednesday 2015-11-11 19:40, Florian Westphal wrote:
> > > >Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >> > Hiding the contents from non-root users does not achieve anything
> > > >> > practical. Possible values are well-known and the specifics can
> > > >> > be inferred from a list of loaded modules on most systems.
> > > 
> > > Conversely, an administrator could just load all modules to give a false 
> > > impression. Since the adversary can in turn expect it, he knows as 
> > > little as before. In particular, containerized environments will have it 
> > > such that many modules are loaded, but each container still has their 
> > > own ruleset.
> > > So yeah, hiding the contents is not going to achieve anything - nor is 
> > > showing. (I am concurring here with the other respondents.)
> > 
> > Sorry - I've gotten confused about who thinks what exactly. I hope
> > the below isn't wasting everyone's time.
> 
> My opinion is that the files should not be exposed. Whenever it does not 
> clash something vital and cannot be resolved I run kernels with grsec, 
> where normal user cannot list the loaded in kernel modules. By exposing 
> these files, some part of that data would then be leaked out.
> 
> > Unhiding /proc/ip_tables_names content achieves something specific: it
> > is used by iptables-save to determine what it should write out (in
> > the absence of a specific table being asked for, which I believe is
> > the norm).
> > 
> > I currently have a workaround using a horrific series of bind mounts
> > which substitutes in a normal file with the values iptables-save expects.
> > 
> > Two alternates on the table are:
> > 
> > - change ownership of the file in the namespace (wherein a user runs
> >   "unshare -U -r -n cat /proc/net/ip_tables_names" to do the same as
> >   "cat /proc/net/ip_tables_names" with 0444 perms, so not adding much
> >   unless unprivileged namespaces are disabled.
> 
> I don't quite understand this: if the ownership of the files are changed, 
> then why do you need to change the access right to 0444?

You're right, I don't. I was just noting point that if unprivileged
namespaces are available, either suffices for a regular user to see the
contents.

Cheers
Phil

[PATCH v2] Root in namespace owns x_tables /proc entries

[Philip Whineray]

Reading these files is impossible in an unprivileged user namespace,
interfering with various firewall tools. For instance, iptables-save
relies on reading /proc/net/ip_tables_names to dump only loaded tables.
---

Please don't apply in current form - it doesn't work. The namespace is
only set up after the /proc entry is created so it keeps the original
owner (an unshare within an unshare can work... mapping root to root).

Since it's in danger of getting quite complicate, would one or more of
the following be acceptable?

- Choose permission in a module parameter

- Allow setting with sysctl e.g. net.netfilter.conf.xtable_proc_perms

- Match permissions of /proc/modules (grsec restricts these so we will
  gain the same policy).

I also worked on a capabilities patch but that made userspace much more
complicated in a namespace than outside one. It would be simpler
to patch programs to read /proc/modules or assume the contents if they
can't read /proc/net/ip_tables_names.

 net/netfilter/x_tables.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 9b42b5e..671654d 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1227,6 +1227,8 @@ int xt_proto_init(struct net *net, u_int8_t af)
 #ifdef CONFIG_PROC_FS
 	char buf[XT_FUNCTION_MAXNAMELEN];
 	struct proc_dir_entry *proc;
+	kuid_t root_uid;
+	kgid_t root_gid;
 #endif
 
 	if (af >= ARRAY_SIZE(xt_prefix))
@@ -1234,12 +1236,16 @@ int xt_proto_init(struct net *net, u_int8_t af)
 
 
 #ifdef CONFIG_PROC_FS
+	root_uid = make_kuid(current_user_ns(), 1000);
+	root_gid = make_kgid(current_user_ns(), 1000);
+
 	strlcpy(buf, xt_prefix[af], sizeof(buf));
 	strlcat(buf, FORMAT_TABLES, sizeof(buf));
 	proc = proc_create_data(buf, 0440, net->proc_net, &xt_table_ops,
 				(void *)(unsigned long)af);
 	if (!proc)
 		goto out;
+	proc_set_user(proc, root_uid, root_gid);
 
 	strlcpy(buf, xt_prefix[af], sizeof(buf));
 	strlcat(buf, FORMAT_MATCHES, sizeof(buf));
-- 
2.1.4

Re: [PATCH v2] Root in namespace owns x_tables /proc entries

[Jozsef Kadlecsik]

Hi Philip,

On Sat, 14 Nov 2015, Philip Whineray wrote:

> Since it's in danger of getting quite complicate, would one or more of
> the following be acceptable?
> 
> - Choose permission in a module parameter
> 
> - Allow setting with sysctl e.g. net.netfilter.conf.xtable_proc_perms
> 
> - Match permissions of /proc/modules (grsec restricts these so we will
>   gain the same policy).

In my opinion either one is good and I'd pick the sysctl setting. That way 
the permissions could be changed without reloading the module and 
independently of the permissions of /proc/modules.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2] Root in namespace owns x_tables /proc entries

[Pablo Neira Ayuso]

On Sun, Nov 15, 2015 at 07:53:53PM +0100, Jozsef Kadlecsik wrote:
> Hi Philip,
> 
> On Sat, 14 Nov 2015, Philip Whineray wrote:
> 
> > Since it's in danger of getting quite complicate, would one or more of
> > the following be acceptable?
> > 
> > - Choose permission in a module parameter
> > 
> > - Allow setting with sysctl e.g. net.netfilter.conf.xtable_proc_perms
> > 
> > - Match permissions of /proc/modules (grsec restricts these so we will
> >   gain the same policy).
> 
> In my opinion either one is good and I'd pick the sysctl setting. That way 
> the permissions could be changed without reloading the module and 
> independently of the permissions of /proc/modules.

I'd rather not to have a sysctl for this thing.

I suspect it will not take long until someone else will follow up with
a similar patch /proc/net/nf_conntrack.

What is the plan of namespace people for unprivileged namespaces with
non-world readable /proc entries?

Re: [PATCH v2] Root in namespace owns x_tables /proc entries

[Phil Whineray]

On Mon, Nov 16, 2015 at 12:56:59PM +0100, Pablo Neira Ayuso wrote:
> On Sun, Nov 15, 2015 at 07:53:53PM +0100, Jozsef Kadlecsik wrote:
> > Hi Philip,
> > 
> > On Sat, 14 Nov 2015, Philip Whineray wrote:
> > 
> > > Since it's in danger of getting quite complicate, would one or more of
> > > the following be acceptable?
> > > 
> > > - Choose permission in a module parameter
> > > 
> > > - Allow setting with sysctl e.g. net.netfilter.conf.xtable_proc_perms
> > > 
> > > - Match permissions of /proc/modules (grsec restricts these so we will
> > >   gain the same policy).
> > 
> > In my opinion either one is good and I'd pick the sysctl setting. That way 
> > the permissions could be changed without reloading the module and 
> > independently of the permissions of /proc/modules.
> 
> I'd rather not to have a sysctl for this thing.
> 
> I suspect it will not take long until someone else will follow up with
> a similar patch /proc/net/nf_conntrack.

That may be true. The two are not equivalent though: the nf_conntrack
information is per-namespace (so setting owner to root in the current
namespace would certainly be sensible), whereas the information in
ip_tables_names is global and directly relates to modules loaded.

> What is the plan of namespace people for unprivileged namespaces with
> non-world readable /proc entries?

It may not have come up as an issue: a quick survey on my systems suggests
pretty much only only netfilter creates files which are group readable but
not world readable:

$ sudo find /proc/ -perm /g=r -a \! -perm /o=r | sed \
     's:proc/[0-9]*/:proc/0/:' | sort -u | less
/proc/0/net/ip6_tables_matches
/proc/0/net/ip6_tables_names
/proc/0/net/ip6_tables_targets
/proc/0/net/ip_tables_matches
/proc/0/net/ip_tables_names
/proc/0/net/ip_tables_targets
/proc/0/net/netfilter/nfnetlink_log
/proc/0/net/nf_conntrack
/proc/0/net/nf_conntrack_expect

Re: [PATCH v2] Root in namespace owns x_tables /proc entries

[Eric W. Biederman]

Philip Whineray <phil@firehol.org> writes:

> Reading these files is impossible in an unprivileged user namespace,
> interfering with various firewall tools. For instance, iptables-save
> relies on reading /proc/net/ip_tables_names to dump only loaded tables.
> ---
>
> Please don't apply in current form - it doesn't work. The namespace is
> only set up after the /proc entry is created so it keeps the original
> owner (an unshare within an unshare can work... mapping root to root).
>
> Since it's in danger of getting quite complicate, would one or more of
> the following be acceptable?
>
> - Choose permission in a module parameter
>
> - Allow setting with sysctl e.g. net.netfilter.conf.xtable_proc_perms
>
> - Match permissions of /proc/modules (grsec restricts these so we will
>   gain the same policy).
>
> I also worked on a capabilities patch but that made userspace much more
> complicated in a namespace than outside one. It would be simpler
> to patch programs to read /proc/modules or assume the contents if they
> can't read /proc/net/ip_tables_names.
>
>  net/netfilter/x_tables.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 9b42b5e..671654d 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1227,6 +1227,8 @@ int xt_proto_init(struct net *net, u_int8_t af)
>  #ifdef CONFIG_PROC_FS
>  	char buf[XT_FUNCTION_MAXNAMELEN];
>  	struct proc_dir_entry *proc;
> +	kuid_t root_uid;
> +	kgid_t root_gid;
>  #endif
>  
>  	if (af >= ARRAY_SIZE(xt_prefix))
> @@ -1234,12 +1236,16 @@ int xt_proto_init(struct net *net, u_int8_t af)
>  
>  
>  #ifdef CONFIG_PROC_FS
> +	root_uid = make_kuid(current_user_ns(), 1000);
> +	root_gid = make_kgid(current_user_ns(), 1000);

These lines are wrong.  They should be:

	root_uid = make_kuid(net->user_ns, 0);
        root_gid = make_kgid(net->user_ns, 0);
        if (!uid_valid(root_uid) || !gid_valid(root_gid))
        	goto out;

>  	strlcpy(buf, xt_prefix[af], sizeof(buf));
>  	strlcat(buf, FORMAT_TABLES, sizeof(buf));
>  	proc = proc_create_data(buf, 0440, net->proc_net, &xt_table_ops,
>  				(void *)(unsigned long)af);
>  	if (!proc)
>  		goto out;
> +	proc_set_user(proc, root_uid, root_gid);
>  
>  	strlcpy(buf, xt_prefix[af], sizeof(buf));
>  	strlcat(buf, FORMAT_MATCHES, sizeof(buf));

Re: [PATCH v2] Root in namespace owns x_tables /proc entries

[Eric W. Biederman]

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

> On Sun, Nov 15, 2015 at 07:53:53PM +0100, Jozsef Kadlecsik wrote:
>> Hi Philip,
>> 
>> On Sat, 14 Nov 2015, Philip Whineray wrote:
>> 
>> > Since it's in danger of getting quite complicate, would one or more of
>> > the following be acceptable?
>> > 
>> > - Choose permission in a module parameter
>> > 
>> > - Allow setting with sysctl e.g. net.netfilter.conf.xtable_proc_perms
>> > 
>> > - Match permissions of /proc/modules (grsec restricts these so we will
>> >   gain the same policy).
>> 
>> In my opinion either one is good and I'd pick the sysctl setting. That way 
>> the permissions could be changed without reloading the module and 
>> independently of the permissions of /proc/modules.
>
> I'd rather not to have a sysctl for this thing.
>
> I suspect it will not take long until someone else will follow up with
> a similar patch /proc/net/nf_conntrack.
>
> What is the plan of namespace people for unprivileged namespaces with
> non-world readable /proc entries?

If it makes sense to have them per namespace and allow manipulation by
the user namespace root, the plan is to make the files owned by the
root user in the user namespace that created the network namespace.

That preserves all of the existing logic.

If we don't trust the user namespace root to read/write the file we
probably want to just avoid creating the file altogether if
(net->user_ns != init_user_ns).

Eric

Re: [PATCH v2] Root in namespace owns x_tables /proc entries

[Phil Whineray]

On Mon, Nov 16, 2015 at 03:56:13PM -0600, Eric W. Biederman wrote:
> Philip Whineray <phil@firehol.org> writes:
> 
> > Reading these files is impossible in an unprivileged user namespace,
> > interfering with various firewall tools. For instance, iptables-save
> > relies on reading /proc/net/ip_tables_names to dump only loaded tables.
> 
> These lines are wrong.  They should be:
> 
>         root_uid = make_kuid(net->user_ns, 0);
>         root_gid = make_kgid(net->user_ns, 0);
>         if (!uid_valid(root_uid) || !gid_valid(root_gid))
>         	goto out;
> 
> >  	strlcpy(buf, xt_prefix[af], sizeof(buf));
> >  	strlcat(buf, FORMAT_TABLES, sizeof(buf));
> >  	proc = proc_create_data(buf, 0440, net->proc_net, &xt_table_ops,
> >  				(void *)(unsigned long)af);
> >  	if (!proc)
> >  		goto out;
> > +	proc_set_user(proc, root_uid, root_gid);

Thanks for the pointer Eric. As written it doesn't quite work because
out is an error path. unshare(CLONE_NEWUSER|CLONE_NEWNET) always fails
due to there not being a mapping for the user yet. Instead:

       root_uid = make_kuid(net->user_ns, 0);
       root_gid = make_kgid(net->user_ns, 0);

followed by:

       if (!proc)
              goto out;
       if (uid_valid(root_uid) && gid_valid(root_gid))
              proc_set_user(proc, root_uid, root_gid);

would preserve the current behaviour but allow the files to be
correctly mapped by first unsharing the user namespace, then setting
the gid map and finally unsharing the namespace.

Or, is it sane to bypass all the above and jump straight to:

        proc_set_user(proc, net->user_ns->owner, net->user_ns->group);

Cheers
Phil

Re: [PATCH v2] Root in namespace owns x_tables /proc entries

[Eric W. Biederman]

Phil Whineray <phil@firehol.org> writes:

> On Mon, Nov 16, 2015 at 03:56:13PM -0600, Eric W. Biederman wrote:
>> Philip Whineray <phil@firehol.org> writes:
>> 
>> > Reading these files is impossible in an unprivileged user namespace,
>> > interfering with various firewall tools. For instance, iptables-save
>> > relies on reading /proc/net/ip_tables_names to dump only loaded tables.
>> 
>> These lines are wrong.  They should be:
>> 
>>         root_uid = make_kuid(net->user_ns, 0);
>>         root_gid = make_kgid(net->user_ns, 0);
>>         if (!uid_valid(root_uid) || !gid_valid(root_gid))
>>         	goto out;
>> 
>> >  	strlcpy(buf, xt_prefix[af], sizeof(buf));
>> >  	strlcat(buf, FORMAT_TABLES, sizeof(buf));
>> >  	proc = proc_create_data(buf, 0440, net->proc_net, &xt_table_ops,
>> >  				(void *)(unsigned long)af);
>> >  	if (!proc)
>> >  		goto out;
>> > +	proc_set_user(proc, root_uid, root_gid);
>
> Thanks for the pointer Eric. As written it doesn't quite work because
> out is an error path. unshare(CLONE_NEWUSER|CLONE_NEWNET) always fails
> due to there not being a mapping for the user yet. Instead:

Point.

Although CLONE_NEWUSER|CLONE_NEWNET are not required to happen
simultaneously.

So you can make what I was suggesting work with:
unshare(CLONE_NEWUSER);
/* Setup the mapping */
unshare(CLONE_NEWNET);

The simplest version of this I can think of is to just not change the
user if the mapping is not setup at the time the proc files are created.

Certainly that would not be worse than what we have today.

>        root_uid = make_kuid(net->user_ns, 0);
>        root_gid = make_kgid(net->user_ns, 0);
>
> followed by:
>
>        if (!proc)
>               goto out;
>        if (uid_valid(root_uid) && gid_valid(root_gid))
>               proc_set_user(proc, root_uid, root_gid);
>
> would preserve the current behaviour but allow the files to be
> correctly mapped by first unsharing the user namespace, then setting
> the gid map and finally unsharing the namespace.

I am not quite certain what you are suggesting above.  It looks and
sounds like my suggest to only call proc_set_user if a mapping exists.
Which is fine. 

> Or, is it sane to bypass all the above and jump straight to:
>
>         proc_set_user(proc, net->user_ns->owner, net->user_ns->group);

It is not.   There are many reasons why typically user_ns->owner and
user_ns->group are rarely mapped in a user namespace.

Eric

Re: [PATCH v2] Root in namespace owns x_tables /proc entries

[Phil Whineray]

On Wed, Nov 18, 2015 at 03:13:51AM -0600, Eric W. Biederman wrote:
> Phil Whineray <phil@firehol.org> writes:
> 
> > On Mon, Nov 16, 2015 at 03:56:13PM -0600, Eric W. Biederman wrote:
> >> Philip Whineray <phil@firehol.org> writes:
> >> 
> >> > Reading these files is impossible in an unprivileged user namespace,
> >> > interfering with various firewall tools. For instance, iptables-save
> >> > relies on reading /proc/net/ip_tables_names to dump only loaded tables.
> >> 
> >> These lines are wrong.  They should be:
> >> 
> >>         root_uid = make_kuid(net->user_ns, 0);
> >>         root_gid = make_kgid(net->user_ns, 0);
> >>         if (!uid_valid(root_uid) || !gid_valid(root_gid))
> >>         	goto out;
> >> 
> >> >  	strlcpy(buf, xt_prefix[af], sizeof(buf));
> >> >  	strlcat(buf, FORMAT_TABLES, sizeof(buf));
> >> >  	proc = proc_create_data(buf, 0440, net->proc_net, &xt_table_ops,
> >> >  				(void *)(unsigned long)af);
> >> >  	if (!proc)
> >> >  		goto out;
> >> > +	proc_set_user(proc, root_uid, root_gid);
> >
> > Thanks for the pointer Eric. As written it doesn't quite work because
> > out is an error path. unshare(CLONE_NEWUSER|CLONE_NEWNET) always fails
> > due to there not being a mapping for the user yet. Instead:
> 
> Point.
> 
> Although CLONE_NEWUSER|CLONE_NEWNET are not required to happen
> simultaneously.
> 
> So you can make what I was suggesting work with:
> unshare(CLONE_NEWUSER);
> /* Setup the mapping */
> unshare(CLONE_NEWNET);
> 
> The simplest version of this I can think of is to just not change the
> user if the mapping is not setup at the time the proc files are created.
> 
> Certainly that would not be worse than what we have today.
> 
> >        root_uid = make_kuid(net->user_ns, 0);
> >        root_gid = make_kgid(net->user_ns, 0);
> >
> > followed by:
> >
> >        if (!proc)
> >               goto out;
> >        if (uid_valid(root_uid) && gid_valid(root_gid))
> >               proc_set_user(proc, root_uid, root_gid);
> >
> > would preserve the current behaviour but allow the files to be
> > correctly mapped by first unsharing the user namespace, then setting
> > the gid map and finally unsharing the namespace.
> 
> I am not quite certain what you are suggesting above.  It looks and
> sounds like my suggest to only call proc_set_user if a mapping exists.
> Which is fine. 

I was indeed trying to get at precisely the same as your suggestion.
Thanks for the much more succinct description.

Patch in due course...

Phil

[PATCH v3] Set /proc/net entries owner to root in namespace

[Philip Whineray]

Various files are owned by root with 0440 permission. Reading them is
impossible in an unprivileged user namespace, interfering with firewall
tools. For instance, iptables-save relies on /proc/net/ip_tables_names
contents to dump only loaded tables.

This patch assigned ownership of the following files to root in the
current namespace:

- /proc/net/*_tables_names
- /proc/net/*_tables_matches
- /proc/net/*_tables_targets
- /proc/net/nf_conntrack
- /proc/net/nf_conntrack_expect
- /proc/net/netfilter/nfnetlink_log

A mapping for root must be available, so this order should be followed:

unshare(CLONE_NEWUSER);
/* Setup the mapping */
unshare(CLONE_NEWNET);

Signed-off-by: Philip Whineray <phil@firehol.org>
---
In addition to the x_tables files ownership change, I added the
connection tracking and netlink logging files which are 0440 since their
contents are handled per-namespace.

I think this patch should address all comments to date.

Regards
Phil

 net/netfilter/nf_conntrack_expect.c     |  7 +++++++
 net/netfilter/nf_conntrack_standalone.c |  7 +++++++
 net/netfilter/nfnetlink_log.c           | 15 +++++++++++++--
 net/netfilter/x_tables.c                | 12 ++++++++++++
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index acf5c7b..278927a 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -596,11 +596,18 @@ static int exp_proc_init(struct net *net)
 {
 #ifdef CONFIG_NF_CONNTRACK_PROCFS
 	struct proc_dir_entry *proc;
+	kuid_t root_uid;
+	kgid_t root_gid;
 
 	proc = proc_create("nf_conntrack_expect", 0440, net->proc_net,
 			   &exp_file_ops);
 	if (!proc)
 		return -ENOMEM;
+
+	root_uid = make_kuid(net->user_ns, 0);
+	root_gid = make_kgid(net->user_ns, 0);
+	if (uid_valid(root_uid) && gid_valid(root_gid))
+		proc_set_user(proc, root_uid, root_gid);
 #endif /* CONFIG_NF_CONNTRACK_PROCFS */
 	return 0;
 }
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 1fb3cac..0f1a45b 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -392,11 +392,18 @@ static const struct file_operations ct_cpu_seq_fops = {
 static int nf_conntrack_standalone_init_proc(struct net *net)
 {
 	struct proc_dir_entry *pde;
+	kuid_t root_uid;
+	kgid_t root_gid;
 
 	pde = proc_create("nf_conntrack", 0440, net->proc_net, &ct_file_ops);
 	if (!pde)
 		goto out_nf_conntrack;
 
+	root_uid = make_kuid(net->user_ns, 0);
+	root_gid = make_kgid(net->user_ns, 0);
+	if (uid_valid(root_uid) && gid_valid(root_gid))
+		proc_set_user(pde, root_uid, root_gid);
+
 	pde = proc_create("nf_conntrack", S_IRUGO, net->proc_net_stat,
 			  &ct_cpu_seq_fops);
 	if (!pde)
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 4670821..74d5117 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -1041,15 +1041,26 @@ static int __net_init nfnl_log_net_init(struct net *net)
 {
 	unsigned int i;
 	struct nfnl_log_net *log = nfnl_log_pernet(net);
+#ifdef CONFIG_PROC_FS
+	struct proc_dir_entry *proc;
+	kuid_t root_uid;
+	kgid_t root_gid;
+#endif
 
 	for (i = 0; i < INSTANCE_BUCKETS; i++)
 		INIT_HLIST_HEAD(&log->instance_table[i]);
 	spin_lock_init(&log->instances_lock);
 
 #ifdef CONFIG_PROC_FS
-	if (!proc_create("nfnetlink_log", 0440,
-			 net->nf.proc_netfilter, &nful_file_ops))
+	proc = proc_create("nfnetlink_log", 0440,
+			   net->nf.proc_netfilter, &nful_file_ops);
+	if (!proc)
 		return -ENOMEM;
+
+	root_uid = make_kuid(net->user_ns, 0);
+	root_gid = make_kgid(net->user_ns, 0);
+	if (uid_valid(root_uid) && gid_valid(root_gid))
+		proc_set_user(proc, root_uid, root_gid);
 #endif
 	return 0;
 }
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 9b42b5e..ff98127 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -26,6 +26,7 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/audit.h>
+#include <linux/user_namespace.h>
 #include <net/net_namespace.h>
 
 #include <linux/netfilter/x_tables.h>
@@ -1227,6 +1228,8 @@ int xt_proto_init(struct net *net, u_int8_t af)
 #ifdef CONFIG_PROC_FS
 	char buf[XT_FUNCTION_MAXNAMELEN];
 	struct proc_dir_entry *proc;
+	kuid_t root_uid;
+	kgid_t root_gid;
 #endif
 
 	if (af >= ARRAY_SIZE(xt_prefix))
@@ -1234,12 +1237,17 @@ int xt_proto_init(struct net *net, u_int8_t af)
 
 
 #ifdef CONFIG_PROC_FS
+	root_uid = make_kuid(net->user_ns, 0);
+	root_gid = make_kgid(net->user_ns, 0);
+
 	strlcpy(buf, xt_prefix[af], sizeof(buf));
 	strlcat(buf, FORMAT_TABLES, sizeof(buf));
 	proc = proc_create_data(buf, 0440, net->proc_net, &xt_table_ops,
 				(void *)(unsigned long)af);
 	if (!proc)
 		goto out;
+	if (uid_valid(root_uid) && gid_valid(root_gid))
+		proc_set_user(proc, root_uid, root_gid);
 
 	strlcpy(buf, xt_prefix[af], sizeof(buf));
 	strlcat(buf, FORMAT_MATCHES, sizeof(buf));
@@ -1247,6 +1255,8 @@ int xt_proto_init(struct net *net, u_int8_t af)
 				(void *)(unsigned long)af);
 	if (!proc)
 		goto out_remove_tables;
+	if (uid_valid(root_uid) && gid_valid(root_gid))
+		proc_set_user(proc, root_uid, root_gid);
 
 	strlcpy(buf, xt_prefix[af], sizeof(buf));
 	strlcat(buf, FORMAT_TARGETS, sizeof(buf));
@@ -1254,6 +1264,8 @@ int xt_proto_init(struct net *net, u_int8_t af)
 				(void *)(unsigned long)af);
 	if (!proc)
 		goto out_remove_matches;
+	if (uid_valid(root_uid) && gid_valid(root_gid))
+		proc_set_user(proc, root_uid, root_gid);
 #endif
 
 	return 0;
-- 
2.1.4

Re: [PATCH v3] Set /proc/net entries owner to root in namespace

[Pablo Neira Ayuso]

On Sun, Nov 22, 2015 at 11:35:07AM +0000, Philip Whineray wrote:
> Various files are owned by root with 0440 permission. Reading them is
> impossible in an unprivileged user namespace, interfering with firewall
> tools. For instance, iptables-save relies on /proc/net/ip_tables_names
> contents to dump only loaded tables.
> 
> This patch assigned ownership of the following files to root in the
> current namespace:
> 
> - /proc/net/*_tables_names
> - /proc/net/*_tables_matches
> - /proc/net/*_tables_targets
> - /proc/net/nf_conntrack
> - /proc/net/nf_conntrack_expect
> - /proc/net/netfilter/nfnetlink_log
> 
> A mapping for root must be available, so this order should be followed:
> 
> unshare(CLONE_NEWUSER);
> /* Setup the mapping */
> unshare(CLONE_NEWNET);

Applied, thanks.