Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1493539 > unrolled thread

Re: [PATCH v2 3/3] net: make net namespace sysctls belong to container's owner

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/3] net: make net namespace sysctls belong to  container's owner Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-09-29 17:50 +0200
    Re: [PATCH v2 3/3] net: make net namespace sysctls belong to  container's owner David Miller <davem@davemloft.net> - 2016-09-30 07:30 +0200
      Re: [PATCH v2 3/3] net: make net namespace sysctls belong to  container's owner Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-10-01 00:30 +0200

#1493539 — Re: [PATCH v2 3/3] net: make net namespace sysctls belong to container's owner

Hi David,

On Wed, Aug 10, 2016 at 2:36 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> If net namespace is attached to a user namespace let's make container's
> root owner of sysctls affecting said network namespace instead of global
> root.
>
> This also allows us to clean up net_ctl_permissions() because we do not
> need to fudge permissions anymore for the container's owner since it now
> owns the objects in question.
>
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I was looking at linux-next today, and I noticed that, when you merged
my patch, you basically reverted the following commit:

commit d6e0d306449bcb5fa3c80e7a3edf11d45abf9ae9
Author: Tyler Hicks <tyhicks@canonical.com>
Date:   Thu Jun 2 23:43:22 2016 -0500

    net: Use ns_capable_noaudit() when determining net sysctl permissions

    The capability check should not be audited since it is only being used
    to determine the inode permissions. A failed check does not indicate a
    violation of security policy but, when an LSM is enabled, a denial audit
    message was being generated.

    The denial audit message caused confusion for some application authors
    because root-running Go applications always triggered the denial. To
    prevent this confusion, the capability check in net_ctl_permissions() is
    switched to the noaudit variant.

    BugLink: https://launchpad.net/bugs/1465724

    Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
    Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
    Signed-off-by: James Morris <james.l.morris@oracle.com>

Thanks!

