From: Paul Moore <paul@paul-moore.com> To: Aaron Goidel <acgoide@tycho.nsa.gov> 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: [RFC PATCH v2] security,capability: pass object information to security_capable Date: Thu, 8 Aug 2019 12:30:28 -0400 Message-ID: <CAHC9VhTSWiz45vh+M+sgu+ePwgFPZ4Mr8GmRZQjsGWQSzkjbLg@mail.gmail.com> (raw) In-Reply-To: <20190801144313.1014-1-acgoide@tycho.nsa.gov> 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=?"); > + } > + } > break; > + } > case LSM_AUDIT_DATA_PATH: { > struct inode *inode; > -- paul moore www.paul-moore.com
next prev parent reply index Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-01 14:43 Aaron Goidel 2019-08-08 16:30 ` Paul Moore [this message] 2019-08-13 15:01 ` [Non-DoD Source] Re: [RFC PATCH v2] security, capability: " Aaron Goidel 2019-08-13 21:27 ` 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=CAHC9VhTSWiz45vh+M+sgu+ePwgFPZ4Mr8GmRZQjsGWQSzkjbLg@mail.gmail.com \ --to=paul@paul-moore.com \ --cc=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=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