On Tue, Jul 23, 2024 at 6:15 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
>
> On Fri, Jul 19, 2024 at 08:27:18AM -0700, Jeff Xu wrote:
> > On Fri, Jul 19, 2024 at 8:04 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > >
> > > On Fri, Jul 19, 2024 at 07:16:55AM -0700, Jeff Xu wrote:
> > > > On Fri, Jul 19, 2024 at 1:45 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > > > >
> > > > > On Thu, Jul 18, 2024 at 06:29:54PM -0700, Jeff Xu wrote:
> > > > > > On Thu, Jul 18, 2024 at 5:24 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Wed, Jul 17, 2024 at 07:08:17PM -0700, Jeff Xu wrote:
> > > > > > > > On Wed, Jul 17, 2024 at 3:01 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Jul 16, 2024 at 11:33:55PM -0700, Jeff Xu wrote:
> > > > > > > > > > On Thu, Jul 4, 2024 at 12:02 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> > > > > > > > > > > allowed for execution. The main use case is for script interpreters and
> > > > > > > > > > > dynamic linkers to check execution permission according to the kernel's
> > > > > > > > > > > security policy. Another use case is to add context to access logs e.g.,
> > > > > > > > > > > which script (instead of interpreter) accessed a file. As any
> > > > > > > > > > > executable code, scripts could also use this check [1].
> > > > > > > > > > >
> > > > > > > > > > > This is different than faccessat(2) which only checks file access
> > > > > > > > > > > rights, but not the full context e.g. mount point's noexec, stack limit,
> > > > > > > > > > > and all potential LSM extra checks (e.g. argv, envp, credentials).
> > > > > > > > > > > Since the use of AT_CHECK follows the exact kernel semantic as for a
> > > > > > > > > > > real execution, user space gets the same error codes.
> > > > > > > > > > >
> > > > > > > > > > So we concluded that execveat(AT_CHECK) will be used to check the
> > > > > > > > > > exec, shared object, script and config file (such as seccomp config),
> > > > >
> > > > > > > > > > I think binfmt_elf.c in the kernel needs to check the ld.so to make
> > > > > > > > > > sure it passes AT_CHECK, before loading it into memory.
> > > > > > > > >
> > > > > > > > > All ELF dependencies are opened and checked with open_exec(), which
> > > > > > > > > perform the main executability checks (with the __FMODE_EXEC flag).
> > > > > > > > > Did I miss something?
> > > > > > > > >
> > > > > > > > I mean the ld-linux-x86-64.so.2 which is loaded by binfmt in the kernel.
> > > > > > > > The app can choose its own dynamic linker path during build, (maybe
> > > > > > > > even statically link one ?) This is another reason that relying on a
> > > > > > > > userspace only is not enough.
> > > > > > >
> > > > > > > The kernel calls open_exec() on all dependencies, including
> > > > > > > ld-linux-x86-64.so.2, so these files are checked for executability too.
> > > > > > >
> > > > > > This might not be entirely true. iiuc, kernel calls open_exec for
> > > > > > open_exec for interpreter, but not all its dependency (e.g. libc.so.6)
> > > > >
> > > > > Correct, the dynamic linker is in charge of that, which is why it must
> > > > > be enlighten with execveat+AT_CHECK and securebits checks.
> > > > >
> > > > > > load_elf_binary() {
> > > > > > interpreter = open_exec(elf_interpreter);
> > > > > > }
> > > > > >
> > > > > > libc.so.6 is opened and mapped by dynamic linker.
> > > > > > so the call sequence is:
> > > > > > execve(a.out)
> > > > > > - open exec(a.out)
> > > > > > - security_bprm_creds(a.out)
> > > > > > - open the exec(ld.so)
> > > > > > - call open_exec() for interruptor (ld.so)
> > > > > > - call execveat(AT_CHECK, ld.so) <-- do we want ld.so going through
> > > > > > the same check and code path as libc.so below ?
> > > > >
> > > > > open_exec() checks are enough. LSMs can use this information (open +
> > > > > __FMODE_EXEC) if needed. execveat+AT_CHECK is only a user space
> > > > > request.
> > > > >
> > > > Then the ld.so doesn't go through the same security_bprm_creds() check
> > > > as other .so.
> > >
> > > Indeed, but...
> > >
> > My point is: we will want all the .so going through the same code
> > path, so security_ functions are called consistently across all the
> > objects, And in the future, if we want to develop additional LSM
> > functionality based on AT_CHECK, it will be applied to all objects.
>
> I'll extend the doc to encourage LSMs to check for __FMODE_EXEC, which
> already is the common security check for all executable dependencies.
> As extra information, they can get explicit requests by looking at
> execveat+AT_CHECK call.
>
I agree that security_file_open + __FMODE_EXEC for checking all
the .so (e.g for executable memfd) is a better option than checking at
security_bprm_creds_for_exec.
But then maybe execveat( AT_CHECK) can return after calling alloc_bprm ?
See below call graph:
do_execveat_common (AT_CHECK)
-> alloc_bprm
->->do_open_execat
->->-> do_filp_open (__FMODE_EXEC)
->->->->->->> security_file_open
-> bprm_execve
->-> prepare_exec_creds
->->-> prepare_creds
->->->-> security_prepare_creds
->-> security_bprm_creds_for_exec
What is the consideration to mark the end at
security_bprm_creds_for_exec ? i.e. including brpm_execve,
prepare_creds, security_prepare_creds, security_bprm_creds_for_exec.
Since dynamic linker doesn't load ld.so (it is by kernel), ld.so
won't go through those security_prepare_creds and
security_bprm_creds_for_exec checks like other .so do.
> >
> > Another thing to consider is: we are asking userspace to make
> > additional syscall before loading the file into memory/get executed,
> > there is a possibility for future expansion of the mechanism, without
> > asking user space to add another syscall again.
>
> AT_CHECK is defined with a specific semantic. Other mechanisms (e.g.
> LSM policies) could enforce other restrictions following the same
> semantic. We need to keep in mind backward compatibility.
>
> >
> > I m still not convinced yet that execveat(AT_CHECK) fits more than
> > faccessat(AT_CHECK)
>
> faccessat2(2) is dedicated to file permission/attribute check.
> execveat(2) is dedicated to execution, which is a superset of file
> permission for executability, plus other checks (e.g. noexec).
>
That sounds reasonable, but if execveat(AT_CHECK) changes behavior of
execveat(), someone might argue that faccessat2(EXEC_CHECK) can be
made for the executability.
I think the decision might depend on what this PATCH intended to
check, i.e. where we draw the line.
do_open_execat() seems to cover lots of checks for executability, if
we are ok with the thing that do_open_execat() checks, then
faccessat(AT_CHECK) calling do_open_execat() is an option, it won't
have those "unrelated" calls in execve path, e.g. bprm_stack_limits,
copy argc/env .
However, you mentioned superset of file permission for executability,
can you elaborate on that ? Is there something not included in
do_open_execat() but still necessary for execveat(AT_CHECK)? maybe
security_bprm_creds_for_exec? (this goes back to my question above)
Thanks
Best regards,
-Jeff
> >
> >
> > > >
> > > > As my previous email, the ChromeOS LSM restricts executable mfd
> > > > through security_bprm_creds(), the end result is that ld.so can still
> > > > be executable memfd, but not other .so.
> > >
> > > The chromeOS LSM can check that with the security_file_open() hook and
> > > the __FMODE_EXEC flag, see Landlock's implementation. I think this
> > > should be the only hook implementation that chromeOS LSM needs to add.
> > >
> > > >
> > > > One way to address this is to refactor the necessary code from
> > > > execveat() code patch, and make it available to call from both kernel
> > > > and execveat() code paths., but if we do that, we might as well use
> > > > faccessat2(AT_CHECK)
> > >
> > > That's why I think it makes sense to rely on the existing __FMODE_EXEC
> > > information.
> > >
> > > >
> > > >
> > > > > > - transfer the control to ld.so)
> > > > > > - ld.so open (libc.so)
> > > > > > - ld.so call execveat(AT_CHECK,libc.so) <-- proposed by this patch,
> > > > > > require dynamic linker change.
> > > > > > - ld.so mmap(libc.so,rx)
> > > > >
> > > > > Explaining these steps is useful. I'll include that in the next patch
> > > > > series.
> >
