On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > 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

On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2018-06-28 15:41, Paul Moore wrote: > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > > > --- > > > 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(a)redhat.com&gt; > 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

On Thu, Jul 19, 2018 at 12:10 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2018-07-18 17:45, Paul Moore wrote: > > On Thu, Jul 12, 2018 at 8:43 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > > On 2018-06-28 15:41, Paul Moore wrote: > > > > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > > > > > --- > > > > > 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(a)redhat.com&gt; > > > 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(a)redhat.com&gt; > 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

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > 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

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(a)redhat.com&gt; --- 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

On 2018-06-28 18:11, Paul Moore wrote: > On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > > --- > > 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com https://www.redhat.com/mailman/listinfo/linux-audit

On Mon, Jul 23, 2018 at 12:43 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; 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(a)redhat.com&gt; > > > > --- > > > > 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(a)redhat.com > > > https://www.redhat.com/mailman/listinfo/linux-audit > > > > - RGB > > > > -- > > Richard Guy Briggs <rgb(a)redhat.com&gt; > > 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(a)redhat.com > > https://www.redhat.com/mailman/listinfo/linux-audit > > - RGB > > -- > Richard Guy Briggs <rgb(a)redhat.com&gt; > 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

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > 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); paul moore

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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