[RFC PATCH ghak59 V1 0/6] audit: config_change normalizations and event record gathering

[RFC PATCH ghak59 V1 0/6] audit: config_change normalizations and event record gathering

[Richard Guy Briggs]

Make a number of changes to normalize CONFIG_CHANGE records by adding
missing op= fields, providing more information in existing op fields and
connecting all records to existing audit events.

The user record patch is included but is *optional* since there is doubt
that we want to disconnect the records from a single event.

Since tree purge records are processed after the EOE record is produced,
the order of operation of the EOE record and the purge will have to be
reversed so that the purge records can be included in the event.

Could I get some feedback on the format of the op field values
themselves?  They shouldn't cause any text processing headaches but
there may be a better way of expressing them.

For reference, here are the calling methods and function tree for all
CONFIG_CHANGE events:
- audit_log_config_change() "op=set"
        - AUDIT_SET:AUDIT_STATUS_PID
        - AUDIT_SET:AUDIT_STATUS_LOST
        - audit_do_config_change()
                - AUDIT_SET:AUDIT_STATUS_FAILURE
                - AUDIT_SET:AUDIT_STATUS_ENABLED
                - AUDIT_SET:AUDIT_STATUS_RATE_LIMIT
                - AUDIT_SET:AUDIT_STATUS_BACKLOG_LIMIT
                - AUDIT_SET:AUDIT_STATUS_BACKLOG_WAIT_TIME
- audit_log_common_recv_msg()
        - AUDIT_*USER* events (not CONFIG_CHANGE like all the rest)
        - AUDIT_LOCKED "op=%s_rule"(add/remove)
        - AUDIT_TRIM "op=trim"
        - AUDIT_MAKE_EQUIV: "op=make_equiv"
        - AUDIT_TTY_SET: "op=tty_set"
- audit_log_rule_change()
        - AUDIT_ADD_RULE -F dir=:
        - AUDIT_DEL_RULE -F dir=:
- audit_mark_log_rule_change()
        - audit_autoremove_mark_rule() "op=autoremove_rule(mark)"
                - audit_mark_handle_event()
                        - audit_mark_fsnotify_ops.handle_event
- audit_tree_log_remove_rule() "op=remove_rule(tree:%s)" from kill_rules()
        - from trim_marked()
                - AUDIT_TRIM: audit_trim_trees() "trim"
                - audit_add_tree_rule() iterate_mounts err "add"
                        - audit_add_rule()
                                - audit_rule_change()
                                        - AUDIT_ADD_RULE -F dir=:
                - AUDIT_MAKE_EQUIV: audit_tag_tree() iterate_mounts err "equiv"
        - from audit_kill_trees()
                - __audit_free() "free"
                        - do_exit()
                        - copy_process() err
                - __audit_syscall_exit() "exit"
        - from evict_chunk() "evict"
                - audit_tree_freeing_mark()
                        - audit_tree_ops.freeing_mark
- audit_watch_log_rule_change()
        - audit_update_watch() "updated_rules(watch:inval)" : "updated_rules(watch:set)"
                - audit_watch_handle_event() FS_CREATE|FS_MOVED_TO, FS_DELETE|FS_MOVED_FROM
                        - audit_watch_fsnotify_ops.handle_event
        - audit_remove_parent_watches() "remove_rule(watch:parent)"
                - audit_watch_handle_event() FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF
                        - audit_watch_fsnotify_ops.handle_event

See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59

Richard Guy Briggs (6):
  audit: give a clue what CONFIG_CHANGE op was involved
  audit: add syscall information to CONFIG_CHANGE records
  audit: exclude user records from syscall context
  audit: hand taken context to audit_kill_trees for syscall logging
  audit: move EOE record after kill_trees for exit/free
  audit: extend config_change mark/watch/tree rule changes

 kernel/audit.c          | 20 ++++++++++++++------
 kernel/audit.h          |  4 ++--
 kernel/audit_fsnotify.c |  4 ++--
 kernel/audit_tree.c     | 28 +++++++++++++++-------------
 kernel/audit_watch.c    |  8 +++++---
 kernel/auditfilter.c    |  2 +-
 kernel/auditsc.c        | 26 ++++++++++++++++++--------
 7 files changed, 57 insertions(+), 35 deletions(-)

-- 
1.8.3.1

[RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

[Richard Guy Briggs]

The failure to add an audit rule due to audit locked gives no clue
what CONFIG_CHANGE operation failed.
Similarly the set operation is the only other operation that doesn't
give the "op=" field to indicate the action.
All other CONFIG_CHANGE records include an op= field to give a clue as
to what sort of configuration change is being executed.

Since these are the only CONFIG_CHANGE records that that do not have an
op= field, add them to bring them in line with the rest.

Old records:
type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0
type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes

New records:
type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0

type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes

See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..ad54339 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -403,7 +403,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
 	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (unlikely(!ab))
 		return rc;
-	audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
+	audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
 	audit_log_session_info(ab);
 	rc = audit_log_task_context(ab);
 	if (rc)
@@ -1365,7 +1365,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			return -EINVAL;
 		if (audit_enabled == AUDIT_LOCKED) {
 			audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
-			audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
+			audit_log_format(ab, " op=%s_rule audit_enabled=%d res=0",
+					 msg_type == AUDIT_ADD_RULE ? "add" : "remove",
+					 audit_enabled);
 			audit_log_end(ab);
 			return -EPERM;
 		}
-- 
1.8.3.1

[RFC PATCH ghak59 V1 2/6] audit: add syscall information to CONFIG_CHANGE records

[Richard Guy Briggs]

Tie syscall information to all CONFIG_CHANGE calls since they are all a
result of user actions.

See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c          | 4 ++--
 kernel/audit_fsnotify.c | 2 +-
 kernel/audit_tree.c     | 2 +-
 kernel/audit_watch.c    | 2 +-
 kernel/auditfilter.c    | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index ad54339..e469234 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -400,7 +400,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
 	struct audit_buffer *ab;
 	int rc = 0;
 
-	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (unlikely(!ab))
 		return rc;
 	audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
@@ -1067,7 +1067,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
 		return;
 	}
 
-	*ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
+	*ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
 	if (unlikely(!*ab))
 		return;
 	audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 52f368b..1640eb6 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
 
 	if (!audit_enabled)
 		return;
-	ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+	ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
 	if (unlikely(!ab))
 		return;
 	audit_log_format(ab, "auid=%u ses=%u op=%s",
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 5e9d1e5..a01b9da 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -499,7 +499,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
 
 	if (!audit_enabled)
 		return;
-	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (unlikely(!ab))
 		return;
 	audit_log_format(ab, "op=remove_rule");
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 9b4836b..da2978b 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc
 
 	if (!audit_enabled)
 		return;
-	ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+	ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
 	if (!ab)
 		return;
 	audit_log_format(ab, "auid=%u ses=%u op=%s",
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index eaa3201..6e19acb 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1093,7 +1093,7 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
 	if (!audit_enabled)
 		return;
 
-	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (!ab)
 		return;
 	audit_log_session_info(ab);
-- 
1.8.3.1

[RFC PATCH ghak59 V1 3/6] audit: exclude user records from syscall context

[Richard Guy Briggs]

Since the function audit_log_common_recv_msg() is shared by a number of
AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
and since the AUDIT_CONFIG_CHANGE message type has been converted to a
syscall accompanied record type, special-case the AUDIT_USER_* range of
messages so they remain standalone records.

See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index e469234..c8c2efc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1057,7 +1057,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
 	return err;
 }
 
-static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+static void __audit_log_common_recv_msg(struct audit_context *context,
+					struct audit_buffer **ab, u16 msg_type)
 {
 	uid_t uid = from_kuid(&init_user_ns, current_uid());
 	pid_t pid = task_tgid_nr(current);
@@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
 		return;
 	}
 
-	*ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
+	*ab = audit_log_start(context, GFP_KERNEL, msg_type);
 	if (unlikely(!*ab))
 		return;
 	audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
@@ -1075,6 +1076,11 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
 	audit_log_task_context(*ab);
 }
 
+static inline void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+	__audit_log_common_recv_msg(audit_context(), ab, msg_type);
+}
+
 int is_audit_feature_set(int i)
 {
 	return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1341,7 +1347,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				if (err)
 					break;
 			}
-			audit_log_common_recv_msg(&ab, msg_type);
+			__audit_log_common_recv_msg(NULL, &ab, msg_type);
 			if (msg_type != AUDIT_USER_TTY)
 				audit_log_format(ab, " msg='%.*s'",
 						 AUDIT_MESSAGE_TEXT_MAX,
-- 
1.8.3.1

[RFC PATCH ghak59 V1 4/6] audit: hand taken context to audit_kill_trees for syscall logging

[Richard Guy Briggs]

Since the context is taken from the task in __audit_syscall_exit() and
__audit_free(), hand it to audit_kill_trees() so it can be used to
associate with a syscall record.  This requires adding the context
parameter to kill_rules() rather than using the current audit_context
(which has been taken).

The callers of trim_marked() and evict_chunk() still have their context.

See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.h      |  4 ++--
 kernel/audit_tree.c | 18 ++++++++++--------
 kernel/auditsc.c    |  4 ++--
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index 214e149..f39f7aa 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -312,7 +312,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
 extern int audit_tag_tree(char *old, char *new);
 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);
+extern void audit_kill_trees(struct audit_context *context);
 #else
 #define audit_remove_tree_rule(rule) BUG()
 #define audit_add_tree_rule(rule) -EINVAL
@@ -321,7 +321,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
 #define audit_put_tree(tree) (void)0
 #define audit_tag_tree(old, new) -EINVAL
 #define audit_tree_path(rule) ""	/* never called */
-#define audit_kill_trees(list) BUG()
+#define audit_kill_trees(context) BUG()
 #endif
 
 extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index a01b9da..2d3e1071 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -493,13 +493,13 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	return 0;
 }
 
-static void audit_tree_log_remove_rule(struct audit_krule *rule)
+static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule)
 {
 	struct audit_buffer *ab;
 
 	if (!audit_enabled)
 		return;
-	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (unlikely(!ab))
 		return;
 	audit_log_format(ab, "op=remove_rule");
@@ -510,7 +510,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
 	audit_log_end(ab);
 }
 
