linux-kernel - Re: [PATCH v1 1/3] sysctl: Allow change system v ipc sysctls inside ipc namespace

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: <YynnI2ySUkpu9j6S@example.org>
Date:   Tue, 20 Sep 2022 18:15:31 +0200
From:   Alexey Gladkov <legion@...nel.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux Containers <containers@...ts.linux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christian Brauner <brauner@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Manfred Spraul <manfred@...orfullife.com>
Subject: Re: [PATCH v1 1/3] sysctl: Allow change system v ipc sysctls inside
 ipc namespace

On Mon, Sep 19, 2022 at 10:26:39AM -0500, Eric W. Biederman wrote:
> >
> > diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
> > index ef313ecfb53a..87eb1b1e42fa 100644
> > --- a/ipc/ipc_sysctl.c
> > +++ b/ipc/ipc_sysctl.c
> > @@ -192,23 +192,47 @@ static int set_is_seen(struct ctl_table_set *set)
> >  
> >  static int ipc_permissions(struct ctl_table_header *head, struct ctl_table *table)
> >  {
> > -	int mode = table->mode;
> > -
> > -#ifdef CONFIG_CHECKPOINT_RESTORE
> >  	struct ipc_namespace *ns = current->nsproxy->ipc_ns;
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Historically that was the best we could do.  But now that we have
> an ipc_set member in struct ipc_namespace you can use container_of
> to compute this value.
> 
> For a permission check that is much safer.

Yes. It make sense.

> > +#ifdef CONFIG_CHECKPOINT_RESTORE
> >  	if (((table->data == &ns->ids[IPC_SEM_IDS].next_id) ||
> >  	     (table->data == &ns->ids[IPC_MSG_IDS].next_id) ||
> >  	     (table->data == &ns->ids[IPC_SHM_IDS].next_id)) &&
> >  	    checkpoint_restore_ns_capable(ns->user_ns))
> > -		mode = 0666;
> > +		return 0666;
> >  #endif
> > -	return mode;
> > +	if (ns->user_ns != &init_user_ns) {
> > +		kuid_t ns_root_uid = make_kuid(ns->user_ns, 0);
> > +		kgid_t ns_root_gid = make_kgid(ns->user_ns, 0);
> > +
> > +		if (uid_valid(ns_root_uid) && uid_eq(current_euid(), ns_root_uid))
> > +			return table->mode >> 6;
> > +
> > +		if (gid_valid(ns_root_gid) && in_egroup_p(ns_root_gid))
> > +			return table->mode >> 3;
> 
> >From 10,000 fee this is fine.  But this has to interact with
> test_perm in proc_systl.c.  So can you please do what
> net_ctl_permissions does and replicate the chosen mode all through
> the mode line.
> 
> Perhaps something like:
> 
> 	kuid_t ns_root_uid;
> 	kgid_t ns_root_gid
> 
> 	ipc_set_ownership(head, table, &ns_root_uid, &ns_root_gid);
> 
> #ifdef CONFIG_CHECKPOINT_RESTORE
> 	if (...)
>         	mode = 0666;
>         else
> #endif        
>         if (uid_eq(current_euid(), ns_root_uid))
> 		mode >>= 6;
>         
>         else if (uid_eq(in_group_p(ns_root_gid))
> 		mode >>= 3;
> 
> 	mode &= 7;
>         mode = (mode << 6) | (mode << 3) | mode;
>         return mode;
>         
> 
> If we always pass through the same logic there is the advantage that we
> will always test it, and there is less room for bugs to slip through.
> 
> I added a couple of unnecessary simplifications in there that I just
> saw as I was writing my example code.

Thanks! It looks better. I'll fix it and send a new version.

-- 
Rgrds, legion