Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 22:26, Paul Moore wrote: > On Thu, Jan 17, 2019 at 6:19 PM Richard Guy Briggs wrote: > > On 2019-01-17 12:58, Paul Moore wrote: > > > On Thu, Jan 17, 2019 at 10:34 AM Richard Guy Briggs > > > wrote: > > > > > > > > On 2019-01-17 08:21, Paul Moore wrote: > > > > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > > > > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore > > > > > > wrote: > > > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > > > > > > wrote: > > > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they > > > > > > > > are > > > > > > > > all a result of user actions. > > > > > > > > > > > > Please don't tie syscall information to this. The syscall will be > > > > > > sendto. We don't need that information, its implicit. Also, doing > > > > > > this > > > > > > will possibly wreck things in libauparse. Please test the events > > > > > > with > > > > > > ausearch --format csv and --format text. IFF the event looks better > > > > > > or > > > > > > the same should we do this. If stuff disappears, the patch is > > > > > > breaking things > > > > > > > > > > We've discussed this quite a bit already; connecting associated > > > > > records into a single event is something that should happen, needs to > > > > > happen, and will happen. Conceptually it makes no sense to record the > > > > > syscall (and any other associated records) which triggers the audit > > > > > configuration change, and the configuration change record itself as > > > > > two distinct events - they are the same event. We've also heard from > > > > > a prominent user that associating records in this way is desirable. > > > > > > > > > > If the ausearch csv and text audit log transformations can't handle > > > > > this particular change, I would consider that a shortcoming of that > > > > > code. We have multi-record events now, and this is only going to > > > > > increase in the future. > > > > > > > > > > Richard, if you can't make the requested changes to this patch and > > > > > resubmit by ... let's say the middle of next week? that should be > > > > > enough time, yes? ... please let me know and I'll make the changes and > > > > > get this merged. > > > > > > > > I would do the change, which should be very trivial, but I'm dense > > > > enough that I still don't know what you want. In the last 6 months I've > > > > asked a number of direct questions that have not been directly > > > > addressed. Perhaps I should be able to figure it out from the more > > > > general or fundamental principles replies I've gotten (which have been > > > > helpful, but perhaps incomplete), but I'm still having some trouble. > > > > Perhaps I'm exposing my limitations. > > > > > > Since code is unambiguous, let me just cut and paste what I was > > > thinking (be warned, this is a cut-n-paste, so the whitespace is > > > probably mangled): > > > > Thank you. Very clear. I don't see the point of > > audit_log_user_recv_msg() for reasons already stated but that's ok. > > That's a fair comment. I think there is some value in having the > function dedicated for the user messages since they are fairly unique > in that we don't ever want to associate them with the current task, > but it does result in a single use function (which I generally > dislike). Who knows, maybe at some future point in time we'll get rid > of audit_log_user_recv_msg(), but let's stick with it for now. > > > Since we're looking at these, here are the various field formats of the > > AUDIT_CONFIG_CHANGE records. > > > > Steve and Paul, are they worth attempting to normalize? > > Right now, my vote is "no". Although I'm voting that way pretty much > just because I want to get this patch in during this development cycle > and I'm fairly certain that trying to reach any consensus around > normalization is going to drag this out. I would strongly encourage > you to just tweak this patch as we've just talked about and leave it > at that for the time being. Agreed. They are already inconsistent, so this patch isn't going to make that worse. > paul moore - RGB -- Richard Guy Briggs 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: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 6:19 PM Richard Guy Briggs wrote: > On 2019-01-17 12:58, Paul Moore wrote: > > On Thu, Jan 17, 2019 at 10:34 AM Richard Guy Briggs wrote: > > > > > > On 2019-01-17 08:21, Paul Moore wrote: > > > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > > > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore > > > > > wrote: > > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > > > > > wrote: > > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they are > > > > > > > all a result of user actions. > > > > > > > > > > Please don't tie syscall information to this. The syscall will be > > > > > sendto. We don't need that information, its implicit. Also, doing this > > > > > will possibly wreck things in libauparse. Please test the events with > > > > > ausearch --format csv and --format text. IFF the event looks better or > > > > > the same should we do this. If stuff disappears, the patch is > > > > > breaking things > > > > > > > > We've discussed this quite a bit already; connecting associated > > > > records into a single event is something that should happen, needs to > > > > happen, and will happen. Conceptually it makes no sense to record the > > > > syscall (and any other associated records) which triggers the audit > > > > configuration change, and the configuration change record itself as > > > > two distinct events - they are the same event. We've also heard from > > > > a prominent user that associating records in this way is desirable. > > > > > > > > If the ausearch csv and text audit log transformations can't handle > > > > this particular change, I would consider that a shortcoming of that > > > > code. We have multi-record events now, and this is only going to > > > > increase in the future. > > > > > > > > Richard, if you can't make the requested changes to this patch and > > > > resubmit by ... let's say the middle of next week? that should be > > > > enough time, yes? ... please let me know and I'll make the changes and > > > > get this merged. > > > > > > I would do the change, which should be very trivial, but I'm dense > > > enough that I still don't know what you want. In the last 6 months I've > > > asked a number of direct questions that have not been directly > > > addressed. Perhaps I should be able to figure it out from the more > > > general or fundamental principles replies I've gotten (which have been > > > helpful, but perhaps incomplete), but I'm still having some trouble. > > > Perhaps I'm exposing my limitations. > > > > Since code is unambiguous, let me just cut and paste what I was > > thinking (be warned, this is a cut-n-paste, so the whitespace is > > probably mangled): > > Thank you. Very clear. I don't see the point of > audit_log_user_recv_msg() for reasons already stated but that's ok. That's a fair comment. I think there is some value in having the function dedicated for the user messages since they are fairly unique in that we don't ever want to associate them with the current task, but it does result in a single use function (which I generally dislike). Who knows, maybe at some future point in time we'll get rid of audit_log_user_recv_msg(), but let's stick with it for now. > Since we're looking at these, here are the various field formats of the > AUDIT_CONFIG_CHANGE records. > > Steve and Paul, are they worth attempting to normalize? Right now, my vote is "no". Although I'm voting that way pretty much just because I want to get this patch in during this development cycle and I'm fairly certain that trying to reach any consensus around normalization is going to drag this out. I would strongly encourage you to just tweak this patch as we've just talked about and leave it at that for the time being. -- paul moore www.paul-moore.com
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 12:58, Paul Moore wrote: > On Thu, Jan 17, 2019 at 10:34 AM Richard Guy Briggs wrote: > > > > On 2019-01-17 08:21, Paul Moore wrote: > > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore > > > > wrote: > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > > > > wrote: > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they are > > > > > > all a result of user actions. > > > > > > > > Please don't tie syscall information to this. The syscall will be > > > > sendto. We don't need that information, its implicit. Also, doing this > > > > will possibly wreck things in libauparse. Please test the events with > > > > ausearch --format csv and --format text. IFF the event looks better or > > > > the same should we do this. If stuff disappears, the patch is > > > > breaking things > > > > > > We've discussed this quite a bit already; connecting associated > > > records into a single event is something that should happen, needs to > > > happen, and will happen. Conceptually it makes no sense to record the > > > syscall (and any other associated records) which triggers the audit > > > configuration change, and the configuration change record itself as > > > two distinct events - they are the same event. We've also heard from > > > a prominent user that associating records in this way is desirable. > > > > > > If the ausearch csv and text audit log transformations can't handle > > > this particular change, I would consider that a shortcoming of that > > > code. We have multi-record events now, and this is only going to > > > increase in the future. > > > > > > Richard, if you can't make the requested changes to this patch and > > > resubmit by ... let's say the middle of next week? that should be > > > enough time, yes? ... please let me know and I'll make the changes and > > > get this merged. > > > > I would do the change, which should be very trivial, but I'm dense > > enough that I still don't know what you want. In the last 6 months I've > > asked a number of direct questions that have not been directly > > addressed. Perhaps I should be able to figure it out from the more > > general or fundamental principles replies I've gotten (which have been > > helpful, but perhaps incomplete), but I'm still having some trouble. > > Perhaps I'm exposing my limitations. > > Since code is unambiguous, let me just cut and paste what I was > thinking (be warned, this is a cut-n-paste, so the whitespace is > probably mangled): Thank you. Very clear. I don't see the point of audit_log_user_recv_msg() for reasons already stated but that's ok. Since we're looking at these, here are the various field formats of the AUDIT_CONFIG_CHANGE records. Steve and Paul, are they worth attempting to normalize? Some can't because there are two parameters changed in the same record. I'm pretty sure some of the suggestions will break Steve's parsers, but I'm also certain that some are already broken in their current state or were never coded in. I've tried triggering all of these, but failed on a couple and have pinged Al Viro a couple of times for some clues how to do so and had no reply. If it isn't worth it, I'll leave them as they are. audit_log_config_change op %s old auid ses subj res op %s old res audit_log_rule_change auid ses subj op key list res op key list res audit_log_common_recv_msg ADD/DEL_RULE pid uid auid ses subj op audit_enabled res op audit_enabled res audit_log_common_recv_msg TRIM pid uid auid ses subj op res op res audit_log_common_recv_msg MAKE_EQUIVpid uid auid ses subj op old new res op dir old res (order/label change) audit_log_common_recv_msg TTY_SET pid uid auid ses subj op old-enabled new-enabled old-log_passwd new-log_passwd res op enabled old-enabled log_passwd old-log_passwd res (order/label change) audit_mark_log_rule_change auid ses op path key list res op path key list res audit_tree_log_remove_rule op dir key list res op dir key list res audit_watch_log_rule_change auid ses op path key list res op path key list res audit_seccomp_actions_loggedop actions old-actions res op actions old res (label change) > diff --git a/kernel/audit.c b/kernel/audit.c > index d412fb4ae6d5..d2caef6ef09e 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -396,7 +396,7 @@ static int audit_log_config_change(char > *function_name, u32 new, u32 old, >struct audit_buffer *ab; >int rc =
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 2:26 PM Richard Guy Briggs wrote: > On 2019-01-17 12:36, Paul Moore wrote: > > On Thu, Jan 17, 2019 at 11:08 AM Steve Grubb wrote: > > > On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore wrote: > > > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > > > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore > > > > > wrote: > > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > > > > > wrote: > > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they > > > > > > > are all a result of user actions. > > > I see you added LKML to the To/CC line. Fun. Unless they've been > > following the audit list for the past several years I'm not sure they > > have all the context to fully understand some of the things in this > > thread, but sure, why not. > > I added LKML in the original patchset posting, not Steve. That you did, my apologies. Why did my mail client highlight that as new? Oh well, my comments stand regardless; I'm just sorry about the LKML noise I added to the top. -- paul moore www.paul-moore.com
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 12:36, Paul Moore wrote: > On Thu, Jan 17, 2019 at 11:08 AM Steve Grubb wrote: > > On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore wrote: > > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore > > > > wrote: > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > > > > wrote: > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they > > > > > > are all a result of user actions. > I see you added LKML to the To/CC line. Fun. Unless they've been > following the audit list for the past several years I'm not sure they > have all the context to fully understand some of the things in this > thread, but sure, why not. I added LKML in the original patchset posting, not Steve. > paul moore - RGB -- Richard Guy Briggs 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: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 11:08 AM Steve Grubb wrote: > On Thu, 17 Jan 2019 08:21:40 -0500 > Paul Moore wrote: > > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > > > On Mon, 14 Jan 2019 17:58:58 -0500 > > > Paul Moore wrote: > > > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > > > wrote: > > > > > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they > > > > > are all a result of user actions. > > > > > > Please don't tie syscall information to this. The syscall will be > > > sendto. We don't need that information, its implicit. Also, doing > > > this will possibly wreck things in libauparse. Please test the > > > events with ausearch --format csv and --format text. IFF the event > > > looks better or the same should we do this. If stuff disappears, > > > the patch is breaking things > > > > We've discussed this quite a bit already; > > Yes, and you still don't seem be listening. You have to cooperate on > modifying events. We as a community need to respect each other's needs > and work together to solve problems. What this is saying sounds a lot > like I don't care if it breaks things, I'm gonna do it my way. Tough > luck. I see you added LKML to the To/CC line. Fun. Unless they've been following the audit list for the past several years I'm not sure they have all the context to fully understand some of the things in this thread, but sure, why not. I understand you are frustrated Steve, I get it. I can also understand how you feel that I'm not listening to you because I'm not agreeing with you on this, I get this too. However, we've talked about this quite a bit and keeping individual audit records split across multiple events when they are all part of the same event just doesn't make sense to me. The users who have commented on this also agree that associated records should be combined into one event. I definitely don't want to put words in Richard's mouth, but I believe he has also said that he believes that associating all related records into a single event makes sense. I'm listening to you Steve - look at all the times I've asked for your perspective when it comes to certification requirements? - I just don't always agree with you. > You do not have to make sense of any of these events. So, what does it > really matter to you how the event is formed? What I'm asking for is > have these changes been vetted to ensure that they are not breaking > things? We are not changing the record formats, all we are doing is changing the timestamp/event counter so that related records are grouped together into the same event. For example, in this particular case (describing this for the LKML crowd that may be reading this) we are combining the syscall audit record that triggers the audit configuration change with the audit configuration change record into a single event. The code currently treats the syscall record and the audit config change record as separate events - which is just silly as they belong to the same event, triggered by the same user action - this patch should fix this by associating the two records so they are part of the same audit event. From my perspective this is a bug fix. > > connecting associated records into a single event is something that > > should happen, needs to happen, and will happen. Conceptually it > > makes no sense to record the syscall (and any other associated > > records) which triggers the audit configuration change, and the > > configuration change record itself as two distinct events - they are > > the same event. > > Except that they are not. The design of the audit system is to save > disk space as much as possible by emitting single record events on > certain event types that are simple. To add a syscall to it adds useless > information (such as a socket address record), slows down processing, > and wastes disk space. If you get a SYSCALL record, that indicates that > you have triggered an event that the system admin has placed explicit > rules on. That is different than the common criteria required > configuration change event. I've said this many times in the past, I believe Richard has said the same too (maybe it was in a GH issue?), but I'll say it again ... if the system's audit config is such that syscall records are not being generated, then this patch will not cause them to be generated; all this patch does is ensure that *if* a syscall record is being generated *and* an audit config change record is being generated, then the two records will share the same event counter and treated as a single event. This change does not cause any change to the amount of information emitted by the kernel, all it does is group the related records together. > > We've also heard from a prominent user that > > associating records in this way is desirable. > > > > If the ausearch csv and text audit log transformations can't handle > > this particular change, I would consider that a shortcoming of that > > code. We
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore wrote: > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > > On Mon, 14 Jan 2019 17:58:58 -0500 > > Paul Moore wrote: > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > > wrote: > > > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they > > > > are all a result of user actions. > > > > Please don't tie syscall information to this. The syscall will be > > sendto. We don't need that information, its implicit. Also, doing > > this will possibly wreck things in libauparse. Please test the > > events with ausearch --format csv and --format text. IFF the event > > looks better or the same should we do this. If stuff disappears, > > the patch is breaking things > > We've discussed this quite a bit already; Yes, and you still don't seem be listening. You have to cooperate on modifying events. We as a community need to respect each other's needs and work together to solve problems. What this is saying sounds a lot like I don't care if it breaks things, I'm gonna do it my way. Tough luck. You do not have to make sense of any of these events. So, what does it really matter to you how the event is formed? What I'm asking for is have these changes been vetted to ensure that they are not breaking things? > connecting associated records into a single event is something that > should happen, needs to happen, and will happen. Conceptually it > makes no sense to record the syscall (and any other associated > records) which triggers the audit configuration change, and the > configuration change record itself as two distinct events - they are > the same event. Except that they are not. The design of the audit system is to save disk space as much as possible by emitting single record events on certain event types that are simple. To add a syscall to it adds useless information (such as a socket address record), slows down processing, and wastes disk space. If you get a SYSCALL record, that indicates that you have triggered an event that the system admin has placed explicit rules on. That is different than the common criteria required configuration change event. > We've also heard from a prominent user that > associating records in this way is desirable. > > If the ausearch csv and text audit log transformations can't handle > this particular change, I would consider that a shortcoming of that > code. We have multi-record events now, and this is only going to > increase in the future. Isn't there some kind a guideline about not breaking user space? -Steve > Richard, if you can't make the requested changes to this patch and > resubmit by ... let's say the middle of next week? that should be > enough time, yes? ... please let me know and I'll make the changes and > get this merged. >
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 10:32, Steve Grubb wrote:
> On Mon, 14 Jan 2019 17:58:58 -0500
> Paul Moore wrote:
>
> > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
> > wrote:
> > >
> > > Tie syscall information to all CONFIG_CHANGE calls since they are
> > > all a result of user actions.
>
> Please don't tie syscall information to this. The syscall will be
> sendto. We don't need that information, its implicit. Also, doing this
> will possibly wreck things in libauparse. Please test the events with
> ausearch --format csv and --format text. IFF the event looks better or
> the same should we do this. If stuff disappears, the patch is
> breaking things
Steve, I hope you aren't talking about the AUDIT_*USER* records, because
this patch intentionally leaves them unassociated with the syscall
record.
The config change records are related.
> -Steve
>
> > > Exclude user records from syscall context:
> > > 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
> > > See: https://github.com/linux-audit/audit-kernel/issues/50
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > kernel/audit.c | 27 +++
> > > kernel/audit_fsnotify.c | 2 +-
> > > kernel/audit_tree.c | 2 +-
> > > kernel/audit_watch.c| 2 +-
> > > kernel/auditfilter.c| 2 +-
> > > 5 files changed, 23 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 0e8026423fbd..a321fea94cc6 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> > > audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
> > > }
> > >
> > > +static inline void audit_log_user_recv_msg(struct audit_buffer
> > > **ab, u16 msg_type) +{
> > > + audit_log_common_recv_msg(NULL, ab, msg_type);
> > > +}
> >
> > This makes sense because this is used by "user" records ...
> >
> > > +static inline void audit_log_config_change_alt(struct audit_buffer
> > > **ab) +{
> > > + audit_log_common_recv_msg(audit_context(), ab,
> > > AUDIT_CONFIG_CHANGE); +}
> >
> > ... and I don't believe this makes sense because there is no real
> > logical grouping with the callers like there is for
> > audit_log_user_recv_msg().
- RGB
--
Richard Guy Briggs
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: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > On Mon, 14 Jan 2019 17:58:58 -0500 > Paul Moore wrote: > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > wrote: > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they are > > > all a result of user actions. > > Please don't tie syscall information to this. The syscall will be > sendto. We don't need that information, its implicit. Also, doing this > will possibly wreck things in libauparse. Please test the events with > ausearch --format csv and --format text. IFF the event looks better or > the same should we do this. If stuff disappears, the patch is > breaking things We've discussed this quite a bit already; connecting associated records into a single event is something that should happen, needs to happen, and will happen. Conceptually it makes no sense to record the syscall (and any other associated records) which triggers the audit configuration change, and the configuration change record itself as two distinct events - they are the same event. We've also heard from a prominent user that associating records in this way is desirable. If the ausearch csv and text audit log transformations can't handle this particular change, I would consider that a shortcoming of that code. We have multi-record events now, and this is only going to increase in the future. Richard, if you can't make the requested changes to this patch and resubmit by ... let's say the middle of next week? that should be enough time, yes? ... please let me know and I'll make the changes and get this merged. -- paul moore www.paul-moore.com
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Mon, 14 Jan 2019 17:58:58 -0500
Paul Moore wrote:
> On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
> wrote:
> >
> > Tie syscall information to all CONFIG_CHANGE calls since they are
> > all a result of user actions.
Please don't tie syscall information to this. The syscall will be
sendto. We don't need that information, its implicit. Also, doing this
will possibly wreck things in libauparse. Please test the events with
ausearch --format csv and --format text. IFF the event looks better or
the same should we do this. If stuff disappears, the patch is
breaking things
-Steve
> > Exclude user records from syscall context:
> > 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
> > See: https://github.com/linux-audit/audit-kernel/issues/50
> > Signed-off-by: Richard Guy Briggs
> > ---
> > kernel/audit.c | 27 +++
> > kernel/audit_fsnotify.c | 2 +-
> > kernel/audit_tree.c | 2 +-
> > kernel/audit_watch.c| 2 +-
> > kernel/auditfilter.c| 2 +-
> > 5 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 0e8026423fbd..a321fea94cc6 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> > audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
> > }
> >
> > +static inline void audit_log_user_recv_msg(struct audit_buffer
> > **ab, u16 msg_type) +{
> > + audit_log_common_recv_msg(NULL, ab, msg_type);
> > +}
>
> This makes sense because this is used by "user" records ...
>
> > +static inline void audit_log_config_change_alt(struct audit_buffer
> > **ab) +{
> > + audit_log_common_recv_msg(audit_context(), ab,
> > AUDIT_CONFIG_CHANGE); +}
>
> ... and I don't believe this makes sense because there is no real
> logical grouping with the callers like there is for
> audit_log_user_recv_msg().
>
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Tue, Jan 15, 2019 at 11:21 AM Richard Guy Briggs wrote:
>
> On 2019-01-14 17:58, Paul Moore wrote:
> > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs wrote:
> > > Tie syscall information to all CONFIG_CHANGE calls since they are all a
> > > result of user actions.
> > >
> > > Exclude user records from syscall context:
> > > 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
> > > See: https://github.com/linux-audit/audit-kernel/issues/50
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > kernel/audit.c | 27 +++
> > > kernel/audit_fsnotify.c | 2 +-
> > > kernel/audit_tree.c | 2 +-
> > > kernel/audit_watch.c| 2 +-
> > > kernel/auditfilter.c| 2 +-
> > > 5 files changed, 23 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 0e8026423fbd..a321fea94cc6 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> > > audit_buffer **ab, u16 msg_type)
> > > audit_log_task_context(*ab);
> > > }
> > >
> > > +static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16
> > > msg_type)
> > > +{
> > > + audit_log_common_recv_msg(NULL, ab, msg_type);
> > > +}
> >
> > This makes sense because this is used by "user" records ...
> >
> > > +static inline void audit_log_config_change_alt(struct audit_buffer **ab)
> > > +{
> > > + audit_log_common_recv_msg(audit_context(), ab,
> > > AUDIT_CONFIG_CHANGE);
> > > +}
> >
> > ... and I don't believe this makes sense because there is no real
> > logical grouping with the callers like there is for
> > audit_log_user_recv_msg().
>
> I don't follow "logical grouping". They are all CONFIG_CHANGE record
> prefixes with the current context.
The audit_log_user_recv_msg() callers have a logical grouping because
they are all user generated records which we've decided shouldn't be
associated with any audit records tied to the current task.
The audit_log_config_change_alt() callers seem only to be grouped by
the fact that they are share some common audit_log_config_change_alt()
parameters; they don't appear to have anything else in common. Yes,
sometimes we do create functions like audit_log_config_change_alt()
for reasons such as these, but I don't believe it is necessary, or
desirable, in this particular patch(set).
My comments on your v2 of this patchset suggested the creation of
audit_log_user_recv_msg() instead of what you did with
__audit_log_common_recv_msg(). You made that suggested change for v3
- good - but with v3 you also introduced audit_log_config_change_alt -
not good.
Get rid of audit_log_config_change_alt() (respin, retest, etc.) and
post this revised single patch (the others in the patchset that are ok
are already in audit/next) and we can get it into audit/next.
> Can you suggest an alternate name or another way of sharing
> audit_log_common_recv_msg() since the only differences between the two
> are a NULL context vs current task's context and the message type. I
> wasn't particularly happy with this name either. I'd really like to
> refactor this with all the rest of the CONFIG_CHANGE records, but there
> is too much of a format difference to make it work without reordering or
> deleting useless fields.
>
> I know you had suggested making two different functions, but I think
> they are more similar than different and merit the common factored code.
>
> > paul moore
>
> - RGB
>
> --
> Richard Guy Briggs
> 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: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-14 17:58, Paul Moore wrote:
> On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs wrote:
> > Tie syscall information to all CONFIG_CHANGE calls since they are all a
> > result of user actions.
> >
> > Exclude user records from syscall context:
> > 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
> > See: https://github.com/linux-audit/audit-kernel/issues/50
> > Signed-off-by: Richard Guy Briggs
> > ---
> > kernel/audit.c | 27 +++
> > kernel/audit_fsnotify.c | 2 +-
> > kernel/audit_tree.c | 2 +-
> > kernel/audit_watch.c| 2 +-
> > kernel/auditfilter.c| 2 +-
> > 5 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 0e8026423fbd..a321fea94cc6 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> > audit_buffer **ab, u16 msg_type)
> > audit_log_task_context(*ab);
> > }
> >
> > +static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16
> > msg_type)
> > +{
> > + audit_log_common_recv_msg(NULL, ab, msg_type);
> > +}
>
> This makes sense because this is used by "user" records ...
>
> > +static inline void audit_log_config_change_alt(struct audit_buffer **ab)
> > +{
> > + audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
> > +}
>
> ... and I don't believe this makes sense because there is no real
> logical grouping with the callers like there is for
> audit_log_user_recv_msg().
I don't follow "logical grouping". They are all CONFIG_CHANGE record
prefixes with the current context.
Can you suggest an alternate name or another way of sharing
audit_log_common_recv_msg() since the only differences between the two
are a NULL context vs current task's context and the message type. I
wasn't particularly happy with this name either. I'd really like to
refactor this with all the rest of the CONFIG_CHANGE records, but there
is too much of a format difference to make it work without reordering or
deleting useless fields.
I know you had suggested making two different functions, but I think
they are more similar than different and merit the common factored code.
> paul moore
- RGB
--
Richard Guy Briggs
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: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs wrote:
>
> Tie syscall information to all CONFIG_CHANGE calls since they are all a
> result of user actions.
>
> Exclude user records from syscall context:
> 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
> See: https://github.com/linux-audit/audit-kernel/issues/50
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/audit.c | 27 +++
> kernel/audit_fsnotify.c | 2 +-
> kernel/audit_tree.c | 2 +-
> kernel/audit_watch.c| 2 +-
> kernel/auditfilter.c| 2 +-
> 5 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 0e8026423fbd..a321fea94cc6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> audit_buffer **ab, u16 msg_type)
> audit_log_task_context(*ab);
> }
>
> +static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16
> msg_type)
> +{
> + audit_log_common_recv_msg(NULL, ab, msg_type);
> +}
This makes sense because this is used by "user" records ...
> +static inline void audit_log_config_change_alt(struct audit_buffer **ab)
> +{
> + audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
> +}
... and I don't believe this makes sense because there is no real
logical grouping with the callers like there is for
audit_log_user_recv_msg().
--
paul moore
www.paul-moore.com
[PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
Tie syscall information to all CONFIG_CHANGE calls since they are all a
result of user actions.
Exclude user records from syscall context:
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
See: https://github.com/linux-audit/audit-kernel/issues/50
Signed-off-by: Richard Guy Briggs
---
kernel/audit.c | 27 +++
kernel/audit_fsnotify.c | 2 +-
kernel/audit_tree.c | 2 +-
kernel/audit_watch.c| 2 +-
kernel/auditfilter.c| 2 +-
5 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 0e8026423fbd..a321fea94cc6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -397,7 +397,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);
@@ -1054,7 +1054,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(_user_ns, current_uid());
pid_t pid = task_tgid_nr(current);
@@ -1064,7 +1065,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(context, GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u ", pid, uid);
@@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
audit_buffer **ab, u16 msg_type)
audit_log_task_context(*ab);
}
+static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16
msg_type)
+{
+ audit_log_common_recv_msg(NULL, ab, msg_type);
+}
+
+static inline void audit_log_config_change_alt(struct audit_buffer **ab)
+{
+ audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1339,7 +1350,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh)
if (err)
break;
}
- audit_log_common_recv_msg(, msg_type);
+ audit_log_user_recv_msg(, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
@@ -1362,7 +1373,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh)
if (nlmsg_len(nlh) < sizeof(struct audit_rule_data))
return -EINVAL;
if (audit_enabled == AUDIT_LOCKED) {
- audit_log_common_recv_msg(, AUDIT_CONFIG_CHANGE);
+ audit_log_config_change_alt();
audit_log_format(ab, " op=%s audit_enabled=%d res=0",
msg_type == AUDIT_ADD_RULE ?
"add_rule" : "remove_rule",
audit_enabled);
@@ -1376,7 +1387,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh)
break;
case AUDIT_TRIM:
audit_trim_trees();
- audit_log_common_recv_msg(, AUDIT_CONFIG_CHANGE);
+ audit_log_config_change_alt();
audit_log_format(ab, " op=trim res=1");
audit_log_end(ab);
break;
@@ -1406,7 +1417,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh)
/* OK, here comes... */
err = audit_tag_tree(old, new);
- audit_log_common_recv_msg(, AUDIT_CONFIG_CHANGE);
+ audit_log_config_change_alt();
audit_log_format(ab, " op=make_equiv old=");
audit_log_untrustedstring(ab, old);
@@ -1474,7 +1485,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh)
old.enabled = t & AUDIT_TTY_ENABLE;
old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD);
-