-static void kill_rules(struct audit_tree *tree)
+static void kill_rules(struct audit_context *context, struct audit_tree *tree)
 {
 	struct audit_krule *rule, *next;
 	struct audit_entry *entry;
@@ -521,7 +521,7 @@ static void kill_rules(struct audit_tree *tree)
 		list_del_init(&rule->rlist);
 		if (rule->tree) {
 			/* not a half-baked one */
-			audit_tree_log_remove_rule(rule);
+			audit_tree_log_remove_rule(context, rule);
 			if (entry->rule.exe)
 				audit_remove_mark(entry->rule.exe);
 			rule->tree = NULL;
@@ -584,7 +584,7 @@ static void trim_marked(struct audit_tree *tree)
 		tree->goner = 1;
 		spin_unlock(&hash_lock);
 		mutex_lock(&audit_filter_mutex);
-		kill_rules(tree);
+		kill_rules(audit_context(), tree);
 		list_del_init(&tree->list);
 		mutex_unlock(&audit_filter_mutex);
 		prune_one(tree);
@@ -924,8 +924,10 @@ static void audit_schedule_prune(void)
  * ... and that one is done if evict_chunk() decides to delay until the end
  * of syscall.  Runs synchronously.
  */
-void audit_kill_trees(struct list_head *list)
+void audit_kill_trees(struct audit_context *context)
 {
+	struct list_head *list = &context->killed_trees;
+
 	audit_ctl_lock();
 	mutex_lock(&audit_filter_mutex);
 
@@ -933,7 +935,7 @@ void audit_kill_trees(struct list_head *list)
 		struct audit_tree *victim;
 
 		victim = list_entry(list->next, struct audit_tree, list);
-		kill_rules(victim);
+		kill_rules(context, victim);
 		list_del_init(&victim->list);
 
 		mutex_unlock(&audit_filter_mutex);
@@ -972,7 +974,7 @@ static void evict_chunk(struct audit_chunk *chunk)
 		list_del_init(&owner->same_root);
 		spin_unlock(&hash_lock);
 		if (!postponed) {
-			kill_rules(owner);
+			kill_rules(audit_context(), owner);
 			list_move(&owner->list, &prune_list);
 			need_prune = 1;
 		} else {
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ceb1c45..2590c9e 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1490,7 +1490,7 @@ void __audit_free(struct task_struct *tsk)
 	if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
 		audit_log_exit(context, tsk);
 	if (!list_empty(&context->killed_trees))
-		audit_kill_trees(&context->killed_trees);
+		audit_kill_trees(context);
 
 	audit_free_context(context);
 }
@@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
 	context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
 
 	if (!list_empty(&context->killed_trees))
-		audit_kill_trees(&context->killed_trees);
+		audit_kill_trees(context);
 
 	audit_free_names(context);
 	unroll_tree_refs(context, NULL, 0);
-- 
1.8.3.1

[RFC PATCH ghak59 V1 5/6] audit: move EOE record after kill_trees for exit/free

[Richard Guy Briggs]

The EOE record was being issued prior to the pruning of the killed_tree
list.

Move the EOE record creation out of audit_log_exit() and into its
callers __audit_free() and __audit_syscall_exit() so that
audit_kill_trees() can be called prior to the EOE record creation and
any purged trees CONFIG_CHANGE records included in the syscall record
event.

See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/auditsc.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 2590c9e..d56aead 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1460,10 +1460,6 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
 
 	audit_log_proctitle(tsk, context);
 
-	/* Send end of event record to help user space know we are finished */
-	ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
-	if (ab)
-		audit_log_end(ab);
 	if (call_panic)
 		audit_panic("error converting sid to string");
 }
@@ -1491,6 +1487,14 @@ void __audit_free(struct task_struct *tsk)
 		audit_log_exit(context, tsk);
 	if (!list_empty(&context->killed_trees))
 		audit_kill_trees(context);
+	if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
+		struct audit_buffer *ab;
+
+		/* Send end of event record to help user space know we are finished */
+		ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
+		if (ab)
+			audit_log_end(ab);
+	}
 
 	audit_free_context(context);
 }
@@ -1572,13 +1576,19 @@ void __audit_syscall_exit(int success, long return_code)
 
 	if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
 		audit_log_exit(context, current);
+	if (!list_empty(&context->killed_trees))
+		audit_kill_trees(context);
+	if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
+		struct audit_buffer *ab;
 
+		/* Send end of event record to help user space know we are finished */
+		ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
+		if (ab)
+			audit_log_end(ab);
+	}
 	context->in_syscall = 0;
 	context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
 
-	if (!list_empty(&context->killed_trees))
-		audit_kill_trees(context);
-
 	audit_free_names(context);
 	unroll_tree_refs(context, NULL, 0);
 	audit_free_aux(context);
-- 
1.8.3.1

[RFC PATCH ghak59 V1 6/6] audit: extend config_change mark/watch/tree rule changes

[Richard Guy Briggs]

Give a clue as to the source of mark, watch and tree rule changes.

See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.h          |  4 ++--
 kernel/audit_fsnotify.c |  2 +-
 kernel/audit_tree.c     | 24 ++++++++++++------------
 kernel/audit_watch.c    |  6 ++++--
 kernel/auditsc.c        |  4 ++--
 5 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index f39f7aa..5e072f5 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -312,7 +312,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
 extern int audit_tag_tree(char *old, char *new);
 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 audit_context *context);
+extern void audit_kill_trees(struct audit_context *context, char *trig);
 #else
 #define audit_remove_tree_rule(rule) BUG()
 #define audit_add_tree_rule(rule) -EINVAL
@@ -321,7 +321,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
 #define audit_put_tree(tree) (void)0
 #define audit_tag_tree(old, new) -EINVAL
 #define audit_tree_path(rule) ""	/* never called */
-#define audit_kill_trees(context) BUG()
+#define audit_kill_trees(context, trig) BUG()
 #endif
 
 extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 1640eb6..c10ba91 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -158,7 +158,7 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark)
 	struct audit_krule *rule = audit_mark->rule;
 	struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
 
-	audit_mark_log_rule_change(audit_mark, "autoremove_rule");
+	audit_mark_log_rule_change(audit_mark, "autoremove_rule(mark)");
 	audit_del_rule(entry);
 }
 
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 2d3e1071..1726cfa 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -493,7 +493,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	return 0;
 }
 
