linux-kernel - Re: [Question] Should `CAP_NET_ADMIN` be needed when opening `/dev/ppp`?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160503155157.GB1304@alphalink.fr>
Date:	Tue, 3 May 2016 17:51:57 +0200
From:	Guillaume Nault <g.nault@...halink.fr>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	Richard Weinberger <richard.weinberger@...il.com>,
	Wang Shanker <shankerwangmiao@...il.com>,
	netdev@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [Question] Should `CAP_NET_ADMIN` be needed when opening
 `/dev/ppp`?

On Tue, May 03, 2016 at 01:23:34PM +0200, Hannes Frederic Sowa wrote:
> On Tue, May 3, 2016, at 12:35, Richard Weinberger wrote:
> > On Tue, May 3, 2016 at 12:12 PM, Guillaume Nault <g.nault@...halink.fr>
> > wrote:
> > > On Sun, May 01, 2016 at 09:38:57PM +0800, Wang Shanker wrote:
> > >> static int ppp_open(struct inode *inode, struct file *file)
> > >> {
> > >>       /*
> > >>        * This could (should?) be enforced by the permissions on /dev/ppp.
> > >>        */
> > >>       if (!capable(CAP_NET_ADMIN))
> > >>               return -EPERM;
> > >>       return 0;
> > >> }
> > >> ```
> > >>
> > >> I wonder why CAP_NET_ADMIN is needed here, rather than leaving it to the
> > >> permission of the device node. If there is no need, I suggest that the
> > >> CAP_NET_ADMIN check be removed.
> > >>
> > > If this test was removed here, then it'd have to be added again in the
> > > PPPIOCNEWUNIT ioctl, at the very least, because creating a netdevice
> > > should require CAP_NET_ADMIN. Therefore that wouldn't help for your
> > > case.
> > > I don't know why the test was placed in ppp_open() in the first place,
> > > but changing it now would have side effects on user space. So I'd
> > > rather leave the code as is.
> > 
> > I think the question is whether we really require having CAP_NET_ADMIN
> > in the initial namespace and not just in the current one.
> > Is ppp not network namespace aware?
> 
> I agree, ns_capable(net->user_ns, CAP_NET_ADMIN), would probably make
> more sense.
>
I guess you assume net is set to current->nsproxy->net_ns here.
Why about ns_capable(current_user_ns(), CAP_NET_ADMIN)?

>From my understanding of the code (I currently have no practical
experience with user namespaces), net->user_ns points to the userns in
which the current netns was created, while current_user_ns() refers to
the caller's userns. Shouldn't we check the later? Otherwise, any
process running in the netns would have the same capabilities regarding
PPP ioctls().

But I'm certainly missing important points. Interactions between netns
and userns are something I never investigated before, and using
net->user_ns seems to be way more common than using current_user_ns()
for checking capabilities in the networking stack.