audit: add containerid support for IMA-audit

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 <[email protected]>

---
 security/integrity/ima/ima_api.c | 3 +++
 1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

Quoted 3 lines · Mimi Zohar

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?

Quoted 27 lines

---

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
 security/integrity/ima/ima_api.c | 3 +++
 1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

--
Richard Guy Briggs <[email protected]>
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

Quoted 7 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 1 line

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

Quoted 26 lines

Quoted 24 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
 security/integrity/ima/ima_api.c | 3 +++
 1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

Quoted 15 lines · Mimi Zohar

Quoted 7 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 59 lines · Richard Guy Briggs

Quoted 15 lines · Mimi Zohar

Quoted 7 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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

Quoted 3 lines

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.

Quoted 4 lines

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

Quoted 59 lines · Richard Guy Briggs

Quoted 15 lines · Mimi Zohar

Quoted 7 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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

Quoted 3 lines

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.

Quoted 4 lines

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

Quoted 15 lines · Mimi Zohar

Quoted 7 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 31 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 25 lines

Quoted 24 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
  security/integrity/ima/ima_api.c | 3 +++
  1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]>
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

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 31 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 25 lines

Quoted 24 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
  security/integrity/ima/ima_api.c | 3 +++
  1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]>
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

Quoted 34 lines · Richard Guy Briggs

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 9 lines

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?

Quoted 5 lines

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'?

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 59 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 29 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 23 lines

Quoted 22 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
  security/integrity/ima/ima_api.c | 3 +++
  1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]>
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

Quoted 44 lines · Stefan Berger

Quoted 34 lines · Richard Guy Briggs

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

What does 'detach it' mean? Does it mean we're not using current->audit_context?

Exactly.

Quoted 7 lines

Quoted 5 lines

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

Quoted 3 lines

Quoted 3 lines

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

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines

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

Quoted 1 line

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.

Quoted 4 lines

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?

Quoted 61 lines

    Stefan

Quoted 59 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 29 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 23 lines

Quoted 22 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
   security/integrity/ima/ima_api.c | 3 +++
   1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]>
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 <[email protected]>
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:

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

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.

On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

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.

Quoted 1 line · Stefan Berger

On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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?

Quoted 11 lines

Quoted 11 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 7 lines

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:

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 3 lines

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

Quoted 33 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 3 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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

Quoted 33 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 3 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 3 lines

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

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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?

Quoted 7 lines

Quoted 3 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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?

Quoted 7 lines

Quoted 3 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 30 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 31 lines · Richard Guy Briggs

Quoted 30 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 31 lines · Richard Guy Briggs

Quoted 30 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 42 lines · Richard Guy Briggs

Quoted 41 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 26 lines

Quoted 23 lines

Quoted 13 lines

Quoted 13 lines

Quoted 6 lines

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.

Quoted 9 lines

Quoted 9 lines

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.

Quoted 2 lines

This sounds right, other than "type=INTEGRITY_RULE" (1805) for ima_audit_measurement(). Could we rename type=1805 to be

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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?

Quoted 42 lines · Richard Guy Briggs

Quoted 41 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 26 lines

Quoted 23 lines

Quoted 13 lines

Quoted 13 lines

Quoted 6 lines

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.

Quoted 9 lines

Quoted 9 lines

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.

Quoted 2 lines

This sounds right, other than "type=INTEGRITY_RULE" (1805) for ima_audit_measurement(). Could we rename type=1805 to be

Quoted 7 lines

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 59 lines · EDT Mimi Zohar

Quoted 35 lines · Richard Guy Briggs

Quoted 34 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 30 lines

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 6 lines

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.

Quoted 9 lines

Quoted 9 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

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()

Quoted 6 lines

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.

Quoted 62 lines · Steve Grubb

Quoted 61 lines · EDT Mimi Zohar

Quoted 33 lines · Richard Guy Briggs

Quoted 32 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 15 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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()

Quoted 4 lines

Quoted 2 lines

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?

Quoted 3 lines