-static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule)
+static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule, char *trig)
 {
 	struct audit_buffer *ab;
 
@@ -502,7 +502,7 @@ static void audit_tree_log_remove_rule(struct audit_context *context, struct aud
 	ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (unlikely(!ab))
 		return;
-	audit_log_format(ab, "op=remove_rule");
+	audit_log_format(ab, "op=remove_rule(tree:%s)", trig);
 	audit_log_format(ab, " dir=");
 	audit_log_untrustedstring(ab, rule->tree->pathname);
 	audit_log_key(ab, rule->filterkey);
@@ -510,7 +510,7 @@ static void audit_tree_log_remove_rule(struct audit_context *context, struct aud
 	audit_log_end(ab);
 }
 
-static void kill_rules(struct audit_context *context, struct audit_tree *tree)
+static void kill_rules(struct audit_context *context, struct audit_tree *tree, char *trig)
 {
 	struct audit_krule *rule, *next;
 	struct audit_entry *entry;
@@ -521,7 +521,7 @@ static void kill_rules(struct audit_context *context, struct audit_tree *tree)
 		list_del_init(&rule->rlist);
 		if (rule->tree) {
 			/* not a half-baked one */
-			audit_tree_log_remove_rule(context, rule);
+			audit_tree_log_remove_rule(context, rule, trig);
 			if (entry->rule.exe)
 				audit_remove_mark(entry->rule.exe);
 			rule->tree = NULL;
@@ -551,7 +551,7 @@ static void prune_one(struct audit_tree *victim)
 
 /* trim the uncommitted chunks from tree */
 
-static void trim_marked(struct audit_tree *tree)
+static void trim_marked(struct audit_tree *tree, char *trig)
 {
 	struct list_head *p, *q;
 	spin_lock(&hash_lock);
@@ -584,7 +584,7 @@ static void trim_marked(struct audit_tree *tree)
 		tree->goner = 1;
 		spin_unlock(&hash_lock);
 		mutex_lock(&audit_filter_mutex);
-		kill_rules(audit_context(), tree);
+		kill_rules(audit_context(), tree, trig);
 		list_del_init(&tree->list);
 		mutex_unlock(&audit_filter_mutex);
 		prune_one(tree);
@@ -665,7 +665,7 @@ void audit_trim_trees(void)
 				node->index &= ~(1U<<31);
 		}
 		spin_unlock(&hash_lock);
-		trim_marked(tree);
+		trim_marked(tree, "trim");
 		drop_collected_mounts(root_mnt);
 skip_it:
 		put_tree(tree);
@@ -798,7 +798,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
 			node->index &= ~(1U<<31);
 		spin_unlock(&hash_lock);
 	} else {
-		trim_marked(tree);
+		trim_marked(tree, "add");
 		goto Err;
 	}
 
@@ -900,7 +900,7 @@ int audit_tag_tree(char *old, char *new)
 				node->index &= ~(1U<<31);
 			spin_unlock(&hash_lock);
 		} else {
-			trim_marked(tree);
+			trim_marked(tree, "equiv");
 		}
 
 		put_tree(tree);
@@ -924,7 +924,7 @@ static void audit_schedule_prune(void)
  * ... and that one is done if evict_chunk() decides to delay until the end
  * of syscall.  Runs synchronously.
  */
-void audit_kill_trees(struct audit_context *context)
+void audit_kill_trees(struct audit_context *context, char *trig)
 {
 	struct list_head *list = &context->killed_trees;
 
@@ -935,7 +935,7 @@ void audit_kill_trees(struct audit_context *context)
 		struct audit_tree *victim;
 
 		victim = list_entry(list->next, struct audit_tree, list);
-		kill_rules(context, victim);
+		kill_rules(context, victim, trig);
 		list_del_init(&victim->list);
 
 		mutex_unlock(&audit_filter_mutex);
@@ -974,7 +974,7 @@ static void evict_chunk(struct audit_chunk *chunk)
 		list_del_init(&owner->same_root);
 		spin_unlock(&hash_lock);
 		if (!postponed) {
-			kill_rules(audit_context(), owner);
+			kill_rules(audit_context(), owner, "evict");
 			list_move(&owner->list, &prune_list);
 			need_prune = 1;
 		} else {
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index da2978b..693d0a8 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -317,7 +317,9 @@ static void audit_update_watch(struct audit_parent *parent,
 			if (oentry->rule.exe)
 				audit_remove_mark(oentry->rule.exe);
 
-			audit_watch_log_rule_change(r, owatch, "updated_rules");
+			audit_watch_log_rule_change(r, owatch, invalidating ?
+						    "updated_rules(watch:inval)" :
+						    "updated_rules(watch:set)");
 
 			call_rcu(&oentry->rcu, audit_free_rule_rcu);
 		}
@@ -345,7 +347,7 @@ static void audit_remove_parent_watches(struct audit_parent *parent)
 	list_for_each_entry_safe(w, nextw, &parent->watches, wlist) {
 		list_for_each_entry_safe(r, nextr, &w->rules, rlist) {
 			e = container_of(r, struct audit_entry, rule);
-			audit_watch_log_rule_change(r, w, "remove_rule");
+			audit_watch_log_rule_change(r, w, "remove_rule(watch:parent)");
 			if (e->rule.exe)
 				audit_remove_mark(e->rule.exe);
 			list_del(&r->rlist);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d56aead..32428a3 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1486,7 +1486,7 @@ void __audit_free(struct task_struct *tsk)
 	if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
 		audit_log_exit(context, tsk);
 	if (!list_empty(&context->killed_trees))
-		audit_kill_trees(context);
+		audit_kill_trees(context, "free");
 	if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
 		struct audit_buffer *ab;
 
@@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
 	if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
 		audit_log_exit(context, current);
 	if (!list_empty(&context->killed_trees))
-		audit_kill_trees(context);
+		audit_kill_trees(context, "exit");
 	if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
 		struct audit_buffer *ab;
 
-- 
1.8.3.1

Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

[Paul Moore]

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> The failure to add an audit rule due to audit locked gives no clue
> what CONFIG_CHANGE operation failed.
> Similarly the set operation is the only other operation that doesn't
> give the "op=" field to indicate the action.
> All other CONFIG_CHANGE records include an op= field to give a clue as
> to what sort of configuration change is being executed.
>
> Since these are the only CONFIG_CHANGE records that that do not have an
> op= field, add them to bring them in line with the rest.

Normally this would be an immediate reject because this patch inserts
a field into an existing record, but the CONFIG_CHANGE record is so
variable (supposedly bad in its own right) that I don't this really
matters.

With that out of the way, I think this patch is fine, but I don't
think it is complete.  At the very least there is another
CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
appear to include an "op" field.  If we want to make sure we have an
"op" field in every CONFIG_CHANGE record, let's actually add them all
:)

There appears to be another one in audit_mark_log_rule_change() ...
and one more in audit_receive_msg().  There may be more.

> Old records:
> type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0
> type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
>
> New records:
> type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0
>
> type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
>
> See: https://github.com/linux-audit/audit-kernel/issues/59
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index e7478cb..ad54339 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -403,7 +403,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
>         ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>         if (unlikely(!ab))
>                 return rc;
> -       audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> +       audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
>         audit_log_session_info(ab);
>         rc = audit_log_task_context(ab);
>         if (rc)
> @@ -1365,7 +1365,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>                         return -EINVAL;
>                 if (audit_enabled == AUDIT_LOCKED) {
>                         audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> -                       audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
> +                       audit_log_format(ab, " op=%s_rule audit_enabled=%d res=0",
> +                                        msg_type == AUDIT_ADD_RULE ? "add" : "remove",
> +                                        audit_enabled);
>                         audit_log_end(ab);
>                         return -EPERM;
>                 }
> --
> 1.8.3.1

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 2/6] audit: add syscall information to CONFIG_CHANGE records

[Paul Moore]

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Tie syscall information to all CONFIG_CHANGE calls since they are all a
> result of user actions.
>
> See: https://github.com/linux-audit/audit-kernel/issues/59
> See: https://github.com/linux-audit/audit-kernel/issues/50
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c          | 4 ++--
>  kernel/audit_fsnotify.c | 2 +-
>  kernel/audit_tree.c     | 2 +-
>  kernel/audit_watch.c    | 2 +-
>  kernel/auditfilter.c    | 2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)

Merged, thanks.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index ad54339..e469234 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -400,7 +400,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
>         struct audit_buffer *ab;
>         int rc = 0;
>
> -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>         if (unlikely(!ab))
>                 return rc;
>         audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
> @@ -1067,7 +1067,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
>                 return;
>         }
>
> -       *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
> +       *ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
>         if (unlikely(!*ab))
>                 return;
>         audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 52f368b..1640eb6 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
>
>         if (!audit_enabled)
>                 return;
> -       ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> +       ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
>         if (unlikely(!ab))
>                 return;
>         audit_log_format(ab, "auid=%u ses=%u op=%s",
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 5e9d1e5..a01b9da 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -499,7 +499,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
>
>         if (!audit_enabled)
>                 return;
> -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>         if (unlikely(!ab))
>                 return;
>         audit_log_format(ab, "op=remove_rule");
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 9b4836b..da2978b 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc
>
>         if (!audit_enabled)
>                 return;
> -       ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> +       ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
>         if (!ab)
>                 return;
>         audit_log_format(ab, "auid=%u ses=%u op=%s",
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index eaa3201..6e19acb 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1093,7 +1093,7 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
>         if (!audit_enabled)
>                 return;
>
> -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>         if (!ab)
>                 return;
>         audit_log_session_info(ab);
> --
> 1.8.3.1
>


-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 2/6] audit: add syscall information to CONFIG_CHANGE records

[Paul Moore]

On Thu, Jun 28, 2018 at 5:47 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > Tie syscall information to all CONFIG_CHANGE calls since they are all a
> > result of user actions.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/59
> > See: https://github.com/linux-audit/audit-kernel/issues/50
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c          | 4 ++--
> >  kernel/audit_fsnotify.c | 2 +-
> >  kernel/audit_tree.c     | 2 +-
> >  kernel/audit_watch.c    | 2 +-
> >  kernel/auditfilter.c    | 2 +-
> >  5 files changed, 6 insertions(+), 6 deletions(-)
>
> Merged, thanks.

Actually, I take that back (good thing I hadn't pushed yet).

Why don't you squash this patch with 3/6 so that a bisect or
cherry-pick backport doesn't end up splitting 2/6 and 3/6 and causing
a regression with the AUDIT_USER_* records.

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index ad54339..e469234 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -400,7 +400,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
> >         struct audit_buffer *ab;
> >         int rc = 0;
> >
> > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> >         if (unlikely(!ab))
> >                 return rc;
> >         audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
> > @@ -1067,7 +1067,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> >                 return;
> >         }
> >
> > -       *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
> > +       *ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
> >         if (unlikely(!*ab))
> >                 return;
> >         audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> > diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> > index 52f368b..1640eb6 100644
> > --- a/kernel/audit_fsnotify.c
> > +++ b/kernel/audit_fsnotify.c
> > @@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c
> >
> >         if (!audit_enabled)
> >                 return;
> > -       ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> > +       ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
> >         if (unlikely(!ab))
> >                 return;
> >         audit_log_format(ab, "auid=%u ses=%u op=%s",
> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index 5e9d1e5..a01b9da 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -499,7 +499,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
> >
> >         if (!audit_enabled)
> >                 return;
> > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> >         if (unlikely(!ab))
> >                 return;
> >         audit_log_format(ab, "op=remove_rule");
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index 9b4836b..da2978b 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc
> >
> >         if (!audit_enabled)
> >                 return;
> > -       ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> > +       ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
> >         if (!ab)
> >                 return;
> >         audit_log_format(ab, "auid=%u ses=%u op=%s",
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index eaa3201..6e19acb 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -1093,7 +1093,7 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
> >         if (!audit_enabled)
> >                 return;
> >
> > -       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> >         if (!ab)
> >                 return;
> >         audit_log_session_info(ab);
> > --
> > 1.8.3.1
> >
>
>
> --
> paul moore
> www.paul-moore.com



-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 3/6] audit: exclude user records from syscall context

[Paul Moore]

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Since the function audit_log_common_recv_msg() is shared by a number of
> AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
> and since the AUDIT_CONFIG_CHANGE message type has been converted to a
> syscall accompanied record type, special-case the AUDIT_USER_* range of
> messages so they remain standalone records.
>
> See: https://github.com/linux-audit/audit-kernel/issues/59
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

I think this is fine, but see my previous comment about combining 2/6
and 3/6 as a safety measure.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index e469234..c8c2efc 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1057,7 +1057,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
>         return err;
>  }
>
> -static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> +static void __audit_log_common_recv_msg(struct audit_context *context,
> +                                       struct audit_buffer **ab, u16 msg_type)
>  {
>         uid_t uid = from_kuid(&init_user_ns, current_uid());
>         pid_t pid = task_tgid_nr(current);
> @@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
>                 return;
>         }
>
> -       *ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
> +       *ab = audit_log_start(context, GFP_KERNEL, msg_type);
>         if (unlikely(!*ab))
>                 return;
>         audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> @@ -1075,6 +1076,11 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
>         audit_log_task_context(*ab);
>  }
>
> +static inline void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> +{
> +       __audit_log_common_recv_msg(audit_context(), ab, msg_type);
> +}
> +
>  int is_audit_feature_set(int i)
>  {
>         return af.features & AUDIT_FEATURE_TO_MASK(i);
> @@ -1341,7 +1347,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>                                 if (err)
>                                         break;
>                         }
> -                       audit_log_common_recv_msg(&ab, msg_type);
> +                       __audit_log_common_recv_msg(NULL, &ab, msg_type);
>                         if (msg_type != AUDIT_USER_TTY)
>                                 audit_log_format(ab, " msg='%.*s'",
>                                                  AUDIT_MESSAGE_TEXT_MAX,
> --
> 1.8.3.1
>


-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 4/6] audit: hand taken context to audit_kill_trees for syscall logging

[Paul Moore]

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Since the context is taken from the task in __audit_syscall_exit() and
> __audit_free(), hand it to audit_kill_trees() so it can be used to
> associate with a syscall record.  This requires adding the context
> parameter to kill_rules() rather than using the current audit_context
> (which has been taken).
>
> The callers of trim_marked() and evict_chunk() still have their context.
>
> See: https://github.com/linux-audit/audit-kernel/issues/50
> See: https://github.com/linux-audit/audit-kernel/issues/59
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.h      |  4 ++--
>  kernel/audit_tree.c | 18 ++++++++++--------
>  kernel/auditsc.c    |  4 ++--
>  3 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 214e149..f39f7aa 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -312,7 +312,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
>  extern int audit_tag_tree(char *old, char *new);
>  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);
> +extern void audit_kill_trees(struct audit_context *context);
>  #else
>  #define audit_remove_tree_rule(rule) BUG()
>  #define audit_add_tree_rule(rule) -EINVAL
> @@ -321,7 +321,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
>  #define audit_put_tree(tree) (void)0
>  #define audit_tag_tree(old, new) -EINVAL
>  #define audit_tree_path(rule) ""       /* never called */
> -#define audit_kill_trees(list) BUG()
> +#define audit_kill_trees(context) BUG()
>  #endif
>
>  extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index a01b9da..2d3e1071 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -493,13 +493,13 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
>         return 0;
>  }
>
> -static void audit_tree_log_remove_rule(struct audit_krule *rule)
> +static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule)
>  {
>         struct audit_buffer *ab;
>
>         if (!audit_enabled)
>                 return;
> -       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>         if (unlikely(!ab))
>                 return;
>         audit_log_format(ab, "op=remove_rule");
> @@ -510,7 +510,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
>         audit_log_end(ab);
>  }
>
> -static void kill_rules(struct audit_tree *tree)
> +static void kill_rules(struct audit_context *context, struct audit_tree *tree)
>  {
>         struct audit_krule *rule, *next;
>         struct audit_entry *entry;
> @@ -521,7 +521,7 @@ static void kill_rules(struct audit_tree *tree)
>                 list_del_init(&rule->rlist);
>                 if (rule->tree) {
>                         /* not a half-baked one */
> -                       audit_tree_log_remove_rule(rule);
> +                       audit_tree_log_remove_rule(context, rule);
>                         if (entry->rule.exe)
>                                 audit_remove_mark(entry->rule.exe);
>                         rule->tree = NULL;
> @@ -584,7 +584,7 @@ static void trim_marked(struct audit_tree *tree)
>                 tree->goner = 1;
>                 spin_unlock(&hash_lock);
>                 mutex_lock(&audit_filter_mutex);
> -               kill_rules(tree);
> +               kill_rules(audit_context(), tree);
>                 list_del_init(&tree->list);
>                 mutex_unlock(&audit_filter_mutex);
>                 prune_one(tree);
> @@ -924,8 +924,10 @@ static void audit_schedule_prune(void)
>   * ... and that one is done if evict_chunk() decides to delay until the end
>   * of syscall.  Runs synchronously.
>   */
> -void audit_kill_trees(struct list_head *list)
> +void audit_kill_trees(struct audit_context *context)
>  {
> +       struct list_head *list = &context->killed_trees;
> +
>         audit_ctl_lock();
>         mutex_lock(&audit_filter_mutex);
>
> @@ -933,7 +935,7 @@ void audit_kill_trees(struct list_head *list)
>                 struct audit_tree *victim;
>
>                 victim = list_entry(list->next, struct audit_tree, list);
> -               kill_rules(victim);
> +               kill_rules(context, victim);
>                 list_del_init(&victim->list);
>
>                 mutex_unlock(&audit_filter_mutex);
> @@ -972,7 +974,7 @@ static void evict_chunk(struct audit_chunk *chunk)
>                 list_del_init(&owner->same_root);
>                 spin_unlock(&hash_lock);
>                 if (!postponed) {
> -                       kill_rules(owner);
> +                       kill_rules(audit_context(), owner);
>                         list_move(&owner->list, &prune_list);
>                         need_prune = 1;
>                 } else {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ceb1c45..2590c9e 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1490,7 +1490,7 @@ void __audit_free(struct task_struct *tsk)
>         if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
>                 audit_log_exit(context, tsk);
>         if (!list_empty(&context->killed_trees))
> -               audit_kill_trees(&context->killed_trees);
> +               audit_kill_trees(context);

See my comment below about the ordering of audit_kill_trees() and
audit_log_exit().

>         audit_free_context(context);
>  }
> @@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
>         context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
>
>         if (!list_empty(&context->killed_trees))
> -               audit_kill_trees(&context->killed_trees);
> +               audit_kill_trees(context);

I wonder if we should move the kill_trees if-block above the
audit_log_exit() block so that any records that are emitted will be
before the SYSCALL record.  I didn't chase down all the code paths,
but it seems like it should be safe, no?

>         audit_free_names(context);
>         unroll_tree_refs(context, NULL, 0);
> --
> 1.8.3.1

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 5/6] audit: move EOE record after kill_trees for exit/free

[Paul Moore]

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> The EOE record was being issued prior to the pruning of the killed_tree
> list.
>
> Move the EOE record creation out of audit_log_exit() and into its
> callers __audit_free() and __audit_syscall_exit() so that
> audit_kill_trees() can be called prior to the EOE record creation and
> any purged trees CONFIG_CHANGE records included in the syscall record
> event.
>
> See: https://github.com/linux-audit/audit-kernel/issues/50
> See: https://github.com/linux-audit/audit-kernel/issues/59
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/auditsc.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)

See my comments in 4/6.  Assuming we are able to shuffle the ordering
of audit_log_exit() and audit_kill_trees() this patch would no longer
be needed, yes?

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2590c9e..d56aead 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1460,10 +1460,6 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
>
>         audit_log_proctitle(tsk, context);
>
> -       /* Send end of event record to help user space know we are finished */
> -       ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
> -       if (ab)
> -               audit_log_end(ab);
>         if (call_panic)
>                 audit_panic("error converting sid to string");
>  }
> @@ -1491,6 +1487,14 @@ void __audit_free(struct task_struct *tsk)
>                 audit_log_exit(context, tsk);
>         if (!list_empty(&context->killed_trees))
>                 audit_kill_trees(context);
> +       if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
> +               struct audit_buffer *ab;
> +
> +               /* Send end of event record to help user space know we are finished */
> +               ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
> +               if (ab)
> +                       audit_log_end(ab);
> +       }
>
>         audit_free_context(context);
>  }
> @@ -1572,13 +1576,19 @@ void __audit_syscall_exit(int success, long return_code)
>
>         if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
>                 audit_log_exit(context, current);
> +       if (!list_empty(&context->killed_trees))
> +               audit_kill_trees(context);
> +       if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
> +               struct audit_buffer *ab;
>
> +               /* Send end of event record to help user space know we are finished */
> +               ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
> +               if (ab)
> +                       audit_log_end(ab);
> +       }
>         context->in_syscall = 0;
>         context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
>
> -       if (!list_empty(&context->killed_trees))
> -               audit_kill_trees(context);
> -
>         audit_free_names(context);
>         unroll_tree_refs(context, NULL, 0);
>         audit_free_aux(context);
> --
> 1.8.3.1
>


