Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 06/01/2018 04:13 PM, Paul Moore wrote: On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger wrote: On 05/30/2018 07:34 PM, Richard Guy Briggs wrote: On 2018-05-30 17:38, Stefan Berger wrote: On 05/30/2018 05:22 PM, Paul Moore wrote: On Wed, May 30, 2018 at 9:08 AM, Stefan Berger wrote: On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: On 2018-05-24 16:11, Stefan Berger wrote: The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Signed-off-by: Stefan Berger --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #define AUDIT_INTEGRITY_STATUS1802 /* Integrity enable status */ #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL2000/* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); Is it possible to connect this record to a syscall by replacing the first parameter (NULL) by current->context? We're likely going to need to "associate" this record (audit speak for making the first parameter non-NULL) with others for the audit container ID work. If you do it now, Richard's patches will likely get a few lines smaller and that will surely make him a bit happier :) Richard is also introducing a local context that we can then create and use instead of the NULL. Can we not use that then? That is for records for which there is no syscall or user associated. In fact there is another recent change that would be better to use than current->audit_context, which is the function audit_context(). See commit cdfb6b3 ("audit: use inline function to get audit context"). Steven seems to say: "We don't want to add syscall records to everything. That messes up schemas and existing code. The integrity events are 1 record in size and should stay that way. This saves disk space and improves readability." We would have to fix current->context in this case since it is NULL. We get to this location by root cat'ing a policy or writing a policy filename into /sys/kernel/security/ima/policy. Perhaps I'm missing something, but current in this case should point to the process which is writing to the policy file, yes? Yes, but current->context is NULL for some reason. Is it always this way? If it isn't, which it should not be, we should find out why. Well, we should find out why this is NULL anyways, since it shouldn't be. When someone writes a policy for IMA into securityfs, it's always NULL. There's another location where IMA uses the current->audit_context, and that's here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L323 At this location we sometimes see a (background) process with an audit_context but in the majority of cases it's current->audit_context is NULL. Starting a process as root or also non-root user, with the appropriate IMA audit policy rules set, we always see a NULL audit_context here. What does your audit configuration look like? Depending on your configuration a NULL audit_context can be expected, see audit_dummy_context(). I believe the default Fedora audit config will leave you with a NULL audit_context for all processes. I also believe that unless you explicitly set "audit=1" on the kernel command line the init/systemd process will have a NULL audit_context (there was actually a range of kernels where even setting "audit=1"
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Fri, Jun 1, 2018 at 4:13 PM, Paul Moore wrote: > On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger > wrote: >> On 05/30/2018 07:34 PM, Richard Guy Briggs wrote: >>> >>> On 2018-05-30 17:38, Stefan Berger wrote: On 05/30/2018 05:22 PM, Paul Moore wrote: > > On Wed, May 30, 2018 at 9:08 AM, Stefan Berger > wrote: >> >> On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: >>> >>> On 2018-05-24 16:11, Stefan Berger wrote: The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Signed-off-by: Stefan Berger --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #define AUDIT_INTEGRITY_STATUS1802 /* Integrity enable status */ #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL2000/* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); >>> >>> Is it possible to connect this record to a syscall by replacing the >>> first parameter (NULL) by current->context? > > We're likely going to need to "associate" this record (audit speak for > making the first parameter non-NULL) with others for the audit > container ID work. If you do it now, Richard's patches will likely > get a few lines smaller and that will surely make him a bit happier :) Richard is also introducing a local context that we can then create and use instead of the NULL. Can we not use that then? >>> >>> That is for records for which there is no syscall or user associated. >>> >>> In fact there is another recent change that would be better to use than >>> current->audit_context, which is the function audit_context(). >>> See commit cdfb6b3 ("audit: use inline function to get audit context"). >>> Steven seems to say: "We don't want to add syscall records to everything. That messes up schemas and existing code. The integrity events are 1 record in size and should stay that way. This saves disk space and improves readability." >> We would have to fix current->context in this case since it is NULL. We >> get >> to this location by root cat'ing a policy or writing a policy filename >> into >> /sys/kernel/security/ima/policy. > > Perhaps I'm missing something, but current in this case should point > to the process which is writing to the policy file, yes? Yes, but current->context is NULL for some reason. >>> >>> Is it always this way? If it isn't, which it should not be, we should >>> find out why. Well, we should find out why this is NULL anyways, since >>> it shouldn't be. >> >> >> When someone writes a policy for IMA into securityfs, it's always NULL. >> There's another location where IMA uses the current->audit_context, and >> that's here: >> >> https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L323 >> >>
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger wrote: > On 05/30/2018 07:34 PM, Richard Guy Briggs wrote: >> >> On 2018-05-30 17:38, Stefan Berger wrote: >>> >>> On 05/30/2018 05:22 PM, Paul Moore wrote: On Wed, May 30, 2018 at 9:08 AM, Stefan Berger wrote: > > On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: >> >> On 2018-05-24 16:11, Stefan Berger wrote: >>> >>> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and >>> the IMA "audit" policy action. This patch defines >>> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. >>> >>> With this change we now call integrity_audit_msg_common() to get >>> common integrity auditing fields. This now produces the following >>> record when parsing an IMA policy rule: >>> >>> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure >>> \ >>> fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ >>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >>> op=policy_update cause=parse_rule comm="echo" >>> exe="/usr/bin/echo" \ >>> tty=tty2 res=1 >>> >>> Signed-off-by: Stefan Berger >>> --- >>> include/uapi/linux/audit.h | 3 ++- >>> security/integrity/ima/ima_policy.c | 5 +++-- >>> 2 files changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >>> index 4e61a9e05132..776e0abd35cf 100644 >>> --- a/include/uapi/linux/audit.h >>> +++ b/include/uapi/linux/audit.h >>> @@ -146,7 +146,8 @@ >>> #define AUDIT_INTEGRITY_STATUS1802 /* Integrity >>> enable >>> status */ >>> #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ >>> #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs >>> */ >>> -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ >>> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy >>> msgs */ >>> +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ >>> #define AUDIT_KERNEL2000/* Asynchronous >>> audit >>> record. NOT A REQUEST. */ >>> diff --git a/security/integrity/ima/ima_policy.c >>> b/security/integrity/ima/ima_policy.c >>> index 3aed25a7178a..a8ae47a386b4 100644 >>> --- a/security/integrity/ima/ima_policy.c >>> +++ b/security/integrity/ima/ima_policy.c >>> @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct >>> ima_rule_entry *entry) >>> int result = 0; >>> ab = integrity_audit_log_start(NULL, GFP_KERNEL, >>> - AUDIT_INTEGRITY_RULE); >>> + AUDIT_INTEGRITY_POLICY_RULE); >> >> Is it possible to connect this record to a syscall by replacing the >> first parameter (NULL) by current->context? We're likely going to need to "associate" this record (audit speak for making the first parameter non-NULL) with others for the audit container ID work. If you do it now, Richard's patches will likely get a few lines smaller and that will surely make him a bit happier :) >>> >>> Richard is also introducing a local context that we can then create and >>> use >>> instead of the NULL. Can we not use that then? >> >> That is for records for which there is no syscall or user associated. >> >> In fact there is another recent change that would be better to use than >> current->audit_context, which is the function audit_context(). >> See commit cdfb6b3 ("audit: use inline function to get audit context"). >> >>> Steven seems to say: "We don't want to add syscall records to everything. >>> That messes up schemas and existing code. The integrity events are 1 >>> record >>> in size and should stay that way. This saves disk space and improves >>> readability." >>> > We would have to fix current->context in this case since it is NULL. We > get > to this location by root cat'ing a policy or writing a policy filename > into > /sys/kernel/security/ima/policy. Perhaps I'm missing something, but current in this case should point to the process which is writing to the policy file, yes? >>> >>> Yes, but current->context is NULL for some reason. >> >> Is it always this way? If it isn't, which it should not be, we should >> find out why. Well, we should find out why this is NULL anyways, since >> it shouldn't be. > > > When someone writes a policy for IMA into securityfs, it's always NULL. > There's another location where IMA uses the current->audit_context, and > that's here: > > https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L323 > > At this location we sometimes see a (background) process with an > audit_context but in the majority of cases it's current->audit_context is > NULL. Starting a
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 05/30/2018 07:34 PM, Richard Guy Briggs wrote: On 2018-05-30 17:38, Stefan Berger wrote: On 05/30/2018 05:22 PM, Paul Moore wrote: On Wed, May 30, 2018 at 9:08 AM, Stefan Berger wrote: On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: On 2018-05-24 16:11, Stefan Berger wrote: The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Signed-off-by: Stefan Berger --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #define AUDIT_INTEGRITY_STATUS1802 /* Integrity enable status */ #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL2000/* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); Is it possible to connect this record to a syscall by replacing the first parameter (NULL) by current->context? We're likely going to need to "associate" this record (audit speak for making the first parameter non-NULL) with others for the audit container ID work. If you do it now, Richard's patches will likely get a few lines smaller and that will surely make him a bit happier :) Richard is also introducing a local context that we can then create and use instead of the NULL. Can we not use that then? That is for records for which there is no syscall or user associated. In fact there is another recent change that would be better to use than current->audit_context, which is the function audit_context(). See commit cdfb6b3 ("audit: use inline function to get audit context"). Steven seems to say: "We don't want to add syscall records to everything. That messes up schemas and existing code. The integrity events are 1 record in size and should stay that way. This saves disk space and improves readability." We would have to fix current->context in this case since it is NULL. We get to this location by root cat'ing a policy or writing a policy filename into /sys/kernel/security/ima/policy. Perhaps I'm missing something, but current in this case should point to the process which is writing to the policy file, yes? Yes, but current->context is NULL for some reason. Is it always this way? If it isn't, which it should not be, we should find out why. Well, we should find out why this is NULL anyways, since it shouldn't be. When someone writes a policy for IMA into securityfs, it's always NULL. There's another location where IMA uses the current->audit_context, and that's here: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L323 At this location we sometimes see a (background) process with an audit_context but in the majority of cases it's current->audit_context is NULL. Starting a process as root or also non-root user, with the appropriate IMA audit policy rules set, we always see a NULL audit_context here. - 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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Wed, May 30, 2018 at 8:46 PM, Lenny Bruzenak wrote: > On 05/30/2018 06:54 PM, Paul Moore wrote: > > ... > >> Finally, since you probably haven't followed all of the discussion >> around associating records into a single event, I wanted to give you >> my side of the story (if you don't care, you can simply skip the rest >> of this email). Currently an audit "event" can consist of multiple >> audit "records"; these records can contain PATH information, SYSCALL >> information, SELinux AVC information, as well as INTEGRITY_PCR >> information. The current kernel code does a poor job of associating >> some of these record types, which can make a single user action look >> like multiple audit events. For example, a single user action/event >> (writing a new IMA policy) could result in multiple audit "events" >> because the SYSCALL record for the write() syscall is not associated >> with the INTEGRITY_POLICY_RULE record; this is bogus because the write >> syscall is inherently linked with the IMA policy update. Keeping the >> records as separate audit "events" is not only conceptually odd, it is >> misleading. > > A vote of support from the user side; thanks Paul for this. The > misleading part of having multiple events for what is conceptually a > unitary event makes life really hard on guys like me trying to explain > to security people what happened when. So while I understand it can be > challenging to associate those events in the kernel, particularly when > they occur at spaced times, trying to patch those together on the > analysis side is problematic too (which I know you well understand). So > - your effort to keep these these records tied to one event is very much > appreciated! Thanks Lenny, I appreciate the feedback. We should have better connection between records when we make the audit container ID changes, if not sooner. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 05/30/2018 06:54 PM, Paul Moore wrote: ... > Finally, since you probably haven't followed all of the discussion > around associating records into a single event, I wanted to give you > my side of the story (if you don't care, you can simply skip the rest > of this email). Currently an audit "event" can consist of multiple > audit "records"; these records can contain PATH information, SYSCALL > information, SELinux AVC information, as well as INTEGRITY_PCR > information. The current kernel code does a poor job of associating > some of these record types, which can make a single user action look > like multiple audit events. For example, a single user action/event > (writing a new IMA policy) could result in multiple audit "events" > because the SYSCALL record for the write() syscall is not associated > with the INTEGRITY_POLICY_RULE record; this is bogus because the write > syscall is inherently linked with the IMA policy update. Keeping the > records as separate audit "events" is not only conceptually odd, it is > misleading. A vote of support from the user side; thanks Paul for this. The misleading part of having multiple events for what is conceptually a unitary event makes life really hard on guys like me trying to explain to security people what happened when. So while I understand it can be challenging to associate those events in the kernel, particularly when they occur at spaced times, trying to patch those together on the analysis side is problematic too (which I know you well understand). So - your effort to keep these these records tied to one event is very much appreciated! LCB -- Lenny Bruzenak MagitekLTD -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Wed, May 30, 2018 at 5:49 PM, Stefan Berger wrote: > On 05/30/2018 05:24 PM, Paul Moore wrote: >> >> On Wed, May 30, 2018 at 3:54 PM, Stefan Berger >> wrote: >>> >>> On 05/30/2018 12:27 PM, Steve Grubb wrote: On Wednesday, May 30, 2018 11:25:05 AM EDT Stefan Berger wrote: > > On 05/30/2018 11:15 AM, Steve Grubb wrote: >> >> On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote: >>> >>> On 05/29/2018 05:30 PM, Steve Grubb wrote: Hello, On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: > > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > the IMA "audit" policy action. This patch defines > AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. > > With this change we now call integrity_audit_msg_common() to get > common integrity auditing fields. This now produces the following > record when parsing an IMA policy rule: > > type=UNKNOWN[1806] msg=audit(1527004216.690:311): > action=dont_measure > \ > > fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ > tty=tty2 res=1 Since this is a new event, do you mind moving the tty field to be between auid= and ses= ? That is the more natural place for it. >>> >>> 6/8 refactors the code so that the integrity audit records produced >>> by >>> IMA follow one format in terms of ordering of the fields, with fields >>> like inode optional, though, and AUDIT_INTEGRITY_RULE in the end >>> being >>> the only one with a different format. Do we really want to change >>> that >>> order just for 1806? >>> >>> 5/8 now produces the following: >>> >>> type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \ >>> uid=0 auid=1000 ses=5 \ >>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >>> op=invalid_pcr cause=open_writers comm="grep" \ >>> name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \ >>> exe="/usr/bin/grep" tty=pts0 res=1 >>> >>> Comparing the two: >>> >>> 1806: action, fsmagic, pid, uid, auid, ses, subj, op, cause, >>> comm,exe, tty, res >>> INTEGRITY_PCR: pid, uid, auid, ses, subj, op, cause, >>> comm, name, dev, ino, exe, tty, res >> >> OK. I guess go with it as is. It passes testing. > > What about the position of 'res' field relative to the two new fields > 'exe' and 'tty'? res (results) is always the last field for every event. We have no events where it is not the last field. I'd prefer to go with it as is. The events pass my testing the way they are. > Do we want to keep them as shown or strictly append the > two new fields 'exe' and 'tty'? I'd prefer the first option to keep things as expected. > Paul seems to request that they appear after 'res'. I'd rather see them dropped, as useful as they could be, than to malform the events. >>> >>> >>> Paul NACK'ed them since he wanted to have them added to the end. You seem >>> to >>> say it's ok to add them before 'res'. Not sure whether to drop them now >>> since we are 'at it.' >> >> I talked about this in the other patch's thread, but the "new fields >> at the end of existing records" policy applies here too. > > > I am not sure what to post in v2. It looks like if I append it to the end > after 'res' I could get the NACK'ed by Steve?! My apologies, you are getting caught in the middle of this and that isn't fair to you. Steve maintains an audit userspace package, I maintain the audit subsystem in the kernel. We always try to come to a consensus, but we sometime reach a "agree to disagree" point and in those cases the maintainer breaks the tie. For the purposes of the audit kernel code, I'm the tie breaker. There have been a lot of messages spread across the threads, but I believe Steve's position was that he would prefer to not include new fields if that meant placing them after the "res" field; combined with my "new fields at the end of existing records" rule that would mean the "best" solution would be to simply not add the new fields to the existing record. New records do not have these restrictions. The good news is that Richard's suggestion of associating the record with other related records should provide most (all?) of the information that you were trying to log in the first place. Finally, since you probably haven't followed all of the discussion around associating records into a single event, I wanted to give you my side of the story (if you don't care, you can simply skip the rest of this email). Currently an audit "event" can consist
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 2018-05-30 17:38, Stefan Berger wrote: > On 05/30/2018 05:22 PM, Paul Moore wrote: > > On Wed, May 30, 2018 at 9:08 AM, Stefan Berger > > wrote: > > > On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: > > > > On 2018-05-24 16:11, Stefan Berger wrote: > > > > > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > > > > > the IMA "audit" policy action. This patch defines > > > > > AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. > > > > > > > > > > With this change we now call integrity_audit_msg_common() to get > > > > > common integrity auditing fields. This now produces the following > > > > > record when parsing an IMA policy rule: > > > > > > > > > > type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure > > > > > \ > > > > > fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ > > > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > > > > > op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" > > > > > \ > > > > > tty=tty2 res=1 > > > > > > > > > > Signed-off-by: Stefan Berger > > > > > --- > > > > >include/uapi/linux/audit.h | 3 ++- > > > > >security/integrity/ima/ima_policy.c | 5 +++-- > > > > >2 files changed, 5 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > > > > index 4e61a9e05132..776e0abd35cf 100644 > > > > > --- a/include/uapi/linux/audit.h > > > > > +++ b/include/uapi/linux/audit.h > > > > > @@ -146,7 +146,8 @@ > > > > >#define AUDIT_INTEGRITY_STATUS1802 /* Integrity enable > > > > > status */ > > > > >#define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ > > > > >#define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ > > > > > -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ > > > > > +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy > > > > > msgs */ > > > > > +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ > > > > > #define AUDIT_KERNEL2000/* Asynchronous audit > > > > > record. NOT A REQUEST. */ > > > > >diff --git a/security/integrity/ima/ima_policy.c > > > > > b/security/integrity/ima/ima_policy.c > > > > > index 3aed25a7178a..a8ae47a386b4 100644 > > > > > --- a/security/integrity/ima/ima_policy.c > > > > > +++ b/security/integrity/ima/ima_policy.c > > > > > @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct > > > > > ima_rule_entry *entry) > > > > > int result = 0; > > > > > ab = integrity_audit_log_start(NULL, GFP_KERNEL, > > > > > - AUDIT_INTEGRITY_RULE); > > > > > + AUDIT_INTEGRITY_POLICY_RULE); > > > > Is it possible to connect this record to a syscall by replacing the > > > > first parameter (NULL) by current->context? > > We're likely going to need to "associate" this record (audit speak for > > making the first parameter non-NULL) with others for the audit > > container ID work. If you do it now, Richard's patches will likely > > get a few lines smaller and that will surely make him a bit happier :) > > Richard is also introducing a local context that we can then create and use > instead of the NULL. Can we not use that then? That is for records for which there is no syscall or user associated. In fact there is another recent change that would be better to use than current->audit_context, which is the function audit_context(). See commit cdfb6b3 ("audit: use inline function to get audit context"). > Steven seems to say: "We don't want to add syscall records to everything. > That messes up schemas and existing code. The integrity events are 1 record > in size and should stay that way. This saves disk space and improves > readability." > > > > We would have to fix current->context in this case since it is NULL. We > > > get > > > to this location by root cat'ing a policy or writing a policy filename > > > into > > > /sys/kernel/security/ima/policy. > > Perhaps I'm missing something, but current in this case should point > > to the process which is writing to the policy file, yes? > > Yes, but current->context is NULL for some reason. Is it always this way? If it isn't, which it should not be, we should find out why. Well, we should find out why this is NULL anyways, since it shouldn't be. - 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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 05/30/2018 06:00 PM, Mimi Zohar wrote: On Wed, 2018-05-30 at 17:49 -0400, Stefan Berger wrote: So the other choice is to only keep patches 1,2, 6, and 7, so leave most of the integrity audit messages untouched. Then only create a different format for the new AUDIT_INTEGRITY_POLICY_RULE (current 8/8) that shares (for consistency reasons) the same format with the existing integrity audit messages but also misses tty= and exe= ? Another option would be for the new AUDIT_INTEGRITY_POLICY_RULE to call audit_log_task_info() similar to what ima_audit_measurement() does. Right. [That would mean keep 1,2, 7 and modify 8.] Is that the best solution? Mimi -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Wed, 2018-05-30 at 17:49 -0400, Stefan Berger wrote: > > So the other choice is to only keep patches 1,2, 6, and 7, so leave most > of the integrity audit messages untouched. Then only create a different > format for the new AUDIT_INTEGRITY_POLICY_RULE (current 8/8) that shares > (for consistency reasons) the same format with the existing integrity > audit messages but also misses tty= and exe= ? Another option would be for the new AUDIT_INTEGRITY_POLICY_RULE to call audit_log_task_info() similar to what ima_audit_measurement() does. Mimi -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 05/30/2018 05:22 PM, Paul Moore wrote: On Wed, May 30, 2018 at 9:08 AM, Stefan Berger wrote: On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: On 2018-05-24 16:11, Stefan Berger wrote: The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Signed-off-by: Stefan Berger --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #define AUDIT_INTEGRITY_STATUS1802 /* Integrity enable status */ #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL2000/* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); Is it possible to connect this record to a syscall by replacing the first parameter (NULL) by current->context? We're likely going to need to "associate" this record (audit speak for making the first parameter non-NULL) with others for the audit container ID work. If you do it now, Richard's patches will likely get a few lines smaller and that will surely make him a bit happier :) Richard is also introducing a local context that we can then create and use instead of the NULL. Can we not use that then? Steven seems to say: "We don't want to add syscall records to everything. That messes up schemas and existing code. The integrity events are 1 record in size and should stay that way. This saves disk space and improves readability." We would have to fix current->context in this case since it is NULL. We get to this location by root cat'ing a policy or writing a policy filename into /sys/kernel/security/ima/policy. Perhaps I'm missing something, but current in this case should point to the process which is writing to the policy file, yes? Yes, but current->context is NULL for some reason. -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Wed, May 30, 2018 at 3:54 PM, Stefan Berger wrote: > On 05/30/2018 12:27 PM, Steve Grubb wrote: >> >> On Wednesday, May 30, 2018 11:25:05 AM EDT Stefan Berger wrote: >>> >>> On 05/30/2018 11:15 AM, Steve Grubb wrote: On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote: > > On 05/29/2018 05:30 PM, Steve Grubb wrote: >> >> Hello, >> >> On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: >>> >>> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and >>> the IMA "audit" policy action. This patch defines >>> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. >>> >>> With this change we now call integrity_audit_msg_common() to get >>> common integrity auditing fields. This now produces the following >>> record when parsing an IMA policy rule: >>> >>> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure >>> \ >>> >>> fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ >>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >>> op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ >>> tty=tty2 res=1 >> >> Since this is a new event, do you mind moving the tty field to be >> between >> auid= and ses= ? That is the more natural place for it. > > 6/8 refactors the code so that the integrity audit records produced by > IMA follow one format in terms of ordering of the fields, with fields > like inode optional, though, and AUDIT_INTEGRITY_RULE in the end being > the only one with a different format. Do we really want to change that > order just for 1806? > > 5/8 now produces the following: > > type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \ > uid=0 auid=1000 ses=5 \ > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > op=invalid_pcr cause=open_writers comm="grep" \ > name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \ > exe="/usr/bin/grep" tty=pts0 res=1 > > Comparing the two: > > 1806: action, fsmagic, pid, uid, auid, ses, subj, op, cause, > comm,exe, tty, res > INTEGRITY_PCR: pid, uid, auid, ses, subj, op, cause, > comm, name, dev, ino, exe, tty, res OK. I guess go with it as is. It passes testing. >>> >>> What about the position of 'res' field relative to the two new fields >>> 'exe' and 'tty'? >> >> res (results) is always the last field for every event. We have no events >> where it is not the last field. I'd prefer to go with it as is. The events >> pass my testing the way they are. >> >>> Do we want to keep them as shown or strictly append the >>> two new fields 'exe' and 'tty'? >> >> I'd prefer the first option to keep things as expected. >> >>> Paul seems to request that they appear after 'res'. >> >> I'd rather see them dropped, as useful as they could be, than to malform >> the >> events. > > > Paul NACK'ed them since he wanted to have them added to the end. You seem to > say it's ok to add them before 'res'. Not sure whether to drop them now > since we are 'at it.' I talked about this in the other patch's thread, but the "new fields at the end of existing records" policy applies here too. Also note Richard's earlier comment about "associating" the IMA records with all of the related audit records. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Wed, May 30, 2018 at 9:08 AM, Stefan Berger wrote: > On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: >> >> On 2018-05-24 16:11, Stefan Berger wrote: >>> >>> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and >>> the IMA "audit" policy action. This patch defines >>> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. >>> >>> With this change we now call integrity_audit_msg_common() to get >>> common integrity auditing fields. This now produces the following >>> record when parsing an IMA policy rule: >>> >>> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ >>>fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ >>>subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >>>op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ >>>tty=tty2 res=1 >>> >>> Signed-off-by: Stefan Berger >>> --- >>> include/uapi/linux/audit.h | 3 ++- >>> security/integrity/ima/ima_policy.c | 5 +++-- >>> 2 files changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >>> index 4e61a9e05132..776e0abd35cf 100644 >>> --- a/include/uapi/linux/audit.h >>> +++ b/include/uapi/linux/audit.h >>> @@ -146,7 +146,8 @@ >>> #define AUDIT_INTEGRITY_STATUS1802 /* Integrity enable >>> status */ >>> #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ >>> #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ >>> -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ >>> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy >>> msgs */ >>> +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ >>> #define AUDIT_KERNEL2000/* Asynchronous audit >>> record. NOT A REQUEST. */ >>> diff --git a/security/integrity/ima/ima_policy.c >>> b/security/integrity/ima/ima_policy.c >>> index 3aed25a7178a..a8ae47a386b4 100644 >>> --- a/security/integrity/ima/ima_policy.c >>> +++ b/security/integrity/ima/ima_policy.c >>> @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct >>> ima_rule_entry *entry) >>> int result = 0; >>> ab = integrity_audit_log_start(NULL, GFP_KERNEL, >>> - AUDIT_INTEGRITY_RULE); >>> + AUDIT_INTEGRITY_POLICY_RULE); >> >> Is it possible to connect this record to a syscall by replacing the >> first parameter (NULL) by current->context? We're likely going to need to "associate" this record (audit speak for making the first parameter non-NULL) with others for the audit container ID work. If you do it now, Richard's patches will likely get a few lines smaller and that will surely make him a bit happier :) > We would have to fix current->context in this case since it is NULL. We get > to this location by root cat'ing a policy or writing a policy filename into > /sys/kernel/security/ima/policy. Perhaps I'm missing something, but current in this case should point to the process which is writing to the policy file, yes? -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 05/30/2018 12:27 PM, Steve Grubb wrote: On Wednesday, May 30, 2018 11:25:05 AM EDT Stefan Berger wrote: On 05/30/2018 11:15 AM, Steve Grubb wrote: On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote: On 05/29/2018 05:30 PM, Steve Grubb wrote: Hello, On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Since this is a new event, do you mind moving the tty field to be between auid= and ses= ? That is the more natural place for it. 6/8 refactors the code so that the integrity audit records produced by IMA follow one format in terms of ordering of the fields, with fields like inode optional, though, and AUDIT_INTEGRITY_RULE in the end being the only one with a different format. Do we really want to change that order just for 1806? 5/8 now produces the following: type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \ uid=0 auid=1000 ses=5 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=invalid_pcr cause=open_writers comm="grep" \ name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \ exe="/usr/bin/grep" tty=pts0 res=1 Comparing the two: 1806: action, fsmagic, pid, uid, auid, ses, subj, op, cause, comm,exe, tty, res INTEGRITY_PCR: pid, uid, auid, ses, subj, op, cause, comm, name, dev, ino, exe, tty, res OK. I guess go with it as is. It passes testing. What about the position of 'res' field relative to the two new fields 'exe' and 'tty'? res (results) is always the last field for every event. We have no events where it is not the last field. I'd prefer to go with it as is. The events pass my testing the way they are. Do we want to keep them as shown or strictly append the two new fields 'exe' and 'tty'? I'd prefer the first option to keep things as expected. Paul seems to request that they appear after 'res'. I'd rather see them dropped, as useful as they could be, than to malform the events. Paul NACK'ed them since he wanted to have them added to the end. You seem to say it's ok to add them before 'res'. Not sure whether to drop them now since we are 'at it.' Stefan -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Wednesday, May 30, 2018 11:25:05 AM EDT Stefan Berger wrote: > On 05/30/2018 11:15 AM, Steve Grubb wrote: > > On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote: > >> On 05/29/2018 05:30 PM, Steve Grubb wrote: > >>> Hello, > >>> > >>> On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > the IMA "audit" policy action. This patch defines > AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. > > With this change we now call integrity_audit_msg_common() to get > common integrity auditing fields. This now produces the following > record when parsing an IMA policy rule: > > type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure > \ > > fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ > tty=tty2 res=1 > >>> > >>> Since this is a new event, do you mind moving the tty field to be > >>> between > >>> auid= and ses= ? That is the more natural place for it. > >> > >> 6/8 refactors the code so that the integrity audit records produced by > >> IMA follow one format in terms of ordering of the fields, with fields > >> like inode optional, though, and AUDIT_INTEGRITY_RULE in the end being > >> the only one with a different format. Do we really want to change that > >> order just for 1806? > >> > >> 5/8 now produces the following: > >> > >> type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \ > >> uid=0 auid=1000 ses=5 \ > >> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > >> op=invalid_pcr cause=open_writers comm="grep" \ > >> name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \ > >> exe="/usr/bin/grep" tty=pts0 res=1 > >> > >> Comparing the two: > >> > >> 1806: action, fsmagic, pid, uid, auid, ses, subj, op, cause, > >> comm,exe, tty, res > >> INTEGRITY_PCR: pid, uid, auid, ses, subj, op, cause, > >> comm, name, dev, ino, exe, tty, res > > > > OK. I guess go with it as is. It passes testing. > > What about the position of 'res' field relative to the two new fields > 'exe' and 'tty'? res (results) is always the last field for every event. We have no events where it is not the last field. I'd prefer to go with it as is. The events pass my testing the way they are. > Do we want to keep them as shown or strictly append the > two new fields 'exe' and 'tty'? I'd prefer the first option to keep things as expected. > Paul seems to request that they appear after 'res'. I'd rather see them dropped, as useful as they could be, than to malform the events. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 05/30/2018 11:15 AM, Steve Grubb wrote: On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote: On 05/29/2018 05:30 PM, Steve Grubb wrote: Hello, On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Since this is a new event, do you mind moving the tty field to be between auid= and ses= ? That is the more natural place for it. 6/8 refactors the code so that the integrity audit records produced by IMA follow one format in terms of ordering of the fields, with fields like inode optional, though, and AUDIT_INTEGRITY_RULE in the end being the only one with a different format. Do we really want to change that order just for 1806? 5/8 now produces the following: type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \ uid=0 auid=1000 ses=5 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=invalid_pcr cause=open_writers comm="grep" \ name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \ exe="/usr/bin/grep" tty=pts0 res=1 Comparing the two: 1806: action, fsmagic, pid, uid, auid, ses, subj, op, cause, comm,exe, tty, res INTEGRITY_PCR: pid, uid, auid, ses, subj, op, cause, comm, name, dev, ino, exe, tty, res OK. I guess go with it as is. It passes testing. What about the position of 'res' field relative to the two new fields 'exe' and 'tty'? Do we want to keep them as shown or strictly append the two new fields 'exe' and 'tty'? Paul seems to request that they appear after 'res'. Stefan -Steve Also, it might be more natural for the op= and cause= fields to be before the pid= portion. This doesn't matter as much to me because those are not searchable fields and they are skipped right over. But moving the tty field is the main comment from me. With the refactoring in 6/8 we at least have consistency among the INTEGRITY_* records, with the only exception being AUDIT_INTEGRITY_RULE that has its own format: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_a pi.c#L324 The other ones currently all format using integrity_audit_msg(). Thanks, -Steve Signed-off-by: Stefan Berger --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */ #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL 2000/* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); entry->uid = INVALID_UID; entry->fowner = INVALID_UID; @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; else if (entry->func == POLICY_CHECK) temp_ima_appraise |= IMA_APPRAISE_POLICY; - audit_log_format(ab, "res=%d", !result); + integrity_audit_msg_common(ab, NULL, NULL, + "policy_update", "parse_rule", result); audit_log_end(ab); return result; } -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote: > On 05/29/2018 05:30 PM, Steve Grubb wrote: > > Hello, > > > > On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: > >> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > >> the IMA "audit" policy action. This patch defines > >> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. > >> > >> With this change we now call integrity_audit_msg_common() to get > >> common integrity auditing fields. This now produces the following > >> record when parsing an IMA policy rule: > >> > >> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ > >> > >>fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ > >>subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > >>op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ > >>tty=tty2 res=1 > > > > Since this is a new event, do you mind moving the tty field to be between > > auid= and ses= ? That is the more natural place for it. > > 6/8 refactors the code so that the integrity audit records produced by > IMA follow one format in terms of ordering of the fields, with fields > like inode optional, though, and AUDIT_INTEGRITY_RULE in the end being > the only one with a different format. Do we really want to change that > order just for 1806? > > 5/8 now produces the following: > > type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \ >uid=0 auid=1000 ses=5 \ >subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >op=invalid_pcr cause=open_writers comm="grep" \ >name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \ >exe="/usr/bin/grep" tty=pts0 res=1 > > Comparing the two: > > 1806: action, fsmagic, pid, uid, auid, ses, subj, op, cause, > comm,exe, tty, res > INTEGRITY_PCR: pid, uid, auid, ses, subj, op, cause, > comm, name, dev, ino, exe, tty, res OK. I guess go with it as is. It passes testing. -Steve > > Also, it might be more natural for the op= and cause= fields to be before > > the pid= portion. This doesn't matter as much to me because those are > > not searchable fields and they are skipped right over. But moving the > > tty field is the main comment from me. > > With the refactoring in 6/8 we at least have consistency among the > INTEGRITY_* records, with the only exception being AUDIT_INTEGRITY_RULE > that has its own format: > > https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_a > pi.c#L324 > > The other ones currently all format using integrity_audit_msg(). > > > Thanks, > > -Steve > > > >> Signed-off-by: Stefan Berger > >> --- > >> > >> include/uapi/linux/audit.h | 3 ++- > >> security/integrity/ima/ima_policy.c | 5 +++-- > >> 2 files changed, 5 insertions(+), 3 deletions(-) > >> > >> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > >> index 4e61a9e05132..776e0abd35cf 100644 > >> --- a/include/uapi/linux/audit.h > >> +++ b/include/uapi/linux/audit.h > >> @@ -146,7 +146,8 @@ > >> > >> #define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */ > >> #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ > >> #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ > >> > >> -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ > >> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy > >> msgs > >> */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ > >> > >> #define AUDIT_KERNEL 2000/* Asynchronous audit record. > >> NOT A > > > > REQUEST. */ > > > >> diff --git a/security/integrity/ima/ima_policy.c > >> b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 > >> 100644 > >> --- a/security/integrity/ima/ima_policy.c > >> +++ b/security/integrity/ima/ima_policy.c > >> @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct > >> ima_rule_entry *entry) int result = 0; > >> > >>ab = integrity_audit_log_start(NULL, GFP_KERNEL, > >> > >> - AUDIT_INTEGRITY_RULE); > >> + AUDIT_INTEGRITY_POLICY_RULE); > >> > >>entry->uid = INVALID_UID; > >>entry->fowner = INVALID_UID; > >> > >> @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct > >> ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; > >> > >>else if (entry->func == POLICY_CHECK) > >> > >>temp_ima_appraise |= IMA_APPRAISE_POLICY; > >> > >> - audit_log_format(ab, "res=%d", !result); > >> + integrity_audit_msg_common(ab, NULL, NULL, > >> + "policy_update", "parse_rule", result); > >> > >>audit_log_end(ab); > >>return result; > >> > >> } -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 05/29/2018 05:30 PM, Steve Grubb wrote: Hello, On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Since this is a new event, do you mind moving the tty field to be between auid= and ses= ? That is the more natural place for it. 6/8 refactors the code so that the integrity audit records produced by IMA follow one format in terms of ordering of the fields, with fields like inode optional, though, and AUDIT_INTEGRITY_RULE in the end being the only one with a different format. Do we really want to change that order just for 1806? 5/8 now produces the following: type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \ uid=0 auid=1000 ses=5 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=invalid_pcr cause=open_writers comm="grep" \ name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \ exe="/usr/bin/grep" tty=pts0 res=1 Comparing the two: 1806: action, fsmagic, pid, uid, auid, ses, subj, op, cause, comm, exe, tty, res INTEGRITY_PCR: pid, uid, auid, ses, subj, op, cause, comm, name, dev, ino, exe, tty, res Also, it might be more natural for the op= and cause= fields to be before the pid= portion. This doesn't matter as much to me because those are not searchable fields and they are skipped right over. But moving the tty field is the main comment from me. With the refactoring in 6/8 we at least have consistency among the INTEGRITY_* records, with the only exception being AUDIT_INTEGRITY_RULE that has its own format: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L324 The other ones currently all format using integrity_audit_msg(). Thanks, -Steve Signed-off-by: Stefan Berger --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #define AUDIT_INTEGRITY_STATUS1802 /* Integrity enable status */ #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL 2000/* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); entry->uid = INVALID_UID; entry->fowner = INVALID_UID; @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; else if (entry->func == POLICY_CHECK) temp_ima_appraise |= IMA_APPRAISE_POLICY; - audit_log_format(ab, "res=%d", !result); + integrity_audit_msg_common(ab, NULL, NULL, + "policy_update", "parse_rule", result); audit_log_end(ab); return result; } -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: On 2018-05-24 16:11, Stefan Berger wrote: The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Signed-off-by: Stefan Berger --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #define AUDIT_INTEGRITY_STATUS1802 /* Integrity enable status */ #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); Is it possible to connect this record to a syscall by replacing the first parameter (NULL) by current->context? We would have to fix current->context in this case since it is NULL. We get to this location by root cat'ing a policy or writing a policy filename into /sys/kernel/security/ima/policy. entry->uid = INVALID_UID; entry->fowner = INVALID_UID; @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; else if (entry->func == POLICY_CHECK) temp_ima_appraise |= IMA_APPRAISE_POLICY; - audit_log_format(ab, "res=%d", !result); + integrity_audit_msg_common(ab, NULL, NULL, + "policy_update", "parse_rule", result); audit_log_end(ab); return result; } - 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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
On Wednesday, May 30, 2018 8:49:20 AM EDT Richard Guy Briggs wrote: > On 2018-05-24 16:11, Stefan Berger wrote: > > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > > the IMA "audit" policy action. This patch defines > > AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. > > > > With this change we now call integrity_audit_msg_common() to get > > common integrity auditing fields. This now produces the following > > record when parsing an IMA policy rule: > > > > type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ > > > > fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > > op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ > > tty=tty2 res=1 > > > > Signed-off-by: Stefan Berger > > --- > > > > include/uapi/linux/audit.h | 3 ++- > > security/integrity/ima/ima_policy.c | 5 +++-- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > index 4e61a9e05132..776e0abd35cf 100644 > > --- a/include/uapi/linux/audit.h > > +++ b/include/uapi/linux/audit.h > > @@ -146,7 +146,8 @@ > > > > #define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */ > > #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ > > #define AUDIT_INTEGRITY_PCR1804 /* PCR invalidation msgs */ > > > > -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ > > +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy > > msgs > > */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ > > > > #define AUDIT_KERNEL 2000/* Asynchronous audit record. > > NOT A REQUEST. > > */ > > > > diff --git a/security/integrity/ima/ima_policy.c > > b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 > > 100644 > > --- a/security/integrity/ima/ima_policy.c > > +++ b/security/integrity/ima/ima_policy.c > > @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct > > ima_rule_entry *entry)> > > int result = 0; > > > > ab = integrity_audit_log_start(NULL, GFP_KERNEL, > > > > - AUDIT_INTEGRITY_RULE); > > + AUDIT_INTEGRITY_POLICY_RULE); > > Is it possible to connect this record to a syscall by replacing the > first parameter (NULL) by current->context? We don't want to add syscall records to everything. That messes up schemas and existing code. The integrity events are 1 record in size and should stay that way. This saves disk space and improves readability. -Steve > > entry->uid = INVALID_UID; > > entry->fowner = INVALID_UID; > > > > @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct > > ima_rule_entry *entry)> > > temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; > > > > else if (entry->func == POLICY_CHECK) > > > > temp_ima_appraise |= IMA_APPRAISE_POLICY; > > > > - audit_log_format(ab, "res=%d", !result); > > + integrity_audit_msg_common(ab, NULL, NULL, > > + "policy_update", "parse_rule", result); > > > > audit_log_end(ab); > > return result; > > > > } > > - 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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
Hello, On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > the IMA "audit" policy action. This patch defines > AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. > > With this change we now call integrity_audit_msg_common() to get > common integrity auditing fields. This now produces the following > record when parsing an IMA policy rule: > > type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ > fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ > tty=tty2 res=1 Since this is a new event, do you mind moving the tty field to be between auid= and ses= ? That is the more natural place for it. Also, it might be more natural for the op= and cause= fields to be before the pid= portion. This doesn't matter as much to me because those are not searchable fields and they are skipped right over. But moving the tty field is the main comment from me. Thanks, -Steve > Signed-off-by: Stefan Berger > --- > include/uapi/linux/audit.h | 3 ++- > security/integrity/ima/ima_policy.c | 5 +++-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index 4e61a9e05132..776e0abd35cf 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -146,7 +146,8 @@ > #define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */ > #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ > #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ > -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ > +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs > */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ > > #define AUDIT_KERNEL 2000/* Asynchronous audit record. NOT A REQUEST. */ > > diff --git a/security/integrity/ima/ima_policy.c > b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 > 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct > ima_rule_entry *entry) int result = 0; > > ab = integrity_audit_log_start(NULL, GFP_KERNEL, > -AUDIT_INTEGRITY_RULE); > +AUDIT_INTEGRITY_POLICY_RULE); > > entry->uid = INVALID_UID; > entry->fowner = INVALID_UID; > @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct > ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; > else if (entry->func == POLICY_CHECK) > temp_ima_appraise |= IMA_APPRAISE_POLICY; > - audit_log_format(ab, "res=%d", !result); > + integrity_audit_msg_common(ab, NULL, NULL, > +"policy_update", "parse_rule", result); > audit_log_end(ab); > return result; > } -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions
The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Signed-off-by: Stefan Berger--- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */ #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ #define AUDIT_INTEGRITY_PCR1804 /* PCR invalidation msgs */ -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL 2000/* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); entry->uid = INVALID_UID; entry->fowner = INVALID_UID; @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; else if (entry->func == POLICY_CHECK) temp_ima_appraise |= IMA_APPRAISE_POLICY; - audit_log_format(ab, "res=%d", !result); + integrity_audit_msg_common(ab, NULL, NULL, + "policy_update", "parse_rule", result); audit_log_end(ab); return result; } -- 2.13.6 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit