[PATCH] Do not require attributes for security_inode_init_security.

[Dr. Greg]

On Tue, Mar 26, 2024 at 03:12:37PM -0400, Paul Moore wrote:

Good morning to everyone.

> On Tue, Mar 26, 2024 at 6:31???AM Dr. Greg <greg at enjellic.com> wrote:
> > On Mon, Mar 25, 2024 at 05:08:54PM -0400, Paul Moore wrote:
> > > On Sun, Mar 24, 2024 at 6:33???PM Greg Wettstein <greg at enjellic.com> wrote:
> > > >
> > > > The integration of the Integrity Measurement Architecture (IMA)
> > > > into the LSM infrastructure introduced a conditional check that
> > > > denies access to the security_inode_init_security() event handler
> > > > if the LSM extended attribute 'blob' size is 0.
> > > >
> > > > This changes the previous behavior of this event handler and
> > > > results in variable behavior of LSM's depending on the LSM boot
> > > > configuration.
> > > >
> > > > Modify the function so that it removes the need for a non-zero
> > > > extended attribute blob size and bypasses the memory allocation
> > > > and freeing that is not needed if the LSM infrastructure is not
> > > > using extended attributes.
> > > >
> > > > Use a break statement to exit the loop that is iterating over the
> > > > defined handlers for this event if a halting error condition is
> > > > generated by one of the invoked LSM handlers.  The checks for how
> > > > to handle cleanup are executed at the end of the loop regardless
> > > > of how the loop terminates.
> > > >
> > > > A two exit label strategy is implemented.  One of the exit
> > > > labels is a target for the no attribute case while the second is
> > > > the target for the case where memory allocated for processing of
> > > > extended attributes needs to be freed.
> > > >
> > > > Signed-off-by: Greg Wettstein <greg at enjellic.com>
> > > > ---
> > > >  security/security.c | 24 ++++++++++++------------
> > > >  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> ...
> 
> > > Looking at this quickly, why does something like the following not work?
> > >
> > > [Warning: copy-n-paste patch, likely whitespace damaged]
> > >
> > > diff --git a/security/security.c b/security/security.c
> > > index 7e118858b545..007ce438e636 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1712,10 +1712,7 @@ int security_inode_init_security(struct inode *inode, str
> > > uct inode *dir,
> > >        if (unlikely(IS_PRIVATE(inode)))
> > >                return 0;
> > >
> > > -       if (!blob_sizes.lbs_xattr_count)
> > > -               return 0;
> > > -
> > > -       if (initxattrs) {
> > > +       if (initxattrs && blob_sizes.lbs_xattr_count) {
> > >                /* Allocate +1 as terminator. */
> > >                new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1,
> > >                                     sizeof(*new_xattrs), GFP_NOFS);
> >
> > We ran with something similar to the above for several days of TSEMv3
> > testing.
> >
> > For the patch that we submitted upstream, we elected to take a 'belt
> > and suspenders' approach that isolated the 'no attributes' execution
> > flow from the flow followed if extended attributes are present.
> >
> > The approach used doesn't make any difference to us as long as we get
> > the functionality of the hook restored.

> I'd prefer the simpler approach.  I'd likely also prefer we park
> this patch until it is needed upstream, or am I misunderstanding
> things and upstream is currently broken without a fix like this?

As of the 6.8 release, a security handler that previously functioned
in a consistent manner now functions inconsistently depending on the
LSM stacking configuration that is in effect.

Perhaps more problematically, when the handler does not function
correctly, there is no indication of that fact passed upward to the
LSM invoking the handler.  This would cause the LSM to conclude that a
security relevant action was conducted when it did not actually occur.

I believe we would all universally conclude that having security
critical infrastructure function in a consistent and reliable manner
is of benefit, so we should return the previous behavior of the
handler, particularly since it can be done with a one line fix if that
is your preference.

If you would be so kind, please put a 'Reported-by:' tag on whatever
you commit upstream.

> > If you go with the simpler approach, it may be worthwhile to at least
> > simplify the handling of the call to the initxattr() function after
> > the evm_inode_init_security() call.
> 
> Starting with v6.9-rc1 there is no longer an explicit call to
> evm_inode_init_security() as it is incorporated into the normal LSM
> hook processing, e.g. `hp->hook.inode_init_security(...)`.  I'm also
> not sure we need to worry about the initxattrs() call near the bottom
> of security_inode_init_security() since in the no
> @blob.lbs_xattr_count case the @xattr_count variable will also be zero
> so the initxattrs() call will be skipped.
> 
> Or were you talking about something else?

We were discussing something else but it isn't as important as getting
the security handler fixed, so lets just proceed with that and we can
call it a day.

> paul-moore.com

Have a good day.

As always,
Dr. Greg

   The Quixote Project - Flailing at the Travails of Cybersecurity
		  https://github.com/Quixote-Project