Date: Wed, 18 Nov 2020 22:19:39 +0100 From: Mariusz Zaborski <oshogbo@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r367819 - in head: sys/kern sys/sys usr.sbin/jail Message-ID: <CAGOYWV_p1WT0ijCS3Xk83DjA1%2Bfqa0oajjv-2EVR2_iF8GL0Yw@mail.gmail.com> In-Reply-To: <CAGudoHGFhjwf=uVh6uKqPRk-0X6yi9VxNZiNhYGG41_Wwu8zSA@mail.gmail.com> References: <202011182107.0AIL78pi053574@repo.freebsd.org> <CAGudoHGFhjwf=uVh6uKqPRk-0X6yi9VxNZiNhYGG41_Wwu8zSA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hello Mateusz, Thank you for pointing out this. I will fix those. Thanks, Mariusz On Wed, 18 Nov 2020 at 22:15, Mateusz Guzik <mjguzik@gmail.com> wrote: > > On 11/18/20, Mariusz Zaborski <oshogbo@freebsd.org> wrote: > > Author: oshogbo > > Date: Wed Nov 18 21:07:08 2020 > > New Revision: 367819 > > URL: https://svnweb.freebsd.org/changeset/base/367819 > > > > Log: > > jail: introduce per jail suser_enabled setting > > > > The suser_enable sysctl allows to remove a privileged rights from uid 0. > > This change introduce per jail setting which allow to make root a > > normal user. > > > > Reviewed by: jamie > > Previous version reviewed by: kevans, emaste, markj, me_igalic.co > > Discussed with: pjd > > Differential Revision: https://reviews.freebsd.org/D27128 > > > > Modified: > > head/sys/kern/kern_jail.c > > head/sys/kern/kern_priv.c > > head/sys/sys/jail.h > > head/usr.sbin/jail/jail.8 > > > > Modified: head/sys/kern/kern_jail.c > > ============================================================================== > > --- head/sys/kern/kern_jail.c Wed Nov 18 20:59:58 2020 (r367818) > > +++ head/sys/kern/kern_jail.c Wed Nov 18 21:07:08 2020 (r367819) > > @@ -199,12 +199,14 @@ static struct bool_flags pr_flag_allow[NBBY * NBPW] = > > > > {"allow.read_msgbuf", "allow.noread_msgbuf", PR_ALLOW_READ_MSGBUF}, > > {"allow.unprivileged_proc_debug", "allow.nounprivileged_proc_debug", > > PR_ALLOW_UNPRIV_DEBUG}, > > + {"allow.suser", "allow.nosuser", PR_ALLOW_SUSER}, > > }; > > const size_t pr_flag_allow_size = sizeof(pr_flag_allow); > > > > #define JAIL_DEFAULT_ALLOW (PR_ALLOW_SET_HOSTNAME | \ > > PR_ALLOW_RESERVED_PORTS | \ > > - PR_ALLOW_UNPRIV_DEBUG) > > + PR_ALLOW_UNPRIV_DEBUG | \ > > + PR_ALLOW_SUSER) > > #define JAIL_DEFAULT_ENFORCE_STATFS 2 > > #define JAIL_DEFAULT_DEVFS_RSNUM 0 > > static unsigned jail_default_allow = JAIL_DEFAULT_ALLOW; > > @@ -3815,6 +3817,8 @@ SYSCTL_JAIL_PARAM(_allow, read_msgbuf, CTLTYPE_INT | > > C > > "B", "Jail may read the kernel message buffer"); > > SYSCTL_JAIL_PARAM(_allow, unprivileged_proc_debug, CTLTYPE_INT | > > CTLFLAG_RW, > > "B", "Unprivileged processes may use process debugging facilities"); > > +SYSCTL_JAIL_PARAM(_allow, suser, CTLTYPE_INT | CTLFLAG_RW, > > + "B", "Processes in jail with uid 0 have privilege"); > > > > SYSCTL_JAIL_PARAM_SUBNODE(allow, mount, "Jail mount/unmount permission > > flags"); > > SYSCTL_JAIL_PARAM(_allow_mount, , CTLTYPE_INT | CTLFLAG_RW, > > > > Modified: head/sys/kern/kern_priv.c > > ============================================================================== > > --- head/sys/kern/kern_priv.c Wed Nov 18 20:59:58 2020 (r367818) > > +++ head/sys/kern/kern_priv.c Wed Nov 18 21:07:08 2020 (r367819) > > @@ -3,6 +3,7 @@ > > * > > * Copyright (c) 2006 nCircle Network Security, Inc. > > * Copyright (c) 2009 Robert N. M. Watson > > + * Copyright (c) 2020 Mariusz Zaborski <oshogbo@FreeBSD.org> > > * All rights reserved. > > * > > * This software was developed by Robert N. M. Watson for the TrustedBSD > > @@ -36,6 +37,9 @@ __FBSDID("$FreeBSD$"); > > #include <sys/param.h> > > #include <sys/jail.h> > > #include <sys/kernel.h> > > +#include <sys/lock.h> > > +#include <sys/mutex.h> > > +#include <sys/sx.h> > > #include <sys/priv.h> > > #include <sys/proc.h> > > #include <sys/sdt.h> > > @@ -54,10 +58,58 @@ __FBSDID("$FreeBSD$"); > > * userland programs, and should not be done without careful consideration > > of > > * the consequences. > > */ > > -static int __read_mostly suser_enabled = 1; > > -SYSCTL_INT(_security_bsd, OID_AUTO, suser_enabled, CTLFLAG_RWTUN, > > - &suser_enabled, 0, "processes with uid 0 have privilege"); > > > > +static bool > > +suser_enabled(struct ucred *cred) > > +{ > > + > > + return (prison_allow(cred, PR_ALLOW_SUSER) ? true : false); > > +} > > + > > This converts a variable read into a function call to prison_allow. > prison_allow should be converted into an inline func and put in a > header. > > Also: > /* This is an atomic read, so no locking is necessary. */ > return (cred->cr_prison->pr_allow & flag); > > happens to probably work in practice, but is wrong. Short of > atomic_store to modify and atomic_load to obtain the content can be > arbitrarily mangled during concurrent concurrent write. > > > +static void inline > > +prison_suser_set(struct prison *pr, int enabled) > > +{ > > + > > + if (enabled) { > > + pr->pr_allow |= PR_ALLOW_SUSER; > > + } else { > > + pr->pr_allow &= ~PR_ALLOW_SUSER; > > + } > > +} > > + > > +static int > > +sysctl_kern_suser_enabled(SYSCTL_HANDLER_ARGS) > > +{ > > + struct prison *pr, *cpr; > > + struct ucred *cred; > > + int descend, error, enabled; > > + > > + cred = req->td->td_ucred; > > + enabled = suser_enabled(cred); > > + > > + error = sysctl_handle_int(oidp, &enabled, 0, req); > > + if (error || !req->newptr) > > + return (error); > > + > > + pr = cred->cr_prison; > > + sx_slock(&allprison_lock); > > + mtx_lock(&pr->pr_mtx); > > + > > + prison_suser_set(pr, enabled); > > + if (!enabled) { > > + FOREACH_PRISON_DESCENDANT_LOCKED(pr, cpr, descend) { > > + prison_suser_set(cpr, 0); > > + } > > + } > > + mtx_unlock(&pr->pr_mtx); > > + sx_sunlock(&allprison_lock); > > + return (0); > > +} > > + > > +SYSCTL_PROC(_security_bsd, OID_AUTO, suser_enabled, CTLTYPE_INT | > > + CTLFLAG_RWTUN | CTLFLAG_PRISON, 0, 0, &sysctl_kern_suser_enabled, "I", > > + "Processes with uid 0 have privilege"); > > + > > This lacks CTLFLAG_MPSAFE. > > > static int unprivileged_mlock = 1; > > SYSCTL_INT(_security_bsd, OID_AUTO, unprivileged_mlock, CTLFLAG_RWTUN, > > &unprivileged_mlock, 0, "Allow non-root users to call mlock(2)"); > > @@ -186,7 +238,7 @@ priv_check_cred(struct ucred *cred, int priv) > > * superuser policy to be globally disabled, although this is > > * currenty of limited utility. > > */ > > - if (suser_enabled) { > > + if (suser_enabled(cred)) { > > switch (priv) { > > case PRIV_MAXFILES: > > case PRIV_MAXPROC: > > @@ -258,7 +310,7 @@ priv_check_cred_vfs_lookup_slow(struct ucred *cred) > > if (error) > > goto out; > > > > - if (cred->cr_uid == 0 && suser_enabled) { > > + if (cred->cr_uid == 0 && suser_enabled(cred)) { > > error = 0; > > goto out; > > } > > @@ -279,7 +331,7 @@ priv_check_cred_vfs_lookup(struct ucred *cred) > > return (priv_check_cred_vfs_lookup_slow(cred)); > > > > error = EPERM; > > - if (cred->cr_uid == 0 && suser_enabled) > > + if (cred->cr_uid == 0 && suser_enabled(cred)) > > error = 0; > > return (error); > > } > > @@ -294,7 +346,7 @@ priv_check_cred_vfs_lookup_nomac(struct ucred *cred) > > return (EAGAIN); > > > > error = EPERM; > > - if (cred->cr_uid == 0 && suser_enabled) > > + if (cred->cr_uid == 0 && suser_enabled(cred)) > > error = 0; > > return (error); > > } > > @@ -313,7 +365,7 @@ priv_check_cred_vfs_generation_slow(struct ucred *cred > > goto out; > > } > > > > - if (cred->cr_uid == 0 && suser_enabled) { > > + if (cred->cr_uid == 0 && suser_enabled(cred)) { > > error = 0; > > goto out; > > } > > @@ -334,7 +386,7 @@ priv_check_cred_vfs_generation(struct ucred *cred) > > return (priv_check_cred_vfs_generation_slow(cred)); > > > > error = EPERM; > > - if (!jailed(cred) && cred->cr_uid == 0 && suser_enabled) > > + if (!jailed(cred) && cred->cr_uid == 0 && suser_enabled(cred)) > > error = 0; > > return (error); > > } > > > > Modified: head/sys/sys/jail.h > > ============================================================================== > > --- head/sys/sys/jail.h Wed Nov 18 20:59:58 2020 (r367818) > > +++ head/sys/sys/jail.h Wed Nov 18 21:07:08 2020 (r367819) > > @@ -232,9 +232,10 @@ struct prison_racct { > > #define PR_ALLOW_MLOCK 0x00000080 > > #define PR_ALLOW_READ_MSGBUF 0x00000100 > > #define PR_ALLOW_UNPRIV_DEBUG 0x00000200 > > +#define PR_ALLOW_SUSER 0x00000400 > > #define PR_ALLOW_RESERVED_PORTS 0x00008000 > > #define PR_ALLOW_KMEM_ACCESS 0x00010000 /* reserved, not used yet */ > > -#define PR_ALLOW_ALL_STATIC 0x000183ff > > +#define PR_ALLOW_ALL_STATIC 0x000187ff > > > > /* > > * PR_ALLOW_DIFFERENCES determines which flags are able to be > > > > Modified: head/usr.sbin/jail/jail.8 > > ============================================================================== > > --- head/usr.sbin/jail/jail.8 Wed Nov 18 20:59:58 2020 (r367818) > > +++ head/usr.sbin/jail/jail.8 Wed Nov 18 21:07:08 2020 (r367819) > > @@ -25,7 +25,7 @@ > > .\" > > .\" $FreeBSD$ > > .\" > > -.Dd May 14, 2020 > > +.Dd November 18, 2020 > > .Dt JAIL 8 > > .Os > > .Sh NAME > > @@ -587,6 +587,13 @@ and resource limits. > > The jail root may bind to ports lower than 1024. > > .It Va allow.unprivileged_proc_debug > > Unprivileged processes in the jail may use debugging facilities. > > +.It Va allow.suser > > +The value of the jail's > > +.Va security.bsd.suser_enabled > > +sysctl. > > +The super-user will be disabled automatically if its parent system has it > > +disabled. > > +The super-user is enabled by default. > > .El > > .El > > .Pp > > @@ -1267,6 +1274,7 @@ Changes to these variables by a jailed process do not > > > > environment, only the jail environment. > > These variables are > > .Va kern.securelevel , > > +.Va security.bsd.suser_enabled , > > .Va kern.hostname , > > .Va kern.domainname , > > .Va kern.hostid , > > > > > -- > Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGOYWV_p1WT0ijCS3Xk83DjA1%2Bfqa0oajjv-2EVR2_iF8GL0Yw>