> ---
>  fs/proc/proc_sysctl.c  |  5 +++++
>  include/linux/sysctl.h |  4 ++++
>  net/sysctl_net.c       | 29 ++++++++++++++++++++---------
>  3 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 5e57c3e..28f9085 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -430,6 +430,7 @@ static int sysctl_perm(struct ctl_table_header *head, struct ctl_table *table, i
>  static struct inode *proc_sys_make_inode(struct super_block *sb,
>                 struct ctl_table_header *head, struct ctl_table *table)
>  {
> +       struct ctl_table_root *root = head->root;
>         struct inode *inode;
>         struct proc_inode *ei;
>
> @@ -457,6 +458,10 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>                 if (is_empty_dir(head))
>                         make_empty_dir_inode(inode);
>         }
> +
> +       if (root->set_ownership)
> +               root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
> +
>  out:
>         return inode;
>  }
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index fa7bc29..55bec2f 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -25,6 +25,7 @@
>  #include <linux/rcupdate.h>
>  #include <linux/wait.h>
>  #include <linux/rbtree.h>
> +#include <linux/uidgid.h>
>  #include <uapi/linux/sysctl.h>
>
>  /* For the /proc/sys support */
> @@ -156,6 +157,9 @@ struct ctl_table_root {
>         struct ctl_table_set default_set;
>         struct ctl_table_set *(*lookup)(struct ctl_table_root *root,
>                                            struct nsproxy *namespaces);
> +       void (*set_ownership)(struct ctl_table_header *head,
> +                             struct ctl_table *table,
> +                             kuid_t *uid, kgid_t *gid);
>         int (*permissions)(struct ctl_table_header *head, struct ctl_table *table);
>  };
>
> diff --git a/net/sysctl_net.c b/net/sysctl_net.c
> index ed98c1f..5bc1a3d 100644
> --- a/net/sysctl_net.c
> +++ b/net/sysctl_net.c
> @@ -42,26 +42,37 @@ static int net_ctl_permissions(struct ctl_table_header *head,
>                                struct ctl_table *table)
>  {
>         struct net *net = container_of(head->set, struct net, sysctls);
> -       kuid_t root_uid = make_kuid(net->user_ns, 0);
> -       kgid_t root_gid = make_kgid(net->user_ns, 0);
>
>         /* Allow network administrator to have same access as root. */
> -       if (ns_capable(net->user_ns, CAP_NET_ADMIN) ||
> -           uid_eq(root_uid, current_euid())) {
> +       if (ns_capable(net->user_ns, CAP_NET_ADMIN)) {
>                 int mode = (table->mode >> 6) & 7;
>                 return (mode << 6) | (mode << 3) | mode;
>         }
> -       /* Allow netns root group to have the same access as the root group */
> -       if (in_egroup_p(root_gid)) {
> -               int mode = (table->mode >> 3) & 7;
> -               return (mode << 3) | mode;
> -       }
> +
>         return table->mode;
>  }
>
> +static void net_ctl_set_ownership(struct ctl_table_header *head,
> +                                 struct ctl_table *table,
> +                                 kuid_t *uid, kgid_t *gid)
> +{
> +       struct net *net = container_of(head->set, struct net, sysctls);
> +       kuid_t ns_root_uid;
> +       kgid_t ns_root_gid;
> +
> +       ns_root_uid = make_kuid(net->user_ns, 0);
> +       if (uid_valid(ns_root_uid))
> +               *uid = ns_root_uid;
> +
> +       ns_root_gid = make_kgid(net->user_ns, 0);
> +       if (gid_valid(ns_root_gid))
> +               *gid = ns_root_gid;
> +}
> +
>  static struct ctl_table_root net_sysctl_root = {
>         .lookup = net_ctl_header_lookup,
>         .permissions = net_ctl_permissions,
> +       .set_ownership = net_ctl_set_ownership,
>  };
>
>  static int __net_init sysctl_net_init(struct net *net)
> --
> 2.8.0.rc3.226.g39d4020
>

-- 
Dmitry

[toc] | [next] | [standalone]

#1493888

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Thu, 29 Sep 2016 08:46:05 -0700

> Hi David,
> 
> On Wed, Aug 10, 2016 at 2:36 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> If net namespace is attached to a user namespace let's make container's
>> root owner of sysctls affecting said network namespace instead of global
>> root.
>>
>> This also allows us to clean up net_ctl_permissions() because we do not
>> need to fudge permissions anymore for the container's owner since it now
>> owns the objects in question.
>>
>> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> I was looking at linux-next today, and I noticed that, when you merged
> my patch, you basically reverted the following commit:
> 
> commit d6e0d306449bcb5fa3c80e7a3edf11d45abf9ae9
> Author: Tyler Hicks <tyhicks@canonical.com>
> Date:   Thu Jun 2 23:43:22 2016 -0500
> 
>     net: Use ns_capable_noaudit() when determining net sysctl permissions

Please send me a fixup patch for this, sorry.

[toc] | [prev] | [next] | [standalone]

#1494313

On Fri, Sep 30, 2016 at 01:21:27AM -0400, David Miller wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Date: Thu, 29 Sep 2016 08:46:05 -0700
> 
> > Hi David,
> > 
> > On Wed, Aug 10, 2016 at 2:36 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >> If net namespace is attached to a user namespace let's make container's
> >> root owner of sysctls affecting said network namespace instead of global
> >> root.
> >>
> >> This also allows us to clean up net_ctl_permissions() because we do not
> >> need to fudge permissions anymore for the container's owner since it now
> >> owns the objects in question.
> >>
> >> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > I was looking at linux-next today, and I noticed that, when you merged
> > my patch, you basically reverted the following commit:
> > 
> > commit d6e0d306449bcb5fa3c80e7a3edf11d45abf9ae9
> > Author: Tyler Hicks <tyhicks@canonical.com>
> > Date:   Thu Jun 2 23:43:22 2016 -0500
> > 
> >     net: Use ns_capable_noaudit() when determining net sysctl permissions
> 
> Please send me a fixup patch for this, sorry.

Just did, look for <20160930222431.GA30208@dtor-ws>

Thanks.

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web