On Wed, Nov 18, 2015 at 03:13:51AM -0600, Eric W. Biederman wrote: > Phil Whineray <phil@xxxxxxxxxxx> writes: > > > On Mon, Nov 16, 2015 at 03:56:13PM -0600, Eric W. Biederman wrote: > >> Philip Whineray <phil@xxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