-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 6/6] audit: extend config_change mark/watch/tree rule changes

[Paul Moore]

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Give a clue as to the source of mark, watch and tree rule changes.
>
> See: https://github.com/linux-audit/audit-kernel/issues/50
> See: https://github.com/linux-audit/audit-kernel/issues/59
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.h          |  4 ++--
>  kernel/audit_fsnotify.c |  2 +-
>  kernel/audit_tree.c     | 24 ++++++++++++------------
>  kernel/audit_watch.c    |  6 ++++--
>  kernel/auditsc.c        |  4 ++--
>  5 files changed, 21 insertions(+), 19 deletions(-)

I think having some additional context here would be helpful for
everyone, so I agree with this on principle.  However, I think we need
to get clarification from Steve that his parser is able to handle
these richer "op" values.

> diff --git a/kernel/audit.h b/kernel/audit.h
> index f39f7aa..5e072f5 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -312,7 +312,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
>  extern int audit_tag_tree(char *old, char *new);
>  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 audit_context *context);
> +extern void audit_kill_trees(struct audit_context *context, char *trig);
>  #else
>  #define audit_remove_tree_rule(rule) BUG()
>  #define audit_add_tree_rule(rule) -EINVAL
> @@ -321,7 +321,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
>  #define audit_put_tree(tree) (void)0
>  #define audit_tag_tree(old, new) -EINVAL
>  #define audit_tree_path(rule) ""       /* never called */
> -#define audit_kill_trees(context) BUG()
> +#define audit_kill_trees(context, trig) BUG()
>  #endif
>
>  extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 1640eb6..c10ba91 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -158,7 +158,7 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark)
>         struct audit_krule *rule = audit_mark->rule;
>         struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
>
> -       audit_mark_log_rule_change(audit_mark, "autoremove_rule");
> +       audit_mark_log_rule_change(audit_mark, "autoremove_rule(mark)");
>         audit_del_rule(entry);
>  }
>
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 2d3e1071..1726cfa 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -493,7 +493,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
>         return 0;
>  }
>
> -static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule)
> +static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule, char *trig)
>  {
>         struct audit_buffer *ab;
>
> @@ -502,7 +502,7 @@ static void audit_tree_log_remove_rule(struct audit_context *context, struct aud
>         ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>         if (unlikely(!ab))
>                 return;
> -       audit_log_format(ab, "op=remove_rule");
> +       audit_log_format(ab, "op=remove_rule(tree:%s)", trig);
>         audit_log_format(ab, " dir=");
>         audit_log_untrustedstring(ab, rule->tree->pathname);
>         audit_log_key(ab, rule->filterkey);
> @@ -510,7 +510,7 @@ static void audit_tree_log_remove_rule(struct audit_context *context, struct aud
>         audit_log_end(ab);
>  }
>
> -static void kill_rules(struct audit_context *context, struct audit_tree *tree)
> +static void kill_rules(struct audit_context *context, struct audit_tree *tree, char *trig)
>  {
>         struct audit_krule *rule, *next;
>         struct audit_entry *entry;
> @@ -521,7 +521,7 @@ static void kill_rules(struct audit_context *context, struct audit_tree *tree)
>                 list_del_init(&rule->rlist);
>                 if (rule->tree) {
>                         /* not a half-baked one */
> -                       audit_tree_log_remove_rule(context, rule);
> +                       audit_tree_log_remove_rule(context, rule, trig);
>                         if (entry->rule.exe)
>                                 audit_remove_mark(entry->rule.exe);
>                         rule->tree = NULL;
> @@ -551,7 +551,7 @@ static void prune_one(struct audit_tree *victim)
>
>  /* trim the uncommitted chunks from tree */
>
> -static void trim_marked(struct audit_tree *tree)
> +static void trim_marked(struct audit_tree *tree, char *trig)
>  {
>         struct list_head *p, *q;
>         spin_lock(&hash_lock);
> @@ -584,7 +584,7 @@ static void trim_marked(struct audit_tree *tree)
>                 tree->goner = 1;
>                 spin_unlock(&hash_lock);
>                 mutex_lock(&audit_filter_mutex);
> -               kill_rules(audit_context(), tree);
> +               kill_rules(audit_context(), tree, trig);
>                 list_del_init(&tree->list);
>                 mutex_unlock(&audit_filter_mutex);
>                 prune_one(tree);
> @@ -665,7 +665,7 @@ void audit_trim_trees(void)
>                                 node->index &= ~(1U<<31);
>                 }
>                 spin_unlock(&hash_lock);
> -               trim_marked(tree);
> +               trim_marked(tree, "trim");
>                 drop_collected_mounts(root_mnt);
>  skip_it:
>                 put_tree(tree);
> @@ -798,7 +798,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
>                         node->index &= ~(1U<<31);
>                 spin_unlock(&hash_lock);
>         } else {
> -               trim_marked(tree);
> +               trim_marked(tree, "add");
>                 goto Err;
>         }
>
> @@ -900,7 +900,7 @@ int audit_tag_tree(char *old, char *new)
>                                 node->index &= ~(1U<<31);
>                         spin_unlock(&hash_lock);
>                 } else {
> -                       trim_marked(tree);
> +                       trim_marked(tree, "equiv");
>                 }
>
>                 put_tree(tree);
> @@ -924,7 +924,7 @@ static void audit_schedule_prune(void)
>   * ... and that one is done if evict_chunk() decides to delay until the end
>   * of syscall.  Runs synchronously.
>   */
> -void audit_kill_trees(struct audit_context *context)
> +void audit_kill_trees(struct audit_context *context, char *trig)
>  {
>         struct list_head *list = &context->killed_trees;
>
> @@ -935,7 +935,7 @@ void audit_kill_trees(struct audit_context *context)
>                 struct audit_tree *victim;
>
>                 victim = list_entry(list->next, struct audit_tree, list);
> -               kill_rules(context, victim);
> +               kill_rules(context, victim, trig);
>                 list_del_init(&victim->list);
>
>                 mutex_unlock(&audit_filter_mutex);
> @@ -974,7 +974,7 @@ static void evict_chunk(struct audit_chunk *chunk)
>                 list_del_init(&owner->same_root);
>                 spin_unlock(&hash_lock);
>                 if (!postponed) {
> -                       kill_rules(audit_context(), owner);
> +                       kill_rules(audit_context(), owner, "evict");
>                         list_move(&owner->list, &prune_list);
>                         need_prune = 1;
>                 } else {
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index da2978b..693d0a8 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -317,7 +317,9 @@ static void audit_update_watch(struct audit_parent *parent,
>                         if (oentry->rule.exe)
>                                 audit_remove_mark(oentry->rule.exe);
>
> -                       audit_watch_log_rule_change(r, owatch, "updated_rules");
> +                       audit_watch_log_rule_change(r, owatch, invalidating ?
> +                                                   "updated_rules(watch:inval)" :
> +                                                   "updated_rules(watch:set)");
>
>                         call_rcu(&oentry->rcu, audit_free_rule_rcu);
>                 }
> @@ -345,7 +347,7 @@ static void audit_remove_parent_watches(struct audit_parent *parent)
>         list_for_each_entry_safe(w, nextw, &parent->watches, wlist) {
>                 list_for_each_entry_safe(r, nextr, &w->rules, rlist) {
>                         e = container_of(r, struct audit_entry, rule);
> -                       audit_watch_log_rule_change(r, w, "remove_rule");
> +                       audit_watch_log_rule_change(r, w, "remove_rule(watch:parent)");
>                         if (e->rule.exe)
>                                 audit_remove_mark(e->rule.exe);
>                         list_del(&r->rlist);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d56aead..32428a3 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1486,7 +1486,7 @@ void __audit_free(struct task_struct *tsk)
>         if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
>                 audit_log_exit(context, tsk);
>         if (!list_empty(&context->killed_trees))
> -               audit_kill_trees(context);
> +               audit_kill_trees(context, "free");
>         if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
>                 struct audit_buffer *ab;
>
> @@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
>         if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
>                 audit_log_exit(context, current);
>         if (!list_empty(&context->killed_trees))
> -               audit_kill_trees(context);
> +               audit_kill_trees(context, "exit");
>         if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) {
>                 struct audit_buffer *ab;
>
> --
> 1.8.3.1
>


-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 6/6] audit: extend config_change mark/watch/tree rule changes

[Steve Grubb]

On Thursday, June 28, 2018 6:28:55 PM EDT Paul Moore wrote:
> On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Give a clue as to the source of mark, watch and tree rule changes.
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/50
> > See: https://github.com/linux-audit/audit-kernel/issues/59
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > kernel/audit.h          |  4 ++--
> > kernel/audit_fsnotify.c |  2 +-
> > kernel/audit_tree.c     | 24 ++++++++++++------------
> > kernel/audit_watch.c    |  6 ++++--
> > kernel/auditsc.c        |  4 ++--
> > 5 files changed, 21 insertions(+), 19 deletions(-)
> 
> I think having some additional context here would be helpful for
> everyone, so I agree with this on principle.  However, I think we need
> to get clarification from Steve that his parser is able to handle
> these richer "op" values.

Op fields are not searchable. So, they normally don't matter. But in general, 
once they are defined, they should not change. For the record, you can 
generally insert non-searchable fields anywhere and it doesn't matter. Only 
the searchable fields like loginuid, uid, pid, exe, etc matter to the parser.

-Steve

Re: [RFC PATCH ghak59 V1 3/6] audit: exclude user records from syscall context

[Richard Guy Briggs]

On 2018-06-28 18:11, Paul Moore wrote:
> On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Since the function audit_log_common_recv_msg() is shared by a number of
> > AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
> > and since the AUDIT_CONFIG_CHANGE message type has been converted to a
> > syscall accompanied record type, special-case the AUDIT_USER_* range of
> > messages so they remain standalone records.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/59
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> I think this is fine, but see my previous comment about combining 2/6
> and 3/6 as a safety measure.

This one I left as a seperate patch for discussion.  We'd previously
talked about connecting all possible records with syscall records if
they exist, but this one I'm unsure about, since we don't really care
what userspace process is issuing this message.  It is just the message
content itself that is important.  Or is it?  Are we concerned about
CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
can get in case they go rogue or pear-shaped?

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index e469234..c8c2efc 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1057,7 +1057,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
> >         return err;
> >  }
> >
> > -static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > +static void __audit_log_common_recv_msg(struct audit_context *context,
> > +                                       struct audit_buffer **ab, u16 msg_type)
> >  {
> >         uid_t uid = from_kuid(&init_user_ns, current_uid());
> >         pid_t pid = task_tgid_nr(current);
> > @@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> >                 return;
> >         }
> >
> > -       *ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
> > +       *ab = audit_log_start(context, GFP_KERNEL, msg_type);
> >         if (unlikely(!*ab))
> >                 return;
> >         audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> > @@ -1075,6 +1076,11 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> >         audit_log_task_context(*ab);
> >  }
> >
> > +static inline void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > +{
> > +       __audit_log_common_recv_msg(audit_context(), ab, msg_type);
> > +}
> > +
> >  int is_audit_feature_set(int i)
> >  {
> >         return af.features & AUDIT_FEATURE_TO_MASK(i);
> > @@ -1341,7 +1347,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >                                 if (err)
> >                                         break;
> >                         }
> > -                       audit_log_common_recv_msg(&ab, msg_type);
> > +                       __audit_log_common_recv_msg(NULL, &ab, msg_type);
> >                         if (msg_type != AUDIT_USER_TTY)
> >                                 audit_log_format(ab, " msg='%.*s'",
> >                                                  AUDIT_MESSAGE_TEXT_MAX,
> > --
> > 1.8.3.1
> >
> 
> 
> -- 
> paul moore
> www.paul-moore.com
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

[Richard Guy Briggs]

On 2018-06-28 15:41, Paul Moore wrote:
> On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > The failure to add an audit rule due to audit locked gives no clue
> > what CONFIG_CHANGE operation failed.
> > Similarly the set operation is the only other operation that doesn't
> > give the "op=" field to indicate the action.
> > All other CONFIG_CHANGE records include an op= field to give a clue as
> > to what sort of configuration change is being executed.
> >
> > Since these are the only CONFIG_CHANGE records that that do not have an
> > op= field, add them to bring them in line with the rest.
> 
> Normally this would be an immediate reject because this patch inserts
> a field into an existing record, but the CONFIG_CHANGE record is so
> variable (supposedly bad in its own right) that I don't this really
> matters.
> 
> With that out of the way, I think this patch is fine, but I don't
> think it is complete.  At the very least there is another
> CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
> appear to include an "op" field.  If we want to make sure we have an
> "op" field in every CONFIG_CHANGE record, let's actually add them all
> :)

The version I'm looking at already had it when it was added in 2009.

This one doesn't add the auid and ses fields because they will be
covered by the linking of this record with the syscall record via the
audit_context() introduced in another patch.

> There appears to be another one in audit_mark_log_rule_change() ...

Same, 2015.

> and one more in audit_receive_msg().  There may be more.

I believe they're covered by other patches in the ghak59 set.

> > Old records:
> > type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0
> > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> >
> > New records:
> > type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0
> >
> > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/59
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index e7478cb..ad54339 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -403,7 +403,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
> >         ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> >         if (unlikely(!ab))
> >                 return rc;
> > -       audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> > +       audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
> >         audit_log_session_info(ab);
> >         rc = audit_log_task_context(ab);
> >         if (rc)
> > @@ -1365,7 +1365,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >                         return -EINVAL;
> >                 if (audit_enabled == AUDIT_LOCKED) {
> >                         audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> > -                       audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
> > +                       audit_log_format(ab, " op=%s_rule audit_enabled=%d res=0",
> > +                                        msg_type == AUDIT_ADD_RULE ? "add" : "remove",
> > +                                        audit_enabled);
> >                         audit_log_end(ab);
> >                         return -EPERM;
> >                 }
> > --
> > 1.8.3.1
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak59 V1 4/6] audit: hand taken context to audit_kill_trees for syscall logging

[Richard Guy Briggs]

On 2018-06-28 18:23, Paul Moore wrote:
> On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Since the context is taken from the task in __audit_syscall_exit() and
> > __audit_free(), hand it to audit_kill_trees() so it can be used to
> > associate with a syscall record.  This requires adding the context
> > parameter to kill_rules() rather than using the current audit_context
> > (which has been taken).
> >
> > The callers of trim_marked() and evict_chunk() still have their context.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/50
> > See: https://github.com/linux-audit/audit-kernel/issues/59
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.h      |  4 ++--
> >  kernel/audit_tree.c | 18 ++++++++++--------
> >  kernel/auditsc.c    |  4 ++--
> >  3 files changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 214e149..f39f7aa 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -312,7 +312,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
> >  extern int audit_tag_tree(char *old, char *new);
> >  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);
> > +extern void audit_kill_trees(struct audit_context *context);
> >  #else
> >  #define audit_remove_tree_rule(rule) BUG()
> >  #define audit_add_tree_rule(rule) -EINVAL
> > @@ -321,7 +321,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
> >  #define audit_put_tree(tree) (void)0
> >  #define audit_tag_tree(old, new) -EINVAL
> >  #define audit_tree_path(rule) ""       /* never called */
> > -#define audit_kill_trees(list) BUG()
> > +#define audit_kill_trees(context) BUG()
> >  #endif
> >
> >  extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index a01b9da..2d3e1071 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -493,13 +493,13 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
> >         return 0;
> >  }
> >
> > -static void audit_tree_log_remove_rule(struct audit_krule *rule)
> > +static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule *rule)
> >  {
> >         struct audit_buffer *ab;
> >
> >         if (!audit_enabled)
> >                 return;
> > -       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> >         if (unlikely(!ab))
> >                 return;
> >         audit_log_format(ab, "op=remove_rule");
> > @@ -510,7 +510,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
> >         audit_log_end(ab);
> >  }
> >
> > -static void kill_rules(struct audit_tree *tree)
> > +static void kill_rules(struct audit_context *context, struct audit_tree *tree)
> >  {
> >         struct audit_krule *rule, *next;
> >         struct audit_entry *entry;
> > @@ -521,7 +521,7 @@ static void kill_rules(struct audit_tree *tree)
> >                 list_del_init(&rule->rlist);
> >                 if (rule->tree) {
> >                         /* not a half-baked one */
> > -                       audit_tree_log_remove_rule(rule);
> > +                       audit_tree_log_remove_rule(context, rule);
> >                         if (entry->rule.exe)
> >                                 audit_remove_mark(entry->rule.exe);
> >                         rule->tree = NULL;
> > @@ -584,7 +584,7 @@ static void trim_marked(struct audit_tree *tree)
> >                 tree->goner = 1;
> >                 spin_unlock(&hash_lock);
> >                 mutex_lock(&audit_filter_mutex);
> > -               kill_rules(tree);
> > +               kill_rules(audit_context(), tree);
> >                 list_del_init(&tree->list);
> >                 mutex_unlock(&audit_filter_mutex);
> >                 prune_one(tree);
> > @@ -924,8 +924,10 @@ static void audit_schedule_prune(void)
> >   * ... and that one is done if evict_chunk() decides to delay until the end
> >   * of syscall.  Runs synchronously.
> >   */
> > -void audit_kill_trees(struct list_head *list)
> > +void audit_kill_trees(struct audit_context *context)
> >  {
> > +       struct list_head *list = &context->killed_trees;
> > +
> >         audit_ctl_lock();
> >         mutex_lock(&audit_filter_mutex);
> >
> > @@ -933,7 +935,7 @@ void audit_kill_trees(struct list_head *list)
> >                 struct audit_tree *victim;
> >
> >                 victim = list_entry(list->next, struct audit_tree, list);
> > -               kill_rules(victim);
> > +               kill_rules(context, victim);
> >                 list_del_init(&victim->list);
> >
> >                 mutex_unlock(&audit_filter_mutex);
> > @@ -972,7 +974,7 @@ static void evict_chunk(struct audit_chunk *chunk)
> >                 list_del_init(&owner->same_root);
> >                 spin_unlock(&hash_lock);
> >                 if (!postponed) {
> > -                       kill_rules(owner);
> > +                       kill_rules(audit_context(), owner);
> >                         list_move(&owner->list, &prune_list);
> >                         need_prune = 1;
> >                 } else {
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index ceb1c45..2590c9e 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1490,7 +1490,7 @@ void __audit_free(struct task_struct *tsk)
> >         if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
> >                 audit_log_exit(context, tsk);
> >         if (!list_empty(&context->killed_trees))
> > -               audit_kill_trees(&context->killed_trees);
> > +               audit_kill_trees(context);
> 
> See my comment below about the ordering of audit_kill_trees() and
> audit_log_exit().
> 
> >         audit_free_context(context);
> >  }
> > @@ -1577,7 +1577,7 @@ void __audit_syscall_exit(int success, long return_code)
> >         context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
> >
> >         if (!list_empty(&context->killed_trees))
> > -               audit_kill_trees(&context->killed_trees);
> > +               audit_kill_trees(context);
> 
> I wonder if we should move the kill_trees if-block above the
> audit_log_exit() block so that any records that are emitted will be
> before the SYSCALL record.  I didn't chase down all the code paths,
> but it seems like it should be safe, no?

Interesting.  I thought I had looked at re-ordering them and rejected
that approach due to that information being needed for audit_log_exit(),
but I don't find any such dependency this pass through the code.

I guess the only concern I have then is that if the state is anything
other than AUDIT_RECORD_CONTEXT it would be an orphan record, but it
would be regardless with the existing code or with my proposed changes.
Perhaps that is a bug to start with, though I'm not sure it is at all
serious, so I'm not concerned about it.

I think re-ordering should be safe and that eliminates the seeming
complexity introduced by the next patch, which is a good thing.

> >         audit_free_names(context);
> >         unroll_tree_refs(context, NULL, 0);
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

[Paul Moore]

On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-06-28 15:41, Paul Moore wrote:
> > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > The failure to add an audit rule due to audit locked gives no clue
> > > what CONFIG_CHANGE operation failed.
> > > Similarly the set operation is the only other operation that doesn't
> > > give the "op=" field to indicate the action.
> > > All other CONFIG_CHANGE records include an op= field to give a clue as
> > > to what sort of configuration change is being executed.
> > >
> > > Since these are the only CONFIG_CHANGE records that that do not have an
> > > op= field, add them to bring them in line with the rest.
> >
> > Normally this would be an immediate reject because this patch inserts
> > a field into an existing record, but the CONFIG_CHANGE record is so
> > variable (supposedly bad in its own right) that I don't this really
> > matters.
> >
> > With that out of the way, I think this patch is fine, but I don't
> > think it is complete.  At the very least there is another
> > CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
> > appear to include an "op" field.  If we want to make sure we have an
> > "op" field in every CONFIG_CHANGE record, let's actually add them all
> > :)
>
> The version I'm looking at already had it when it was added in 2009.

Yup, there it is ... now I'm wondering what tree I was looking at as a
reference while reviewing this?

/me scratches head

> This one doesn't add the auid and ses fields because they will be
> covered by the linking of this record with the syscall record via the
> audit_context() introduced in another patch.

Yeah, I'm not concerned about that for the reasons you state.

> > and one more in audit_receive_msg().  There may be more.
>
> I believe they're covered by other patches in the ghak59 set.

If they are in the later patches it might be good to move those "op="
additions into this patch.

> > > Old records:
> > > type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0
> > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> > >
> > > New records:
> > > type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0
> > >
> > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  kernel/audit.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index e7478cb..ad54339 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -403,7 +403,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
> > >         ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > >         if (unlikely(!ab))
> > >                 return rc;
> > > -       audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> > > +       audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
> > >         audit_log_session_info(ab);
> > >         rc = audit_log_task_context(ab);
> > >         if (rc)
> > > @@ -1365,7 +1365,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > >                         return -EINVAL;
> > >                 if (audit_enabled == AUDIT_LOCKED) {
> > >                         audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> > > -                       audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
> > > +                       audit_log_format(ab, " op=%s_rule audit_enabled=%d res=0",
> > > +                                        msg_type == AUDIT_ADD_RULE ? "add" : "remove",
> > > +                                        audit_enabled);
> > >                         audit_log_end(ab);
> > >                         return -EPERM;
> > >                 }
> > > --
> > > 1.8.3.1
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635



-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

[Richard Guy Briggs]

On 2018-07-18 17:45, Paul Moore wrote:
> On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-06-28 15:41, Paul Moore wrote:
> > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > The failure to add an audit rule due to audit locked gives no clue
> > > > what CONFIG_CHANGE operation failed.
> > > > Similarly the set operation is the only other operation that doesn't
> > > > give the "op=" field to indicate the action.
> > > > All other CONFIG_CHANGE records include an op= field to give a clue as
> > > > to what sort of configuration change is being executed.
> > > >
> > > > Since these are the only CONFIG_CHANGE records that that do not have an
> > > > op= field, add them to bring them in line with the rest.
> > >
> > > Normally this would be an immediate reject because this patch inserts
> > > a field into an existing record, but the CONFIG_CHANGE record is so
> > > variable (supposedly bad in its own right) that I don't this really
> > > matters.
> > >
> > > With that out of the way, I think this patch is fine, but I don't
> > > think it is complete.  At the very least there is another
> > > CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
> > > appear to include an "op" field.  If we want to make sure we have an
> > > "op" field in every CONFIG_CHANGE record, let's actually add them all
> > > :)
> >
> > The version I'm looking at already had it when it was added in 2009.
> 
> Yup, there it is ... now I'm wondering what tree I was looking at as a
> reference while reviewing this?
> 
> /me scratches head
> 
> > This one doesn't add the auid and ses fields because they will be
> > covered by the linking of this record with the syscall record via the
> > audit_context() introduced in another patch.
> 
> Yeah, I'm not concerned about that for the reasons you state.
> 
> > > and one more in audit_receive_msg().  There may be more.
> >
> > I believe they're covered by other patches in the ghak59 set.
> 
> If they are in the later patches it might be good to move those "op="
> additions into this patch.

I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
that are missing op= field.  Can you narrow it down?

> > > > Old records:
> > > > type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0
> > > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> > > >
> > > > New records:
> > > > type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0
> > > >
> > > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  kernel/audit.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index e7478cb..ad54339 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -403,7 +403,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
> > > >         ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > > >         if (unlikely(!ab))
> > > >                 return rc;
> > > > -       audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> > > > +       audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
> > > >         audit_log_session_info(ab);
> > > >         rc = audit_log_task_context(ab);
> > > >         if (rc)
> > > > @@ -1365,7 +1365,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > > >                         return -EINVAL;
> > > >                 if (audit_enabled == AUDIT_LOCKED) {
> > > >                         audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> > > > -                       audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
> > > > +                       audit_log_format(ab, " op=%s_rule audit_enabled=%d res=0",
> > > > +                                        msg_type == AUDIT_ADD_RULE ? "add" : "remove",
> > > > +                                        audit_enabled);
> > > >                         audit_log_end(ab);
> > > >                         return -EPERM;
> > > >                 }
> > > > --
> > > > 1.8.3.1
> > >
> > > --
> > > paul moore
> > > www.paul-moore.com
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> 
> 
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

[Paul Moore]

On Thu, Jul 19, 2018 at 12:10 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-18 17:45, Paul Moore wrote:
> > On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2018-06-28 15:41, Paul Moore wrote:
> > > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > The failure to add an audit rule due to audit locked gives no clue
> > > > > what CONFIG_CHANGE operation failed.
> > > > > Similarly the set operation is the only other operation that doesn't
> > > > > give the "op=" field to indicate the action.
> > > > > All other CONFIG_CHANGE records include an op= field to give a clue as
> > > > > to what sort of configuration change is being executed.
> > > > >
> > > > > Since these are the only CONFIG_CHANGE records that that do not have an
> > > > > op= field, add them to bring them in line with the rest.
> > > >
> > > > Normally this would be an immediate reject because this patch inserts
> > > > a field into an existing record, but the CONFIG_CHANGE record is so
> > > > variable (supposedly bad in its own right) that I don't this really
> > > > matters.
> > > >
> > > > With that out of the way, I think this patch is fine, but I don't
> > > > think it is complete.  At the very least there is another
> > > > CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
> > > > appear to include an "op" field.  If we want to make sure we have an
> > > > "op" field in every CONFIG_CHANGE record, let's actually add them all
> > > > :)
> > >
> > > The version I'm looking at already had it when it was added in 2009.
> >
> > Yup, there it is ... now I'm wondering what tree I was looking at as a
> > reference while reviewing this?
> >
> > /me scratches head
> >
> > > This one doesn't add the auid and ses fields because they will be
> > > covered by the linking of this record with the syscall record via the
> > > audit_context() introduced in another patch.
> >
> > Yeah, I'm not concerned about that for the reasons you state.
> >
> > > > and one more in audit_receive_msg().  There may be more.
> > >
> > > I believe they're covered by other patches in the ghak59 set.
> >
> > If they are in the later patches it might be good to move those "op="
> > additions into this patch.
>
> I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
> that are missing op= field.  Can you narrow it down?

