On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@xxxxxxxxxxxxx> wrote:
> From: Nicholas Franck <nhfran2@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Signed-off-by: Aaron Goidel <acgoide@xxxxxxxxxxxxx>
> ---
> 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;
>
--
paul moore
www.paul-moore.com
