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

On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide(a)tycho.nsa.gov&gt; wrote: > From: Nicholas Franck <nhfran2(a)tycho.nsa.gov&gt; > > 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(a)tycho.nsa.gov&gt; > Signed-off-by: Aaron Goidel <acgoide(a)tycho.nsa.gov&gt; > --- > 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=?");

> + } > + } > break; > + } > case LSM_AUDIT_DATA_PATH: { > struct inode *inode; >