Well, just grep'ing my way through audit_receive_msg() I see that
AUDIT_ADD/DEL_RULE generates a CONFIG_CHANGE record.

> > > > > Old records:
> > > > > type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0
> > > > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> > > > >
> > > > > New records:
> > > > > type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0
> > > > >
> > > > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > >  kernel/audit.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > index e7478cb..ad54339 100644
> > > > > --- a/kernel/audit.c
> > > > > +++ b/kernel/audit.c
> > > > > @@ -403,7 +403,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
> > > > >         ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > > > >         if (unlikely(!ab))
> > > > >                 return rc;
> > > > > -       audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> > > > > +       audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
> > > > >         audit_log_session_info(ab);
> > > > >         rc = audit_log_task_context(ab);
> > > > >         if (rc)
> > > > > @@ -1365,7 +1365,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > > > >                         return -EINVAL;
> > > > >                 if (audit_enabled == AUDIT_LOCKED) {
> > > > >                         audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> > > > > -                       audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
> > > > > +                       audit_log_format(ab, " op=%s_rule audit_enabled=%d res=0",
> > > > > +                                        msg_type == AUDIT_ADD_RULE ? "add" : "remove",
> > > > > +                                        audit_enabled);
> > > > >                         audit_log_end(ab);
> > > > >                         return -EPERM;
> > > > >                 }
> > > > > --
> > > > > 1.8.3.1
> > > >
> > > > --
> > > > paul moore
> > > > www.paul-moore.com
> > >
> > > - RGB
> > >
> > > --
> > > Richard Guy Briggs <rgb@redhat.com>
> > > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > > Remote, Ottawa, Red Hat Canada
> > > IRC: rgb, SunRaycer
> > > Voice: +1.647.777.2635, Internal: (81) 32635
> >
> >
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635



