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

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

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).
>> +                       }
>> +               }
>>                  break;
>> +       }
>>          case LSM_AUDIT_DATA_PATH: {
>>                  struct inode *inode;
>>
> 

-- 
Aaron