From 4b3e4ed6b0d958d7fb2f160bb8ebfb4f0db19382 Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Thu, 20 Apr 2017 13:07:30 -0400 Subject: [PATCH 1/5] audit: unswing cap_* fields in PATH records The cap_* fields swing in and out of PATH records. If no capabilities are set, the cap_* fields are completely missing and when one of the cap_fi or cap_fp values is empty, that field is omitted. Original: type=PATH msg=audit(04/20/2017 12:17:11.222:193) : item=1 name=/lib64/ld-linux-x86-64.so.2 inode=787694 dev=08:03 mode=file,755 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:ld_so_t:s0 nametype=NORMAL type=PATH msg=audit(04/20/2017 12:17:11.222:193) : item=0 name=/home/sleep inode=1319469 dev=08:03 mode=file,suid,755 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:bin_t:s0 nametype=NORMAL cap_fp=sys_admin cap_fe=1 cap_fver=2 Normalize the PATH record by always printing all 4 cap_* fields. Fixed: type=PATH msg=audit(04/20/2017 13:01:31.679:201) : item=1 name=/lib64/ld-linux-x86-64.so.2 inode=787694 dev=08:03 mode=file,755 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:ld_so_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(04/20/2017 13:01:31.679:201) : item=0 name=/home/sleep inode=1319469 dev=08:03 mode=file,suid,755 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:bin_t:s0 nametype=NORMAL cap_fp=sys_admin cap_fi=none cap_fe=1 cap_fver=2 See: https://github.com/linux-audit/audit-kernel/issues/42 Signed-off-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index a7c6a50477aa..b2e877100242 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1999,22 +1999,10 @@ void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap) static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name) { - kernel_cap_t *perm = &name->fcap.permitted; - kernel_cap_t *inh = &name->fcap.inheritable; - int log = 0; - - if (!cap_isclear(*perm)) { - audit_log_cap(ab, "cap_fp", perm); - log = 1; - } - if (!cap_isclear(*inh)) { - audit_log_cap(ab, "cap_fi", inh); - log = 1; - } - - if (log) - audit_log_format(ab, " cap_fe=%d cap_fver=%x", - name->fcap.fE, name->fcap_ver); + audit_log_cap(ab, "cap_fp", &name->fcap.permitted); + audit_log_cap(ab, "cap_fi", &name->fcap.inheritable); + audit_log_format(ab, " cap_fe=%d cap_fver=%x", + name->fcap.fE, name->fcap_ver); } static inline int audit_copy_fcaps(struct audit_names *name, From 7786f6b6dfc12d17eea2df04116de6ebac50c884 Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Fri, 7 Apr 2017 10:17:27 -0400 Subject: [PATCH 2/5] audit: add ambient capabilities to CAPSET and BPRM_FCAPS records Capabilities were augmented to include ambient capabilities in v4.3 commit 58319057b784 ("capabilities: ambient capabilities"). Add ambient capabilities to the audit BPRM_FCAPS and CAPSET records. The record contains fields "old_pp", "old_pi", "old_pe", "new_pp", "new_pi", "new_pe" so in keeping with the previous record normalizations, change the "new_*" variants to simply drop the "new_" prefix. A sample of the replaced BPRM_FCAPS record: RAW: type=BPRM_FCAPS msg=audit(1491468034.252:237): fver=2 fp=0000000000200000 fi=0000000000000000 fe=1 old_pp=0000000000000000 old_pi=0000000000000000 old_pe=0000000000000000 old_pa=0000000000000000 pp=0000000000200000 pi=0000000000000000 pe=0000000000200000 pa=0000000000000000 INTERPRET: type=BPRM_FCAPS msg=audit(04/06/2017 04:40:34.252:237): fver=2 fp=sys_admin fi=none fe=chown old_pp=none old_pi=none old_pe=none old_pa=none pp=sys_admin pi=none pe=sys_admin pa=none A sample of the replaced CAPSET record: RAW: type=CAPSET msg=audit(1491469502.371:242): pid=833 cap_pi=0000003fffffffff cap_pp=0000003fffffffff cap_pe=0000003fffffffff cap_pa=0000000000000000 INTERPRET: type=CAPSET msg=audit(04/06/2017 05:05:02.371:242) : pid=833 cap_pi=chown,dac_override,dac_read_search,fowner,fsetid,kill, setgid,setuid,setpcap,linux_immutable,net_bind_service,net_broadcast, net_admin,net_raw,ipc_lock,ipc_owner,sys_module,sys_rawio,sys_chroot, sys_ptrace,sys_pacct,sys_admin,sys_boot,sys_nice,sys_resource,sys_time, sys_tty_config,mknod,lease,audit_write,audit_control,setfcap, mac_override,mac_admin,syslog,wake_alarm,block_suspend,audit_read cap_pp=chown,dac_override,dac_read_search,fowner,fsetid,kill,setgid, setuid,setpcap,linux_immutable,net_bind_service,net_broadcast, net_admin,net_raw,ipc_lock,ipc_owner,sys_module,sys_rawio,sys_chroot, sys_ptrace,sys_pacct,sys_admin,sys_boot,sys_nice,sys_resource, sys_time,sys_tty_config,mknod,lease,audit_write,audit_control,setfcap, mac_override,mac_admin,syslog,wake_alarm,block_suspend,audit_read cap_pe=chown,dac_override,dac_read_search,fowner,fsetid,kill,setgid, setuid,setpcap,linux_immutable,net_bind_service,net_broadcast, net_admin,net_raw,ipc_lock,ipc_owner,sys_module,sys_rawio,sys_chroot, sys_ptrace,sys_pacct,sys_admin,sys_boot,sys_nice,sys_resource, sys_time,sys_tty_config,mknod,lease,audit_write,audit_control,setfcap, mac_override,mac_admin,syslog,wake_alarm,block_suspend,audit_read cap_pa=none See: https://github.com/linux-audit/audit-kernel/issues/40 Signed-off-by: Richard Guy Briggs Acked-by: Serge Hallyn Signed-off-by: Paul Moore --- kernel/audit.h | 1 + kernel/auditsc.c | 12 +++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/kernel/audit.h b/kernel/audit.h index ddfce2ea4891..bb3a4e14b7e5 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -68,6 +68,7 @@ struct audit_cap_data { unsigned int fE; /* effective bit of file cap */ kernel_cap_t effective; /* effective set of process */ }; + kernel_cap_t ambient; }; /* When fs/namei.c:getname() is called, we store the pointer in name and bump diff --git a/kernel/auditsc.c b/kernel/auditsc.c index b2dcbe637b7c..5fa68d10032f 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -1260,6 +1260,7 @@ static void show_special(struct audit_context *context, int *call_panic) audit_log_cap(ab, "cap_pi", &context->capset.cap.inheritable); audit_log_cap(ab, "cap_pp", &context->capset.cap.permitted); audit_log_cap(ab, "cap_pe", &context->capset.cap.effective); + audit_log_cap(ab, "cap_pa", &context->capset.cap.ambient); break; case AUDIT_MMAP: audit_log_format(ab, "fd=%d flags=0x%x", context->mmap.fd, @@ -1381,9 +1382,11 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts audit_log_cap(ab, "old_pp", &axs->old_pcap.permitted); audit_log_cap(ab, "old_pi", &axs->old_pcap.inheritable); audit_log_cap(ab, "old_pe", &axs->old_pcap.effective); - audit_log_cap(ab, "new_pp", &axs->new_pcap.permitted); - audit_log_cap(ab, "new_pi", &axs->new_pcap.inheritable); - audit_log_cap(ab, "new_pe", &axs->new_pcap.effective); + audit_log_cap(ab, "old_pa", &axs->old_pcap.ambient); + audit_log_cap(ab, "pp", &axs->new_pcap.permitted); + audit_log_cap(ab, "pi", &axs->new_pcap.inheritable); + audit_log_cap(ab, "pe", &axs->new_pcap.effective); + audit_log_cap(ab, "pa", &axs->new_pcap.ambient); break; } } @@ -2341,10 +2344,12 @@ int __audit_log_bprm_fcaps(struct linux_binprm *bprm, ax->old_pcap.permitted = old->cap_permitted; ax->old_pcap.inheritable = old->cap_inheritable; ax->old_pcap.effective = old->cap_effective; + ax->old_pcap.ambient = old->cap_ambient; ax->new_pcap.permitted = new->cap_permitted; ax->new_pcap.inheritable = new->cap_inheritable; ax->new_pcap.effective = new->cap_effective; + ax->new_pcap.ambient = new->cap_ambient; return 0; } @@ -2363,6 +2368,7 @@ void __audit_log_capset(const struct cred *new, const struct cred *old) context->capset.cap.effective = new->cap_effective; context->capset.cap.inheritable = new->cap_effective; context->capset.cap.permitted = new->cap_permitted; + context->capset.cap.ambient = new->cap_ambient; context->type = AUDIT_CAPSET; } From e4c1a0d15b62a3ed2f8cd560e9fa589dbe880c13 Mon Sep 17 00:00:00 2001 From: Derek Robson Date: Mon, 12 Jun 2017 14:33:03 +1200 Subject: [PATCH 3/5] audit: style fix Fixed checkpatch.pl warnings of "function definition argument FOO should also have an identifier name" Signed-off-by: Derek Robson Reviewed-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit.h | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/kernel/audit.h b/kernel/audit.h index bb3a4e14b7e5..b331d9b83f63 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -248,13 +248,13 @@ struct audit_netlink_list { struct sk_buff_head q; }; -int audit_send_list(void *); +int audit_send_list(void *_dest); extern int selinux_audit_rule_update(void); extern struct mutex audit_filter_mutex; -extern int audit_del_rule(struct audit_entry *); -extern void audit_free_rule_rcu(struct rcu_head *); +extern int audit_del_rule(struct audit_entry *entry); +extern void audit_free_rule_rcu(struct rcu_head *head); extern struct list_head audit_filter_list[]; extern struct audit_entry *audit_dupe_rule(struct audit_krule *old); @@ -302,17 +302,17 @@ extern int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark #endif /* CONFIG_AUDIT_WATCH */ #ifdef CONFIG_AUDIT_TREE -extern struct audit_chunk *audit_tree_lookup(const struct inode *); -extern void audit_put_chunk(struct audit_chunk *); -extern bool audit_tree_match(struct audit_chunk *, struct audit_tree *); -extern int audit_make_tree(struct audit_krule *, char *, u32); -extern int audit_add_tree_rule(struct audit_krule *); -extern int audit_remove_tree_rule(struct audit_krule *); +extern struct audit_chunk *audit_tree_lookup(const struct inode *inode); +extern void audit_put_chunk(struct audit_chunk *chunk); +extern bool audit_tree_match(struct audit_chunk *chunk, struct audit_tree *tree); +extern int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op); +extern int audit_add_tree_rule(struct audit_krule *rule); +extern int audit_remove_tree_rule(struct audit_krule *rule); extern void audit_trim_trees(void); extern int audit_tag_tree(char *old, char *new); -extern const char *audit_tree_path(struct audit_tree *); -extern void audit_put_tree(struct audit_tree *); -extern void audit_kill_trees(struct list_head *); +extern const char *audit_tree_path(struct audit_tree *tree); +extern void audit_put_tree(struct audit_tree *tree); +extern void audit_kill_trees(struct list_head *list); #else #define audit_remove_tree_rule(rule) BUG() #define audit_add_tree_rule(rule) -EINVAL @@ -324,7 +324,7 @@ extern void audit_kill_trees(struct list_head *); #define audit_kill_trees(list) BUG() #endif -extern char *audit_unpack_string(void **, size_t *, size_t); +extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); extern pid_t audit_sig_pid; extern kuid_t audit_sig_uid; @@ -334,7 +334,7 @@ extern int audit_filter(int msgtype, unsigned int listtype); #ifdef CONFIG_AUDITSYSCALL extern int audit_signal_info(int sig, struct task_struct *t); -extern void audit_filter_inodes(struct task_struct *, struct audit_context *); +extern void audit_filter_inodes(struct task_struct *tsk, struct audit_context *ctx); extern struct list_head *audit_killed_trees(void); #else #define audit_signal_info(s,t) AUDIT_DISABLED From c81be52a3ac0267aa830a2c4cb769030ea3483c9 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Mon, 12 Jun 2017 09:35:24 -0400 Subject: [PATCH 4/5] audit: fix a race condition with the auditd tracking code Originally reported by Adam and Dusty, it appears we have a small race window in kauditd_thread(), as documented in the Fedora BZ: * https://bugzilla.redhat.com/show_bug.cgi?id=1459326#c35 "This issue is partly due to the read-copy nature of RCU, and partly due to how we sync the auditd_connection state across kauditd_thread and the audit control channel. The kauditd_thread thread is always running so it can service the record queues and emit the multicast messages, if it happens to be just past the "main_queue" label, but before the "if (sk == NULL || ...)" if-statement which calls auditd_reset() when the new auditd connection is registered it could end up resetting the auditd connection, regardless of if it is valid or not. This is a rather small window and the variable nature of multi-core scheduling explains why this is proving rather difficult to reproduce." The fix is to have functions only call auditd_reset() when they believe that the kernel/auditd connection is still valid, e.g. non-NULL, and to have these callers pass their local copy of the auditd_connection pointer to auditd_reset() where it can be compared with the current connection state before resetting. If the caller has a stale state tracking pointer then the reset is ignored. We also make a small change to kauditd_thread() so that if the kernel/auditd connection is dead we skip the retry queue and send the records straight to the hold queue. This is necessary as we used to rely on auditd_reset() to occasionally purge the retry queue but we are going to be calling the reset function much less now and we want to make sure the retry queue doesn't grow unbounded. Reported-by: Adam Williamson Reported-by: Dusty Mabe Reviewed-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index b2e877100242..e1e2b3abfb93 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -575,12 +575,16 @@ static void kauditd_retry_skb(struct sk_buff *skb) /** * auditd_reset - Disconnect the auditd connection + * @ac: auditd connection state * * Description: * Break the auditd/kauditd connection and move all the queued records into the - * hold queue in case auditd reconnects. + * hold queue in case auditd reconnects. It is important to note that the @ac + * pointer should never be dereferenced inside this function as it may be NULL + * or invalid, you can only compare the memory address! If @ac is NULL then + * the connection will always be reset. */ -static void auditd_reset(void) +static void auditd_reset(const struct auditd_connection *ac) { unsigned long flags; struct sk_buff *skb; @@ -590,6 +594,11 @@ static void auditd_reset(void) spin_lock_irqsave(&auditd_conn_lock, flags); ac_old = rcu_dereference_protected(auditd_conn, lockdep_is_held(&auditd_conn_lock)); + if (ac && ac != ac_old) { + /* someone already registered a new auditd connection */ + spin_unlock_irqrestore(&auditd_conn_lock, flags); + return; + } rcu_assign_pointer(auditd_conn, NULL); spin_unlock_irqrestore(&auditd_conn_lock, flags); @@ -649,8 +658,8 @@ static int auditd_send_unicast_skb(struct sk_buff *skb) return rc; err: - if (rc == -ECONNREFUSED) - auditd_reset(); + if (ac && rc == -ECONNREFUSED) + auditd_reset(ac); return rc; } @@ -795,9 +804,9 @@ static int kauditd_thread(void *dummy) rc = kauditd_send_queue(sk, portid, &audit_hold_queue, UNICAST_RETRIES, NULL, kauditd_rehold_skb); - if (rc < 0) { + if (ac && rc < 0) { sk = NULL; - auditd_reset(); + auditd_reset(ac); goto main_queue; } @@ -805,9 +814,9 @@ static int kauditd_thread(void *dummy) rc = kauditd_send_queue(sk, portid, &audit_retry_queue, UNICAST_RETRIES, NULL, kauditd_hold_skb); - if (rc < 0) { + if (ac && rc < 0) { sk = NULL; - auditd_reset(); + auditd_reset(ac); goto main_queue; } @@ -815,12 +824,13 @@ main_queue: /* process the main queue - do the multicast send and attempt * unicast, dump failed record sends to the retry queue; if * sk == NULL due to previous failures we will just do the - * multicast send and move the record to the retry queue */ + * multicast send and move the record to the hold queue */ rc = kauditd_send_queue(sk, portid, &audit_queue, 1, kauditd_send_multicast_skb, - kauditd_retry_skb); - if (sk == NULL || rc < 0) - auditd_reset(); + (sk ? + kauditd_retry_skb : kauditd_hold_skb)); + if (ac && rc < 0) + auditd_reset(ac); sk = NULL; /* drop our netns reference, no auditd sends past this line */ @@ -1230,7 +1240,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) auditd_pid, 1); /* unregister the auditd connection */ - auditd_reset(); + auditd_reset(NULL); } } if (s.mask & AUDIT_STATUS_RATE_LIMIT) { From cd33f5f2cbfaadc21270f3ddac7c3c33e0a1a28c Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Mon, 12 Jun 2017 11:53:09 -0400 Subject: [PATCH 5/5] audit: make sure we never skip the multicast broadcast When the auditd connection is reset, either intentionally or due to a failure, any records that were in the main backlog queue would not be sent in a multicast broadcast. This patch fixes this problem by not flushing the main backlog queue on a connection reset, the main kauditd_thread() will take care of that normally. Resolves: https://github.com/linux-audit/audit-kernel/issues/41 Reviewed-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index e1e2b3abfb93..7cad70214b81 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -605,11 +605,10 @@ static void auditd_reset(const struct auditd_connection *ac) if (ac_old) call_rcu(&ac_old->rcu, auditd_conn_free); - /* flush all of the main and retry queues to the hold queue */ + /* flush the retry queue to the hold queue, but don't touch the main + * queue since we need to process that normally for multicast */ while ((skb = skb_dequeue(&audit_retry_queue))) kauditd_hold_skb(skb); - while ((skb = skb_dequeue(&audit_queue))) - kauditd_hold_skb(skb); } /**