-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

[Richard Guy Briggs]

On 2018-07-19 18:47, Paul Moore wrote:
> On Thu, Jul 19, 2018 at 12:10 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-07-18 17:45, Paul Moore wrote:
> > > On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > On 2018-06-28 15:41, Paul Moore wrote:
> > > > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > > The failure to add an audit rule due to audit locked gives no clue
> > > > > > what CONFIG_CHANGE operation failed.
> > > > > > Similarly the set operation is the only other operation that doesn't
> > > > > > give the "op=" field to indicate the action.
> > > > > > All other CONFIG_CHANGE records include an op= field to give a clue as
> > > > > > to what sort of configuration change is being executed.
> > > > > >
> > > > > > Since these are the only CONFIG_CHANGE records that that do not have an
> > > > > > op= field, add them to bring them in line with the rest.
> > > > >
> > > > > Normally this would be an immediate reject because this patch inserts
> > > > > a field into an existing record, but the CONFIG_CHANGE record is so
> > > > > variable (supposedly bad in its own right) that I don't this really
> > > > > matters.
> > > > >
> > > > > With that out of the way, I think this patch is fine, but I don't
> > > > > think it is complete.  At the very least there is another
> > > > > CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
> > > > > appear to include an "op" field.  If we want to make sure we have an
> > > > > "op" field in every CONFIG_CHANGE record, let's actually add them all
> > > > > :)
> > > >
> > > > The version I'm looking at already had it when it was added in 2009.
> > >
> > > Yup, there it is ... now I'm wondering what tree I was looking at as a
> > > reference while reviewing this?
> > >
> > > /me scratches head
> > >
> > > > This one doesn't add the auid and ses fields because they will be
> > > > covered by the linking of this record with the syscall record via the
> > > > audit_context() introduced in another patch.
> > >
> > > Yeah, I'm not concerned about that for the reasons you state.
> > >
> > > > > and one more in audit_receive_msg().  There may be more.
> > > >
> > > > I believe they're covered by other patches in the ghak59 set.
> > >
> > > If they are in the later patches it might be good to move those "op="
> > > additions into this patch.
> >
> > I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
> > that are missing op= field.  Can you narrow it down?
> 
> Well, just grep'ing my way through audit_receive_msg() I see that
> AUDIT_ADD/DEL_RULE generates a CONFIG_CHANGE record.

