Message-ID: <CALmYWFuVJiRZgB0ye9eR95dvBOigoOVShgS9i_ESjEre-H5pLA@mail.gmail.com>
Date: Mon, 8 Jul 2024 14:15:44 -0700
From: Jeff Xu <jeffxu@...gle.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Al Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, 
	Kees Cook <keescook@...omium.org>, Linus Torvalds <torvalds@...ux-foundation.org>, 
	Paul Moore <paul@...l-moore.com>, "Theodore Ts'o" <tytso@....edu>, Alejandro Colomar <alx@...nel.org>, 
	Aleksa Sarai <cyphar@...har.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Andy Lutomirski <luto@...nel.org>, Arnd Bergmann <arnd@...db.de>, 
	Casey Schaufler <casey@...aufler-ca.com>, Christian Heimes <christian@...hon.org>, 
	Dmitry Vyukov <dvyukov@...gle.com>, Eric Biggers <ebiggers@...nel.org>, 
	Eric Chiang <ericchiang@...gle.com>, Fan Wu <wufan@...ux.microsoft.com>, 
	Florian Weimer <fweimer@...hat.com>, Geert Uytterhoeven <geert@...ux-m68k.org>, 
	James Morris <jamorris@...ux.microsoft.com>, Jan Kara <jack@...e.cz>, 
	Jann Horn <jannh@...gle.com>, Jonathan Corbet <corbet@....net>, 
	Jordan R Abrahams <ajordanr@...gle.com>, Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>, 
	Luca Boccassi <bluca@...ian.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	"Madhavan T . Venkataraman" <madvenka@...ux.microsoft.com>, Matt Bobrowski <mattbobrowski@...gle.com>, 
	Matthew Garrett <mjg59@...f.ucam.org>, Matthew Wilcox <willy@...radead.org>, 
	Miklos Szeredi <mszeredi@...hat.com>, Mimi Zohar <zohar@...ux.ibm.com>, 
	Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>, Scott Shell <scottsh@...rosoft.com>, 
	Shuah Khan <shuah@...nel.org>, Stephen Rothwell <sfr@...b.auug.org.au>, 
	Steve Dower <steve.dower@...hon.org>, Steve Grubb <sgrubb@...hat.com>, 
	Thibaut Sautereau <thibaut.sautereau@....gouv.fr>, 
	Vincent Strubel <vincent.strubel@....gouv.fr>, Xiaoming Ni <nixiaoming@...wei.com>, 
	Yin Fengwei <fengwei.yin@...el.com>, kernel-hardening@...ts.openwall.com, 
	linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH v19 2/5] security: Add new SHOULD_EXEC_CHECK and
 SHOULD_EXEC_RESTRICT securebits

On Mon, Jul 8, 2024 at 11:48 AM Mickaël Salaün <mic@...ikod.net> wrote:
>
> On Mon, Jul 08, 2024 at 10:53:11AM -0700, Jeff Xu wrote:
> > On Mon, Jul 8, 2024 at 9:17 AM Jeff Xu <jeffxu@...gle.com> wrote:
> > >
> > > Hi
> > >
> > > On Thu, Jul 4, 2024 at 12:02 PM Mickaël Salaün <mic@...ikod.net> wrote:
> > > >
> > > > These new SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT, and
> > > > their *_LOCKED counterparts are designed to be set by processes setting
> > > > up an execution environment, such as a user session, a container, or a
> > > > security sandbox.  Like seccomp filters or Landlock domains, the
> > > > securebits are inherited across proceses.
> > > >
> > > > When SECBIT_SHOULD_EXEC_CHECK is set, programs interpreting code should
> > > > check executable resources with execveat(2) + AT_CHECK (see previous
> > > > patch).
> > > >
> > > > When SECBIT_SHOULD_EXEC_RESTRICT is set, a process should only allow
> > > > execution of approved resources, if any (see SECBIT_SHOULD_EXEC_CHECK).
> > > >
> > > Do we need both bits ?
> > > When CHECK is set and RESTRICT is not, the "check fail" executable
> > > will still get executed, so CHECK is for logging ?
> > > Does RESTRICT imply CHECK is set, e.g. What if CHECK=0 and RESTRICT = 1 ?
> > >
> > The intention might be "permissive mode"?  if so, consider reuse
> > existing selinux's concept, and still with 2 bits:
> > SECBIT_SHOULD_EXEC_RESTRICT
> > SECBIT_SHOULD_EXEC_RESTRICT_PERMISSIVE
>
> SECBIT_SHOULD_EXEC_CHECK is for user space to check with execveat+AT_CHECK.
>
> SECBIT_SHOULD_EXEC_RESTRICT is for user space to restrict execution by
> default, and potentially allow some exceptions from the list of
> checked-and-allowed files, if SECBIT_SHOULD_EXEC_CHECK is set.
>
> Without SECBIT_SHOULD_EXEC_CHECK, SECBIT_SHOULD_EXEC_RESTRICT is to deny
> any kind of execution/interpretation.
>
Do you mean "deny any kinds of executable/interpretation" or just
those that failed with "AT_CHECK"  ( I assume this)?

