[Non-DoD Source] Re: [RFC PATCH v2] security, capability: pass object information to security_capable
Aaron Goidel
acgoide at tycho.nsa.gov
Thu Aug 15 13:10:08 UTC 2019
On 8/14/19 5:27 PM, Paul Moore wrote:
> On Wed, Aug 14, 2019 at 5:08 PM Stephen Smalley <sds at 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 at 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 at tycho.nsa.gov> wrote:
>>>>>>> From: Nicholas Franck <nhfran2 at 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 at tycho.nsa.gov>
>>>>>>> Signed-off-by: Aaron Goidel <acgoide at 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
More information about the Linux-security-module-archive
mailing list