The failure case is addressed in this patch.  The success case is
addressed in audit_log_rule_change().  The latter already has it.  What
is the problem?  What tree are you looking at?  What am I missing?

> > > > > > Old records:
> > > > > > type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0
> > > > > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> > > > > >
> > > > > > New records:
> > > > > > type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0
> > > > > >
> > > > > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes
> > > > > >
> > > > > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > ---
> > > > > >  kernel/audit.c | 6 ++++--
> > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > > index e7478cb..ad54339 100644
> > > > > > --- a/kernel/audit.c
> > > > > > +++ b/kernel/audit.c
> > > > > > @@ -403,7 +403,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
> > > > > >         ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> > > > > >         if (unlikely(!ab))
> > > > > >                 return rc;
> > > > > > -       audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> > > > > > +       audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old);
> > > > > >         audit_log_session_info(ab);
> > > > > >         rc = audit_log_task_context(ab);
> > > > > >         if (rc)
> > > > > > @@ -1365,7 +1365,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > > > > >                         return -EINVAL;
> > > > > >                 if (audit_enabled == AUDIT_LOCKED) {
> > > > > >                         audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
> > > > > > -                       audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
> > > > > > +                       audit_log_format(ab, " op=%s_rule audit_enabled=%d res=0",
> > > > > > +                                        msg_type == AUDIT_ADD_RULE ? "add" : "remove",
> > > > > > +                                        audit_enabled);
> > > > > >                         audit_log_end(ab);
> > > > > >                         return -EPERM;
> > > > > >                 }
> > > > > > --
> > > > > > 1.8.3.1
> > > > >
> > > > > --
> > > > > paul moore
> > > > > www.paul-moore.com
> > > >
> > > > - RGB
> > > >
> > > > --
> > > > Richard Guy Briggs <rgb@redhat.com>
> > > > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > > > Remote, Ottawa, Red Hat Canada
> > > > IRC: rgb, SunRaycer
> > > > Voice: +1.647.777.2635, Internal: (81) 32635
> > >
> > >
> > >
> > > --
> > > paul moore
> > > www.paul-moore.com
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> 
> 
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

[Paul Moore]

On Fri, Jul 20, 2018 at 9:30 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-19 18:47, Paul Moore wrote:
> > On Thu, Jul 19, 2018 at 12:10 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2018-07-18 17:45, Paul Moore wrote:
> > > > On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > On 2018-06-28 15:41, Paul Moore wrote:
> > > > > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > > > The failure to add an audit rule due to audit locked gives no clue
> > > > > > > what CONFIG_CHANGE operation failed.
> > > > > > > Similarly the set operation is the only other operation that doesn't
> > > > > > > give the "op=" field to indicate the action.
> > > > > > > All other CONFIG_CHANGE records include an op= field to give a clue as
> > > > > > > to what sort of configuration change is being executed.
> > > > > > >
> > > > > > > Since these are the only CONFIG_CHANGE records that that do not have an
> > > > > > > op= field, add them to bring them in line with the rest.
> > > > > >
> > > > > > Normally this would be an immediate reject because this patch inserts
> > > > > > a field into an existing record, but the CONFIG_CHANGE record is so
> > > > > > variable (supposedly bad in its own right) that I don't this really
> > > > > > matters.
> > > > > >
> > > > > > With that out of the way, I think this patch is fine, but I don't
> > > > > > think it is complete.  At the very least there is another
> > > > > > CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't
> > > > > > appear to include an "op" field.  If we want to make sure we have an
> > > > > > "op" field in every CONFIG_CHANGE record, let's actually add them all
> > > > > > :)
> > > > >
> > > > > The version I'm looking at already had it when it was added in 2009.
> > > >
> > > > Yup, there it is ... now I'm wondering what tree I was looking at as a
> > > > reference while reviewing this?
> > > >
> > > > /me scratches head
> > > >
> > > > > This one doesn't add the auid and ses fields because they will be
> > > > > covered by the linking of this record with the syscall record via the
> > > > > audit_context() introduced in another patch.
> > > >
> > > > Yeah, I'm not concerned about that for the reasons you state.
> > > >
> > > > > > and one more in audit_receive_msg().  There may be more.
> > > > >
> > > > > I believe they're covered by other patches in the ghak59 set.
> > > >
> > > > If they are in the later patches it might be good to move those "op="
> > > > additions into this patch.
> > >
> > > I don't see any CONFIG_CHANGE records generated in audit_receive_msg()
> > > that are missing op= field.  Can you narrow it down?
> >
> > Well, just grep'ing my way through audit_receive_msg() I see that
> > AUDIT_ADD/DEL_RULE generates a CONFIG_CHANGE record.
>
> The failure case is addressed in this patch.  The success case is
> addressed in audit_log_rule_change().  The latter already has it.  What
> is the problem?  What tree are you looking at?  What am I missing?

So it does.  This discussion dragged out long enough that I forgot to
check the actual patch submission.

I think this patch is fine, I would recommend updating this patchset
using the feedback on the other individual patches and resubmitting.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 3/6] audit: exclude user records from syscall context

[Richard Guy Briggs]

On 2018-07-12 17:46, Richard Guy Briggs wrote:
> On 2018-06-28 18:11, Paul Moore wrote:
> > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Since the function audit_log_common_recv_msg() is shared by a number of
> > > AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
> > > and since the AUDIT_CONFIG_CHANGE message type has been converted to a
> > > syscall accompanied record type, special-case the AUDIT_USER_* range of
> > > messages so they remain standalone records.
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  kernel/audit.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > I think this is fine, but see my previous comment about combining 2/6
> > and 3/6 as a safety measure.
> 
> This one I left as a seperate patch for discussion.  We'd previously
> talked about connecting all possible records with syscall records if
> they exist, but this one I'm unsure about, since we don't really care
> what userspace process is issuing this message.  It is just the message
> content itself that is important.  Or is it?  Are we concerned about
> CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
> can get in case they go rogue or pear-shaped?

I'm waiting on re-spinning this patchset because of this open question.

Is connecting AUDIT_USER* records desirable or a liability?

> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index e469234..c8c2efc 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -1057,7 +1057,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
> > >         return err;
> > >  }
> > >
> > > -static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > +static void __audit_log_common_recv_msg(struct audit_context *context,
> > > +                                       struct audit_buffer **ab, u16 msg_type)
> > >  {
> > >         uid_t uid = from_kuid(&init_user_ns, current_uid());
> > >         pid_t pid = task_tgid_nr(current);
> > > @@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > >                 return;
> > >         }
> > >
> > > -       *ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
> > > +       *ab = audit_log_start(context, GFP_KERNEL, msg_type);
> > >         if (unlikely(!*ab))
> > >                 return;
> > >         audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> > > @@ -1075,6 +1076,11 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > >         audit_log_task_context(*ab);
> > >  }
> > >
> > > +static inline void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > +{
> > > +       __audit_log_common_recv_msg(audit_context(), ab, msg_type);
> > > +}
> > > +
> > >  int is_audit_feature_set(int i)
> > >  {
> > >         return af.features & AUDIT_FEATURE_TO_MASK(i);
> > > @@ -1341,7 +1347,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > >                                 if (err)
> > >                                         break;
> > >                         }
> > > -                       audit_log_common_recv_msg(&ab, msg_type);
> > > +                       __audit_log_common_recv_msg(NULL, &ab, msg_type);
> > >                         if (msg_type != AUDIT_USER_TTY)
> > >                                 audit_log_format(ab, " msg='%.*s'",
> > >                                                  AUDIT_MESSAGE_TEXT_MAX,
> > > --
> > > 1.8.3.1
> > >
> > 
> > 
> > -- 
> > paul moore
> > www.paul-moore.com
> > 
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
> 
> - RGB
> 
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak59 V1 3/6] audit: exclude user records from syscall context

