Re: [PATCH v4 04/14] Add primary TSEM implementation file.

[Casey Schaufler <casey@schaufler-ca.com>]

On 8/27/2024 3:52 AM, Dr. Greg wrote:
> On Mon, Aug 26, 2024 at 08:53:31AM -0700, Casey Schaufler wrote:
>
> Good morning Casey, I hope this note finds your day starting well.
>
> Greetings to others on this 'last' week of summer.
>
>> On 8/26/2024 3:37 AM, Greg Wettstein wrote:
>>> The tsem.c file is the 'master' file in the TSEM implementation. It is
>>> responsible for initializing the LSM and providing the implementation of the
>>> security event handlers.
>>> ---
>>>  security/tsem/tsem.c | 2446 ++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 2446 insertions(+)
>>>  create mode 100644 security/tsem/tsem.c
>>>
>>> diff --git a/security/tsem/tsem.c b/security/tsem/tsem.c
>>> new file mode 100644
>>> index 000000000000..76d65b3e62b3
>>> --- /dev/null
>>> +++ b/security/tsem/tsem.c
>>> @@ -0,0 +1,2446 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +/*
>>> + * Copyright (C) 2024 Enjellic Systems Development, LLC
>>> + * Author: Dr. Greg Wettstein <greg@enjellic.com>
>>> + *
>>> + * This file is the primary implementation file for the tsem LSM.
>>> + *
>>> + * It implements initialization and setup functions that interpret
>>> + * kernel command-line arguments and prepares TSEM for operation.
>>> + *
>>> + * In addition it contains all of the TSEM specific security event
>>> + * handlers that are responsible for handling the LSM events that TSEM
>>> + * models.
>>> + *
>>> + * Each TSEM event handler calls the tsem_allocate_event() function to
>>> + * allocate a structure that will be used to describe the event.  The
>>> + * CELL union of this structure contains various structures that are
>>> + * used to hold these parameters.
>>> + *
>>> + * Since the event characterization parameters need to be retained for
>>> + * the lifetime of the tsem_event structure that is allocated.  In the
>>> + * case of internally modeled namespaces this lifespan is the lifetime
>>> + * of the security modeling namespace.  In the case of externally
>>> + * modeled namespaces, the lifespan is until the security event
>>> + * description is exported to an external trust orchestrator.
>>> + *
>>> + * In order to support this model, the event description structures
>>> + * are typically composed of a union over 'in' and 'out' structures.
>>> + * The 'in' structures are used to hold arguments to the event handler
>>> + * that may only be relevant for the duration of the call.  These
>>> + * values are translated into members of the 'out' structure that
>>> + * retain the values until the end of the lifetime of the tsem_event
>>> + * structure.
>>> + *
>>> + * Each TSEM event handler is responsible for allocating a tsem_event
>>> + * structure and populating the appropriate CELL structure with the
>>> + * input characteristics of the event.  The dispatch_event() function
>>> + * is called to handle the modeling of the event.  This function
>>> + * returns the permission value that is returned as the result of the
>>> + * LSM event handler.
>>> + *
>>> + * The dispatch_event() calls the tsem_event_init() function that is
>>> + * responsible for translating the input parameters into values that
>>> + * will be retained for the lifetime of the security event
>>> + * description.  The populated event description is then dispatched to
>>> + * either the tsem_model_event() or the tsem_export_event() for
>>> + * modeling by either the internal TMA or by a TMA associated with an
>>> + * external trust orchestrator.
>>> + */
>>> +
>>> + ...
>>> +
>>> +static int tsem_file_open(struct file *file)
>>> +{
>>> +	struct inode *inode = file_inode(file);
>>> +	struct tsem_event *ep;
>>> +
>>> +	if (static_branch_unlikely(&tsem_not_ready))
>>> +		return 0;
>>> +	if (bypass_event(TSEM_FILE_OPEN))
>>> +		return 0;
>>> +	if (unlikely(tsem_inode(inode)->status == TSEM_INODE_CONTROL_PLANE)) {
>>> +		if (capable(CAP_MAC_ADMIN))
>> Don't you mean CAP_MAC_OVERRIDE? CAP_MAC_ADMIN is for changes to the
>> security state of the system, where CAP_MAC_OVERRIDE is for access
>> control decision exceptions. Here (and elsewhere) you use the former
>> in access checks.
> You are clearly the mechanistic expert on capabilities so we would
> take your lead on this.
>
> Some background information to hopefully assist in a discussion on the
> types of capability checks that should be implemented.
>
> The capability checks we apply in TSEM gate the following five types
> of actions:
>
> 1.) The ability to issue TSEM control commands.
>
> 2.) The ability to register an event processing module.
>
> 3.) Access to state information on kernel based modeling agent instances.
>
> 4.) The ability to send signals to trust orchestration processes.
>
> 5.) The ability to send a signal to a different security modeling namespace.
>
> If we understand the differentiation that you suggest between
> CAP_MAC_ADMIN and CAP_MAC_OVERRIDE we would conclude the following:
>
> Checks 1, 2 and 4 would seem, in our opinion, have the ability to
> change the security state of a system.  As such it would seem
> appropriate to use CAP_MAC_ADMIN for those checks.
>
> Rather than belabor the issue now, we can entertain a subsequent
> discussion, if needed, on why we believe that actions 1, 2 and 4 can
> change the security state of the system.
>
> By your definition, check type 3 would seem to be consistent with
> CAP_MAC_OVERRIDE, since it is gating access to potentially security
> sensitive information but which does not imply the ability to change
> the security state of the system.
>
> That leaves category 5 as a possible open question.  Given the trust
> orchestration model for externally modeled namespaces, we concluded
> that the only entities that should be able to issue signals that can
> manipulate, particularly terminate a process, should only come from
> within the security modeling namespace that the target process is
> running in.  Given that, we would consider such operations as possibly
> affecting the security state of the system and thus suitable for
> CAP_MAC_ADMIN.
>
> Based on what we have always understood, and that is confirmed by 'git
> grep', the only thing at this time that is using CAP_MAC_OVERRIDE is
> SMACK.  If our analysis is correct, would you have any issues with us
> changing the type 3 checks to CAP_MAC_OVERRIDE?
>
> With respect to the check that you call out in
> tsem.c:tsem_open_file(), the capability check is to avoid a model
> deadlock situation.  If we adopt the model we discuss above, we would
> need to unequivocably allow the open if the process is carrying
> CAP_MAC_ADMIN or CAP_MAC_OVERRIDE in order to avoid a control
> deadlock.
>
> We will look forward to your thoughts on if we should proceed with the
> above changes.

It seems you were right all along, that none of these capability checks
really fit with CAP_MAC_OVERRIDE. Thank you for the clarification. 

>
> Have a good day.
>
> As always,
> Dr. Greg
>
> The Quixote Project - Flailing at the Travails of Cybersecurity
>               https://github.com/Quixote-Project