Credentials not fully initialized before bprm_check LSM hook

Credentials not fully initialized before bprm_check LSM hook

[Roberto Sassu]

Hi everyone

recently I discovered a problem in the implementation of our IMA
bprm_check hook, in particular when the policy is matched against the
bprm credentials (to be committed later during execve().

Before commit 56305aa9b6fab ("exec: Compute file based creds only
once"), bprm_fill_uid() was called in prepare_binprm() and filled the
euid/egid before calling security_bprm_check(), which in turns calls
IMA.

After that commit, bprm_fill_uid() was moved to begin_new_exec(), which
is when the last interpreter is found.

The consequence is that IMA still sees the not yet ready credentials
and an IMA rule like:

measure func=CREDS_CHECK euid=0

will not be matched for sudo-like applications.

It does work however with SELinux, because it computes the transition
before IMA in the bprm_creds_for_exec hook.

Since IMA needs to be involved for each execution in the chain of
interpreters, we cannot move to the bprm_creds_from_file hook.

How do we solve this problem? The commit mentioned that it is an
optimization, so probably would not be too hard to partially revert it
(and keeping what is good).

Thanks

Roberto

Re: Credentials not fully initialized before bprm_check LSM hook

[sergeh]

On Thu, Apr 10, 2025 at 01:47:07PM +0200, Roberto Sassu wrote:
> Hi everyone
> 
> recently I discovered a problem in the implementation of our IMA
> bprm_check hook, in particular when the policy is matched against the
> bprm credentials (to be committed later during execve().
> 
> Before commit 56305aa9b6fab ("exec: Compute file based creds only
> once"), bprm_fill_uid() was called in prepare_binprm() and filled the
> euid/egid before calling security_bprm_check(), which in turns calls
> IMA.
> 
> After that commit, bprm_fill_uid() was moved to begin_new_exec(), which
> is when the last interpreter is found.
> 
> The consequence is that IMA still sees the not yet ready credentials
> and an IMA rule like:
> 
> measure func=CREDS_CHECK euid=0

"IMA still sees" at which point exactly?

Do I understand right that the problem is that ima's version of
security_bprm_creds_for_exec() needs to run after
bprm_creds_from_file()?

Given that Eric's commit message said that no bprm handlers use
the uid, it seems it should be safe to just move that?

> will not be matched for sudo-like applications.
> 
> It does work however with SELinux, because it computes the transition
> before IMA in the bprm_creds_for_exec hook.
> 
> Since IMA needs to be involved for each execution in the chain of
> interpreters, we cannot move to the bprm_creds_from_file hook.
> 
> How do we solve this problem? The commit mentioned that it is an
> optimization, so probably would not be too hard to partially revert it
> (and keeping what is good).
> 
> Thanks
> 
> Roberto
> 

Re: Credentials not fully initialized before bprm_check LSM hook

[Roberto Sassu]

On Thu, 2025-04-10 at 17:24 +0000, sergeh@kernel.org wrote:
> On Thu, Apr 10, 2025 at 01:47:07PM +0200, Roberto Sassu wrote:
> > Hi everyone
> > 
> > recently I discovered a problem in the implementation of our IMA
> > bprm_check hook, in particular when the policy is matched against the
> > bprm credentials (to be committed later during execve().
> > 
> > Before commit 56305aa9b6fab ("exec: Compute file based creds only
> > once"), bprm_fill_uid() was called in prepare_binprm() and filled the
> > euid/egid before calling security_bprm_check(), which in turns calls
> > IMA.
> > 
> > After that commit, bprm_fill_uid() was moved to begin_new_exec(), which
> > is when the last interpreter is found.
> > 
> > The consequence is that IMA still sees the not yet ready credentials
> > and an IMA rule like:
> > 
> > measure func=CREDS_CHECK euid=0
> 
> "IMA still sees" at which point exactly?

IMA sees the credentials in bprm->cred prepared with
prepare_bprm_creds(), where the euid/egid are taken from the current
process.

> Do I understand right that the problem is that ima's version of
> security_bprm_creds_for_exec() needs to run after
> bprm_creds_from_file()?

IMA's version of security_bprm_check(). security_bprm_creds_for_exec()
is for checking scripts executed by the interpreters with execveat()
and the AT_EXECVE_CHECK flag.

Uhm, it would not be technically a problem to move the IMA hook later,
but it would miss the intermediate binary search steps, which are
visible with security_bprm_check().

> Given that Eric's commit message said that no bprm handlers use
> the uid, it seems it should be safe to just move that?

Well, we just found one :)

Thanks

Roberto

> > will not be matched for sudo-like applications.
> > 
> > It does work however with SELinux, because it computes the transition
> > before IMA in the bprm_creds_for_exec hook.
> > 
> > Since IMA needs to be involved for each execution in the chain of
> > interpreters, we cannot move to the bprm_creds_from_file hook.
> > 
> > How do we solve this problem? The commit mentioned that it is an
> > optimization, so probably would not be too hard to partially revert it
> > (and keeping what is good).
> > 
> > Thanks
> > 
> > Roberto
> > 

Re: Credentials not fully initialized before bprm_check LSM hook

[Paul Moore]

On Fri, Apr 11, 2025 at 5:07 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Thu, 2025-04-10 at 17:24 +0000, sergeh@kernel.org wrote:
> > On Thu, Apr 10, 2025 at 01:47:07PM +0200, Roberto Sassu wrote:
> > > Hi everyone
> > >
> > > recently I discovered a problem in the implementation of our IMA
> > > bprm_check hook, in particular when the policy is matched against the
> > > bprm credentials (to be committed later during execve().
> > >
> > > Before commit 56305aa9b6fab ("exec: Compute file based creds only
> > > once"), bprm_fill_uid() was called in prepare_binprm() and filled the
> > > euid/egid before calling security_bprm_check(), which in turns calls
> > > IMA.
> > >
> > > After that commit, bprm_fill_uid() was moved to begin_new_exec(), which
> > > is when the last interpreter is found.
> > >
> > > The consequence is that IMA still sees the not yet ready credentials
> > > and an IMA rule like:
> > >
> > > measure func=CREDS_CHECK euid=0
> >
> > "IMA still sees" at which point exactly?
>
> IMA sees the credentials in bprm->cred prepared with
> prepare_bprm_creds(), where the euid/egid are taken from the current
> process.
>
> > Do I understand right that the problem is that ima's version of
> > security_bprm_creds_for_exec() needs to run after
> > bprm_creds_from_file()?
>
> IMA's version of security_bprm_check(). security_bprm_creds_for_exec()
> is for checking scripts executed by the interpreters with execveat()
> and the AT_EXECVE_CHECK flag.
>
> Uhm, it would not be technically a problem to move the IMA hook later,
> but it would miss the intermediate binary search steps, which are
> visible with security_bprm_check().

I'm still trying to make sure I understand everything here, so I've
got a few questions:

* How important is it for IMA to vet the intermediate binaries?  Those
binaries don't actually do anything with the program/scripts, right?

* Based on the comment block at the top of begin_new_exec(), I'm
assuming that using the security_bprm_creds_from_file() hook would be
a problem due to challenges in returning an error code?  There might
also be an issue for any LSMs that run *before* capabilities, but I
think that would only be Lockdown in the default case so likely not a
big problem.

* This patch has been out for almost five years and presumably offers
a performance bump when doing an exec; I'm skeptical that Eric, Linus,
or anyone outside of security/ would be interested in doing a revert
to better support the AT_EXECVE_CHECK for a LSM.  Yes, I might be
wrong, but for a moment let's assume a revert is not an option, what
would you propose to solve this?  If you can't think of a general
solution, can you think of an IMA specific solution?

-- 
paul-moore.com