Re: Discuss the implementation of pidns_install()

Re: Discuss the implementation of pidns_install()

[Kees Cook]

On Wed, Sep 20, 2023 at 05:37:20PM +0800, yunhui cui wrote:
> Hi Kees, jeff, Andrew, Christian,
> 
> We hope that containers at the same level can also switch pid namespace.
> To fork() the entire pstree of a container, we need to switch from the pid
> namespace of the template container to the  target container's pid
> namespace. But it is blocked by the following code:
> 
> ...
>         while (ancestor->level > active->level)
>                 ancestor = ancestor->parent;
>         if (ancestor != active)
>                 return -EINVAL;
> ...
> 
> Can we give this code permission, such as CAP_SYS_ADMIN ..., so that those
> with this permission can switch to sibling pid namespace. Rather than just
> parent, parent ...
> 
> Do you think this plan is okay?  Or can you give me some suggestions?

I'll defer to Christian on this, but it seems like moving processes
sideways is kind of unexpected. I agree it'd need privilege if we did
it, though.

-- 
Kees Cook

Re: [External] Re: Discuss the implementation of pidns_install()

[yunhui cui]

Hi Kees Cook，

On Tue, Oct 10, 2023 at 4:08 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Sep 20, 2023 at 05:37:20PM +0800, yunhui cui wrote:
> > Hi Kees, jeff, Andrew, Christian,
> >
> > We hope that containers at the same level can also switch pid namespace.
> > To fork() the entire pstree of a container, we need to switch from the pid
> > namespace of the template container to the  target container's pid
> > namespace. But it is blocked by the following code:
> >
> > ...
> >         while (ancestor->level > active->level)
> >                 ancestor = ancestor->parent;
> >         if (ancestor != active)
> >                 return -EINVAL;
> > ...
> >
> > Can we give this code permission, such as CAP_SYS_ADMIN ..., so that those
> > with this permission can switch to sibling pid namespace. Rather than just
> > parent, parent ...
> >
> > Do you think this plan is okay?  Or can you give me some suggestions?
>
> I'll defer to Christian on this, but it seems like moving processes
> sideways is kind of unexpected. I agree it'd need privilege if we did
> it, though.
>
>

I'll defer to Christian on this, but it seems like moving processes
sideways is kind of unexpected. I agree it'd need privilege if we did
it, though.

Thanks for your suggestion, my plan is to give this code SYS_ADMIN permissions.

CAP_SYS_ADMIN has been checked in pidns_install(), as follows:
static int pidns_install(struct nsproxy *nsproxy, struct ns_common *ns)
{
...
if (!ns_capable(new->user_ns, CAP_SYS_ADMIN) ||
!ns_capable(current_user_ns(), CAP_SYS_ADMIN))
return -EPERM;
...
}

So can I delete this code:
ancestor = new;
while (ancestor->level > active->level)
ancestor = ancestor->parent;
if (ancestor != active)
return -EINVAL；

Thanks，
Yunhui

Re: [External] Re: Discuss the implementation of pidns_install()

[yunhui cui]

Hi Christian,

On Tue, Oct 10, 2023 at 11:29 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Kees Cook，
>
> On Tue, Oct 10, 2023 at 4:08 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Sep 20, 2023 at 05:37:20PM +0800, yunhui cui wrote:
> > > Hi Kees, jeff, Andrew, Christian,
> > >
> > > We hope that containers at the same level can also switch pid namespace.
> > > To fork() the entire pstree of a container, we need to switch from the pid
> > > namespace of the template container to the  target container's pid
> > > namespace. But it is blocked by the following code:
> > >
> > > ...
> > >         while (ancestor->level > active->level)
> > >                 ancestor = ancestor->parent;
> > >         if (ancestor != active)
> > >                 return -EINVAL;
> > > ...
> > >
> > > Can we give this code permission, such as CAP_SYS_ADMIN ..., so that those
> > > with this permission can switch to sibling pid namespace. Rather than just
> > > parent, parent ...
> > >
> > > Do you think this plan is okay?  Or can you give me some suggestions?
> >
> > I'll defer to Christian on this, but it seems like moving processes
> > sideways is kind of unexpected. I agree it'd need privilege if we did
> > it, though.
> >
> >
>
> I'll defer to Christian on this, but it seems like moving processes
> sideways is kind of unexpected. I agree it'd need privilege if we did
> it, though.
>
> Thanks for your suggestion, my plan is to give this code SYS_ADMIN permissions.
>
> CAP_SYS_ADMIN has been checked in pidns_install(), as follows:
> static int pidns_install(struct nsproxy *nsproxy, struct ns_common *ns)
> {
> ...
> if (!ns_capable(new->user_ns, CAP_SYS_ADMIN) ||
> !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
> return -EPERM;
> ...
> }
>
> So can I delete this code:
> ancestor = new;
> while (ancestor->level > active->level)
> ancestor = ancestor->parent;
> if (ancestor != active)
> return -EINVAL；
>
> Thanks，
> Yunhui

Could you help me look into this issue?