Re: [RFC PATCH v3] security,capability: pass object information to security_capable
On Thu, 15 Aug 2019, Aaron Goidel wrote: > In SELinux this new information is leveraged here to perform an > additional inode based check for capabilities relevant to inodes. Since > the inode provided to capable_wrt_inode_uidgid() is a const argument, > this also required propagating const down to dump_common_audit_data() and > dropping the use of d_find_alias() to find an alias for the inode. This > was sketchy to begin with and should be obsoleted by a separate change > that will allow LSMs to trigger audit collection for all file-related > information. Will the audit logs look the same once the 2nd patch is applied? We need to be careful about breaking existing userland. -- James Morris -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[RFC PATCH v3] security, capability: pass object information to security_capable
From: Nicholas Franck
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 perform an
additional inode based check for capabilities relevant to inodes. Since
the inode provided to capable_wrt_inode_uidgid() is a const argument,
this also required propagating const down to dump_common_audit_data() and
dropping the use of d_find_alias() to find an alias for the inode. This
was sketchy to begin with and should be obsoleted by a separate change
that will allow LSMs to trigger audit collection for all file-related
information.
A new security class is defined for the inode capability permission
checks. This is because the existing file-related classes were already
too close to being full to support the addition of these permissions.
This has the limitation of not supporting per-file-class distinctions
(e.g. distinguishing regular files from directories or special files if
they have the same security label). An alternative would be to
instantiate a separate class for every existing file class, or, to widen
access vectors to 64 bits. Neither seems justified.
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
Signed-off-by: Aaron Goidel
---
v3:
- No longer change the audit record for capability checks
- Introduce an inode-based check in selinux_capable
- Drop attempts to find dentry when only inode is available
v2:
- Changed order of audit prints so optional information comes second
---
include/linux/capability.h | 7 +++
include/linux/lsm_audit.h | 2 +-
include/linux/lsm_hooks.h | 3 +-
include/linux/security.h | 23 ++---
kernel/capability.c| 33 +
kernel/seccomp.c | 2 +-
security/apparmor/capability.c | 3 +-
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 | 10 +---
security/safesetid/lsm.c | 3 +-
security/security.c| 5 +-
security/selinux/hooks.c | 66 +++---
security/selinux/include/classmap.h| 3 ++
security/smack/smack_access.c | 2 +-
18 files changed, 136 insertions(+), 50 deletions(-)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index ecce0f43c73a..f72de64c179d 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -211,6 +211,8 @@ extern bool capable(int cap);
extern bool ns_capable(struct user_namespace *ns, int cap);
extern bool ns_capable_noaudit(struct user_namespace *ns, int cap);
extern bool ns_capable_setid(struct user_namespace *ns, int cap);
+extern bool ns_capable_inode(struct user_namespace *ns, int cap,
+const struct inode *inode);
#else
static inline bool has_capability(struct task_struct *t, int cap)
{
@@ -246,6 +248,11 @@ static inline bool ns_capable_setid(struct user_namespace
*ns, int cap)
{
return true;
}
+static bool ns_capable_inode(struct user_namespace *ns, int cap,
+const struct inode *inode)
+{
+ return true;
+}
#endif /* CONFIG_MULTIUSER */
extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const
struct inode *inode);
extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index 915330abf6e5..dc7d00c7ee67 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -77,7 +77,7 @@ struct common_audit_data {
union {
struct path path;
struct dentry *dentry;
- struct inode *inode;
+ const struct inode *inode;
struct lsm_network_audit *net;
int cap;
int ipc_id;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ead98af9c602..3270b8af3498 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1472,7 +1472,8 @@ union security_list_options {
int (*capable)(const struct cred *cred,
struct user_n
[RFC PATCH] audit, security: allow LSMs to selectively enable audit collection
Presently, there is no way for LSMs to enable collection of supplemental
audit records such as path and inode information when a permission denial
occurs. Provide a LSM hook to allow LSMs to selectively enable collection
on a per-task basis, even if the audit configuration would otherwise
disable auditing of a task and/or contains no audit filter rules. If the
hook returns a non-zero result, collect all available audit information. If
the hook generates its own audit record, then supplemental audit
information will be emitted at syscall exit.
In SELinux, we implement this hook by returning the result of a permission
check on the process. If the new process2:audit_enable permission is
allowed by the policy, then audit collection will be enabled for that
process. Otherwise, SELinux will defer to the audit configuration.
Signed-off-by: Aaron Goidel
---
include/linux/lsm_hooks.h | 7 +++
include/linux/security.h| 7 ++-
kernel/auditsc.c| 10 +++---
security/security.c | 5 +
security/selinux/hooks.c| 11 +++
security/selinux/include/classmap.h | 2 +-
6 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ead98af9c602..7d70a6759621 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1380,6 +1380,11 @@
* audit_rule_init.
* @lsmrule contains the allocated rule
*
+ * @audit_enable:
+ * Allow the security module to selectively enable audit collection
+ * on permission denials based on whether or not @tsk has the
+ * process2:audit_enable permission.
+ *
* @inode_invalidate_secctx:
* Notify the security module that it must revalidate the security context
* of an inode.
@@ -1800,6 +1805,7 @@ union security_list_options {
int (*audit_rule_known)(struct audit_krule *krule);
int (*audit_rule_match)(u32 secid, u32 field, u32 op, void *lsmrule);
void (*audit_rule_free)(void *lsmrule);
+ int (*audit_enable)(struct task_struct *tsk);
#endif /* CONFIG_AUDIT */
#ifdef CONFIG_BPF_SYSCALL
@@ -2043,6 +2049,7 @@ struct security_hook_heads {
struct hlist_head audit_rule_known;
struct hlist_head audit_rule_match;
struct hlist_head audit_rule_free;
+ struct hlist_head audit_enable;
#endif /* CONFIG_AUDIT */
#ifdef CONFIG_BPF_SYSCALL
struct hlist_head bpf;
diff --git a/include/linux/security.h b/include/linux/security.h
index 7d9c1da1f659..7be66db8de4e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1719,7 +1719,7 @@ int security_audit_rule_init(u32 field, u32 op, char
*rulestr, void **lsmrule);
int security_audit_rule_known(struct audit_krule *krule);
int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
void security_audit_rule_free(void *lsmrule);
-
+int security_audit_enable(struct task_struct *tsk);
#else
static inline int security_audit_rule_init(u32 field, u32 op, char *rulestr,
@@ -1742,6 +1742,11 @@ static inline int security_audit_rule_match(u32 secid,
u32 field, u32 op,
static inline void security_audit_rule_free(void *lsmrule)
{ }
+static inline int security_audit_enable(struct task_struct *tsk)
+{
+ return 0;
+}
+
#endif /* CONFIG_SECURITY */
#endif /* CONFIG_AUDIT */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 95ae27edd417..7e052b71bc42 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -906,8 +906,12 @@ int audit_alloc(struct task_struct *tsk)
state = audit_filter_task(tsk, &key);
if (state == AUDIT_DISABLED) {
- clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
- return 0;
+ if (security_audit_enable(tsk)) {
+ state = AUDIT_BUILD_CONTEXT;
+ } else {
+ clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
+ return 0;
+ }
}
if (!(context = audit_alloc_context(state))) {
@@ -1623,7 +1627,7 @@ void __audit_syscall_entry(int major, unsigned long a1,
unsigned long a2,
if (state == AUDIT_DISABLED)
return;
- context->dummy = !audit_n_rules;
+ context->dummy = !audit_n_rules && !security_audit_enable(current);
if (!context->dummy && state == AUDIT_BUILD_CONTEXT) {
context->prio = 0;
if (auditd_test_task(current))
diff --git a/security/security.c b/security/security.c
index 30687e1366b7..04e160e5d4ab 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2333,6 +2333,11 @@ int security_audit_rule_match(u32 secid, u32 field, u32
op, void *lsmrule)
{
return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
}
+
+int security_audit_enable(struct task_struct *tsk)
+{
+ return call_int_hook(audit_enable, 0, tsk);
+}
#endif /* CONFIG_AUDIT */
#ifdef CONFIG_BPF_SYSCALL
d
Re: [Non-DoD Source] Re: [RFC PATCH v2] security, capability: pass object information to security_capable
On 8/14/19 5:27 PM, Paul Moore wrote:
On Wed, Aug 14, 2019 at 5:08 PM Stephen Smalley wrote:
On 8/14/19 3:59 PM, Paul Moore wrote:
On Tue, Aug 13, 2019 at 5:27 PM Richard Guy Briggs wrote:
On 2019-08-13 11:01, Aaron Goidel wrote:
On 8/8/19 12:30 PM, Paul Moore wrote:
On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel wrote:
From: Nicholas Franck
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
Signed-off-by: Aaron Goidel
---
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).
Yup. This record is already a mess due to that.
The general solution is to either use a new record type for each
variant, or