> With only SECBIT_SHOULD_EXEC_CHECK, user space should just check and log
> any denied access, but ignore them.  So yes, it is similar to the
> SELinux's permissive mode.
>
IIUC:
CHECK=0, RESTRICT=0: do nothing, current behavior
CHECK=1, RESTRICT=0: permissive mode - ignore AT_CHECK results.
CHECK=0, RESTRICT=1: call AT_CHECK, deny if AT_CHECK failed, no exception.
CHECK=1, RESTRICT=1: call AT_CHECK, deny if AT_CHECK failed, except
those in the "checked-and-allowed" list.

So CHECK is basically trying to form a allowlist?
If there is a need for a allowlist, that is the task of "interruptor
or dynamic linker" to maintain this list, and the list is known in
advance, i.e. not something from execveat(AT_CHECK), and kernel
shouldn't have the knowledge of this allowlist.
Secondly, the concept of allow-list  seems to be an attack factor for
me, I would rather it be fully enforced, or permissive mode.
And Check=1 and RESTRICT=1 is less secure than CHECK=0, RESTRICT=1,
this might also be not obvious to dev.

Unless I understood the CHECK wrong.

> This is explained in the next patch as comments.
>
The next patch is a selftest patch, it is better to define them in the
current commit and in the securebits.h.

> The *_LOCKED variants are useful and part of the securebits concept.
>
The locked state is easy to understand.

Thanks
Best regards
-Jeff

> >
> >
> > -Jeff
> >
> >
> >
> >
> > > > For a secure environment, we might also want
> > > > SECBIT_SHOULD_EXEC_CHECK_LOCKED and SECBIT_SHOULD_EXEC_RESTRICT_LOCKED
> > > > to be set.  For a test environment (e.g. testing on a fleet to identify
> > > > potential issues), only the SECBIT_SHOULD_EXEC_CHECK* bits can be set to
> > > > still be able to identify potential issues (e.g. with interpreters logs
> > > > or LSMs audit entries).
> > > >
> > > > It should be noted that unlike other security bits, the
> > > > SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT bits are
> > > > dedicated to user space willing to restrict itself.  Because of that,
> > > > they only make sense in the context of a trusted environment (e.g.
> > > > sandbox, container, user session, full system) where the process
> > > > changing its behavior (according to these bits) and all its parent
> > > > processes are trusted.  Otherwise, any parent process could just execute
> > > > its own malicious code (interpreting a script or not), or even enforce a
> > > > seccomp filter to mask these bits.
> > > >
> > > > Such a secure environment can be achieved with an appropriate access
> > > > control policy (e.g. mount's noexec option, file access rights, LSM
> > > > configuration) and an enlighten ld.so checking that libraries are
> > > > allowed for execution e.g., to protect against illegitimate use of
> > > > LD_PRELOAD.
> > > >
> > > > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > > > environment variables), but that is outside the scope of the kernel.
> > > >
> > > > The only restriction enforced by the kernel is the right to ptrace
> > > > another process.  Processes are denied to ptrace less restricted ones,
> > > > unless the tracer has CAP_SYS_PTRACE.  This is mainly a safeguard to
> > > > avoid trivial privilege escalations e.g., by a debugging process being
> > > > abused with a confused deputy attack.
> > > >
> > > > Cc: Al Viro <viro@...iv.linux.org.uk>
> > > > Cc: Christian Brauner <brauner@...nel.org>
> > > > Cc: Kees Cook <keescook@...omium.org>
> > > > Cc: Paul Moore <paul@...l-moore.com>
> > > > Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> > > > Link: https://lore.kernel.org/r/20240704190137.696169-3-mic@digikod.net
> > > > ---

Powered by blists - more mailing lists