[Paul Moore]

On Mon, Jul 23, 2018 at 12:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-12 17:46, Richard Guy Briggs wrote:
> > On 2018-06-28 18:11, Paul Moore wrote:
> > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Since the function audit_log_common_recv_msg() is shared by a number of
> > > > AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
> > > > and since the AUDIT_CONFIG_CHANGE message type has been converted to a
> > > > syscall accompanied record type, special-case the AUDIT_USER_* range of
> > > > messages so they remain standalone records.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  kernel/audit.c | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > I think this is fine, but see my previous comment about combining 2/6
> > > and 3/6 as a safety measure.
> >
> > This one I left as a seperate patch for discussion.  We'd previously
> > talked about connecting all possible records with syscall records if
> > they exist, but this one I'm unsure about, since we don't really care
> > what userspace process is issuing this message.  It is just the message
> > content itself that is important.  Or is it?  Are we concerned about
> > CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
> > can get in case they go rogue or pear-shaped?
>
> I'm waiting on re-spinning this patchset because of this open question.
>
> Is connecting AUDIT_USER* records desirable or a liability?

Like I said, I think special casing the AUDIT_USER* records so they
are *not* associated with other records is okay, and perhaps even the
right thing to do.  The problem is that we don't have the necessary
context (har har) to match any kernel events (and there is the
possibility that there are none) to the userspace generated
AUDIT_USER* event.  Further, I don't think this is something we would
ever be able to solve in a reasonable manner.

> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index e469234..c8c2efc 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -1057,7 +1057,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
> > > >         return err;
> > > >  }
> > > >
> > > > -static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > > +static void __audit_log_common_recv_msg(struct audit_context *context,
> > > > +                                       struct audit_buffer **ab, u16 msg_type)
> > > >  {
> > > >         uid_t uid = from_kuid(&init_user_ns, current_uid());
> > > >         pid_t pid = task_tgid_nr(current);
> > > > @@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > >                 return;
> > > >         }
> > > >
> > > > -       *ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
> > > > +       *ab = audit_log_start(context, GFP_KERNEL, msg_type);
> > > >         if (unlikely(!*ab))
> > > >                 return;
> > > >         audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> > > > @@ -1075,6 +1076,11 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > >         audit_log_task_context(*ab);
> > > >  }
> > > >
> > > > +static inline void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > > +{
> > > > +       __audit_log_common_recv_msg(audit_context(), ab, msg_type);
> > > > +}
> > > > +
> > > >  int is_audit_feature_set(int i)
> > > >  {
> > > >         return af.features & AUDIT_FEATURE_TO_MASK(i);
> > > > @@ -1341,7 +1347,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > > >                                 if (err)
> > > >                                         break;
> > > >                         }
> > > > -                       audit_log_common_recv_msg(&ab, msg_type);
> > > > +                       __audit_log_common_recv_msg(NULL, &ab, msg_type);
> > > >                         if (msg_type != AUDIT_USER_TTY)
> > > >                                 audit_log_format(ab, " msg='%.*s'",
> > > >                                                  AUDIT_MESSAGE_TEXT_MAX,
> > > > --
> > > > 1.8.3.1
> > > >
> > >
> > >
> > > --
> > > paul moore
> > > www.paul-moore.com
> > >
> > > --
> > > Linux-audit mailing list
> > > Linux-audit@redhat.com
> > > https://www.redhat.com/mailman/listinfo/linux-audit
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635



-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak59 V1 3/6] audit: exclude user records from syscall context

[Richard Guy Briggs]

On 2018-07-23 17:00, Paul Moore wrote:
> On Mon, Jul 23, 2018 at 12:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-07-12 17:46, Richard Guy Briggs wrote:
> > > On 2018-06-28 18:11, Paul Moore wrote:
> > > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > Since the function audit_log_common_recv_msg() is shared by a number of
> > > > > AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
> > > > > and since the AUDIT_CONFIG_CHANGE message type has been converted to a
> > > > > syscall accompanied record type, special-case the AUDIT_USER_* range of
> > > > > messages so they remain standalone records.
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > >  kernel/audit.c | 12 +++++++++---
> > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > >
> > > > I think this is fine, but see my previous comment about combining 2/6
> > > > and 3/6 as a safety measure.
> > >
> > > This one I left as a seperate patch for discussion.  We'd previously
> > > talked about connecting all possible records with syscall records if
> > > they exist, but this one I'm unsure about, since we don't really care
> > > what userspace process is issuing this message.  It is just the message
> > > content itself that is important.  Or is it?  Are we concerned about
> > > CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
> > > can get in case they go rogue or pear-shaped?
> >
> > I'm waiting on re-spinning this patchset because of this open question.
> >
> > Is connecting AUDIT_USER* records desirable or a liability?
> 
> Like I said, I think special casing the AUDIT_USER* records so they
> are *not* associated with other records is okay, and perhaps even the
> right thing to do.  The problem is that we don't have the necessary
> context (har har) to match any kernel events (and there is the
> possibility that there are none) to the userspace generated
> AUDIT_USER* event.  Further, I don't think this is something we would
> ever be able to solve in a reasonable manner.

Ok, having said that, I think I'd still prefer to keep this patch
seperate, partly to retain the simplicity of the previous patch and make
very clear what each one is doing, and partly if we decide to change our
mind in the future that these AUDIT_USER* records should be autonomous.

> > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > index e469234..c8c2efc 100644
> > > > > --- a/kernel/audit.c
> > > > > +++ b/kernel/audit.c
> > > > > @@ -1057,7 +1057,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
> > > > >         return err;
> > > > >  }
> > > > >
> > > > > -static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > > > +static void __audit_log_common_recv_msg(struct audit_context *context,
> > > > > +                                       struct audit_buffer **ab, u16 msg_type)
> > > > >  {
> > > > >         uid_t uid = from_kuid(&init_user_ns, current_uid());
> > > > >         pid_t pid = task_tgid_nr(current);
> > > > > @@ -1067,7 +1068,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > > >                 return;
> > > > >         }
> > > > >
> > > > > -       *ab = audit_log_start(audit_context(), GFP_KERNEL, msg_type);
> > > > > +       *ab = audit_log_start(context, GFP_KERNEL, msg_type);
> > > > >         if (unlikely(!*ab))
> > > > >                 return;
> > > > >         audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> > > > > @@ -1075,6 +1076,11 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > > >         audit_log_task_context(*ab);
> > > > >  }
> > > > >
> > > > > +static inline void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> > > > > +{
> > > > > +       __audit_log_common_recv_msg(audit_context(), ab, msg_type);
> > > > > +}
> > > > > +
> > > > >  int is_audit_feature_set(int i)
> > > > >  {
> > > > >         return af.features & AUDIT_FEATURE_TO_MASK(i);
> > > > > @@ -1341,7 +1347,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > > > >                                 if (err)
> > > > >                                         break;
> > > > >                         }
> > > > > -                       audit_log_common_recv_msg(&ab, msg_type);
> > > > > +                       __audit_log_common_recv_msg(NULL, &ab, msg_type);
> > > > >                         if (msg_type != AUDIT_USER_TTY)
> > > > >                                 audit_log_format(ab, " msg='%.*s'",
> > > > >                                                  AUDIT_MESSAGE_TEXT_MAX,
> > > > > --
> > > > > 1.8.3.1
> > > > >
> > > >
> > > >
> > > > --
> > > > paul moore
> > > > www.paul-moore.com
> > > >
> > > > --
> > > > Linux-audit mailing list
> > > > Linux-audit@redhat.com
> > > > https://www.redhat.com/mailman/listinfo/linux-audit
> > >
> > > - RGB
> > >
> > > --
> > > Richard Guy Briggs <rgb@redhat.com>
> > > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > > Remote, Ottawa, Red Hat Canada
> > > IRC: rgb, SunRaycer
> > > Voice: +1.647.777.2635, Internal: (81) 32635
> > >
> > > --
> > > Linux-audit mailing list
> > > Linux-audit@redhat.com
> > > https://www.redhat.com/mailman/listinfo/linux-audit
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> 
> 
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak59 V1 3/6] audit: exclude user records from syscall context

[Paul Moore]

On Tue, Jul 24, 2018 at 9:05 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-23 17:00, Paul Moore wrote:
> > On Mon, Jul 23, 2018 at 12:43 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2018-07-12 17:46, Richard Guy Briggs wrote:
> > > > On 2018-06-28 18:11, Paul Moore wrote:
> > > > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > > Since the function audit_log_common_recv_msg() is shared by a number of
> > > > > > AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
> > > > > > and since the AUDIT_CONFIG_CHANGE message type has been converted to a
> > > > > > syscall accompanied record type, special-case the AUDIT_USER_* range of
> > > > > > messages so they remain standalone records.
> > > > > >
> > > > > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > ---
> > > > > >  kernel/audit.c | 12 +++++++++---
> > > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > >
> > > > > I think this is fine, but see my previous comment about combining 2/6
> > > > > and 3/6 as a safety measure.
> > > >
> > > > This one I left as a seperate patch for discussion.  We'd previously
> > > > talked about connecting all possible records with syscall records if
> > > > they exist, but this one I'm unsure about, since we don't really care
> > > > what userspace process is issuing this message.  It is just the message
> > > > content itself that is important.  Or is it?  Are we concerned about
> > > > CAP_AUDIT_WRITE holders/abusers and want as much info about them as we
> > > > can get in case they go rogue or pear-shaped?
> > >
> > > I'm waiting on re-spinning this patchset because of this open question.
> > >
> > > Is connecting AUDIT_USER* records desirable or a liability?
> >
> > Like I said, I think special casing the AUDIT_USER* records so they
> > are *not* associated with other records is okay, and perhaps even the
> > right thing to do.  The problem is that we don't have the necessary
> > context (har har) to match any kernel events (and there is the
> > possibility that there are none) to the userspace generated
> > AUDIT_USER* event.  Further, I don't think this is something we would
> > ever be able to solve in a reasonable manner.
>
> Ok, having said that, I think I'd still prefer to keep this patch
> seperate, partly to retain the simplicity of the previous patch and make
> very clear what each one is doing, and partly if we decide to change our
> mind in the future that these AUDIT_USER* records should be autonomous.

Okay, I'll buy that argument.

-- 
paul moore
www.paul-moore.com