Discussion:
[PATCH] audit: add containerid support for IMA-audit
(too old to reply)
Mimi Zohar
2018-03-05 13:43:13 UTC
Permalink
Raw Message
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 <***@linux.vnet.ibm.com>
---
security/integrity/ima/ima_api.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 33b4458cdbef..41d29a06f28f 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_untrustedstring(ab, algo_hash);

audit_log_task_info(ab, current);
+ if (audit_containerid_set(current))
+ audit_log_format(ab, " contid=%llu",
+ audit_get_containerid(current));
audit_log_end(ab);

iint->flags |= IMA_AUDITED;
--
2.7.5
Richard Guy Briggs
2018-03-05 13:50:08 UTC
Permalink
Raw Message
Post by 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?
Post by Mimi Zohar
---
If the containerid is defined, include it in the IMA-audit record.
---
security/integrity/ima/ima_api.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 33b4458cdbef..41d29a06f28f 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_untrustedstring(ab, algo_hash);
audit_log_task_info(ab, current);
+ if (audit_containerid_set(current))
+ audit_log_format(ab, " contid=%llu",
+ audit_get_containerid(current));
audit_log_end(ab);
iint->flags |= IMA_AUDITED;
--
2.7.5
- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Mimi Zohar
2018-03-05 14:24:14 UTC
Permalink
Raw Message
Post by Richard Guy Briggs
Post by 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.
Post by Richard Guy Briggs
I'll have a look to see where it fits in the
IMA record. It might be better if it were an AUDIT_CONTAINER_INFO
auxiliary record, but I'll have a look at the circumstances of the
event. Can you suggest a procedure to test it?
Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on
policy. The example IMA policy, below, includes IMA-audit messages for
files executed. 'cat' the policy to /sys/kernel/security/ima/policy.

/etc/ima/ima-policy:
audit func=BPRM_CHECK

There's a FireEye blog titled "Extending Linux Executable Logging With
The Integrity Measurement Architecture"* that explains how to augment
their existing system security analytics with file hashes.

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


Mimi
Post by Richard Guy Briggs
Post by Mimi Zohar
---
If the containerid is defined, include it in the IMA-audit record.
---
security/integrity/ima/ima_api.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 33b4458cdbef..41d29a06f28f 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_untrustedstring(ab, algo_hash);
audit_log_task_info(ab, current);
+ if (audit_containerid_set(current))
+ audit_log_format(ab, " contid=%llu",
+ audit_get_containerid(current));
audit_log_end(ab);
iint->flags |= IMA_AUDITED;
--
2.7.5
- RGB
--
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
Richard Guy Briggs
2018-03-08 11:21:04 UTC
Permalink
Raw Message
Post by Mimi Zohar
Post by Richard Guy Briggs
Post by 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.
Post by Richard Guy Briggs
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.
Post by Mimi Zohar
Post by Richard Guy Briggs
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.
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
Post by Richard Guy Briggs
Post by Mimi Zohar
If the containerid is defined, include it in the IMA-audit record.
---
security/integrity/ima/ima_api.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 33b4458cdbef..41d29a06f28f 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -335,6 +335,9 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_untrustedstring(ab, algo_hash);
audit_log_task_info(ab, current);
+ if (audit_containerid_set(current))
+ audit_log_format(ab, " contid=%llu",
+ audit_get_containerid(current));
audit_log_end(ab);
iint->flags |= IMA_AUDITED;
--
2.7.5
- RGB
- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Mimi Zohar
2018-03-08 18:02:45 UTC
Permalink
Raw Message
Post by Richard Guy Briggs
Post by Mimi Zohar
Post by Richard Guy Briggs
Post by 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.
Post by Richard Guy Briggs
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
https://github.com/linux-audit/audit-kernel/issues/52
Can you briefly describe the circumstances under which these two
different identically-numbered records are produced as a first step
towards splitting them into two distict records?
Agreed, the two uses should really be separated.  ima_parse_rule()
generates audit messages, when the IMA policy is initially loaded,
replaced, or extended, the policy rules are included in the audit log.
When IMA is namespaced, there will be a host policy and namespace
policies.  We'll need to be able differentiate between the host policy
rules and IMA namespaced policy rules, and between IMA namespaced
policy rules.

The audit messages produced by ima_audit_measurement() were originally
upstreamed for forensics, and as seen by the FireEye blog are now used
to augment existing security analytics.  These records are probably
being used independently of any other audit records.  A single record
is generated per file, per system.  With IMA namespacing, these
records need to be generated once per file, per namespace as well.  In
order to differentiate the records between the host and namespace, and
between namespaces, these records should include the container id.

To disambiguate between these audit messages and the policy rule
messages, we could rename these audit messages to AUDIT_INTEGRITY_IMA.
Post by Richard Guy Briggs
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
Richard Guy Briggs
2018-03-13 05:53:34 UTC
Permalink
Raw Message
Post by Mimi Zohar
Post by Richard Guy Briggs
Post by Mimi Zohar
Post by Richard Guy Briggs
Post by 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.
Post by Richard Guy Briggs
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
https://github.com/linux-audit/audit-kernel/issues/52
Can you briefly describe the circumstances under which these two
different identically-numbered records are produced as a first step
towards splitting them into two distict records?
Agreed, the two uses should really be separated.  ima_parse_rule()
generates audit messages, when the IMA policy is initially loaded,
replaced, or extended, the policy rules are included in the audit log.
When IMA is namespaced, there will be a host policy and namespace
policies.  We'll need to be able differentiate between the host policy
rules and IMA namespaced policy rules, and between IMA namespaced
policy rules.
I would argue this type of message/record should be converted to an
accompanied syscall record, or have the subject attributes added to the
record so that it is clear what user/process initiated this action. It
now occurs to me that to save audit communications and disk bandwidth,
one syscall record could accompany all the rule records, but if the list
is long enough it might overwhelm userspace audit event parsing code.
Steve?

Regardless, the ima_parse_rule() record format needs to be fixed to
address the non-standard use of "<" and ">" operators instead of the
standard "=" field/value separator.
Post by Mimi Zohar
The audit messages produced by ima_audit_measurement() were originally
upstreamed for forensics, and as seen by the FireEye blog are now used
to augment existing security analytics.  These records are probably
being used independently of any other audit records.  A single record
is generated per file, per system.  With IMA namespacing, these
records need to be generated once per file, per namespace as well.  In
order to differentiate the records between the host and namespace, and
between namespaces, these records should include the container id.
Ok, so this might be one circumstance where the container id field
could make sense to include it in the primary record, but it will
already be automatically getting an AUDIT_CONTAINER_INFO record by
virtue of being a syscall auxiliary record. You say "These records are
probably being used independently of any other audit records.", but
unless you are certain these are not used by any other tools, it will
have to remain as a syscall auxiliary record. This means it will
duplicate the container id info if the container id field is added to
the AUDIT_INTEGRITY_RULE record or cause your tools to need to be able
to parse audit *events* rather than just individual audit *records* to
get the associated container id if the container id field is not added
to the AUDIT_INTEGRITY_RULE.
Post by Mimi Zohar
To disambiguate between these audit messages and the policy rule
messages, we could rename these audit messages to AUDIT_INTEGRITY_IMA.
This makes sense to me, but will depend on other users of this record
type. Since there are already two very different formats for this one
existing record type, changing the record type either doesn't matter if
nothing else has noticed or this is what triggered the examination of
this issue in the first place and should be fixed.
Post by Mimi Zohar
Post by Richard Guy Briggs
The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records
appear to be already properly covered for AUDIT_CONTAINER_INFO records
by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record
appears to be unused.
Ok
Mimi
- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Loading...