(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

Quoted 62 lines · Steve Grubb

Quoted 61 lines · EDT Mimi Zohar

Quoted 33 lines · Richard Guy Briggs

Quoted 32 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 15 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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()

Quoted 4 lines

Quoted 2 lines

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?

Quoted 3 lines

(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

Quoted 59 lines · EDT Mimi Zohar

Quoted 35 lines · Richard Guy Briggs

Quoted 34 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 30 lines

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

Quoted 6 lines

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.

Quoted 9 lines

Quoted 9 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

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()

Quoted 6 lines

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.

Quoted 30 lines · Mimi Zohar

Quoted 1 line · Stefan Berger

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line · Stefan Berger

On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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?

Quoted 11 lines

Quoted 11 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 7 lines

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

Quoted 1 line · Stefan Berger

On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:

[...]

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 1 line

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?

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 1 line · Stefan Berger

On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:

[...]

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 1 line

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?

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 66 lines · Richard Guy Briggs

Quoted 44 lines · Stefan Berger

Quoted 34 lines · Richard Guy Briggs

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

What does 'detach it' mean? Does it mean we're not using current->audit_context?

Exactly.

Quoted 7 lines

Quoted 5 lines

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

Quoted 3 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 21 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 12 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 73 lines

Does this help?

Quoted 61 lines

    Stefan

Quoted 59 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 29 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 23 lines

Quoted 22 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
   security/integrity/ima/ima_api.c | 3 +++
   1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]> 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 <[email protected]> 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

Quoted 70 lines · Stefan Berger

Quoted 69 lines · Richard Guy Briggs

Quoted 44 lines · Stefan Berger

Quoted 34 lines · Richard Guy Briggs

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

What does 'detach it' mean? Does it mean we're not using current->audit_context?

Exactly.

Quoted 7 lines

Quoted 5 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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.

Quoted 58 lines

Quoted 22 lines

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 2 lines

[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.

Quoted 9 lines

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

Quoted 3 lines

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?

Quoted 79 lines

The justification for the change is that the INTEGRITY_RULE, as produced by ima_parse_rule(), is broken.

    Stefan

Quoted 72 lines

Does this help?

Quoted 61 lines

    Stefan

Quoted 59 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 29 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 23 lines

Quoted 22 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
    security/integrity/ima/ima_api.c | 3 +++
    1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]>
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 <[email protected]>
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 <[email protected]>
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

Quoted 71 lines · Richard Guy Briggs

Quoted 70 lines · Stefan Berger

Quoted 69 lines · Richard Guy Briggs

Quoted 44 lines · Stefan Berger

Quoted 34 lines · Richard Guy Briggs

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

What does 'detach it' mean? Does it mean we're not using current->audit_context?

Exactly.

Quoted 7 lines

Quoted 5 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

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?'

Quoted 57 lines

Quoted 57 lines

Quoted 21 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 12 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 5 lines

Quoted 2 lines

[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.

Quoted 9 lines

Quoted 9 lines

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

Quoted 4 lines

Quoted 3 lines

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.

Quoted 88 lines

Quoted 79 lines

The justification for the change is that the INTEGRITY_RULE, as produced by ima_parse_rule(), is broken.

    Stefan

Quoted 72 lines

Does this help?

Quoted 61 lines

    Stefan

Quoted 59 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 29 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 23 lines

Quoted 22 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
    security/integrity/ima/ima_api.c | 3 +++
    1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]> 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 <[email protected]> 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 <[email protected]> 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

Quoted 70 lines · Stefan Berger

Quoted 69 lines · Richard Guy Briggs

Quoted 44 lines · Stefan Berger

Quoted 34 lines · Richard Guy Briggs

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

What does 'detach it' mean? Does it mean we're not using current->audit_context?

Exactly.

Quoted 7 lines

Quoted 5 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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.

Quoted 58 lines

Quoted 22 lines

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 2 lines

[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.

Quoted 9 lines

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

Quoted 3 lines

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?

Quoted 79 lines

The justification for the change is that the INTEGRITY_RULE, as produced by ima_parse_rule(), is broken.

    Stefan

Quoted 72 lines

Does this help?

Quoted 61 lines

    Stefan

Quoted 59 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 29 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 23 lines

Quoted 22 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
    security/integrity/ima/ima_api.c | 3 +++
    1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]>
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 <[email protected]>
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 <[email protected]>
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

Quoted 44 lines · Stefan Berger

Quoted 34 lines · Richard Guy Briggs

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

What does 'detach it' mean? Does it mean we're not using current->audit_context?

Exactly.

Quoted 7 lines

Quoted 5 lines

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

Quoted 3 lines

Quoted 3 lines

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

Quoted 3 lines

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines

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

Quoted 1 line

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.

Quoted 4 lines

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?

Quoted 61 lines

    Stefan

Quoted 59 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 29 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 23 lines

Quoted 22 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
   security/integrity/ima/ima_api.c | 3 +++
   1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]>
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 <[email protected]>
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

Quoted 34 lines · Richard Guy Briggs

Quoted 13 lines · Mimi Zohar

Quoted 6 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 9 lines

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?

Quoted 5 lines

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'?

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 59 lines

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.

Quoted 12 lines

Quoted 1 line

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.

Quoted 29 lines

* https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
_exec.html

Mimi

Quoted 23 lines

Quoted 22 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
  security/integrity/ima/ima_api.c | 3 +++
  1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

- RGB

--
Richard Guy Briggs <[email protected]> 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

Quoted 3 lines

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?

Quoted 3 lines

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?

Quoted 8 lines · EDT Stefan Berger

Quoted 3 lines

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'?

Quoted 4 lines

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?

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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

Quoted 29 lines · Steve Grubb

Quoted 9 lines · EDT Stefan Berger

Quoted 3 lines

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'?

Quoted 4 lines

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?

Quoted 6 lines

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

Quoted 2 lines

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

Quoted 11 lines

Quoted 2 lines

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

Quoted 4 lines

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?

Quoted 3 lines

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

Quoted 4 lines

