[Question] Should `CAP_NET_ADMIN` be needed when opening `/dev/ppp`?

[Question] Should `CAP_NET_ADMIN` be needed when opening `/dev/ppp`?

[Wang Shanker]

Hi, all.

                                                                               
I’ve recently met some problems when trying to create a pppoe network link 
inside a unprivileged container. There is a uid namespace which maps root 
inside to a normal user outside. There is also a separate net namespace in the 
container. I create a dev node inside the container and set right 
permission.    

However, `/dev/ppp` cannot get opened since the mapped normal user does not 
have `CAP_NET_ADMIN`. The related code is in `drivers/net/ppp/ppp_generic.c`: 
`int ppp_open()` 

```
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. 

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

[Guillaume Nault]

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.

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

[Richard Weinberger]

On Tue, May 3, 2016 at 12:12 PM, Guillaume Nault <g.nault@alphalink.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?

-- 
Thanks,
//richard

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

[Hannes Frederic Sowa]

On Tue, May 3, 2016, at 12:35, Richard Weinberger wrote:
> On Tue, May 3, 2016 at 12:12 PM, Guillaume Nault <g.nault@alphalink.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.

Bye,
Hannes

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

[王邈]

> 在 2016年5月3日，下午7:23，Hannes Frederic Sowa <hannes@stressinduktion.org> 写道：
> 
> On Tue, May 3, 2016, at 12:35, Richard Weinberger wrote:
>> On Tue, May 3, 2016 at 12:12 PM, Guillaume Nault <g.nault@alphalink.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 agree with that.
> 
> Bye,
> Hannes

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

[Guillaume Nault]

On Tue, May 03, 2016 at 12:35:12PM +0200, Richard Weinberger wrote:
> On Tue, May 3, 2016 at 12:12 PM, Guillaume Nault <g.nault@alphalink.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?
> 
Indeed, I overlooked the namespace aspect of the problem. PPP is netns
aware, but ioctls performed on /dev/ppp file descriptors are all
serialised with ppp_mutex. A user could therefore affect other PPP
users by artificially creating contention on the ppp_mutex lock.

Other than that, I agree it'd make sense to test for user capabilies in
the current namespace rather than in the initial one.

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

[Guillaume Nault]

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@alphalink.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.

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

[Hannes Frederic Sowa]

On 03.05.2016 17:51, Guillaume Nault wrote:
> 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@alphalink.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().

We want to test our (*current) capability in the user namespace the net
namespace was created. current is implied here.

If you create a new user_namespace ontop the same network stack you
shouldn't have those capabilities, otherwise you can elevate capabilities.

Bye,
Hannes