| Message ID | 1520257393.10396.291.camel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-03-05 08:43, Mimi Zohar wrote: > Hi Richard, > > This patch has been compiled, but not runtime tested. Ok, great, thank you. I assume you are offering this patch to be included in this patchset? I'll have a look to see where it fits in the IMA record. It might be better if it were an AUDIT_CONTAINER_INFO auxiliary record, but I'll have a look at the circumstances of the event. Can you suggest a procedure to test it? > --- > > If the containerid is defined, include it in the IMA-audit record. > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > --- > security/integrity/ima/ima_api.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > index 33b4458cdbef..41d29a06f28f 100644 > --- a/security/integrity/ima/ima_api.c > +++ b/security/integrity/ima/ima_api.c > @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, > audit_log_untrustedstring(ab, algo_hash); > > audit_log_task_info(ab, current); > + if (audit_containerid_set(current)) > + audit_log_format(ab, " contid=%llu", > + audit_get_containerid(current)); > audit_log_end(ab); > > iint->flags |= IMA_AUDITED; > -- > 2.7.5 > - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: > On 2018-03-05 08:43, Mimi Zohar wrote: > > Hi Richard, > > > > This patch has been compiled, but not runtime tested. > > Ok, great, thank you. I assume you are offering this patch to be > included in this patchset? Yes, thank you. > I'll have a look to see where it fits in the > IMA record. It might be better if it were an AUDIT_CONTAINER_INFO > auxiliary record, but I'll have a look at the circumstances of the > event. Can you suggest a procedure to test it? Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on policy. The example IMA policy, below, includes IMA-audit messages for files executed. 'cat' the policy to /sys/kernel/security/ima/policy. /etc/ima/ima-policy: audit func=BPRM_CHECK There's a FireEye blog titled "Extending Linux Executable Logging With The Integrity Measurement Architecture"* that explains how to augment their existing system security analytics with file hashes. * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux _exec.html Mimi > > > --- > > > > If the containerid is defined, include it in the IMA-audit record. > > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > --- > > security/integrity/ima/ima_api.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > > index 33b4458cdbef..41d29a06f28f 100644 > > --- a/security/integrity/ima/ima_api.c > > +++ b/security/integrity/ima/ima_api.c > > @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, > > audit_log_untrustedstring(ab, algo_hash); > > > > audit_log_task_info(ab, current); > > + if (audit_containerid_set(current)) > > + audit_log_format(ab, " contid=%llu", > > + audit_get_containerid(current)); > > audit_log_end(ab); > > > > iint->flags |= IMA_AUDITED; > > -- > > 2.7.5 > > > > - RGB > > -- > Richard Guy Briggs <rgb@redhat.com> > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 >
On 2018-03-05 09:24, Mimi Zohar wrote: > On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: > > On 2018-03-05 08:43, Mimi Zohar wrote: > > > Hi Richard, > > > > > > This patch has been compiled, but not runtime tested. > > > > Ok, great, thank you. I assume you are offering this patch to be > > included in this patchset? > > Yes, thank you. > > > I'll have a look to see where it fits in the > > IMA record. It might be better if it were an AUDIT_CONTAINER_INFO > > auxiliary record, but I'll have a look at the circumstances of the > > event. I had a look at the context of this record to see if adding the contid field to it made sense. I think the only records for which the contid field makes sense are the two newly proposed records, AUDIT_CONTAINER which introduces the container ID and the and AUDIT_CONTAINER_INFO which documents the presence of the container ID in a process event (or process-less network event). All others should use the auxiliary record AUDIT_CONTAINER_INFO rather than include the contid field directly itself. There are several reasons for this including record length, the ability to filter unwanted records, the difficulty of changing the order of or removing fields in the future. Syscalls get this information automatically if the container ID is set for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a syscall event is one that uses the task's audit_context while a standalone event uses NULL or builds a local audit_context that is discarded immediately after the local use. Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it appears that they should be split into two distinct audit record types. The record created in ima_audit_measurement() is a syscall record that could possibly stand on its own since the subject attributes are present. If it remains a syscall auxiliary record it will automatically have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is decided to detach it (which would save cpu/netlink/disk bandwidth but is not recommended due to not wanting to throw away any other syscall information or other involved records (PATH, CWD, etc...) then a local audit_context would be created for the AUDIT_INTEGRITY_RULE and AUDIT_CONTAINERID_INFO records only and immediately discarded. The record created in ima_parse_rule() is not currently a syscall record since it is passed an audit_context of NULL and it has a very different format that does not include any subject attributes (except subj_*=). At first glance it appears this one should be a syscall accompanied auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO auxiliary record either by being converted to a syscall auxiliary record by using current->audit_context rather than NULL when calling audit_log_start(), or creating a local audit_context and calling audit_log_container_info() then releasing the local context. This version of the record has additional concerns covered here: https://github.com/linux-audit/audit-kernel/issues/52 Can you briefly describe the circumstances under which these two different identically-numbered records are produced as a first step towards splitting them into two distict records? The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records appear to be already properly covered for AUDIT_CONTAINER_INFO records by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record appears to be unused. > > Can you suggest a procedure to test it? > > Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on > policy. The example IMA policy, below, includes IMA-audit messages for > files executed. 'cat' the policy to /sys/kernel/security/ima/policy. > > /etc/ima/ima-policy: > audit func=BPRM_CHECK > > There's a FireEye blog titled "Extending Linux Executable Logging With > The Integrity Measurement Architecture"* that explains how to augment > their existing system security analytics with file hashes. > > * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux > _exec.html > > > Mimi > > > > If the containerid is defined, include it in the IMA-audit record. > > > > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > > --- > > > security/integrity/ima/ima_api.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > > > index 33b4458cdbef..41d29a06f28f 100644 > > > --- a/security/integrity/ima/ima_api.c > > > +++ b/security/integrity/ima/ima_api.c > > > @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, > > > audit_log_untrustedstring(ab, algo_hash); > > > > > > audit_log_task_info(ab, current); > > > + if (audit_containerid_set(current)) > > > + audit_log_format(ab, " contid=%llu", > > > + audit_get_containerid(current)); > > > audit_log_end(ab); > > > > > > iint->flags |= IMA_AUDITED; > > > -- > > > 2.7.5 > > > > > > > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Thu, 2018-03-08 at 06:21 -0500, Richard Guy Briggs wrote: > On 2018-03-05 09:24, Mimi Zohar wrote: > > On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: > > > On 2018-03-05 08:43, Mimi Zohar wrote: > > > > Hi Richard, > > > > > > > > This patch has been compiled, but not runtime tested. > > > > > > Ok, great, thank you. I assume you are offering this patch to be > > > included in this patchset? > > > > Yes, thank you. > > > > > I'll have a look to see where it fits in the > > > IMA record. It might be better if it were an AUDIT_CONTAINER_INFO > > > auxiliary record, but I'll have a look at the circumstances of the > > > event. > > I had a look at the context of this record to see if adding the contid > field to it made sense. I think the only records for which the contid > field makes sense are the two newly proposed records, AUDIT_CONTAINER > which introduces the container ID and the and AUDIT_CONTAINER_INFO which > documents the presence of the container ID in a process event (or > process-less network event). All others should use the auxiliary record > AUDIT_CONTAINER_INFO rather than include the contid field directly > itself. There are several reasons for this including record length, the > ability to filter unwanted records, the difficulty of changing the order > of or removing fields in the future. > > Syscalls get this information automatically if the container ID is set > for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a > syscall event is one that uses the task's audit_context while a > standalone event uses NULL or builds a local audit_context that is > discarded immediately after the local use. > > Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it > appears that they should be split into two distinct audit record types. > > The record created in ima_audit_measurement() is a syscall record that > could possibly stand on its own since the subject attributes are > present. If it remains a syscall auxiliary record it will automatically > have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is > decided to detach it (which would save cpu/netlink/disk bandwidth but is > not recommended due to not wanting to throw away any other syscall > information or other involved records (PATH, CWD, etc...) then a local > audit_context would be created for the AUDIT_INTEGRITY_RULE and > AUDIT_CONTAINERID_INFO records only and immediately discarded. > > The record created in ima_parse_rule() is not currently a syscall record > since it is passed an audit_context of NULL and it has a very different > format that does not include any subject attributes (except subj_*=). > At first glance it appears this one should be a syscall accompanied > auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO > auxiliary record either by being converted to a syscall auxiliary record > by using current->audit_context rather than NULL when calling > audit_log_start(), or creating a local audit_context and calling > audit_log_container_info() then releasing the local context. This > version of the record has additional concerns covered here: > https://github.com/linux-audit/audit-kernel/issues/52 > > Can you briefly describe the circumstances under which these two > different identically-numbered records are produced as a first step > towards splitting them into two distict records? Agreed, the two uses should really be separated. ima_parse_rule() generates audit messages, when the IMA policy is initially loaded, replaced, or extended, the policy rules are included in the audit log. When IMA is namespaced, there will be a host policy and namespace policies. We'll need to be able differentiate between the host policy rules and IMA namespaced policy rules, and between IMA namespaced policy rules. The audit messages produced by ima_audit_measurement() were originally upstreamed for forensics, and as seen by the FireEye blog are now used to augment existing security analytics. These records are probably being used independently of any other audit records. A single record is generated per file, per system. With IMA namespacing, these records need to be generated once per file, per namespace as well. In order to differentiate the records between the host and namespace, and between namespaces, these records should include the container id. To disambiguate between these audit messages and the policy rule messages, we could rename these audit messages to AUDIT_INTEGRITY_IMA. > The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records > appear to be already properly covered for AUDIT_CONTAINER_INFO records > by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record > appears to be unused. Ok Mimi
On 2018-03-08 13:02, Mimi Zohar wrote: > On Thu, 2018-03-08 at 06:21 -0500, Richard Guy Briggs wrote: > > On 2018-03-05 09:24, Mimi Zohar wrote: > > > On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: > > > > On 2018-03-05 08:43, Mimi Zohar wrote: > > > > > Hi Richard, > > > > > > > > > > This patch has been compiled, but not runtime tested. > > > > > > > > Ok, great, thank you. I assume you are offering this patch to be > > > > included in this patchset? > > > > > > Yes, thank you. > > > > > > > I'll have a look to see where it fits in the > > > > IMA record. It might be better if it were an AUDIT_CONTAINER_INFO > > > > auxiliary record, but I'll have a look at the circumstances of the > > > > event. > > > > I had a look at the context of this record to see if adding the contid > > field to it made sense. I think the only records for which the contid > > field makes sense are the two newly proposed records, AUDIT_CONTAINER > > which introduces the container ID and the and AUDIT_CONTAINER_INFO which > > documents the presence of the container ID in a process event (or > > process-less network event). All others should use the auxiliary record > > AUDIT_CONTAINER_INFO rather than include the contid field directly > > itself. There are several reasons for this including record length, the > > ability to filter unwanted records, the difficulty of changing the order > > of or removing fields in the future. > > > > Syscalls get this information automatically if the container ID is set > > for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a > > syscall event is one that uses the task's audit_context while a > > standalone event uses NULL or builds a local audit_context that is > > discarded immediately after the local use. > > > > Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it > > appears that they should be split into two distinct audit record types. > > > > The record created in ima_audit_measurement() is a syscall record that > > could possibly stand on its own since the subject attributes are > > present. If it remains a syscall auxiliary record it will automatically > > have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is > > decided to detach it (which would save cpu/netlink/disk bandwidth but is > > not recommended due to not wanting to throw away any other syscall > > information or other involved records (PATH, CWD, etc...) then a local > > audit_context would be created for the AUDIT_INTEGRITY_RULE and > > AUDIT_CONTAINERID_INFO records only and immediately discarded. > > > > The record created in ima_parse_rule() is not currently a syscall record > > since it is passed an audit_context of NULL and it has a very different > > format that does not include any subject attributes (except subj_*=). > > At first glance it appears this one should be a syscall accompanied > > auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO > > auxiliary record either by being converted to a syscall auxiliary record > > by using current->audit_context rather than NULL when calling > > audit_log_start(), or creating a local audit_context and calling > > audit_log_container_info() then releasing the local context. This > > version of the record has additional concerns covered here: > > https://github.com/linux-audit/audit-kernel/issues/52 > > > > Can you briefly describe the circumstances under which these two > > different identically-numbered records are produced as a first step > > towards splitting them into two distict records? > > Agreed, the two uses should really be separated. ima_parse_rule() > generates audit messages, when the IMA policy is initially loaded, > replaced, or extended, the policy rules are included in the audit log. > When IMA is namespaced, there will be a host policy and namespace > policies. We'll need to be able differentiate between the host policy > rules and IMA namespaced policy rules, and between IMA namespaced > policy rules. I would argue this type of message/record should be converted to an accompanied syscall record, or have the subject attributes added to the record so that it is clear what user/process initiated this action. It now occurs to me that to save audit communications and disk bandwidth, one syscall record could accompany all the rule records, but if the list is long enough it might overwhelm userspace audit event parsing code. Steve? Regardless, the ima_parse_rule() record format needs to be fixed to address the non-standard use of "<" and ">" operators instead of the standard "=" field/value separator. > The audit messages produced by ima_audit_measurement() were originally > upstreamed for forensics, and as seen by the FireEye blog are now used > to augment existing security analytics. These records are probably > being used independently of any other audit records. A single record > is generated per file, per system. With IMA namespacing, these > records need to be generated once per file, per namespace as well. In > order to differentiate the records between the host and namespace, and > between namespaces, these records should include the container id. Ok, so this might be one circumstance where the container id field could make sense to include it in the primary record, but it will already be automatically getting an AUDIT_CONTAINER_INFO record by virtue of being a syscall auxiliary record. You say "These records are probably being used independently of any other audit records.", but unless you are certain these are not used by any other tools, it will have to remain as a syscall auxiliary record. This means it will duplicate the container id info if the container id field is added to the AUDIT_INTEGRITY_RULE record or cause your tools to need to be able to parse audit *events* rather than just individual audit *records* to get the associated container id if the container id field is not added to the AUDIT_INTEGRITY_RULE. > To disambiguate between these audit messages and the policy rule > messages, we could rename these audit messages to AUDIT_INTEGRITY_IMA. This makes sense to me, but will depend on other users of this record type. Since there are already two very different formats for this one existing record type, changing the record type either doesn't matter if nothing else has noticed or this is what triggered the examination of this issue in the first place and should be fixed. > > The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records > > appear to be already properly covered for AUDIT_CONTAINER_INFO records > > by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record > > appears to be unused. > > Ok > > Mimi - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On 03/08/2018 06:21 AM, Richard Guy Briggs wrote: > On 2018-03-05 09:24, Mimi Zohar wrote: >> On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: >>> On 2018-03-05 08:43, Mimi Zohar wrote: >>>> Hi Richard, >>>> >>>> This patch has been compiled, but not runtime tested. >>> Ok, great, thank you. I assume you are offering this patch to be >>> included in this patchset? >> Yes, thank you. >> >>> I'll have a look to see where it fits in the >>> IMA record. It might be better if it were an AUDIT_CONTAINER_INFO >>> auxiliary record, but I'll have a look at the circumstances of the >>> event. > I had a look at the context of this record to see if adding the contid > field to it made sense. I think the only records for which the contid > field makes sense are the two newly proposed records, AUDIT_CONTAINER > which introduces the container ID and the and AUDIT_CONTAINER_INFO which > documents the presence of the container ID in a process event (or > process-less network event). All others should use the auxiliary record > AUDIT_CONTAINER_INFO rather than include the contid field directly > itself. There are several reasons for this including record length, the > ability to filter unwanted records, the difficulty of changing the order > of or removing fields in the future. > > Syscalls get this information automatically if the container ID is set > for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a > syscall event is one that uses the task's audit_context while a > standalone event uses NULL or builds a local audit_context that is > discarded immediately after the local use. > > Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it > appears that they should be split into two distinct audit record types. > > The record created in ima_audit_measurement() is a syscall record that > could possibly stand on its own since the subject attributes are > present. If it remains a syscall auxiliary record it will automatically > have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is > decided to detach it (which would save cpu/netlink/disk bandwidth but is > not recommended due to not wanting to throw away any other syscall > information or other involved records (PATH, CWD, etc...) then a local > audit_context would be created for the AUDIT_INTEGRITY_RULE and > AUDIT_CONTAINERID_INFO records only and immediately discarded. What does 'detach it' mean? Does it mean we're not using current->audit_context? > > The record created in ima_parse_rule() is not currently a syscall record > since it is passed an audit_context of NULL and it has a very different > format that does not include any subject attributes (except subj_*=). > At first glance it appears this one should be a syscall accompanied > auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO Do you have an example (pointer) to the format for a 'syscall accompanied auxiliary record'? > auxiliary record either by being converted to a syscall auxiliary record > by using current->audit_context rather than NULL when calling > audit_log_start(), or creating a local audit_context and calling ima_parse_rule() is invoked when setting a policy by writing it into /sys/kernel/security/ima/policy. We unfortunately don't have the current->audit_context in this case. > audit_log_container_info() then releasing the local context. This > version of the record has additional concerns covered here: > https://github.com/linux-audit/audit-kernel/issues/52 Following the discussion there and the concern with breaking user space, how can we split up the AUDIT_INTEGRITY_RULE that is used in ima_audit_measurement() and ima_parse_rule(), without 'breaking user space'? A message produced by ima_parse_rule() looks like this here: type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" fsmagic="0x9fa0" res=1 in contrast to that an INTEGRITY_PCR record type: type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op="invalid_pcr" cause="open_writers" comm="scp" name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE? If so, which ones? We could probably refactor the current integrity_audit_message() and have ima_parse_rule() call into it to get those fields as well. I suppose adding new fields to it wouldn't be considered breaking user space? Stefan > > Can you briefly describe the circumstances under which these two > different identically-numbered records are produced as a first step > towards splitting them into two distict records? > > The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records > appear to be already properly covered for AUDIT_CONTAINER_INFO records > by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record > appears to be unused. > >>> Can you suggest a procedure to test it? >> Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on >> policy. The example IMA policy, below, includes IMA-audit messages for >> files executed. 'cat' the policy to /sys/kernel/security/ima/policy. >> >> /etc/ima/ima-policy: >> audit func=BPRM_CHECK >> >> There's a FireEye blog titled "Extending Linux Executable Logging With >> The Integrity Measurement Architecture"* that explains how to augment >> their existing system security analytics with file hashes. >> >> * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux >> _exec.html >> >> >> Mimi >> >>>> If the containerid is defined, include it in the IMA-audit record. >>>> >>>> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> >>>> --- >>>> security/integrity/ima/ima_api.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c >>>> index 33b4458cdbef..41d29a06f28f 100644 >>>> --- a/security/integrity/ima/ima_api.c >>>> +++ b/security/integrity/ima/ima_api.c >>>> @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, >>>> audit_log_untrustedstring(ab, algo_hash); >>>> >>>> audit_log_task_info(ab, current); >>>> + if (audit_containerid_set(current)) >>>> + audit_log_format(ab, " contid=%llu", >>>> + audit_get_containerid(current)); >>>> audit_log_end(ab); >>>> >>>> iint->flags |= IMA_AUDITED; >>>> -- >>>> 2.7.5 >>>> >>> - RGB > - RGB > > -- > Richard Guy Briggs <rgb@redhat.com> > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 >
On 2018-05-17 10:18, Stefan Berger wrote: > On 03/08/2018 06:21 AM, Richard Guy Briggs wrote: > > On 2018-03-05 09:24, Mimi Zohar wrote: > > > On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: > > > > On 2018-03-05 08:43, Mimi Zohar wrote: > > > > > Hi Richard, > > > > > > > > > > This patch has been compiled, but not runtime tested. > > > > Ok, great, thank you. I assume you are offering this patch to be > > > > included in this patchset? > > > Yes, thank you. > > > > > > > I'll have a look to see where it fits in the > > > > IMA record. It might be better if it were an AUDIT_CONTAINER_INFO > > > > auxiliary record, but I'll have a look at the circumstances of the > > > > event. > > I had a look at the context of this record to see if adding the contid > > field to it made sense. I think the only records for which the contid > > field makes sense are the two newly proposed records, AUDIT_CONTAINER > > which introduces the container ID and the and AUDIT_CONTAINER_INFO which > > documents the presence of the container ID in a process event (or > > process-less network event). All others should use the auxiliary record > > AUDIT_CONTAINER_INFO rather than include the contid field directly > > itself. There are several reasons for this including record length, the > > ability to filter unwanted records, the difficulty of changing the order > > of or removing fields in the future. > > > > Syscalls get this information automatically if the container ID is set > > for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a > > syscall event is one that uses the task's audit_context while a > > standalone event uses NULL or builds a local audit_context that is > > discarded immediately after the local use. > > > > Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it > > appears that they should be split into two distinct audit record types. > > > > The record created in ima_audit_measurement() is a syscall record that > > could possibly stand on its own since the subject attributes are > > present. If it remains a syscall auxiliary record it will automatically > > have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is > > decided to detach it (which would save cpu/netlink/disk bandwidth but is > > not recommended due to not wanting to throw away any other syscall > > information or other involved records (PATH, CWD, etc...) then a local > > audit_context would be created for the AUDIT_INTEGRITY_RULE and > > AUDIT_CONTAINERID_INFO records only and immediately discarded. > > What does 'detach it' mean? Does it mean we're not using > current->audit_context? Exactly. > > The record created in ima_parse_rule() is not currently a syscall record > > since it is passed an audit_context of NULL and it has a very different > > format that does not include any subject attributes (except subj_*=). > > At first glance it appears this one should be a syscall accompanied > > auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO > > Do you have an example (pointer) to the format for a 'syscall accompanied > auxiliary record'? Any that uses current->audit_context (or recently proposed audit_context() function) will be a syscall auxiliary record. Well formed record formats are <fieldname>=<value> and named as listed: https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv > > auxiliary record either by being converted to a syscall auxiliary record > > by using current->audit_context rather than NULL when calling > > audit_log_start(), or creating a local audit_context and calling > > ima_parse_rule() is invoked when setting a policy by writing it into > /sys/kernel/security/ima/policy. We unfortunately don't have the > current->audit_context in this case. Sure you do. What process writes to that file? That's the one we care about, unless it is somehow handed off to a queue and processed later in a different context. > > audit_log_container_info() then releasing the local context. This > > version of the record has additional concerns covered here: > > https://github.com/linux-audit/audit-kernel/issues/52 > > Following the discussion there and the concern with breaking user space, how > can we split up the AUDIT_INTEGRITY_RULE that is used in > ima_audit_measurement() and ima_parse_rule(), without 'breaking user space'? Arguably userspace is already broken by this wildly diverging pair of formats for the same record. > A message produced by ima_parse_rule() looks like this here: > > type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" > fsmagic="0x9fa0" res=1 > > in contrast to that an INTEGRITY_PCR record type: > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > op="invalid_pcr" cause="open_writers" comm="scp" > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 > > Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE? Not necessarily. There are a number of records in the PCR record that would be redundant when connected to a syscall record, but removing them is discouraged to avoid breaking parsers that expect them. I don't see any need to touch the PCR record. > If so, which ones? We could probably refactor the current > integrity_audit_message() and have ima_parse_rule() call into it to get > those fields as well. I suppose adding new fields to it wouldn't be > considered breaking user space? Changing the order of existing fields or inserting fields could break stuff and is strongly discouraged without a good reason, but appending fields is usually the right way to add information. There are exceptions, and in this case, I'd pick the "more standard" of the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick with that, abandoning the other format, renaming the less standard version of the record (ima_parse_rule?) and perhpas adopting that abandonned format for the new record type while using current->audit_context. Does this help? > Stefan > > > Can you briefly describe the circumstances under which these two > > different identically-numbered records are produced as a first step > > towards splitting them into two distict records? > > > > The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records > > appear to be already properly covered for AUDIT_CONTAINER_INFO records > > by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record > > appears to be unused. > > > > > > Can you suggest a procedure to test it? > > > Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on > > > policy. The example IMA policy, below, includes IMA-audit messages for > > > files executed. 'cat' the policy to /sys/kernel/security/ima/policy. > > > > > > /etc/ima/ima-policy: > > > audit func=BPRM_CHECK > > > > > > There's a FireEye blog titled "Extending Linux Executable Logging With > > > The Integrity Measurement Architecture"* that explains how to augment > > > their existing system security analytics with file hashes. > > > > > > * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux > > > _exec.html > > > > > > > > > Mimi > > > > > > > > If the containerid is defined, include it in the IMA-audit record. > > > > > > > > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > > > > --- > > > > > security/integrity/ima/ima_api.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > > > > > index 33b4458cdbef..41d29a06f28f 100644 > > > > > --- a/security/integrity/ima/ima_api.c > > > > > +++ b/security/integrity/ima/ima_api.c > > > > > @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, > > > > > audit_log_untrustedstring(ab, algo_hash); > > > > > audit_log_task_info(ab, current); > > > > > + if (audit_containerid_set(current)) > > > > > + audit_log_format(ab, " contid=%llu", > > > > > + audit_get_containerid(current)); > > > > > audit_log_end(ab); > > > > > iint->flags |= IMA_AUDITED; > > > > > -- > > > > > 2.7.5 > > > > > > > > > - RGB > > - RGB > > > > -- > > Richard Guy Briggs <rgb@redhat.com> > > Sr. S/W Engineer, Kernel Security, Base Operating Systems > > Remote, Ottawa, Red Hat Canada > > IRC: rgb, SunRaycer > > Voice: +1.647.777.2635, Internal: (81) 32635 > > > - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On 05/17/2018 05:30 PM, Richard Guy Briggs wrote: > On 2018-05-17 10:18, Stefan Berger wrote: >> On 03/08/2018 06:21 AM, Richard Guy Briggs wrote: >>> On 2018-03-05 09:24, Mimi Zohar wrote: >>>> On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: >>>>> On 2018-03-05 08:43, Mimi Zohar wrote: >>>>>> Hi Richard, >>>>>> >>>>>> This patch has been compiled, but not runtime tested. >>>>> Ok, great, thank you. I assume you are offering this patch to be >>>>> included in this patchset? >>>> Yes, thank you. >>>> >>>>> I'll have a look to see where it fits in the >>>>> IMA record. It might be better if it were an AUDIT_CONTAINER_INFO >>>>> auxiliary record, but I'll have a look at the circumstances of the >>>>> event. >>> I had a look at the context of this record to see if adding the contid >>> field to it made sense. I think the only records for which the contid >>> field makes sense are the two newly proposed records, AUDIT_CONTAINER >>> which introduces the container ID and the and AUDIT_CONTAINER_INFO which >>> documents the presence of the container ID in a process event (or >>> process-less network event). All others should use the auxiliary record >>> AUDIT_CONTAINER_INFO rather than include the contid field directly >>> itself. There are several reasons for this including record length, the >>> ability to filter unwanted records, the difficulty of changing the order >>> of or removing fields in the future. >>> >>> Syscalls get this information automatically if the container ID is set >>> for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a >>> syscall event is one that uses the task's audit_context while a >>> standalone event uses NULL or builds a local audit_context that is >>> discarded immediately after the local use. >>> >>> Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it >>> appears that they should be split into two distinct audit record types. >>> >>> The record created in ima_audit_measurement() is a syscall record that >>> could possibly stand on its own since the subject attributes are >>> present. If it remains a syscall auxiliary record it will automatically >>> have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is >>> decided to detach it (which would save cpu/netlink/disk bandwidth but is >>> not recommended due to not wanting to throw away any other syscall >>> information or other involved records (PATH, CWD, etc...) then a local >>> audit_context would be created for the AUDIT_INTEGRITY_RULE and >>> AUDIT_CONTAINERID_INFO records only and immediately discarded. >> What does 'detach it' mean? Does it mean we're not using >> current->audit_context? > Exactly. > >>> The record created in ima_parse_rule() is not currently a syscall record >>> since it is passed an audit_context of NULL and it has a very different >>> format that does not include any subject attributes (except subj_*=). >>> At first glance it appears this one should be a syscall accompanied >>> auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO >> Do you have an example (pointer) to the format for a 'syscall accompanied >> auxiliary record'? > Any that uses current->audit_context (or recently proposed > audit_context() function) will be a syscall auxiliary record. Well > formed record formats are <fieldname>=<value> and named as listed: > > https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events > https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv > >>> auxiliary record either by being converted to a syscall auxiliary record >>> by using current->audit_context rather than NULL when calling >>> audit_log_start(), or creating a local audit_context and calling >> ima_parse_rule() is invoked when setting a policy by writing it into >> /sys/kernel/security/ima/policy. We unfortunately don't have the >> current->audit_context in this case. > Sure you do. What process writes to that file? That's the one we care > about, unless it is somehow handed off to a queue and processed later in > a different context. I just printk'd it again. current->audit_context is NULL in this case. > >>> audit_log_container_info() then releasing the local context. This >>> version of the record has additional concerns covered here: >>> https://github.com/linux-audit/audit-kernel/issues/52 >> Following the discussion there and the concern with breaking user space, how >> can we split up the AUDIT_INTEGRITY_RULE that is used in >> ima_audit_measurement() and ima_parse_rule(), without 'breaking user space'? > Arguably userspace is already broken by this wildly diverging pair of > formats for the same record. > >> A message produced by ima_parse_rule() looks like this here: >> >> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" >> fsmagic="0x9fa0" res=1 >> >> in contrast to that an INTEGRITY_PCR record type: >> >> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 >> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 >> op="invalid_pcr" cause="open_writers" comm="scp" >> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 >> >> Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE? > Not necessarily. There are a number of records in the PCR record that > would be redundant when connected to a syscall record, but removing them > is discouraged to avoid breaking parsers that expect them. > > I don't see any need to touch the PCR record. I wasn't going to touch the PCR record. >> If so, which ones? We could probably refactor the current >> integrity_audit_message() and have ima_parse_rule() call into it to get >> those fields as well. I suppose adding new fields to it wouldn't be >> considered breaking user space? > Changing the order of existing fields or inserting fields could break > stuff and is strongly discouraged without a good reason, but appending > fields is usually the right way to add information. > > There are exceptions, and in this case, I'd pick the "more standard" of > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > with that, abandoning the other format, renaming the less standard > version of the record (ima_parse_rule?) and perhpas adopting that > abandonned format for the new record type while using > current->audit_context. Since current->audit_context is NULL I built on your patch, but I am not sure whether it is justifyable to use that before your container id series is applied. This is what ima_parse_rule() produces now after having it call audit_log_task_info() as well and by introducing 1806 for ima_parse_rule() only ( https://lkml.org/lkml/2018/5/11/386 ): type=UNKNOWN[1806] msg=audit(1526643702.328:304): action="dont_measure" fsmagic="0x9fa0" res=1 ppid=1563 pid=1595 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="cat" exe="/usr/bin/cat" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 [before: type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" fsmagic="0x9fa0" res=1] For comparison the INTEGRITY_RULE: type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b" ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 1806 would be in sync with INTEGRITY_RULE now for process related info. If this looks good, I'll remove the dependency on your local context creation and post the series. The justification for the change is that the INTEGRITY_RULE, as produced by ima_parse_rule(), is broken. Stefan > > Does this help? > >> Stefan >> >>> Can you briefly describe the circumstances under which these two >>> different identically-numbered records are produced as a first step >>> towards splitting them into two distict records? >>> >>> The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records >>> appear to be already properly covered for AUDIT_CONTAINER_INFO records >>> by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record >>> appears to be unused. >>> >>>>> Can you suggest a procedure to test it? >>>> Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on >>>> policy. The example IMA policy, below, includes IMA-audit messages for >>>> files executed. 'cat' the policy to /sys/kernel/security/ima/policy. >>>> >>>> /etc/ima/ima-policy: >>>> audit func=BPRM_CHECK >>>> >>>> There's a FireEye blog titled "Extending Linux Executable Logging With >>>> The Integrity Measurement Architecture"* that explains how to augment >>>> their existing system security analytics with file hashes. >>>> >>>> * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux >>>> _exec.html >>>> >>>> >>>> Mimi >>>> >>>>>> If the containerid is defined, include it in the IMA-audit record. >>>>>> >>>>>> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> >>>>>> --- >>>>>> security/integrity/ima/ima_api.c | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c >>>>>> index 33b4458cdbef..41d29a06f28f 100644 >>>>>> --- a/security/integrity/ima/ima_api.c >>>>>> +++ b/security/integrity/ima/ima_api.c >>>>>> @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, >>>>>> audit_log_untrustedstring(ab, algo_hash); >>>>>> audit_log_task_info(ab, current); >>>>>> + if (audit_containerid_set(current)) >>>>>> + audit_log_format(ab, " contid=%llu", >>>>>> + audit_get_containerid(current)); >>>>>> audit_log_end(ab); >>>>>> iint->flags |= IMA_AUDITED; >>>>>> -- >>>>>> 2.7.5 >>>>>> >>>>> - RGB >>> - RGB >>> >>> -- >>> Richard Guy Briggs <rgb@redhat.com> >>> Sr. S/W Engineer, Kernel Security, Base Operating Systems >>> Remote, Ottawa, Red Hat Canada >>> IRC: rgb, SunRaycer >>> Voice: +1.647.777.2635, Internal: (81) 32635 >>> > - RGB > > -- > Richard Guy Briggs <rgb@redhat.com> > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 >
On Fri, 2018-05-18 at 07:49 -0400, Stefan Berger wrote: > On 05/17/2018 05:30 PM, Richard Guy Briggs wrote: [...] > >>> auxiliary record either by being converted to a syscall auxiliary record > >>> by using current->audit_context rather than NULL when calling > >>> audit_log_start(), or creating a local audit_context and calling > >> ima_parse_rule() is invoked when setting a policy by writing it into > >> /sys/kernel/security/ima/policy. We unfortunately don't have the > >> current->audit_context in this case. > > Sure you do. What process writes to that file? That's the one we care > > about, unless it is somehow handed off to a queue and processed later in > > a different context. > > I just printk'd it again. current->audit_context is NULL in this case. The builtin policy rules are loaded at __init. Subsequently a custom policy can replace the builtin policy rules by writing to the securityfs file. Is the audit_context NULL in both cases? > >> If so, which ones? We could probably refactor the current > >> integrity_audit_message() and have ima_parse_rule() call into it to get > >> those fields as well. I suppose adding new fields to it wouldn't be > >> considered breaking user space? > > Changing the order of existing fields or inserting fields could break > > stuff and is strongly discouraged without a good reason, but appending > > fields is usually the right way to add information. > > > > There are exceptions, and in this case, I'd pick the "more standard" of > > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > > with that, abandoning the other format, renaming the less standard > > version of the record (ima_parse_rule?) and perhpas adopting that > > abandonned format for the new record type while using > > current->audit_context. This sounds right, other than "type=INTEGRITY_RULE" (1805) for ima_audit_measurement(). Could we rename type=1805 to be INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT? The new type=1806 audit message could be named INTEGRITY_RULE or, if that would be confusing, INTEGRITY_POLICY_RULE. > 1806 would be in sync with INTEGRITY_RULE now for process related info. > If this looks good, I'll remove the dependency on your local context > creation and post the series. > > The justification for the change is that the INTEGRITY_RULE, as produced > by ima_parse_rule(), is broken. Post which series? The IMA namespacing patch set? This change should be upstreamed independently of IMA namespacing. Mimi
On 05/18/2018 08:53 AM, Mimi Zohar wrote: > On Fri, 2018-05-18 at 07:49 -0400, Stefan Berger wrote: >> On 05/17/2018 05:30 PM, Richard Guy Briggs wrote: > [...] > >>>>> auxiliary record either by being converted to a syscall auxiliary record >>>>> by using current->audit_context rather than NULL when calling >>>>> audit_log_start(), or creating a local audit_context and calling >>>> ima_parse_rule() is invoked when setting a policy by writing it into >>>> /sys/kernel/security/ima/policy. We unfortunately don't have the >>>> current->audit_context in this case. >>> Sure you do. What process writes to that file? That's the one we care >>> about, unless it is somehow handed off to a queue and processed later in >>> a different context. >> I just printk'd it again. current->audit_context is NULL in this case. > The builtin policy rules are loaded at __init. Subsequently a custom > policy can replace the builtin policy rules by writing to the > securityfs file. Is the audit_context NULL in both cases? The builtin policy rules are not parsed from what I can see. They seem to be encoded in the format the parser would produce. I get current->audit_context == NULL in the case the user cat's the policy into IMA's policy securityfs file. > > > >>>> If so, which ones? We could probably refactor the current >>>> integrity_audit_message() and have ima_parse_rule() call into it to get >>>> those fields as well. I suppose adding new fields to it wouldn't be >>>> considered breaking user space? >>> Changing the order of existing fields or inserting fields could break >>> stuff and is strongly discouraged without a good reason, but appending >>> fields is usually the right way to add information. >>> >>> There are exceptions, and in this case, I'd pick the "more standard" of >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick >>> with that, abandoning the other format, renaming the less standard >>> version of the record (ima_parse_rule?) and perhpas adopting that >>> abandonned format for the new record type while using >>> current->audit_context. > This sounds right, other than "type=INTEGRITY_RULE" (1805) for > ima_audit_measurement(). Could we rename type=1805 to be So do we want to change both? I thought that what ima_audit_measurement() produces looks ok but may not have a good name for the 'type'. Now in this case I would not want to 'break user space'. The only change I was going to make was to what ima_parse_rule() produces. > INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT? The new type=1806 audit > message could be named INTEGRITY_RULE or, if that would be confusing, > INTEGRITY_POLICY_RULE. For 1806, as we would use it in ima_parse_rule(), we could change that in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better for IMA to produce but that's inconsistent then. > >> 1806 would be in sync with INTEGRITY_RULE now for process related info. >> If this looks good, I'll remove the dependency on your local context >> creation and post the series. >> >> The justification for the change is that the INTEGRITY_RULE, as produced >> by ima_parse_rule(), is broken. > Post which series? The IMA namespacing patch set? This change should > be upstreamed independently of IMA namespacing. Without Richard's local context patch it may just be one or two patches. Stefan > > Mimi
On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: > On 05/18/2018 08:53 AM, Mimi Zohar wrote: [..] > >>>> If so, which ones? We could probably refactor the current > >>>> integrity_audit_message() and have ima_parse_rule() call into it to get > >>>> those fields as well. I suppose adding new fields to it wouldn't be > >>>> considered breaking user space? > >>> Changing the order of existing fields or inserting fields could break > >>> stuff and is strongly discouraged without a good reason, but appending > >>> fields is usually the right way to add information. > >>> > >>> There are exceptions, and in this case, I'd pick the "more standard" of > >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > >>> with that, abandoning the other format, renaming the less standard > >>> version of the record (ima_parse_rule?) and perhpas adopting that > >>> abandonned format for the new record type while using > >>> current->audit_context. > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for > > ima_audit_measurement(). Could we rename type=1805 to be > > So do we want to change both? I thought that what > ima_audit_measurement() produces looks ok but may not have a good name > for the 'type'. Now in this case I would not want to 'break user space'. > The only change I was going to make was to what ima_parse_rule() produces. The only change for now is separating the IMA policy rules from the IMA-audit messages. Richard, when the containerid is appended to the IMA-audit messages, would we make the audit type name change then? > > > INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT? The new type=1806 audit > > message could be named INTEGRITY_RULE or, if that would be confusing, > > INTEGRITY_POLICY_RULE. > > For 1806, as we would use it in ima_parse_rule(), we could change that > in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better > for IMA to produce but that's inconsistent then. Ok > > > > >> 1806 would be in sync with INTEGRITY_RULE now for process related info. > >> If this looks good, I'll remove the dependency on your local context > >> creation and post the series. > >> > >> The justification for the change is that the INTEGRITY_RULE, as produced > >> by ima_parse_rule(), is broken. > > Post which series? The IMA namespacing patch set? This change should > > be upstreamed independently of IMA namespacing. > > Without Richard's local context patch it may just be one or two patches. Richard, if we separate the ima_parse_rules() audit messages, changing the audit rule number now, without the call to audit_log_task_info(), would adding the call later be breaking userspace? Mimi
On 05/18/2018 10:39 AM, Mimi Zohar wrote: > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: >> On 05/18/2018 08:53 AM, Mimi Zohar wrote: > [..] > >>>>>> If so, which ones? We could probably refactor the current >>>>>> integrity_audit_message() and have ima_parse_rule() call into it to get >>>>>> those fields as well. I suppose adding new fields to it wouldn't be >>>>>> considered breaking user space? >>>>> Changing the order of existing fields or inserting fields could break >>>>> stuff and is strongly discouraged without a good reason, but appending >>>>> fields is usually the right way to add information. >>>>> >>>>> There are exceptions, and in this case, I'd pick the "more standard" of >>>>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick >>>>> with that, abandoning the other format, renaming the less standard >>>>> version of the record (ima_parse_rule?) and perhpas adopting that >>>>> abandonned format for the new record type while using >>>>> current->audit_context. >>> This sounds right, other than "type=INTEGRITY_RULE" (1805) for >>> ima_audit_measurement(). Could we rename type=1805 to be >> So do we want to change both? I thought that what >> ima_audit_measurement() produces looks ok but may not have a good name >> for the 'type'. Now in this case I would not want to 'break user space'. >> The only change I was going to make was to what ima_parse_rule() produces. > The only change for now is separating the IMA policy rules from the > IMA-audit messages. > > Richard, when the containerid is appended to the IMA-audit messages, > would we make the audit type name change then? > >>> INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT? The new type=1806 audit >>> message could be named INTEGRITY_RULE or, if that would be confusing, >>> INTEGRITY_POLICY_RULE. >> For 1806, as we would use it in ima_parse_rule(), we could change that >> in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better >> for IMA to produce but that's inconsistent then. > Ok One other question is whether IMA's audit calls should all adhere to CONFIG_INTEGRITY_AUDIT. Most do but those two that currently use AUDIT_INTEGRITY_RULE do not. Should that be changed as well? Stefan
On 2018-05-18 07:49, Stefan Berger wrote: > On 05/17/2018 05:30 PM, Richard Guy Briggs wrote: > > On 2018-05-17 10:18, Stefan Berger wrote: > > > On 03/08/2018 06:21 AM, Richard Guy Briggs wrote: > > > > On 2018-03-05 09:24, Mimi Zohar wrote: > > > > > On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: > > > > > > On 2018-03-05 08:43, Mimi Zohar wrote: > > > > > > > Hi Richard, > > > > > > > > > > > > > > This patch has been compiled, but not runtime tested. > > > > > > Ok, great, thank you. I assume you are offering this patch to be > > > > > > included in this patchset? > > > > > Yes, thank you. > > > > > > > > > > > I'll have a look to see where it fits in the > > > > > > IMA record. It might be better if it were an AUDIT_CONTAINER_INFO > > > > > > auxiliary record, but I'll have a look at the circumstances of the > > > > > > event. > > > > I had a look at the context of this record to see if adding the contid > > > > field to it made sense. I think the only records for which the contid > > > > field makes sense are the two newly proposed records, AUDIT_CONTAINER > > > > which introduces the container ID and the and AUDIT_CONTAINER_INFO which > > > > documents the presence of the container ID in a process event (or > > > > process-less network event). All others should use the auxiliary record > > > > AUDIT_CONTAINER_INFO rather than include the contid field directly > > > > itself. There are several reasons for this including record length, the > > > > ability to filter unwanted records, the difficulty of changing the order > > > > of or removing fields in the future. > > > > > > > > Syscalls get this information automatically if the container ID is set > > > > for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a > > > > syscall event is one that uses the task's audit_context while a > > > > standalone event uses NULL or builds a local audit_context that is > > > > discarded immediately after the local use. > > > > > > > > Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it > > > > appears that they should be split into two distinct audit record types. > > > > > > > > The record created in ima_audit_measurement() is a syscall record that > > > > could possibly stand on its own since the subject attributes are > > > > present. If it remains a syscall auxiliary record it will automatically > > > > have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is > > > > decided to detach it (which would save cpu/netlink/disk bandwidth but is > > > > not recommended due to not wanting to throw away any other syscall > > > > information or other involved records (PATH, CWD, etc...) then a local > > > > audit_context would be created for the AUDIT_INTEGRITY_RULE and > > > > AUDIT_CONTAINERID_INFO records only and immediately discarded. > > > What does 'detach it' mean? Does it mean we're not using > > > current->audit_context? > > Exactly. > > > > > > The record created in ima_parse_rule() is not currently a syscall record > > > > since it is passed an audit_context of NULL and it has a very different > > > > format that does not include any subject attributes (except subj_*=). > > > > At first glance it appears this one should be a syscall accompanied > > > > auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO > > > Do you have an example (pointer) to the format for a 'syscall accompanied > > > auxiliary record'? > > Any that uses current->audit_context (or recently proposed > > audit_context() function) will be a syscall auxiliary record. Well > > formed record formats are <fieldname>=<value> and named as listed: > > > > https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events > > https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv > > > > > > auxiliary record either by being converted to a syscall auxiliary record > > > > by using current->audit_context rather than NULL when calling > > > > audit_log_start(), or creating a local audit_context and calling > > > ima_parse_rule() is invoked when setting a policy by writing it into > > > /sys/kernel/security/ima/policy. We unfortunately don't have the > > > current->audit_context in this case. > > Sure you do. What process writes to that file? That's the one we care > > about, unless it is somehow handed off to a queue and processed later in > > a different context. > > I just printk'd it again. current->audit_context is NULL in this case. Oops, that sounds like some of the netfilter empty table initializations, whereas usually rules have a user actor. > > > > audit_log_container_info() then releasing the local context. This > > > > version of the record has additional concerns covered here: > > > > https://github.com/linux-audit/audit-kernel/issues/52 > > > Following the discussion there and the concern with breaking user space, how > > > can we split up the AUDIT_INTEGRITY_RULE that is used in > > > ima_audit_measurement() and ima_parse_rule(), without 'breaking user space'? > > Arguably userspace is already broken by this wildly diverging pair of > > formats for the same record. > > > > > A message produced by ima_parse_rule() looks like this here: > > > > > > type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" > > > fsmagic="0x9fa0" res=1 > > > > > > in contrast to that an INTEGRITY_PCR record type: > > > > > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 > > > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > op="invalid_pcr" cause="open_writers" comm="scp" > > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 > > > > > > Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE? > > Not necessarily. There are a number of records in the PCR record that > > would be redundant when connected to a syscall record, but removing them > > is discouraged to avoid breaking parsers that expect them. > > > > I don't see any need to touch the PCR record. > > I wasn't going to touch the PCR record. > > > > > If so, which ones? We could probably refactor the current > > > integrity_audit_message() and have ima_parse_rule() call into it to get > > > those fields as well. I suppose adding new fields to it wouldn't be > > > considered breaking user space? > > Changing the order of existing fields or inserting fields could break > > stuff and is strongly discouraged without a good reason, but appending > > fields is usually the right way to add information. > > > > There are exceptions, and in this case, I'd pick the "more standard" of > > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > > with that, abandoning the other format, renaming the less standard > > version of the record (ima_parse_rule?) and perhpas adopting that > > abandonned format for the new record type while using > > current->audit_context. > > Since current->audit_context is NULL I built on your patch, but I am not > sure whether it is justifyable to use that before your container id series > is applied. > > This is what ima_parse_rule() produces now after having it call > audit_log_task_info() as well and by introducing 1806 for ima_parse_rule() > only ( https://lkml.org/lkml/2018/5/11/386 ): > > type=UNKNOWN[1806] msg=audit(1526643702.328:304): action="dont_measure" > fsmagic="0x9fa0" res=1 ppid=1563 pid=1595 auid=0 uid=0 gid=0 euid=0 suid=0 > fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="cat" exe="/usr/bin/cat" > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > [before: type=INTEGRITY_RULE msg=audit(1526566213.870:305): > action="dont_measure" fsmagic="0x9fa0" res=1] Since this appears to be a a user action, use current->audit_context to make it a syscall auxiliary record rather than adding all these redundant fields. > For comparison the INTEGRITY_RULE: > > type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b" > ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 > fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp" > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > 1806 would be in sync with INTEGRITY_RULE now for process related info. If > this looks good, I'll remove the dependency on your local context creation > and post the series. What would be the macro name for 1806? > The justification for the change is that the INTEGRITY_RULE, as produced by > ima_parse_rule(), is broken. > > Stefan > > > > > > Does this help? > > > > > Stefan > > > > > > > Can you briefly describe the circumstances under which these two > > > > different identically-numbered records are produced as a first step > > > > towards splitting them into two distict records? > > > > > > > > The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records > > > > appear to be already properly covered for AUDIT_CONTAINER_INFO records > > > > by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record > > > > appears to be unused. > > > > > > > > > > Can you suggest a procedure to test it? > > > > > Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on > > > > > policy. The example IMA policy, below, includes IMA-audit messages for > > > > > files executed. 'cat' the policy to /sys/kernel/security/ima/policy. > > > > > > > > > > /etc/ima/ima-policy: > > > > > audit func=BPRM_CHECK > > > > > > > > > > There's a FireEye blog titled "Extending Linux Executable Logging With > > > > > The Integrity Measurement Architecture"* that explains how to augment > > > > > their existing system security analytics with file hashes. > > > > > > > > > > * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux > > > > > _exec.html > > > > > > > > > > > > > > > Mimi > > > > > > > > > > > > If the containerid is defined, include it in the IMA-audit record. > > > > > > > > > > > > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > > > > > > --- > > > > > > > security/integrity/ima/ima_api.c | 3 +++ > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > > > > > > > index 33b4458cdbef..41d29a06f28f 100644 > > > > > > > --- a/security/integrity/ima/ima_api.c > > > > > > > +++ b/security/integrity/ima/ima_api.c > > > > > > > @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, > > > > > > > audit_log_untrustedstring(ab, algo_hash); > > > > > > > audit_log_task_info(ab, current); > > > > > > > + if (audit_containerid_set(current)) > > > > > > > + audit_log_format(ab, " contid=%llu", > > > > > > > + audit_get_containerid(current)); > > > > > > > audit_log_end(ab); > > > > > > > iint->flags |= IMA_AUDITED; > > > > > > > -- > > > > > > > 2.7.5 > > > > > > > > > > > > > - RGB > > > > - RGB > > > > > > > > -- > > > > Richard Guy Briggs <rgb@redhat.com> > > > > Sr. S/W Engineer, Kernel Security, Base Operating Systems > > > > Remote, Ottawa, Red Hat Canada > > > > IRC: rgb, SunRaycer > > > > Voice: +1.647.777.2635, Internal: (81) 32635 > > > > > > - RGB > > > > -- > > Richard Guy Briggs <rgb@redhat.com> > > Sr. S/W Engineer, Kernel Security, Base Operating Systems > > Remote, Ottawa, Red Hat Canada > > IRC: rgb, SunRaycer > > Voice: +1.647.777.2635, Internal: (81) 32635 > > > - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On 2018-05-18 08:53, Mimi Zohar wrote: > On Fri, 2018-05-18 at 07:49 -0400, Stefan Berger wrote: > > On 05/17/2018 05:30 PM, Richard Guy Briggs wrote: > > [...] > > > >>> auxiliary record either by being converted to a syscall auxiliary record > > >>> by using current->audit_context rather than NULL when calling > > >>> audit_log_start(), or creating a local audit_context and calling > > >> ima_parse_rule() is invoked when setting a policy by writing it into > > >> /sys/kernel/security/ima/policy. We unfortunately don't have the > > >> current->audit_context in this case. > > > Sure you do. What process writes to that file? That's the one we care > > > about, unless it is somehow handed off to a queue and processed later in > > > a different context. > > > > I just printk'd it again. current->audit_context is NULL in this case. > > The builtin policy rules are loaded at __init. Subsequently a custom > policy can replace the builtin policy rules by writing to the > securityfs file. Is the audit_context NULL in both cases? I wondered the same. > > >> If so, which ones? We could probably refactor the current > > >> integrity_audit_message() and have ima_parse_rule() call into it to get > > >> those fields as well. I suppose adding new fields to it wouldn't be > > >> considered breaking user space? > > > Changing the order of existing fields or inserting fields could break > > > stuff and is strongly discouraged without a good reason, but appending > > > fields is usually the right way to add information. > > > > > > There are exceptions, and in this case, I'd pick the "more standard" of > > > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > > > with that, abandoning the other format, renaming the less standard > > > version of the record (ima_parse_rule?) and perhpas adopting that > > > abandonned format for the new record type while using > > > current->audit_context. > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for > ima_audit_measurement(). Could we rename type=1805 to be > INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT? The new type=1806 audit > message could be named INTEGRITY_RULE or, if that would be confusing, > INTEGRITY_POLICY_RULE. I'm reluctant to change the macro/value. Just use the existing AUDIT_INTEGRITY_RULE 1805 as appropriate and create a new AUDIT_INTEGRITY_AUDIT 1806. > > 1806 would be in sync with INTEGRITY_RULE now for process related info. > > If this looks good, I'll remove the dependency on your local context > > creation and post the series. > > > > The justification for the change is that the INTEGRITY_RULE, as produced > > by ima_parse_rule(), is broken. > > Post which series? The IMA namespacing patch set? This change should > be upstreamed independently of IMA namespacing. > > Mimi > - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On 2018-05-18 10:39, Mimi Zohar wrote: > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: > > On 05/18/2018 08:53 AM, Mimi Zohar wrote: > > [..] > > > >>>> If so, which ones? We could probably refactor the current > > >>>> integrity_audit_message() and have ima_parse_rule() call into it to get > > >>>> those fields as well. I suppose adding new fields to it wouldn't be > > >>>> considered breaking user space? > > >>> Changing the order of existing fields or inserting fields could break > > >>> stuff and is strongly discouraged without a good reason, but appending > > >>> fields is usually the right way to add information. > > >>> > > >>> There are exceptions, and in this case, I'd pick the "more standard" of > > >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > > >>> with that, abandoning the other format, renaming the less standard > > >>> version of the record (ima_parse_rule?) and perhpas adopting that > > >>> abandonned format for the new record type while using > > >>> current->audit_context. > > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for > > > ima_audit_measurement(). Could we rename type=1805 to be > > > > So do we want to change both? I thought that what > > ima_audit_measurement() produces looks ok but may not have a good name > > for the 'type'. Now in this case I would not want to 'break user space'. > > The only change I was going to make was to what ima_parse_rule() produces. > > The only change for now is separating the IMA policy rules from the > IMA-audit messages. > > Richard, when the containerid is appended to the IMA-audit messages, > would we make the audit type name change then? No, go ahead and make the change now. I'm expecting that the containerid record will just be another auxiliary record and should not affect you folks. > > > INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT? The new type=1806 audit > > > message could be named INTEGRITY_RULE or, if that would be confusing, > > > INTEGRITY_POLICY_RULE. > > > > For 1806, as we would use it in ima_parse_rule(), we could change that > > in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better > > for IMA to produce but that's inconsistent then. > > Ok > > > > > > > > >> 1806 would be in sync with INTEGRITY_RULE now for process related info. > > >> If this looks good, I'll remove the dependency on your local context > > >> creation and post the series. > > >> > > >> The justification for the change is that the INTEGRITY_RULE, as produced > > >> by ima_parse_rule(), is broken. > > > Post which series? The IMA namespacing patch set? This change should > > > be upstreamed independently of IMA namespacing. > > > > Without Richard's local context patch it may just be one or two patches. > > Richard, if we separate the ima_parse_rules() audit messages, changing > the audit rule number now, without the call to audit_log_task_info(), > would adding the call later be breaking userspace? Userspace is arguably already broken due to two formats and one usage that isn't an auxiliary record. All that should be necessary for now is to use a different record number and pass it current->audit_context instead of NULL. > Mimi - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On 2018-05-18 10:52, Stefan Berger wrote: > On 05/18/2018 10:39 AM, Mimi Zohar wrote: > > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: > > > On 05/18/2018 08:53 AM, Mimi Zohar wrote: > > [..] > > > > > > > > > If so, which ones? We could probably refactor the current > > > > > > > integrity_audit_message() and have ima_parse_rule() call into it to get > > > > > > > those fields as well. I suppose adding new fields to it wouldn't be > > > > > > > considered breaking user space? > > > > > > Changing the order of existing fields or inserting fields could break > > > > > > stuff and is strongly discouraged without a good reason, but appending > > > > > > fields is usually the right way to add information. > > > > > > > > > > > > There are exceptions, and in this case, I'd pick the "more standard" of > > > > > > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > > > > > > with that, abandoning the other format, renaming the less standard > > > > > > version of the record (ima_parse_rule?) and perhpas adopting that > > > > > > abandonned format for the new record type while using > > > > > > current->audit_context. > > > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for > > > > ima_audit_measurement(). Could we rename type=1805 to be > > > So do we want to change both? I thought that what > > > ima_audit_measurement() produces looks ok but may not have a good name > > > for the 'type'. Now in this case I would not want to 'break user space'. > > > The only change I was going to make was to what ima_parse_rule() produces. > > The only change for now is separating the IMA policy rules from the > > IMA-audit messages. > > > > Richard, when the containerid is appended to the IMA-audit messages, > > would we make the audit type name change then? > > > > > > INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT? The new type=1806 audit > > > > message could be named INTEGRITY_RULE or, if that would be confusing, > > > > INTEGRITY_POLICY_RULE. > > > For 1806, as we would use it in ima_parse_rule(), we could change that > > > in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better > > > for IMA to produce but that's inconsistent then. > > Ok > > One other question is whether IMA's audit calls should all adhere to > CONFIG_INTEGRITY_AUDIT. If I understand your question correctly, then no, since each one is a different type of record, hence the half dozen IMA record types: #define AUDIT_INTEGRITY_DATA 1800 /* Data integrity verification */ #define AUDIT_INTEGRITY_METADATA 1801 /* Metadata integrity verification */ #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 */ > Most do but those two that currently use > AUDIT_INTEGRITY_RULE do not. Should that be changed as well? As far as I can tell, all the other IMA audit record types are fine. > Stefan - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Fri, 2018-05-18 at 11:56 -0400, Richard Guy Briggs wrote: > On 2018-05-18 10:39, Mimi Zohar wrote: > > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: > > > On 05/18/2018 08:53 AM, Mimi Zohar wrote: > > > > [..] > > > > > >>>> If so, which ones? We could probably refactor the current > > > >>>> integrity_audit_message() and have ima_parse_rule() call into it to get > > > >>>> those fields as well. I suppose adding new fields to it wouldn't be > > > >>>> considered breaking user space? > > > >>> Changing the order of existing fields or inserting fields could break > > > >>> stuff and is strongly discouraged without a good reason, but appending > > > >>> fields is usually the right way to add information. > > > >>> > > > >>> There are exceptions, and in this case, I'd pick the "more standard" of > > > >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > > > >>> with that, abandoning the other format, renaming the less standard > > > >>> version of the record (ima_parse_rule?) and perhpas adopting that > > > >>> abandonned format for the new record type while using > > > >>> current->audit_context. > > > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for > > > > ima_audit_measurement(). Could we rename type=1805 to be > > > > > > So do we want to change both? I thought that what > > > ima_audit_measurement() produces looks ok but may not have a good name > > > for the 'type'. Now in this case I would not want to 'break user space'. > > > The only change I was going to make was to what ima_parse_rule() produces. > > > > The only change for now is separating the IMA policy rules from the > > IMA-audit messages. > > > > Richard, when the containerid is appended to the IMA-audit messages, > > would we make the audit type name change then? > > No, go ahead and make the change now. I'm expecting that the > containerid record will just be another auxiliary record and should not > affect you folks. To summarize, we need to disambiguate the 1805, as both ima_parse_rule() and ima_audit_measurement() are using the same number with different formats. The main usage of 1805 that we are aware of is ima_audit_measurement(). Yet the "type=" name for ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not INTEGRITY_RULE. option 1: breaks both uses 1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() option 2: breaks the most common usage 1805 - INTEGRITY_RULE - ima_parse_rule() 1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() option 3: leaves the most common usage with the wrong name, and breaks the other less common usage 1805 - INTEGRITY_RULE - ima_audit_measurement() 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() So option 3 is the best option? Mimi
On 05/18/2018 11:45 AM, Richard Guy Briggs wrote: > On 2018-05-18 07:49, Stefan Berger wrote: >> On 05/17/2018 05:30 PM, Richard Guy Briggs wrote: >>> On 2018-05-17 10:18, Stefan Berger wrote: >>>> On 03/08/2018 06:21 AM, Richard Guy Briggs wrote: >>>>> On 2018-03-05 09:24, Mimi Zohar wrote: >>>>>> On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: >>>>>>> On 2018-03-05 08:43, Mimi Zohar wrote: >>>>>>>> Hi Richard, >>>>>>>> >>>>>>>> This patch has been compiled, but not runtime tested. >>>>>>> Ok, great, thank you. I assume you are offering this patch to be >>>>>>> included in this patchset? >>>>>> Yes, thank you. >>>>>> >>>>>>> I'll have a look to see where it fits in the >>>>>>> IMA record. It might be better if it were an AUDIT_CONTAINER_INFO >>>>>>> auxiliary record, but I'll have a look at the circumstances of the >>>>>>> event. >>>>> I had a look at the context of this record to see if adding the contid >>>>> field to it made sense. I think the only records for which the contid >>>>> field makes sense are the two newly proposed records, AUDIT_CONTAINER >>>>> which introduces the container ID and the and AUDIT_CONTAINER_INFO which >>>>> documents the presence of the container ID in a process event (or >>>>> process-less network event). All others should use the auxiliary record >>>>> AUDIT_CONTAINER_INFO rather than include the contid field directly >>>>> itself. There are several reasons for this including record length, the >>>>> ability to filter unwanted records, the difficulty of changing the order >>>>> of or removing fields in the future. >>>>> >>>>> Syscalls get this information automatically if the container ID is set >>>>> for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a >>>>> syscall event is one that uses the task's audit_context while a >>>>> standalone event uses NULL or builds a local audit_context that is >>>>> discarded immediately after the local use. >>>>> >>>>> Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it >>>>> appears that they should be split into two distinct audit record types. >>>>> >>>>> The record created in ima_audit_measurement() is a syscall record that >>>>> could possibly stand on its own since the subject attributes are >>>>> present. If it remains a syscall auxiliary record it will automatically >>>>> have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is >>>>> decided to detach it (which would save cpu/netlink/disk bandwidth but is >>>>> not recommended due to not wanting to throw away any other syscall >>>>> information or other involved records (PATH, CWD, etc...) then a local >>>>> audit_context would be created for the AUDIT_INTEGRITY_RULE and >>>>> AUDIT_CONTAINERID_INFO records only and immediately discarded. >>>> What does 'detach it' mean? Does it mean we're not using >>>> current->audit_context? >>> Exactly. >>> >>>>> The record created in ima_parse_rule() is not currently a syscall record >>>>> since it is passed an audit_context of NULL and it has a very different >>>>> format that does not include any subject attributes (except subj_*=). >>>>> At first glance it appears this one should be a syscall accompanied >>>>> auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO >>>> Do you have an example (pointer) to the format for a 'syscall accompanied >>>> auxiliary record'? >>> Any that uses current->audit_context (or recently proposed >>> audit_context() function) will be a syscall auxiliary record. Well >>> formed record formats are <fieldname>=<value> and named as listed: >>> >>> https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events >>> https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv >>> >>>>> auxiliary record either by being converted to a syscall auxiliary record >>>>> by using current->audit_context rather than NULL when calling >>>>> audit_log_start(), or creating a local audit_context and calling >>>> ima_parse_rule() is invoked when setting a policy by writing it into >>>> /sys/kernel/security/ima/policy. We unfortunately don't have the >>>> current->audit_context in this case. >>> Sure you do. What process writes to that file? That's the one we care >>> about, unless it is somehow handed off to a queue and processed later in >>> a different context. >> I just printk'd it again. current->audit_context is NULL in this case. > Oops, that sounds like some of the netfilter empty table > initializations, whereas usually rules have a user actor. So it's a bug elsewhere not a 'feature?' > >>>>> audit_log_container_info() then releasing the local context. This >>>>> version of the record has additional concerns covered here: >>>>> https://github.com/linux-audit/audit-kernel/issues/52 >>>> Following the discussion there and the concern with breaking user space, how >>>> can we split up the AUDIT_INTEGRITY_RULE that is used in >>>> ima_audit_measurement() and ima_parse_rule(), without 'breaking user space'? >>> Arguably userspace is already broken by this wildly diverging pair of >>> formats for the same record. >>> >>>> A message produced by ima_parse_rule() looks like this here: >>>> >>>> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" >>>> fsmagic="0x9fa0" res=1 >>>> >>>> in contrast to that an INTEGRITY_PCR record type: >>>> >>>> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 >>>> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 >>>> op="invalid_pcr" cause="open_writers" comm="scp" >>>> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 >>>> >>>> Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE? >>> Not necessarily. There are a number of records in the PCR record that >>> would be redundant when connected to a syscall record, but removing them >>> is discouraged to avoid breaking parsers that expect them. >>> >>> I don't see any need to touch the PCR record. >> I wasn't going to touch the PCR record. >> >> >>>> If so, which ones? We could probably refactor the current >>>> integrity_audit_message() and have ima_parse_rule() call into it to get >>>> those fields as well. I suppose adding new fields to it wouldn't be >>>> considered breaking user space? >>> Changing the order of existing fields or inserting fields could break >>> stuff and is strongly discouraged without a good reason, but appending >>> fields is usually the right way to add information. >>> >>> There are exceptions, and in this case, I'd pick the "more standard" of >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick >>> with that, abandoning the other format, renaming the less standard >>> version of the record (ima_parse_rule?) and perhpas adopting that >>> abandonned format for the new record type while using >>> current->audit_context. >> Since current->audit_context is NULL I built on your patch, but I am not >> sure whether it is justifyable to use that before your container id series >> is applied. >> >> This is what ima_parse_rule() produces now after having it call >> audit_log_task_info() as well and by introducing 1806 for ima_parse_rule() >> only ( https://lkml.org/lkml/2018/5/11/386 ): >> >> type=UNKNOWN[1806] msg=audit(1526643702.328:304): action="dont_measure" >> fsmagic="0x9fa0" res=1 ppid=1563 pid=1595 auid=0 uid=0 gid=0 euid=0 suid=0 >> fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="cat" exe="/usr/bin/cat" >> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 >> >> [before: type=INTEGRITY_RULE msg=audit(1526566213.870:305): >> action="dont_measure" fsmagic="0x9fa0" res=1] > Since this appears to be a a user action, use current->audit_context > to make it a syscall auxiliary record rather than adding all these > redundant fields. Sure, once we have a non-NULL pointer in current->audit_context I'd do that. > >> For comparison the INTEGRITY_RULE: >> >> type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b" >> ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 >> fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp" >> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 >> >> >> 1806 would be in sync with INTEGRITY_RULE now for process related info. If >> this looks good, I'll remove the dependency on your local context creation >> and post the series. > What would be the macro name for 1806? I currently work with AUDIT_INTEGRITY_POLICY_RULE. > >> The justification for the change is that the INTEGRITY_RULE, as produced by >> ima_parse_rule(), is broken. >> >> Stefan >> >> >>> Does this help? >>> >>>> Stefan >>>> >>>>> Can you briefly describe the circumstances under which these two >>>>> different identically-numbered records are produced as a first step >>>>> towards splitting them into two distict records? >>>>> >>>>> The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records >>>>> appear to be already properly covered for AUDIT_CONTAINER_INFO records >>>>> by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record >>>>> appears to be unused. >>>>> >>>>>>> Can you suggest a procedure to test it? >>>>>> Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on >>>>>> policy. The example IMA policy, below, includes IMA-audit messages for >>>>>> files executed. 'cat' the policy to /sys/kernel/security/ima/policy. >>>>>> >>>>>> /etc/ima/ima-policy: >>>>>> audit func=BPRM_CHECK >>>>>> >>>>>> There's a FireEye blog titled "Extending Linux Executable Logging With >>>>>> The Integrity Measurement Architecture"* that explains how to augment >>>>>> their existing system security analytics with file hashes. >>>>>> >>>>>> * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux >>>>>> _exec.html >>>>>> >>>>>> >>>>>> Mimi >>>>>> >>>>>>>> If the containerid is defined, include it in the IMA-audit record. >>>>>>>> >>>>>>>> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> >>>>>>>> --- >>>>>>>> security/integrity/ima/ima_api.c | 3 +++ >>>>>>>> 1 file changed, 3 insertions(+) >>>>>>>> >>>>>>>> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c >>>>>>>> index 33b4458cdbef..41d29a06f28f 100644 >>>>>>>> --- a/security/integrity/ima/ima_api.c >>>>>>>> +++ b/security/integrity/ima/ima_api.c >>>>>>>> @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, >>>>>>>> audit_log_untrustedstring(ab, algo_hash); >>>>>>>> audit_log_task_info(ab, current); >>>>>>>> + if (audit_containerid_set(current)) >>>>>>>> + audit_log_format(ab, " contid=%llu", >>>>>>>> + audit_get_containerid(current)); >>>>>>>> audit_log_end(ab); >>>>>>>> iint->flags |= IMA_AUDITED; >>>>>>>> -- >>>>>>>> 2.7.5 >>>>>>>> >>>>>>> - RGB >>>>> - RGB >>>>> >>>>> -- >>>>> Richard Guy Briggs <rgb@redhat.com> >>>>> Sr. S/W Engineer, Kernel Security, Base Operating Systems >>>>> Remote, Ottawa, Red Hat Canada >>>>> IRC: rgb, SunRaycer >>>>> Voice: +1.647.777.2635, Internal: (81) 32635 >>>>> >>> - RGB >>> >>> -- >>> Richard Guy Briggs <rgb@redhat.com> >>> Sr. S/W Engineer, Kernel Security, Base Operating Systems >>> Remote, Ottawa, Red Hat Canada >>> IRC: rgb, SunRaycer >>> Voice: +1.647.777.2635, Internal: (81) 32635 >>> > - RGB > > -- > Richard Guy Briggs <rgb@redhat.com> > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 >
On 2018-05-18 12:34, Mimi Zohar wrote: > On Fri, 2018-05-18 at 11:56 -0400, Richard Guy Briggs wrote: > > On 2018-05-18 10:39, Mimi Zohar wrote: > > > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: > > > > On 05/18/2018 08:53 AM, Mimi Zohar wrote: > > > > > > [..] > > > > > > > >>>> If so, which ones? We could probably refactor the current > > > > >>>> integrity_audit_message() and have ima_parse_rule() call into it to get > > > > >>>> those fields as well. I suppose adding new fields to it wouldn't be > > > > >>>> considered breaking user space? > > > > >>> Changing the order of existing fields or inserting fields could break > > > > >>> stuff and is strongly discouraged without a good reason, but appending > > > > >>> fields is usually the right way to add information. > > > > >>> > > > > >>> There are exceptions, and in this case, I'd pick the "more standard" of > > > > >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > > > > >>> with that, abandoning the other format, renaming the less standard > > > > >>> version of the record (ima_parse_rule?) and perhpas adopting that > > > > >>> abandonned format for the new record type while using > > > > >>> current->audit_context. > > > > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for > > > > > ima_audit_measurement(). Could we rename type=1805 to be > > > > > > > > So do we want to change both? I thought that what > > > > ima_audit_measurement() produces looks ok but may not have a good name > > > > for the 'type'. Now in this case I would not want to 'break user space'. > > > > The only change I was going to make was to what ima_parse_rule() produces. > > > > > > The only change for now is separating the IMA policy rules from the > > > IMA-audit messages. > > > > > > Richard, when the containerid is appended to the IMA-audit messages, > > > would we make the audit type name change then? > > > > No, go ahead and make the change now. I'm expecting that the > > containerid record will just be another auxiliary record and should not > > affect you folks. > > To summarize, we need to disambiguate the 1805, as both > ima_parse_rule() and ima_audit_measurement() are using the same number > with different formats. The main usage of 1805 that we are aware of > is ima_audit_measurement(). Yet the "type=" name for > ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not > INTEGRITY_RULE. > > option 1: breaks both uses > 1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() > 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() > > option 2: breaks the most common usage > 1805 - INTEGRITY_RULE - ima_parse_rule() > 1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() > > option 3: leaves the most common usage with the wrong name, and breaks > the other less common usage > 1805 - INTEGRITY_RULE - ima_audit_measurement() > 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() > > So option 3 is the best option? Yes, I think so, but option 2 I would be willing to consider. I'd like to get Paul and Steve's opinions on this. > Mimi - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On 2018-05-18 12:49, Stefan Berger wrote: > On 05/18/2018 11:45 AM, Richard Guy Briggs wrote: > > On 2018-05-18 07:49, Stefan Berger wrote: > > > On 05/17/2018 05:30 PM, Richard Guy Briggs wrote: > > > > On 2018-05-17 10:18, Stefan Berger wrote: > > > > > On 03/08/2018 06:21 AM, Richard Guy Briggs wrote: > > > > > > On 2018-03-05 09:24, Mimi Zohar wrote: > > > > > > > On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: > > > > > > > > On 2018-03-05 08:43, Mimi Zohar wrote: > > > > > > > > > Hi Richard, > > > > > > > > > > > > > > > > > > This patch has been compiled, but not runtime tested. > > > > > > > > Ok, great, thank you. I assume you are offering this patch to be > > > > > > > > included in this patchset? > > > > > > > Yes, thank you. > > > > > > > > > > > > > > > I'll have a look to see where it fits in the > > > > > > > > IMA record. It might be better if it were an AUDIT_CONTAINER_INFO > > > > > > > > auxiliary record, but I'll have a look at the circumstances of the > > > > > > > > event. > > > > > > I had a look at the context of this record to see if adding the contid > > > > > > field to it made sense. I think the only records for which the contid > > > > > > field makes sense are the two newly proposed records, AUDIT_CONTAINER > > > > > > which introduces the container ID and the and AUDIT_CONTAINER_INFO which > > > > > > documents the presence of the container ID in a process event (or > > > > > > process-less network event). All others should use the auxiliary record > > > > > > AUDIT_CONTAINER_INFO rather than include the contid field directly > > > > > > itself. There are several reasons for this including record length, the > > > > > > ability to filter unwanted records, the difficulty of changing the order > > > > > > of or removing fields in the future. > > > > > > > > > > > > Syscalls get this information automatically if the container ID is set > > > > > > for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a > > > > > > syscall event is one that uses the task's audit_context while a > > > > > > standalone event uses NULL or builds a local audit_context that is > > > > > > discarded immediately after the local use. > > > > > > > > > > > > Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it > > > > > > appears that they should be split into two distinct audit record types. > > > > > > > > > > > > The record created in ima_audit_measurement() is a syscall record that > > > > > > could possibly stand on its own since the subject attributes are > > > > > > present. If it remains a syscall auxiliary record it will automatically > > > > > > have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is > > > > > > decided to detach it (which would save cpu/netlink/disk bandwidth but is > > > > > > not recommended due to not wanting to throw away any other syscall > > > > > > information or other involved records (PATH, CWD, etc...) then a local > > > > > > audit_context would be created for the AUDIT_INTEGRITY_RULE and > > > > > > AUDIT_CONTAINERID_INFO records only and immediately discarded. > > > > > What does 'detach it' mean? Does it mean we're not using > > > > > current->audit_context? > > > > Exactly. > > > > > > > > > > The record created in ima_parse_rule() is not currently a syscall record > > > > > > since it is passed an audit_context of NULL and it has a very different > > > > > > format that does not include any subject attributes (except subj_*=). > > > > > > At first glance it appears this one should be a syscall accompanied > > > > > > auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO > > > > > Do you have an example (pointer) to the format for a 'syscall accompanied > > > > > auxiliary record'? > > > > Any that uses current->audit_context (or recently proposed > > > > audit_context() function) will be a syscall auxiliary record. Well > > > > formed record formats are <fieldname>=<value> and named as listed: > > > > > > > > https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events > > > > https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv > > > > > > > > > > auxiliary record either by being converted to a syscall auxiliary record > > > > > > by using current->audit_context rather than NULL when calling > > > > > > audit_log_start(), or creating a local audit_context and calling > > > > > ima_parse_rule() is invoked when setting a policy by writing it into > > > > > /sys/kernel/security/ima/policy. We unfortunately don't have the > > > > > current->audit_context in this case. > > > > Sure you do. What process writes to that file? That's the one we care > > > > about, unless it is somehow handed off to a queue and processed later in > > > > a different context. > > > I just printk'd it again. current->audit_context is NULL in this case. > > Oops, that sounds like some of the netfilter empty table > > initializations, whereas usually rules have a user actor. > > So it's a bug elsewhere not a 'feature?' Er, it is a challenge we're aware of and have already solved a number of them. What happens if you boot with "audit=1" on the kernel command line? > > > > > > audit_log_container_info() then releasing the local context. This > > > > > > version of the record has additional concerns covered here: > > > > > > https://github.com/linux-audit/audit-kernel/issues/52 > > > > > Following the discussion there and the concern with breaking user space, how > > > > > can we split up the AUDIT_INTEGRITY_RULE that is used in > > > > > ima_audit_measurement() and ima_parse_rule(), without 'breaking user space'? > > > > Arguably userspace is already broken by this wildly diverging pair of > > > > formats for the same record. > > > > > > > > > A message produced by ima_parse_rule() looks like this here: > > > > > > > > > > type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" > > > > > fsmagic="0x9fa0" res=1 > > > > > > > > > > in contrast to that an INTEGRITY_PCR record type: > > > > > > > > > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 > > > > > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > > > op="invalid_pcr" cause="open_writers" comm="scp" > > > > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 > > > > > > > > > > Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE? > > > > Not necessarily. There are a number of records in the PCR record that > > > > would be redundant when connected to a syscall record, but removing them > > > > is discouraged to avoid breaking parsers that expect them. > > > > > > > > I don't see any need to touch the PCR record. > > > I wasn't going to touch the PCR record. > > > > > > > > > > > If so, which ones? We could probably refactor the current > > > > > integrity_audit_message() and have ima_parse_rule() call into it to get > > > > > those fields as well. I suppose adding new fields to it wouldn't be > > > > > considered breaking user space? > > > > Changing the order of existing fields or inserting fields could break > > > > stuff and is strongly discouraged without a good reason, but appending > > > > fields is usually the right way to add information. > > > > > > > > There are exceptions, and in this case, I'd pick the "more standard" of > > > > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick > > > > with that, abandoning the other format, renaming the less standard > > > > version of the record (ima_parse_rule?) and perhpas adopting that > > > > abandonned format for the new record type while using > > > > current->audit_context. > > > Since current->audit_context is NULL I built on your patch, but I am not > > > sure whether it is justifyable to use that before your container id series > > > is applied. > > > > > > This is what ima_parse_rule() produces now after having it call > > > audit_log_task_info() as well and by introducing 1806 for ima_parse_rule() > > > only ( https://lkml.org/lkml/2018/5/11/386 ): > > > > > > type=UNKNOWN[1806] msg=audit(1526643702.328:304): action="dont_measure" > > > fsmagic="0x9fa0" res=1 ppid=1563 pid=1595 auid=0 uid=0 gid=0 euid=0 suid=0 > > > fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="cat" exe="/usr/bin/cat" > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > > > > [before: type=INTEGRITY_RULE msg=audit(1526566213.870:305): > > > action="dont_measure" fsmagic="0x9fa0" res=1] > > Since this appears to be a a user action, use current->audit_context > > to make it a syscall auxiliary record rather than adding all these > > redundant fields. > > Sure, once we have a non-NULL pointer in current->audit_context I'd do that. > > > > > > For comparison the INTEGRITY_RULE: > > > > > > type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b" > > > ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 > > > fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp" > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > > > > > > > 1806 would be in sync with INTEGRITY_RULE now for process related info. If > > > this looks good, I'll remove the dependency on your local context creation > > > and post the series. > > What would be the macro name for 1806? > > I currently work with AUDIT_INTEGRITY_POLICY_RULE. > > > > > > > The justification for the change is that the INTEGRITY_RULE, as produced by > > > ima_parse_rule(), is broken. > > > > > > Stefan > > > > > > > > > > Does this help? > > > > > > > > > Stefan > > > > > > > > > > > Can you briefly describe the circumstances under which these two > > > > > > different identically-numbered records are produced as a first step > > > > > > towards splitting them into two distict records? > > > > > > > > > > > > The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records > > > > > > appear to be already properly covered for AUDIT_CONTAINER_INFO records > > > > > > by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record > > > > > > appears to be unused. > > > > > > > > > > > > > > Can you suggest a procedure to test it? > > > > > > > Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on > > > > > > > policy. The example IMA policy, below, includes IMA-audit messages for > > > > > > > files executed. 'cat' the policy to /sys/kernel/security/ima/policy. > > > > > > > > > > > > > > /etc/ima/ima-policy: > > > > > > > audit func=BPRM_CHECK > > > > > > > > > > > > > > There's a FireEye blog titled "Extending Linux Executable Logging With > > > > > > > The Integrity Measurement Architecture"* that explains how to augment > > > > > > > their existing system security analytics with file hashes. > > > > > > > > > > > > > > * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux > > > > > > > _exec.html > > > > > > > > > > > > > > > > > > > > > Mimi > > > > > > > > > > > > > > > > If the containerid is defined, include it in the IMA-audit record. > > > > > > > > > > > > > > > > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > > > > > > > > --- > > > > > > > > > security/integrity/ima/ima_api.c | 3 +++ > > > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c > > > > > > > > > index 33b4458cdbef..41d29a06f28f 100644 > > > > > > > > > --- a/security/integrity/ima/ima_api.c > > > > > > > > > +++ b/security/integrity/ima/ima_api.c > > > > > > > > > @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, > > > > > > > > > audit_log_untrustedstring(ab, algo_hash); > > > > > > > > > audit_log_task_info(ab, current); > > > > > > > > > + if (audit_containerid_set(current)) > > > > > > > > > + audit_log_format(ab, " contid=%llu", > > > > > > > > > + audit_get_containerid(current)); > > > > > > > > > audit_log_end(ab); > > > > > > > > > iint->flags |= IMA_AUDITED; > > > > > > > > > -- > > > > > > > > > 2.7.5 > > > > > > > > > > > > > > > > > - RGB > > > > > > - RGB > > > > > > > > > > > > -- > > > > > > Richard Guy Briggs <rgb@redhat.com> > > > > > > Sr. S/W Engineer, Kernel Security, Base Operating Systems > > > > > > Remote, Ottawa, Red Hat Canada > > > > > > IRC: rgb, SunRaycer > > > > > > Voice: +1.647.777.2635, Internal: (81) 32635 > > > > > > > > > > - RGB > > > > > > > > -- > > > > Richard Guy Briggs <rgb@redhat.com> > > > > Sr. S/W Engineer, Kernel Security, Base Operating Systems > > > > Remote, Ottawa, Red Hat Canada > > > > IRC: rgb, SunRaycer > > > > Voice: +1.647.777.2635, Internal: (81) 32635 > > > > > > - RGB > > > > -- > > Richard Guy Briggs <rgb@redhat.com> > > Sr. S/W Engineer, Kernel Security, Base Operating Systems > > Remote, Ottawa, Red Hat Canada > > IRC: rgb, SunRaycer > > Voice: +1.647.777.2635, Internal: (81) 32635 > > > - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote: > > audit_log_container_info() then releasing the local context. This > > version of the record has additional concerns covered here: > > https://github.com/linux-audit/audit-kernel/issues/52 > > Following the discussion there and the concern with breaking user space, > how can we split up the AUDIT_INTEGRITY_RULE that is used in > ima_audit_measurement() and ima_parse_rule(), without 'breaking user > space'? > > A message produced by ima_parse_rule() looks like this here: > > type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" > fsmagic="0x9fa0" res=1 Why is action and fsmagic being logged as untrusted strings? Untrusted strings are used when an unprivileged user can affect the contents of the field such as creating a file with space or special characters in the name. Also, subject and object information is missing. Who loaded this rule? > in contrast to that an INTEGRITY_PCR record type: > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > op="invalid_pcr" cause="open_writers" comm="scp" > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 Why is op & cause being logged as an untrusted string? This also has incomplete subject information. > Should some of the fields from INTEGRITY_PCR also appear in > INTEGRITY_RULE? If so, which ones? pid, uid, auid, tty, session, subj, comm, exe, res. <- these are required to be searchable > We could probably refactor the current integrity_audit_message() and have > ima_parse_rule() call into it to get those fields as well. I suppose adding > new fields to it wouldn't be considered breaking user space? The audit user space utilities pretty much expects those fields in that order for any IMA originating events. You can add things like op or cause before that. The reason why you can do that is those additional fields are not required to be searchable by common criteria. -Steve
On Friday, May 18, 2018 12:34:24 PM EDT Mimi Zohar wrote: > On Fri, 2018-05-18 at 11:56 -0400, Richard Guy Briggs wrote: > > On 2018-05-18 10:39, Mimi Zohar wrote: > > > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: > > > > On 05/18/2018 08:53 AM, Mimi Zohar wrote: > > > [..] > > > > > > > >>>> If so, which ones? We could probably refactor the current > > > > >>>> integrity_audit_message() and have ima_parse_rule() call into it > > > > >>>> to get > > > > >>>> those fields as well. I suppose adding new fields to it wouldn't > > > > >>>> be > > > > >>>> considered breaking user space? > > > > >>> > > > > >>> Changing the order of existing fields or inserting fields could > > > > >>> break > > > > >>> stuff and is strongly discouraged without a good reason, but > > > > >>> appending > > > > >>> fields is usually the right way to add information. > > > > >>> > > > > >>> There are exceptions, and in this case, I'd pick the "more > > > > >>> standard" of > > > > >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and > > > > >>> stick > > > > >>> with that, abandoning the other format, renaming the less > > > > >>> standard > > > > >>> version of the record (ima_parse_rule?) and perhpas adopting that > > > > >>> abandonned format for the new record type while using > > > > >>> current->audit_context. > > > > > > > > > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for > > > > > ima_audit_measurement(). Could we rename type=1805 to be > > > > > > > > So do we want to change both? I thought that what > > > > ima_audit_measurement() produces looks ok but may not have a good > > > > name > > > > for the 'type'. Now in this case I would not want to 'break user > > > > space'. > > > > The only change I was going to make was to what ima_parse_rule() > > > > produces. > > > > > > The only change for now is separating the IMA policy rules from the > > > IMA-audit messages. > > > > > > Richard, when the containerid is appended to the IMA-audit messages, > > > would we make the audit type name change then? > > > > No, go ahead and make the change now. I'm expecting that the > > containerid record will just be another auxiliary record and should not > > affect you folks. > > To summarize, we need to disambiguate the 1805, as both > ima_parse_rule() and ima_audit_measurement() are using the same number > with different formats. The main usage of 1805 that we are aware of > is ima_audit_measurement(). Yet the "type=" name for > ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not > INTEGRITY_RULE. > > option 1: breaks both uses > 1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() > 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() > > option 2: breaks the most common usage > 1805 - INTEGRITY_RULE - ima_parse_rule() > 1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() > > option 3: leaves the most common usage with the wrong name, and breaks > the other less common usage > 1805 - INTEGRITY_RULE - ima_audit_measurement() > 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() > > So option 3 is the best option? From a user space perspective, I don't care as long the event is well formed (No unnecessary untrusted string logging) and we have the required fields for searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And the object is identifiable in the event. -Steve
On 05/21/2018 12:58 PM, Steve Grubb wrote: > On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote: >>> audit_log_container_info() then releasing the local context. This >>> version of the record has additional concerns covered here: >>> https://github.com/linux-audit/audit-kernel/issues/52 >> Following the discussion there and the concern with breaking user space, >> how can we split up the AUDIT_INTEGRITY_RULE that is used in >> ima_audit_measurement() and ima_parse_rule(), without 'breaking user >> space'? >> >> A message produced by ima_parse_rule() looks like this here: >> >> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" >> fsmagic="0x9fa0" res=1 > Why is action and fsmagic being logged as untrusted strings? Untrusted > strings are used when an unprivileged user can affect the contents of the > field such as creating a file with space or special characters in the name. > > Also, subject and object information is missing. Who loaded this rule? > >> in contrast to that an INTEGRITY_PCR record type: >> >> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 >> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 >> op="invalid_pcr" cause="open_writers" comm="scp" >> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 > Why is op & cause being logged as an untrusted string? This also has > incomplete subject information. It's calling audit_log_string() in both cases: https://elixir.bootlin.com/linux/latest/source/security/integrity/integrity_audit.c#L48 > >> Should some of the fields from INTEGRITY_PCR also appear in >> INTEGRITY_RULE? If so, which ones? > pid, uid, auid, tty, session, subj, comm, exe, res. <- these are required to > be searchable > >> We could probably refactor the current integrity_audit_message() and have >> ima_parse_rule() call into it to get those fields as well. I suppose adding >> new fields to it wouldn't be considered breaking user space? > The audit user space utilities pretty much expects those fields in that order > for any IMA originating events. You can add things like op or cause before We will call into audit_log_task, which will put the parameters into correct order: auid uid gid ses subj pid comm exe https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433 > that. The reason why you can do that is those additional fields are not > required to be searchable by common criteria. > > -Steve > >
On 05/21/2018 01:21 PM, Steve Grubb wrote: > On Friday, May 18, 2018 12:34:24 PM EDT Mimi Zohar wrote: >> On Fri, 2018-05-18 at 11:56 -0400, Richard Guy Briggs wrote: >>> On 2018-05-18 10:39, Mimi Zohar wrote: >>>> On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: >>>>> On 05/18/2018 08:53 AM, Mimi Zohar wrote: >>>> [..] >>>> >>>>>>>>> If so, which ones? We could probably refactor the current >>>>>>>>> integrity_audit_message() and have ima_parse_rule() call into it >>>>>>>>> to get >>>>>>>>> those fields as well. I suppose adding new fields to it wouldn't >>>>>>>>> be >>>>>>>>> considered breaking user space? >>>>>>>> Changing the order of existing fields or inserting fields could >>>>>>>> break >>>>>>>> stuff and is strongly discouraged without a good reason, but >>>>>>>> appending >>>>>>>> fields is usually the right way to add information. >>>>>>>> >>>>>>>> There are exceptions, and in this case, I'd pick the "more >>>>>>>> standard" of >>>>>>>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and >>>>>>>> stick >>>>>>>> with that, abandoning the other format, renaming the less >>>>>>>> standard >>>>>>>> version of the record (ima_parse_rule?) and perhpas adopting that >>>>>>>> abandonned format for the new record type while using >>>>>>>> current->audit_context. >>>>>> This sounds right, other than "type=INTEGRITY_RULE" (1805) for >>>>>> ima_audit_measurement(). Could we rename type=1805 to be >>>>> So do we want to change both? I thought that what >>>>> ima_audit_measurement() produces looks ok but may not have a good >>>>> name >>>>> for the 'type'. Now in this case I would not want to 'break user >>>>> space'. >>>>> The only change I was going to make was to what ima_parse_rule() >>>>> produces. >>>> The only change for now is separating the IMA policy rules from the >>>> IMA-audit messages. >>>> >>>> Richard, when the containerid is appended to the IMA-audit messages, >>>> would we make the audit type name change then? >>> No, go ahead and make the change now. I'm expecting that the >>> containerid record will just be another auxiliary record and should not >>> affect you folks. >> To summarize, we need to disambiguate the 1805, as both >> ima_parse_rule() and ima_audit_measurement() are using the same number >> with different formats. The main usage of 1805 that we are aware of >> is ima_audit_measurement(). Yet the "type=" name for >> ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not >> INTEGRITY_RULE. >> >> option 1: breaks both uses >> 1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() >> 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() >> >> option 2: breaks the most common usage >> 1805 - INTEGRITY_RULE - ima_parse_rule() >> 1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() >> >> option 3: leaves the most common usage with the wrong name, and breaks >> the other less common usage >> 1805 - INTEGRITY_RULE - ima_audit_measurement() >> 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() >> >> So option 3 is the best option? > From a user space perspective, I don't care as long the event is well formed Are you saying this because of the tools you referred to that require proper ordering and all fields need to be available? > (No unnecessary untrusted string logging) and we have the required fields for > searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And the > object is identifiable in the event. There's at least one documented user that showed the existing format... https://www.fireeye.com/blog/threat-research/2016/11/extending_linux_exec.html
Hello Stefan, On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote: > On 05/21/2018 12:58 PM, Steve Grubb wrote: > > On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote: > >>> audit_log_container_info() then releasing the local context. This > >>> version of the record has additional concerns covered here: > >>> https://github.com/linux-audit/audit-kernel/issues/52 > >> > >> Following the discussion there and the concern with breaking user space, > >> how can we split up the AUDIT_INTEGRITY_RULE that is used in > >> ima_audit_measurement() and ima_parse_rule(), without 'breaking user > >> space'? > >> > >> A message produced by ima_parse_rule() looks like this here: > >> > >> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" > >> fsmagic="0x9fa0" res=1 > > > > Why is action and fsmagic being logged as untrusted strings? Untrusted > > strings are used when an unprivileged user can affect the contents of the > > field such as creating a file with space or special characters in the > > name. > > > > Also, subject and object information is missing. Who loaded this rule? > > > >> in contrast to that an INTEGRITY_PCR record type: > >> > >> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 > >> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > >> op="invalid_pcr" cause="open_writers" comm="scp" > >> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 > > > > Why is op & cause being logged as an untrusted string? This also has > > incomplete subject information. > > It's calling audit_log_string() in both cases: > > https://elixir.bootlin.com/linux/latest/source/security/integrity/integrity > _audit.c#L48 I see. Looking things over, I see that it seems like it should do the right thing. But the original purpose for this function is here: https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944 This is where it is logging an untrusted string and has to decide to encode it or just place it in quotes. If it has quotes, that means it's an untrusted string but has no control characters in it. I think you want to use audit_log_format() for any string that is trustworthy. As an aside, I wonder why audit_log_string() is in the API when it is a helper to audit_log_untrustedstring() ? Without understanding the rules of untrusted strings, it can't be used correctly without re-inventing audit_log_untrustedstring() by hand. > >> Should some of the fields from INTEGRITY_PCR also appear in > >> INTEGRITY_RULE? If so, which ones? > > > > pid, uid, auid, tty, session, subj, comm, exe, res. <- these are > > required to be searchable > > > >> We could probably refactor the current integrity_audit_message() and > >> have ima_parse_rule() call into it to get those fields as well. I > >> suppose adding new fields to it wouldn't be considered breaking user > >> space? > > > > The audit user space utilities pretty much expects those fields in that > > order for any IMA originating events. You can add things like op or > > cause before > > We will call into audit_log_task, which will put the parameters into > correct order: > > auid uid gid ses subj pid comm exe I'm telling you what the correct order is. :-) A long time ago, the IMA system had audit events with the order I'm mentioning. For example, here's one from a log I collected back in 2013: type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op="add_template_measure" cause="hash_added" comm="init" name="01parse-kernel.sh" dev=rootfs ino=5413 res=0 it was missing "tty" and "exe", but the order is as I mentioned. The expectation is that INTEGRITY events maintain this established order across all events. Thanks, -Steve > https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433 > > > that. The reason why you can do that is those additional fields are not > > required to be searchable by common criteria. > > > > -Steve
Hello Stefan, On Monday, May 21, 2018 2:04:08 PM EDT Stefan Berger wrote: > On 05/21/2018 01:21 PM, Steve Grubb wrote: > > On Friday, May 18, 2018 12:34:24 PM EDT Mimi Zohar wrote: > >> On Fri, 2018-05-18 at 11:56 -0400, Richard Guy Briggs wrote: > >>> On 2018-05-18 10:39, Mimi Zohar wrote: > >>>> On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote: > >>>>> On 05/18/2018 08:53 AM, Mimi Zohar wrote: > >>>> [..] > >>>> > >>>>>>>>> If so, which ones? We could probably refactor the current > >>>>>>>>> integrity_audit_message() and have ima_parse_rule() call into it > >>>>>>>>> to get those fields as well. I suppose adding new fields to it > >>>>>>>>> wouldn't be considered breaking user space? > >>>>>>>> > >>>>>>>> Changing the order of existing fields or inserting fields could > >>>>>>>> break stuff and is strongly discouraged without a good reason, but > >>>>>>>> appending fields is usually the right way to add information. > >>>>>>>> > >>>>>>>> There are exceptions, and in this case, I'd pick the "more > >>>>>>>> standard" of the formats for AUDIT_INTEGRITY_RULE > >>>>>>>> (ima_audit_measurement?) and stick with that, abandoning the other > >>>>>>>> format, renaming the less standard version of the record > >>>>>>>> (ima_parse_rule?) and perhpas adopting that abandonned format for > >>>>>>>> the new record type while using current->audit_context. > >>>>>> > >>>>>> This sounds right, other than "type=INTEGRITY_RULE" (1805) for > >>>>>> ima_audit_measurement(). Could we rename type=1805 to be > >>>>> > >>>>> So do we want to change both? I thought that what > >>>>> ima_audit_measurement() produces looks ok but may not have a good > >>>>> name for the 'type'. Now in this case I would not want to 'break user > >>>>> space'. The only change I was going to make was to what > >>>>> ima_parse_rule() produces. > >>>> > >>>> The only change for now is separating the IMA policy rules from the > >>>> IMA-audit messages. > >>>> > >>>> Richard, when the containerid is appended to the IMA-audit messages, > >>>> would we make the audit type name change then? > >>> > >>> No, go ahead and make the change now. I'm expecting that the > >>> containerid record will just be another auxiliary record and should not > >>> affect you folks. > >> > >> To summarize, we need to disambiguate the 1805, as both > >> ima_parse_rule() and ima_audit_measurement() are using the same number > >> with different formats. The main usage of 1805 that we are aware of > >> is ima_audit_measurement(). Yet the "type=" name for > >> ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not > >> INTEGRITY_RULE. > >> > >> option 1: breaks both uses > >> 1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() > >> 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() > >> > >> option 2: breaks the most common usage > >> 1805 - INTEGRITY_RULE - ima_parse_rule() > >> 1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement() > >> > >> option 3: leaves the most common usage with the wrong name, and breaks > >> the other less common usage > >> 1805 - INTEGRITY_RULE - ima_audit_measurement() > >> 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule() > >> > >> So option 3 is the best option? > >> > > From a user space perspective, I don't care as long the event is well > > formed > > Are you saying this because of the tools you referred to that require > proper ordering and all fields need to be available? Its because the order was established a long time ago. User space tools still expect that ordering. > > (No unnecessary untrusted string logging) and we have the required fields > > for searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And > > the object is identifiable in the event. > > There's at least one documented user that showed the existing format... > > https://www.fireeye.com/blog/threat-research/2016/11/extending_linux_exec.h > tml Hmm. I see. If that event was ever sent to linux-audit mail list for feedback, we would have had some discussion because it's not a proper event. The order of the fields changed, new fields were added, untrusted string logging is being used, field names are not documented in the field name dictionary, existing field names are not being used, subject information is incomplete, results is missing, and its not entirely clear what the object is. -Steve
On 05/21/2018 02:30 PM, Steve Grubb wrote: > Hello Stefan, > > On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote: >> On 05/21/2018 12:58 PM, Steve Grubb wrote: >>> On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote: >>>>> audit_log_container_info() then releasing the local context. This >>>>> version of the record has additional concerns covered here: >>>>> https://github.com/linux-audit/audit-kernel/issues/52 >>>> Following the discussion there and the concern with breaking user space, >>>> how can we split up the AUDIT_INTEGRITY_RULE that is used in >>>> ima_audit_measurement() and ima_parse_rule(), without 'breaking user >>>> space'? >>>> >>>> A message produced by ima_parse_rule() looks like this here: >>>> >>>> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" >>>> fsmagic="0x9fa0" res=1 >>> Why is action and fsmagic being logged as untrusted strings? Untrusted >>> strings are used when an unprivileged user can affect the contents of the >>> field such as creating a file with space or special characters in the >>> name. >>> >>> Also, subject and object information is missing. Who loaded this rule? >>> >>>> in contrast to that an INTEGRITY_PCR record type: >>>> >>>> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 >>>> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 >>>> op="invalid_pcr" cause="open_writers" comm="scp" >>>> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 >>> Why is op & cause being logged as an untrusted string? This also has >>> incomplete subject information. >> It's calling audit_log_string() in both cases: >> >> https://elixir.bootlin.com/linux/latest/source/security/integrity/integrity >> _audit.c#L48 > I see. Looking things over, I see that it seems like it should do the right > thing. But the original purpose for this function is here: > > https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944 > > This is where it is logging an untrusted string and has to decide to encode > it or just place it in quotes. If it has quotes, that means it's an untrusted > string but has no control characters in it. I think you want to use > audit_log_format() for any string that is trustworthy. Replacing all occurrences (in IMA) of audit_log_string() with audit_log_format(). > > > As an aside, I wonder why audit_log_string() is in the API when it is a > helper to audit_log_untrustedstring() ? Without understanding the rules of > untrusted strings, it can't be used correctly without re-inventing > audit_log_untrustedstring() by hand. > > >>>> Should some of the fields from INTEGRITY_PCR also appear in >>>> INTEGRITY_RULE? If so, which ones? >>> pid, uid, auid, tty, session, subj, comm, exe, res. <- these are >>> required to be searchable >>> >>>> We could probably refactor the current integrity_audit_message() and >>>> have ima_parse_rule() call into it to get those fields as well. I >>>> suppose adding new fields to it wouldn't be considered breaking user >>>> space? >>> The audit user space utilities pretty much expects those fields in that >>> order for any IMA originating events. You can add things like op or >>> cause before >> We will call into audit_log_task, which will put the parameters into >> correct order: >> >> auid uid gid ses subj pid comm exe > I'm telling you what the correct order is. :-) A long time ago, the IMA :-) Thanks. Was getting confused. > system had audit events with the order I'm mentioning. For example, here's > one from a log I collected back in 2013: > > type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 auid=4294967295 > ses=4294967295 subj=kernel op="add_template_measure" cause="hash_added" > comm="init" name="01parse-kernel.sh" dev=rootfs ino=5413 res=0 > > it was missing "tty" and "exe", but the order is as I mentioned. The > expectation is that INTEGRITY events maintain this established order across > all events. I am *appending* exe= and tty= now: type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0 ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op="invalid_pcr" cause="open_writers" comm="ssh" name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1 exe="/usr/bin/ssh" tty=tty2 Stefan > > Thanks, > -Steve > > >> https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433 >> >>> that. The reason why you can do that is those additional fields are not >>> required to be searchable by common criteria. >>> >>> -Steve > > >
On 2018-05-21 17:57, Stefan Berger wrote: > On 05/21/2018 02:30 PM, Steve Grubb wrote: > > Hello Stefan, > > > > On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote: > > > On 05/21/2018 12:58 PM, Steve Grubb wrote: > > > > On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote: > > > > > > audit_log_container_info() then releasing the local context. This > > > > > > version of the record has additional concerns covered here: > > > > > > https://github.com/linux-audit/audit-kernel/issues/52 > > > > > Following the discussion there and the concern with breaking user space, > > > > > how can we split up the AUDIT_INTEGRITY_RULE that is used in > > > > > ima_audit_measurement() and ima_parse_rule(), without 'breaking user > > > > > space'? > > > > > > > > > > A message produced by ima_parse_rule() looks like this here: > > > > > > > > > > type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" > > > > > fsmagic="0x9fa0" res=1 > > > > Why is action and fsmagic being logged as untrusted strings? Untrusted > > > > strings are used when an unprivileged user can affect the contents of the > > > > field such as creating a file with space or special characters in the > > > > name. > > > > > > > > Also, subject and object information is missing. Who loaded this rule? > > > > > > > > > in contrast to that an INTEGRITY_PCR record type: > > > > > > > > > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 > > > > > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > > > op="invalid_pcr" cause="open_writers" comm="scp" > > > > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 > > > > Why is op & cause being logged as an untrusted string? This also has > > > > incomplete subject information. > > > It's calling audit_log_string() in both cases: > > > > > > https://elixir.bootlin.com/linux/latest/source/security/integrity/integrity > > > _audit.c#L48 > > I see. Looking things over, I see that it seems like it should do the right > > thing. But the original purpose for this function is here: > > > > https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944 > > > > This is where it is logging an untrusted string and has to decide to encode > > it or just place it in quotes. If it has quotes, that means it's an untrusted > > string but has no control characters in it. I think you want to use > > audit_log_format() for any string that is trustworthy. > > Replacing all occurrences (in IMA) of audit_log_string() with > audit_log_format(). > > > > > > As an aside, I wonder why audit_log_string() is in the API when it is a > > helper to audit_log_untrustedstring() ? Without understanding the rules of > > untrusted strings, it can't be used correctly without re-inventing > > audit_log_untrustedstring() by hand. > > > > > > > > > Should some of the fields from INTEGRITY_PCR also appear in > > > > > INTEGRITY_RULE? If so, which ones? > > > > pid, uid, auid, tty, session, subj, comm, exe, res. <- these are > > > > required to be searchable > > > > > > > > > We could probably refactor the current integrity_audit_message() and > > > > > have ima_parse_rule() call into it to get those fields as well. I > > > > > suppose adding new fields to it wouldn't be considered breaking user > > > > > space? > > > > The audit user space utilities pretty much expects those fields in that > > > > order for any IMA originating events. You can add things like op or > > > > cause before > > > We will call into audit_log_task, which will put the parameters into > > > correct order: > > > > > > auid uid gid ses subj pid comm exe > > I'm telling you what the correct order is. :-) A long time ago, the IMA > > :-) Thanks. Was getting confused. > > > system had audit events with the order I'm mentioning. For example, here's > > one from a log I collected back in 2013: > > > > type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 auid=4294967295 > > ses=4294967295 subj=kernel op="add_template_measure" cause="hash_added" > > comm="init" name="01parse-kernel.sh" dev=rootfs ino=5413 res=0 > > > > it was missing "tty" and "exe", but the order is as I mentioned. The > > expectation is that INTEGRITY events maintain this established order across > > all events. > > I am *appending* exe= and tty= now: > > type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0 > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > op="invalid_pcr" cause="open_writers" comm="ssh" > name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1 > exe="/usr/bin/ssh" tty=tty2 This isn't necessary since they already covered in the already connected SYSCALL record which duplicates even more information than is already. > Stefan > > > -Steve > > > > > https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433 > > > > > > > that. The reason why you can do that is those additional fields are not > > > > required to be searchable by common criteria. > > > > > > > > -Steve - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Monday, May 21, 2018 5:57:29 PM EDT Stefan Berger wrote: > >>>> Should some of the fields from INTEGRITY_PCR also appear in > >>>> INTEGRITY_RULE? If so, which ones? > >>> > >>> pid, uid, auid, tty, session, subj, comm, exe, res. <- these are > >>> required to be searchable > >>> > >>>> We could probably refactor the current integrity_audit_message() and > >>>> have ima_parse_rule() call into it to get those fields as well. I > >>>> suppose adding new fields to it wouldn't be considered breaking user > >>>> space? > >>> > >>> The audit user space utilities pretty much expects those fields in that > >>> order for any IMA originating events. You can add things like op or > >>> cause before > >> > >> We will call into audit_log_task, which will put the parameters into > >> correct order: > >> > >> auid uid gid ses subj pid comm exe > > > > I'm telling you what the correct order is. :-) A long time ago, the IMA > > Thanks. Was getting confused. > > > system had audit events with the order I'm mentioning. For example, > > here's one from a log I collected back in 2013: > > > > type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 > > auid=4294967295 ses=4294967295 subj=kernel op="add_template_measure" > > cause="hash_added" comm="init" name="01parse-kernel.sh" dev=rootfs > > ino=5413 res=0 > > > > it was missing "tty" and "exe", but the order is as I mentioned. The > > expectation is that INTEGRITY events maintain this established order > > across all events. > > I am *appending* exe= and tty= now: > > type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0 > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > op="invalid_pcr" cause="open_writers" comm="ssh" > name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1 > exe="/usr/bin/ssh" tty=tty2 It is safe to put them where they belong. The tools currently skip over tty and exe if they would be present. So, nothing would get messed up. Its very simple to add an "if" statement to check for these new fields and collect them for searching. Also, "res" is traditionally the last field in any event. Thanks, -Steve
On Tuesday, May 22, 2018 9:43:46 AM EDT Richard Guy Briggs wrote: > On 2018-05-21 17:57, Stefan Berger wrote: > > On 05/21/2018 02:30 PM, Steve Grubb wrote: > > > Hello Stefan, > > > > > > On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote: > > > > On 05/21/2018 12:58 PM, Steve Grubb wrote: > > > > > On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote: > > > > > > > audit_log_container_info() then releasing the local context. > > > > > > > This > > > > > > > version of the record has additional concerns covered here: > > > > > > > https://github.com/linux-audit/audit-kernel/issues/52 > > > > > > > > > > > > Following the discussion there and the concern with breaking user > > > > > > space, > > > > > > how can we split up the AUDIT_INTEGRITY_RULE that is used in > > > > > > ima_audit_measurement() and ima_parse_rule(), without 'breaking > > > > > > user > > > > > > space'? > > > > > > > > > > > > A message produced by ima_parse_rule() looks like this here: > > > > > > > > > > > > type=INTEGRITY_RULE msg=audit(1526566213.870:305): > > > > > > action="dont_measure" > > > > > > fsmagic="0x9fa0" res=1 > > > > > > > > > > Why is action and fsmagic being logged as untrusted strings? > > > > > Untrusted > > > > > strings are used when an unprivileged user can affect the contents > > > > > of the > > > > > field such as creating a file with space or special characters in > > > > > the > > > > > name. > > > > > > > > > > Also, subject and object information is missing. Who loaded this > > > > > rule? > > > > > > > > > > > in contrast to that an INTEGRITY_PCR record type: > > > > > > > > > > > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 > > > > > > auid=0 > > > > > > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > > > > op="invalid_pcr" cause="open_writers" comm="scp" > > > > > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 > > > > > > > > > > Why is op & cause being logged as an untrusted string? This also > > > > > has > > > > > incomplete subject information. > > > > > > > > It's calling audit_log_string() in both cases: > > > > > > > > https://elixir.bootlin.com/linux/latest/source/security/integrity/int > > > > egrity _audit.c#L48 > > > > > > I see. Looking things over, I see that it seems like it should do the > > > right thing. But the original purpose for this function is here: > > > > > > https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944 > > > > > > This is where it is logging an untrusted string and has to decide to > > > encode it or just place it in quotes. If it has quotes, that means > > > it's an untrusted string but has no control characters in it. I think > > > you want to use audit_log_format() for any string that is trustworthy. > > > > Replacing all occurrences (in IMA) of audit_log_string() with > > audit_log_format(). > > > > > As an aside, I wonder why audit_log_string() is in the API when it is a > > > helper to audit_log_untrustedstring() ? Without understanding the > > > rules of untrusted strings, it can't be used correctly without > > > re-inventing audit_log_untrustedstring() by hand. > > > > > > > > > Should some of the fields from INTEGRITY_PCR also appear in > > > > > > INTEGRITY_RULE? If so, which ones? > > > > > > > > > > pid, uid, auid, tty, session, subj, comm, exe, res. <- these are > > > > > required to be searchable > > > > > > > > > > > We could probably refactor the current integrity_audit_message() > > > > > > and > > > > > > have ima_parse_rule() call into it to get those fields as well. I > > > > > > suppose adding new fields to it wouldn't be considered breaking > > > > > > user > > > > > > space? > > > > > > > > > > The audit user space utilities pretty much expects those fields in > > > > > that > > > > > order for any IMA originating events. You can add things like op or > > > > > cause before > > > > > > > > We will call into audit_log_task, which will put the parameters into > > > > correct order: > > > > > > > > auid uid gid ses subj pid comm exe > > > > > > I'm telling you what the correct order is. :-) A long time ago, the > > > IMA > > : > > :-) Thanks. Was getting confused. > > : > > > system had audit events with the order I'm mentioning. For example, > > > here's > > > one from a log I collected back in 2013: > > > > > > type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 > > > auid=4294967295 ses=4294967295 subj=kernel op="add_template_measure" > > > cause="hash_added" comm="init" name="01parse-kernel.sh" dev=rootfs > > > ino=5413 res=0 > > > > > > it was missing "tty" and "exe", but the order is as I mentioned. The > > > expectation is that INTEGRITY events maintain this established order > > > across all events. > > > > I am *appending* exe= and tty= now: > > > > type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0 > > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > op="invalid_pcr" cause="open_writers" comm="ssh" > > name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1 > > exe="/usr/bin/ssh" tty=tty2 > > This isn't necessary since they already covered in the already > connected SYSCALL record which duplicates even more information than is > already. My logs don't show any syscall record being attached. Nor should it. This is a simple event that should stand on its own. -Steve
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 33b4458cdbef..41d29a06f28f 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, audit_log_untrustedstring(ab, algo_hash); audit_log_task_info(ab, current); + if (audit_containerid_set(current)) + audit_log_format(ab, " contid=%llu", + audit_get_containerid(current)); audit_log_end(ab); iint->flags |= IMA_AUDITED;
Hi Richard, This patch has been compiled, but not runtime tested. --- If the containerid is defined, include it in the IMA-audit record. Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> --- security/integrity/ima/ima_api.c | 3 +++ 1 file changed, 3 insertions(+)