From: Aaron Goidel <acgoide@tycho.nsa.gov> To: Paul Moore <paul@paul-moore.com> Cc: selinux@vger.kernel.org, linux-security-module@vger.kernel.org, luto@amacapital.net, James Morris <jmorris@namei.org>, Stephen Smalley <sds@tycho.nsa.gov>, keescook@chromium.org, Serge Hallyn <serge@hallyn.com>, john.johansen@canonical.com, casey@schaufler-ca.com, mortonm@chromium.org, rgb@redhat.com, Nicholas Franck <nhfran2@tycho.nsa.gov>, linux-audit@redhat.com Subject: Re: [Non-DoD Source] Re: [RFC PATCH v2] security, capability: pass object information to security_capable Date: Tue, 13 Aug 2019 11:01:01 -0400 Message-ID: <b79617aa-2b40-40bf-9f35-0f5be8e34d3f@tycho.nsa.gov> (raw) In-Reply-To: <CAHC9VhTSWiz45vh+M+sgu+ePwgFPZ4Mr8GmRZQjsGWQSzkjbLg@mail.gmail.com> 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
next prev parent reply index Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-01 14:43 [RFC PATCH v2] security,capability: " Aaron Goidel 2019-08-08 16:30 ` Paul Moore 2019-08-13 15:01 ` Aaron Goidel [this message] 2019-08-13 21:27 ` [Non-DoD Source] Re: [RFC PATCH v2] security, capability: " Richard Guy Briggs 2019-08-14 19:59 ` Paul Moore 2019-08-14 21:08 ` Stephen Smalley 2019-08-14 21:27 ` Paul Moore 2019-08-15 13:10 ` [Non-DoD Source] " Aaron Goidel 2019-08-16 16:29 ` Paul Moore
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b79617aa-2b40-40bf-9f35-0f5be8e34d3f@tycho.nsa.gov \ --to=acgoide@tycho.nsa.gov \ --cc=casey@schaufler-ca.com \ --cc=jmorris@namei.org \ --cc=john.johansen@canonical.com \ --cc=keescook@chromium.org \ --cc=linux-audit@redhat.com \ --cc=linux-security-module@vger.kernel.org \ --cc=luto@amacapital.net \ --cc=mortonm@chromium.org \ --cc=nhfran2@tycho.nsa.gov \ --cc=paul@paul-moore.com \ --cc=rgb@redhat.com \ --cc=sds@tycho.nsa.gov \ --cc=selinux@vger.kernel.org \ --cc=serge@hallyn.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
SELinux Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \ selinux@vger.kernel.org public-inbox-index selinux Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.selinux AGPL code for this site: git clone https://public-inbox.org/public-inbox.git