that. The reason why you can do that is those additional fields are not required to be searchable by common criteria.

-Steve

Hello Stefan,

Quoted 27 lines · EDT Stefan Berger

Quoted 26 lines · Steve Grubb

Quoted 8 lines · EDT Stefan Berger

Quoted 3 lines

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'?

Quoted 4 lines

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?

Quoted 6 lines

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

Quoted 6 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

Quoted 2 lines

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

Quoted 4 lines

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?

Quoted 7 lines

Quoted 3 lines

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

Quoted 6 lines

https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433

Quoted 4 lines

that. The reason why you can do that is those additional fields are not required to be searchable by common criteria.

-Steve

Quoted 39 lines · Steve Grubb

Hello Stefan,

Quoted 27 lines · EDT Stefan Berger

Quoted 26 lines · Steve Grubb

Quoted 8 lines · EDT Stefan Berger

Quoted 3 lines

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'?

Quoted 4 lines

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?

Quoted 6 lines

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

Quoted 6 lines

Quoted 2 lines

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:

Quoted 6 lines

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().

Quoted 17 lines

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.

Quoted 9 lines

Quoted 9 lines

Quoted 2 lines

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

Quoted 4 lines

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?

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 10 lines

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

Quoted 8 lines

-Steve

Quoted 6 lines

https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433

Quoted 4 lines

that. The reason why you can do that is those additional fields are not required to be searchable by common criteria.

-Steve

Quoted 111 lines · Stefan Berger

Quoted 54 lines · Steve Grubb

Hello Stefan,

Quoted 39 lines · EDT Stefan Berger

Quoted 38 lines · Steve Grubb

Quoted 12 lines · EDT Stefan Berger

Quoted 4 lines

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'?

Quoted 5 lines

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?

Quoted 7 lines

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

Quoted 8 lines

Quoted 3 lines

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:

Quoted 6 lines

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().

Quoted 23 lines

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.

Quoted 18 lines

Quoted 17 lines

Quoted 2 lines

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

Quoted 6 lines

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

Quoted 7 lines

Quoted 4 lines

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.
:

Quoted 4 lines

system had audit events with the order I'm mentioning. For example, here's
one from a log I collected back in 2013:

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 111 lines · Stefan Berger

Quoted 54 lines · Steve Grubb

Hello Stefan,

Quoted 39 lines · EDT Stefan Berger

Quoted 38 lines · Steve Grubb

Quoted 12 lines · EDT Stefan Berger

Quoted 4 lines

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'?

Quoted 5 lines

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?

Quoted 7 lines

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

Quoted 8 lines

Quoted 3 lines

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:

Quoted 6 lines

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().

Quoted 23 lines

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.

Quoted 18 lines

Quoted 17 lines

Quoted 2 lines

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

Quoted 6 lines

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

Quoted 7 lines

Quoted 4 lines

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.
:

Quoted 4 lines

system had audit events with the order I'm mentioning. For example, here's
one from a log I collected back in 2013:

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 39 lines · Steve Grubb

Hello Stefan,

Quoted 27 lines · EDT Stefan Berger

Quoted 26 lines · Steve Grubb

Quoted 8 lines · EDT Stefan Berger

Quoted 3 lines

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'?

Quoted 4 lines

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?

Quoted 6 lines

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

Quoted 6 lines

Quoted 2 lines

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:

Quoted 6 lines

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().

Quoted 17 lines

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.

Quoted 9 lines

Quoted 9 lines

Quoted 2 lines

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

Quoted 4 lines

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?

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 10 lines

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

Quoted 8 lines

-Steve

Quoted 6 lines

https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433

Quoted 4 lines

that. The reason why you can do that is those additional fields are not required to be searchable by common criteria.

-Steve

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

Quoted 2 lines

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

Quoted 4 lines

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

Quoted 6 lines

Quoted 4 lines

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.

Quoted 11 lines

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

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

Quoted 2 lines

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

Quoted 4 lines

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

Quoted 6 lines

Quoted 4 lines

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.

Quoted 11 lines

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

Quoted 8 lines · EDT Stefan Berger

Quoted 3 lines

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'?

Quoted 4 lines

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?

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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

Quoted 7 lines · Richard Guy Briggs

Quoted 3 lines · Mimi Zohar

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.

Quoted 4 lines

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.

Quoted 1 line

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

Quoted 26 lines

Quoted 24 lines

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
 security/integrity/ima/ima_api.c | 3 +++
 1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

Quoted 3 lines · Mimi Zohar

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?

Quoted 27 lines

---

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by Mimi Zohar <[email protected]>

---
 security/integrity/ima/ima_api.c | 3 +++
 1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5

- RGB

--
Richard Guy Briggs <[email protected]>
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

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 <[email protected]>

---
 security/integrity/ima/ima_api.c | 3 +++
 1 file changed, 3 insertions(+)

security/integrity/ima/ima_api.c +3 -0

index 33b4458cdbef..41d29a06f28f 100644
@@ -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;

Signature · 1 line

2.7.5