Re: [Non-DoD Source] Re: [RFC PATCH v2] security, capability: pass object information to security_capable

[Aaron Goidel <acgoide@tycho.nsa.gov>]

On 8/14/19 5:27 PM, Paul Moore wrote:
> On Wed, Aug 14, 2019 at 5:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 8/14/19 3:59 PM, Paul Moore wrote:
>>> On Tue, Aug 13, 2019 at 5:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>>>> On 2019-08-13 11:01, Aaron Goidel wrote:
>>>>> On 8/8/19 12:30 PM, Paul Moore wrote:
>>>>>> On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>>>>>>> From: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>>>>>>
>>>>>>> At present security_capable does not pass any object information
>>>>>>> and therefore can neither audit the particular object nor take it
>>>>>>> into account. Augment the security_capable interface to support
>>>>>>> passing supplementary data. Use this facility initially to convey
>>>>>>> the inode for capability checks relevant to inodes. This only
>>>>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
>>>>>>> relevant to inodes will be addressed in subsequent changes. In the
>>>>>>> future, this will be further extended to pass object information for
>>>>>>> other capability checks such as the target task for CAP_KILL.
>>>>>>>
>>>>>>> In SELinux this new information is leveraged here to include the inode
>>>>>>> in the audit message. In the future, it could also be used to perform
>>>>>>> a per inode capability checks.
>>>>>>>
>>>>>>> It would be possible to fold the existing opts argument into this new
>>>>>>> supplementary data structure. This was omitted from this change to
>>>>>>> minimize changes.
>>>>>>>
>>>>>>> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>>>>>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> - Changed order of audit prints so optional information comes second
>>>>>>> ---
>>>>>>>     include/linux/capability.h             |  7 ++++++
>>>>>>>     include/linux/lsm_audit.h              |  5 +++-
>>>>>>>     include/linux/lsm_hooks.h              |  3 ++-
>>>>>>>     include/linux/security.h               | 23 +++++++++++++-----
>>>>>>>     kernel/capability.c                    | 33 ++++++++++++++++++--------
>>>>>>>     kernel/seccomp.c                       |  2 +-
>>>>>>>     security/apparmor/capability.c         |  8 ++++---
>>>>>>>     security/apparmor/include/capability.h |  4 +++-
>>>>>>>     security/apparmor/ipc.c                |  2 +-
>>>>>>>     security/apparmor/lsm.c                |  5 ++--
>>>>>>>     security/apparmor/resource.c           |  2 +-
>>>>>>>     security/commoncap.c                   | 11 +++++----
>>>>>>>     security/lsm_audit.c                   | 21 ++++++++++++++--
>>>>>>>     security/safesetid/lsm.c               |  3 ++-
>>>>>>>     security/security.c                    |  5 ++--
>>>>>>>     security/selinux/hooks.c               | 20 +++++++++-------
>>>>>>>     security/smack/smack_access.c          |  2 +-
>>>>>>>     17 files changed, 110 insertions(+), 46 deletions(-)
>>>>>>
>>>>>> You should CC the linux-audit list, I've added them on this mail.
>>>>>>
>>>>>> I had hoped to see some thought put into the idea of dynamically
>>>>>> emitting the proper audit records as I mentioned in the previous patch
>>>>>> set, but regardless there are some comments on this code as written
>>>>>> ...
>>>>>>
>>>>>>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>>>>>>> index 33028c098ef3..18cc7c956b69 100644
>>>>>>> --- a/security/lsm_audit.c
>>>>>>> +++ b/security/lsm_audit.c
>>>>>>> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>>>>>>>            case LSM_AUDIT_DATA_IPC:
>>>>>>>                    audit_log_format(ab, " key=%d ", a->u.ipc_id);
>>>>>>>                    break;
>>>>>>> -       case LSM_AUDIT_DATA_CAP:
>>>>>>> -               audit_log_format(ab, " capability=%d ", a->u.cap);
>>>>>>> +       case LSM_AUDIT_DATA_CAP: {
>>>>>>> +               const struct inode *inode;
>>>>>>> +
>>>>>>> +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
>>>>>>> +               if (a->u.cap_struct.cad) {
>>>>>>> +                       switch (a->u.cap_struct.cad->type) {
>>>>>>> +                       case CAP_AUX_DATA_INODE: {
>>>>>>> +                               inode = a->u.cap_struct.cad->u.inode;
>>>>>>> +
>>>>>>> +                               audit_log_format(ab, " dev=");
>>>>>>> +                               audit_log_untrustedstring(ab,
>>>>>>> +                                       inode->i_sb->s_id);
>>>>>>> +                               audit_log_format(ab, " ino=%lu",
>>>>>>> +                                       inode->i_ino);
>>>>>>> +                               break;
>>>>>>> +                       }
>>>>>>
>>>>>> Since you are declaring "inode" further up, there doesn't appear to be
>>>>>> any need for the CAP_AUX_DATA_INODE braces, please remove them.
>>>>>>
>>>>>> The general recommended practice when it comes to "sometimes" fields
>>>>>> in an audit record, is to always record them in the record, but use a
>>>>>> value of "?" when there is nothing relevant to record.  For example,
>>>>>> when *not* recording inode information you would do something like the
>>>>>> following:
>>>>>>
>>>>>>      audit_log_format(ab, " dev=? ino=?");
>>>>>>
>>>>> The issue this brings up is what happens when this is expanded to more
>>>>> cases? Assuming there will be a case here for logging audit data for task
>>>>> based capabilities (CAP_AUX_DATA_TASK), what do we want to have happen if we
>>>>> are recording *neither* inode information nor task information (say a PID)?
>>>>> If we log something in the inode case, we presumably don't want to call
>>>>> audit_log_format(ab, " dev=?, pid=?") as well. (And vice versa for when we
>>>>> log a pid and no inode).
>>>>
>>>> Yup.  This record is already a mess due to that.
>>>>
>>>> The general solution is to either use a new record type for each
>>>> variant, or to use an auxiliary record for each additional piece of
>>>> information.  The long term solution that has been suggested it to move
>>>> away from a text message format.
>>>
>>> This is why I was suggesting that Aaron look into allowing the LSMs to
>>> request generation of audit records in the earlier thread (and
>>> mentioned it again at the top of my comments in this thread).
>>
>> How would that work? The behavior we want is to capture some information
>> about the inode whenever there is a capability denial upon an attempted
>> access to that inode.  Allowing the LSM to enable audit collection on a
>> per-process basis doesn't appear to help with that goal, because this is
>> something we want for all processes.  Further, we don't really want the
>> rest of the audit collection machinery engaged here ...
> 
> I read this as "we want to audit this information, but we don't to
> turn on the very thing which is designed to do this".  At some point,
> if you want to audit things, you actually need to turn on auditing.
> 
>> ... we just want the
>> inode information for this specific inode, and we have the inode readily
>> accessible in the caller of the LSM hook already, so we don't need to do
>> it earlier.
> 
> Aaron appeared to be complaining that if we stuck to the current best
> practices regarding record formatting for the LSM generated audit
> record, the record was going to get complicated when people started
> adding additional information.  FWIW, I don't disagree.  The only real
> alternative I've seen thus far is to look into having the LSM enable
> certain records, if anyone has another idea I'm all ears/eyes.
> 
>> Further, in the future we want to be able to take the inode security
>> label into consideration as part of the capability checking, which is
>> independent of audit entirely.  So the rest of the patch will still be
>> required even if the audit solution ends up being different.
> 
> That's a different topic, I don't think there are any remaining
> objections to passing the inode information here.
> 

I'm looking at how to enable LSMs to selectively turn on audit 
collection. So there seems to be two key points: audit_alloc() and 
__audit_syscall_entry(). Would it suffice to define a single boolean 
hook that takes the task and call it from both functions, to decide 
whether to override an AUDIT_DISABLED state in audit_alloc() and to 
override a 0 audit_n_rules in __audit_syscall_entry(). In audit_alloc() 
if audit_filter_task() returned AUDIT_DISABLED and the hook returned 
true, we would change the state to AUDIT_BUILD_CONTEXT. In 
__audit_syscall_entry(), if the hook returned true, we would set dummy 
to 0. Obviously, we could have a more general hook which lets us return 
arbitrary audit states, but, it isn't clear how we would reconcile 
conflicting results from audit_filter_task() and the hook for any 
situation other than AUDIT_DISABLED. We could also potentially use a 
different hook in __audit_syscall_entry(), though I don't think that we 
want the LSMs trying to interpret the syscall number or arguments.

Do you think that is sufficiently general or would you suggest something 
different?

-- 
Aaron