| Message ID | 20180524201105.3179904-6-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 24, 2018 at 4:11 PM, Stefan Berger <stefanb@linux.vnet.ibm.com> wrote: > Use the new public audit functions to add the exe= and tty= > parts to the integrity audit records. We place them before > res=. > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > Suggested-by: Steve Grubb <sgrubb@redhat.com> > --- > security/integrity/integrity_audit.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c > index db30763d5525..8d25d3c4dcca 100644 > --- a/security/integrity/integrity_audit.c > +++ b/security/integrity/integrity_audit.c > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, > audit_log_untrustedstring(ab, inode->i_sb->s_id); > audit_log_format(ab, " ino=%lu", inode->i_ino); > } > + audit_log_d_path_exe(ab, current->mm); > + audit_log_tty(ab, current); NACK Please add the new fields to the end of the audit record, thank you. > audit_log_format(ab, " res=%d", !result); > audit_log_end(ab); > }
On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote: > On Thu, May 24, 2018 at 4:11 PM, Stefan Berger > > <stefanb@linux.vnet.ibm.com> wrote: > > Use the new public audit functions to add the exe= and tty= > > parts to the integrity audit records. We place them before > > res=. > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > > --- > > > > security/integrity/integrity_audit.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/security/integrity/integrity_audit.c > > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca > > 100644 > > --- a/security/integrity/integrity_audit.c > > +++ b/security/integrity/integrity_audit.c > > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode > > *inode,> > > audit_log_untrustedstring(ab, inode->i_sb->s_id); > > audit_log_format(ab, " ino=%lu", inode->i_ino); > > > > } > > > > + audit_log_d_path_exe(ab, current->mm); > > + audit_log_tty(ab, current); > > NACK > > Please add the new fields to the end of the audit record, thank you. Let's see what an example event looks like before NACK'ing this. Way back in 2013 the IMA events were good. I think this is repairing the event after some drift. Thanks, -Steve > > audit_log_format(ab, " res=%d", !result); > > audit_log_end(ab); > > > > }
On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote: > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote: >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger >> >> <stefanb@linux.vnet.ibm.com> wrote: >> > Use the new public audit functions to add the exe= and tty= >> > parts to the integrity audit records. We place them before >> > res=. >> > >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >> > Suggested-by: Steve Grubb <sgrubb@redhat.com> >> > --- >> > >> > security/integrity/integrity_audit.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/security/integrity/integrity_audit.c >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca >> > 100644 >> > --- a/security/integrity/integrity_audit.c >> > +++ b/security/integrity/integrity_audit.c >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode >> > *inode,> >> > audit_log_untrustedstring(ab, inode->i_sb->s_id); >> > audit_log_format(ab, " ino=%lu", inode->i_ino); >> > >> > } >> > >> > + audit_log_d_path_exe(ab, current->mm); >> > + audit_log_tty(ab, current); >> >> NACK >> >> Please add the new fields to the end of the audit record, thank you. > > Let's see what an example event looks like before NACK'ing this. Way back in > 2013 the IMA events were good. I think this is repairing the event after some > drift. Can you reference a specific commit, or point in time during 2013? Looking at the git log quickly, if I go back to commit d726d8d719b6 ("integrity: move integrity_audit_msg()") from March 18, 2013 (the commit that created integrity_audit.c) the field ordering appears to be the same as it today. My NACK still stands.
On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote: > On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote: > > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote: > >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger > >> > >> <stefanb@linux.vnet.ibm.com> wrote: > >> > Use the new public audit functions to add the exe= and tty= > >> > parts to the integrity audit records. We place them before > >> > res=. > >> > > >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > >> > Suggested-by: Steve Grubb <sgrubb@redhat.com> > >> > --- > >> > > >> > security/integrity/integrity_audit.c | 2 ++ > >> > 1 file changed, 2 insertions(+) > >> > > >> > diff --git a/security/integrity/integrity_audit.c > >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca > >> > 100644 > >> > --- a/security/integrity/integrity_audit.c > >> > +++ b/security/integrity/integrity_audit.c > >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode > >> > *inode,> > >> > audit_log_untrustedstring(ab, inode->i_sb->s_id); > >> > audit_log_format(ab, " ino=%lu", inode->i_ino); > >> > > >> > } > >> > > >> > + audit_log_d_path_exe(ab, current->mm); > >> > + audit_log_tty(ab, current); > >> > >> NACK > >> > >> Please add the new fields to the end of the audit record, thank you. > > > > Let's see what an example event looks like before NACK'ing this. Way back in > > 2013 the IMA events were good. I think this is repairing the event after some > > drift. > > Can you reference a specific commit, or point in time during 2013? > Looking at the git log quickly, if I go back to commit d726d8d719b6 > ("integrity: move integrity_audit_msg()") from March 18, 2013 (the > commit that created integrity_audit.c) the field ordering appears to > be the same as it today. > > My NACK still stands. There hasn't been any changes up to now. This patch set refactors integrity_audit_msg(), creating integrity_audit_msg_common(), which will be called from both ima_audit_measurement() and ima_parse_rule(). Previously the audit record generated by ima_parse_rule() did not include this info. The change in this patch will affect both the existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records. Mimi
On 05/29/2018 05:19 PM, Paul Moore wrote: > On Thu, May 24, 2018 at 4:11 PM, Stefan Berger > <stefanb@linux.vnet.ibm.com> wrote: >> Use the new public audit functions to add the exe= and tty= >> parts to the integrity audit records. We place them before >> res=. >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >> Suggested-by: Steve Grubb <sgrubb@redhat.com> >> --- >> security/integrity/integrity_audit.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c >> index db30763d5525..8d25d3c4dcca 100644 >> --- a/security/integrity/integrity_audit.c >> +++ b/security/integrity/integrity_audit.c >> @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, >> audit_log_untrustedstring(ab, inode->i_sb->s_id); >> audit_log_format(ab, " ino=%lu", inode->i_ino); >> } >> + audit_log_d_path_exe(ab, current->mm); >> + audit_log_tty(ab, current); > NACK > > Please add the new fields to the end of the audit record, thank you. I put it there since Steve said '"res" is traditionally the last field in any event' (https://lkml.org/lkml/2018/5/22/539). I don't mind breaking with this tradition... > >> audit_log_format(ab, " res=%d", !result); >> audit_log_end(ab); >> }
On Tue, 2018-05-29 at 18:58 -0400, Mimi Zohar wrote: > On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote: > > On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote: > > > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote: > > >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger > > >> > > >> <stefanb@linux.vnet.ibm.com> wrote: > > >> > Use the new public audit functions to add the exe= and tty= > > >> > parts to the integrity audit records. We place them before > > >> > res=. > > >> > > > >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > >> > Suggested-by: Steve Grubb <sgrubb@redhat.com> > > >> > --- > > >> > > > >> > security/integrity/integrity_audit.c | 2 ++ > > >> > 1 file changed, 2 insertions(+) > > >> > > > >> > diff --git a/security/integrity/integrity_audit.c > > >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca > > >> > 100644 > > >> > --- a/security/integrity/integrity_audit.c > > >> > +++ b/security/integrity/integrity_audit.c > > >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode > > >> > *inode,> > > >> > audit_log_untrustedstring(ab, inode->i_sb->s_id); > > >> > audit_log_format(ab, " ino=%lu", inode->i_ino); > > >> > > > >> > } > > >> > > > >> > + audit_log_d_path_exe(ab, current->mm); > > >> > + audit_log_tty(ab, current); > > >> > > >> NACK > > >> > > >> Please add the new fields to the end of the audit record, thank you. > > > > > > Let's see what an example event looks like before NACK'ing this. Way back in > > > 2013 the IMA events were good. I think this is repairing the event after some > > > drift. > > > > Can you reference a specific commit, or point in time during 2013? > > Looking at the git log quickly, if I go back to commit d726d8d719b6 > > ("integrity: move integrity_audit_msg()") from March 18, 2013 (the > > commit that created integrity_audit.c) the field ordering appears to > > be the same as it today. > > > > My NACK still stands. > > There hasn't been any changes up to now. This patch set refactors > integrity_audit_msg(), creating integrity_audit_msg_common(), which > will be called from both ima_audit_measurement() and > ima_parse_rule(). That should have been "from integrity_audit_msg() and ima_parse_rule()", not ima_audit_measurement(). > Previously the audit record generated by ima_parse_rule() did not > include this info. The change in this patch will affect both the > existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records.
On Wed, May 30, 2018 at 8:17 AM, Stefan Berger <stefanb@linux.vnet.ibm.com> wrote: > On 05/29/2018 05:19 PM, Paul Moore wrote: >> >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger >> <stefanb@linux.vnet.ibm.com> wrote: >>> >>> Use the new public audit functions to add the exe= and tty= >>> parts to the integrity audit records. We place them before >>> res=. >>> >>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> Suggested-by: Steve Grubb <sgrubb@redhat.com> >>> --- >>> security/integrity/integrity_audit.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/security/integrity/integrity_audit.c >>> b/security/integrity/integrity_audit.c >>> index db30763d5525..8d25d3c4dcca 100644 >>> --- a/security/integrity/integrity_audit.c >>> +++ b/security/integrity/integrity_audit.c >>> @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode >>> *inode, >>> audit_log_untrustedstring(ab, inode->i_sb->s_id); >>> audit_log_format(ab, " ino=%lu", inode->i_ino); >>> } >>> + audit_log_d_path_exe(ab, current->mm); >>> + audit_log_tty(ab, current); >> >> NACK >> >> Please add the new fields to the end of the audit record, thank you. > > I put it there since Steve said '"res" is traditionally the last field in > any event' (https://lkml.org/lkml/2018/5/22/539). I don't mind breaking with > this tradition... Unfortunately Steve and I don't see eye-to-eye on everything, and this is perhaps one of the more prominent issues. I'll save you several years of arguments, on and off-list, and simply say that the "safe" option, and the only option I'm likely to ACK, would be to add new fields at the end of existing records. We have made exceptions in the past, but those were pretty extreme cases.
On Wed, May 30, 2018 at 9:04 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > On Tue, 2018-05-29 at 18:58 -0400, Mimi Zohar wrote: >> On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote: >> > On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote: >> > > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote: >> > >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger >> > >> >> > >> <stefanb@linux.vnet.ibm.com> wrote: >> > >> > Use the new public audit functions to add the exe= and tty= >> > >> > parts to the integrity audit records. We place them before >> > >> > res=. >> > >> > >> > >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >> > >> > Suggested-by: Steve Grubb <sgrubb@redhat.com> >> > >> > --- >> > >> > >> > >> > security/integrity/integrity_audit.c | 2 ++ >> > >> > 1 file changed, 2 insertions(+) >> > >> > >> > >> > diff --git a/security/integrity/integrity_audit.c >> > >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca >> > >> > 100644 >> > >> > --- a/security/integrity/integrity_audit.c >> > >> > +++ b/security/integrity/integrity_audit.c >> > >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode >> > >> > *inode,> >> > >> > audit_log_untrustedstring(ab, inode->i_sb->s_id); >> > >> > audit_log_format(ab, " ino=%lu", inode->i_ino); >> > >> > >> > >> > } >> > >> > >> > >> > + audit_log_d_path_exe(ab, current->mm); >> > >> > + audit_log_tty(ab, current); >> > >> >> > >> NACK >> > >> >> > >> Please add the new fields to the end of the audit record, thank you. >> > > >> > > Let's see what an example event looks like before NACK'ing this. Way back in >> > > 2013 the IMA events were good. I think this is repairing the event after some >> > > drift. >> > >> > Can you reference a specific commit, or point in time during 2013? >> > Looking at the git log quickly, if I go back to commit d726d8d719b6 >> > ("integrity: move integrity_audit_msg()") from March 18, 2013 (the >> > commit that created integrity_audit.c) the field ordering appears to >> > be the same as it today. >> > >> > My NACK still stands. >> >> There hasn't been any changes up to now. This patch set refactors >> integrity_audit_msg(), creating integrity_audit_msg_common(), which >> will be called from both ima_audit_measurement() and >> ima_parse_rule(). > > That should have been "from integrity_audit_msg() and > ima_parse_rule()", not ima_audit_measurement(). No worries, the important part is that the record format really hasn't changed from 2013 as far as I can tell. >> Previously the audit record generated by ima_parse_rule() did not >> include this info. The change in this patch will affect both the >> existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records.
diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca 100644 --- a/security/integrity/integrity_audit.c +++ b/security/integrity/integrity_audit.c @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, audit_log_untrustedstring(ab, inode->i_sb->s_id); audit_log_format(ab, " ino=%lu", inode->i_ino); } + audit_log_d_path_exe(ab, current->mm); + audit_log_tty(ab, current); audit_log_format(ab, " res=%d", !result); audit_log_end(ab); }
Use the new public audit functions to add the exe= and tty= parts to the integrity audit records. We place them before res=. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> Suggested-by: Steve Grubb <sgrubb@redhat.com> --- security/integrity/integrity_audit.c | 2 ++ 1 file changed, 2 insertions(+)