[PATCH] audit: add containerid support for IMA-audit

[Mimi Zohar]

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;

[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? 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]

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]

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]

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]

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

[Stefan Berger]

[..]

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it to get
those fields as well. I suppose adding new fields to it wouldn't be
considered breaking user space?

Changing the order of existing fields or inserting fields could break
stuff and is strongly discouraged without a good reason, but appending
fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
with that, abandoning the other format, renaming the less standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

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

So do we want to change both? I thought that what
ima_audit_measurement() produces looks ok but may not have a good name
for the 'type'. Now in this case I would not want to 'break user space'.
The only change I was going to make was to what ima_parse_rule() produces.

The only change for now is separating the IMA policy rules from the
IMA-audit messages.
Richard, when the containerid is appended to the IMA-audit messages,
would we make the audit type name change then?

INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
message could be named INTEGRITY_RULE or, if that would be confusing,
INTEGRITY_POLICY_RULE.

For 1806, as we would use it in ima_parse_rule(), we could change that
in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better
for IMA to produce but that's inconsistent then.

Ok

One other question is whether IMA's audit calls should all adhere to
CONFIG_INTEGRITY_AUDIT. Most do but those two that currently use
AUDIT_INTEGRITY_RULE do not. Should that be changed as well?

    Stefan

[Richard Guy Briggs]

Post by Stefan Berger

[..]

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it to get
those fields as well. I suppose adding new fields to it wouldn't be
considered breaking user space?

Changing the order of existing fields or inserting fields could break
stuff and is strongly discouraged without a good reason, but appending
fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
with that, abandoning the other format, renaming the less standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

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

So do we want to change both? I thought that what
ima_audit_measurement() produces looks ok but may not have a good name
for the 'type'. Now in this case I would not want to 'break user space'.
The only change I was going to make was to what ima_parse_rule() produces.

The only change for now is separating the IMA policy rules from the
IMA-audit messages.
Richard, when the containerid is appended to the IMA-audit messages,
would we make the audit type name change then?

INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
message could be named INTEGRITY_RULE or, if that would be confusing,
INTEGRITY_POLICY_RULE.

For 1806, as we would use it in ima_parse_rule(), we could change that
in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better
for IMA to produce but that's inconsistent then.

Ok

One other question is whether IMA's audit calls should all adhere to
CONFIG_INTEGRITY_AUDIT.

If I understand your question correctly, then no, since each one is a
different type of record, hence the half dozen IMA record types:
#define AUDIT_INTEGRITY_DATA 1800 /* Data integrity verification */
#define AUDIT_INTEGRITY_METADATA 1801 /* Metadata integrity verification */
#define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */
#define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */
#define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */
#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */

Post by Stefan Berger
Most do but those two that currently use
AUDIT_INTEGRITY_RULE do not. Should that be changed as well?

As far as I can tell, all the other IMA audit record types are fine.

Post by Stefan Berger
    Stefan

- 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

[Richard Guy Briggs]

[...]

Post by Richard Guy Briggs
auxiliary record either by being converted to a syscall auxiliary record
by using current->audit_context rather than NULL when calling
audit_log_start(), or creating a local audit_context and calling

ima_parse_rule() is invoked when setting a policy by writing it into
/sys/kernel/security/ima/policy. We unfortunately don't have the
current->audit_context in this case.

Sure you do. What process writes to that file? That's the one we care
about, unless it is somehow handed off to a queue and processed later in
a different context.

I just printk'd it again. current->audit_context is NULL in this case.

The builtin policy rules are loaded at __init.  Subsequently a custom
policy can replace the builtin policy rules by writing to the
securityfs file.  Is the audit_context NULL in both cases?

I wondered the same.

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it to get
those fields as well. I suppose adding new fields to it wouldn't be
considered breaking user space?

Changing the order of existing fields or inserting fields could break
stuff and is strongly discouraged without a good reason, but appending
fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
with that, abandoning the other format, renaming the less standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

This sounds right, other than "type=INTEGRITY_RULE" (1805) for
ima_audit_measurement().  Could we rename type=1805 to be
INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
message could be named INTEGRITY_RULE or, if that would be confusing,
INTEGRITY_POLICY_RULE.

I'm reluctant to change the macro/value. Just use the existing
AUDIT_INTEGRITY_RULE 1805 as appropriate and create a new
AUDIT_INTEGRITY_AUDIT 1806.

1806 would be in sync with INTEGRITY_RULE now for process related info.
If this looks good, I'll remove the dependency on your local context
creation and post the series.
The justification for the change is that the INTEGRITY_RULE, as produced
by ima_parse_rule(), is broken.

Post which series?  The IMA namespacing patch set?  This change should
be upstreamed independently of IMA namespacing.
Mimi

- RGB

--
Richard Guy Briggs <***@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

[Richard Guy Briggs]

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.

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

Exactly.

Post by Richard Guy Briggs
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
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

Post by Richard Guy Briggs
auxiliary record either by being converted to a syscall auxiliary record
by using current->audit_context rather than NULL when calling
audit_log_start(), or creating a local audit_context and calling

ima_parse_rule() is invoked when setting a policy by writing it into
/sys/kernel/security/ima/policy. We unfortunately don't have the
current->audit_context in this case.

Sure you do. What process writes to that file? That's the one we care
about, unless it is somehow handed off to a queue and processed later in
a different context.

I just printk'd it again. current->audit_context is NULL in this case.

Oops, that sounds like some of the netfilter empty table
initializations, whereas usually rules have a user actor.

Post by Richard Guy Briggs
audit_log_container_info() then releasing the local context. This
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.

type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure"
fsmagic="0x9fa0" res=1
type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0
ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
op="invalid_pcr" cause="open_writers" comm="scp"
name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE?

Not necessarily. There are a number of records in the PCR record that
would be redundant when connected to a syscall record, but removing them
is discouraged to avoid breaking parsers that expect them.
I don't see any need to touch the PCR record.

I wasn't going to touch the PCR record.

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it to get
those fields as well. I suppose adding new fields to it wouldn't be
considered breaking user space?

Changing the order of existing fields or inserting fields could break
stuff and is strongly discouraged without a good reason, but appending
fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
with that, abandoning the other format, renaming the less standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

Since current->audit_context is NULL I built on your patch, but I am not
sure whether it is justifyable to use that before your container id series
is applied.
This is what ima_parse_rule() produces now after having it call
audit_log_task_info() as well and by introducing 1806 for ima_parse_rule()
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
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.

type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b"
ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp"
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
1806 would be in sync with INTEGRITY_RULE now for process related info. If
this looks good, I'll remove the dependency on your local context creation
and post the series.

What would be the macro name for 1806?

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

Does this help?

    Stefan

Post by Richard Guy Briggs
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
--
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
--
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 <***@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

[Richard Guy Briggs]

Post by Richard Guy Briggs

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.

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

Exactly.

Post by Richard Guy Briggs
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
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

Post by Richard Guy Briggs
auxiliary record either by being converted to a syscall auxiliary record
by using current->audit_context rather than NULL when calling
audit_log_start(), or creating a local audit_context and calling

ima_parse_rule() is invoked when setting a policy by writing it into
/sys/kernel/security/ima/policy. We unfortunately don't have the
current->audit_context in this case.

Sure you do. What process writes to that file? That's the one we care
about, unless it is somehow handed off to a queue and processed later in
a different context.

I just printk'd it again. current->audit_context is NULL in this case.

Oops, that sounds like some of the netfilter empty table
initializations, whereas usually rules have a user actor.

So it's a bug elsewhere not a 'feature?'

Er, it is a challenge we're aware of and have already solved a number of
them.

What happens if you boot with "audit=1" on the kernel command line?

Post by Richard Guy Briggs

Post by Richard Guy Briggs
audit_log_container_info() then releasing the local context. This
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.

type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure"
fsmagic="0x9fa0" res=1
type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0
ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
op="invalid_pcr" cause="open_writers" comm="scp"
name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE?

Not necessarily. There are a number of records in the PCR record that
would be redundant when connected to a syscall record, but removing them
is discouraged to avoid breaking parsers that expect them.
I don't see any need to touch the PCR record.

I wasn't going to touch the PCR record.

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it to get
those fields as well. I suppose adding new fields to it wouldn't be
considered breaking user space?

Changing the order of existing fields or inserting fields could break
stuff and is strongly discouraged without a good reason, but appending
fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
with that, abandoning the other format, renaming the less standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

Since current->audit_context is NULL I built on your patch, but I am not
sure whether it is justifyable to use that before your container id series
is applied.
This is what ima_parse_rule() produces now after having it call
audit_log_task_info() as well and by introducing 1806 for ima_parse_rule()
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
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.

Post by Richard Guy Briggs

type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b"
ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp"
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
1806 would be in sync with INTEGRITY_RULE now for process related info. If
this looks good, I'll remove the dependency on your local context creation
and post the series.

What would be the macro name for 1806?

I currently work with AUDIT_INTEGRITY_POLICY_RULE.

Post by Richard Guy Briggs

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

Does this help?

    Stefan

Post by Richard Guy Briggs
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
--
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
--
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
--
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 <***@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

[Richard Guy Briggs]

[..]

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it to get
those fields as well. I suppose adding new fields to it wouldn't be
considered breaking user space?

Changing the order of existing fields or inserting fields could break
stuff and is strongly discouraged without a good reason, but appending
fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
with that, abandoning the other format, renaming the less standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

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

So do we want to change both? I thought that what
ima_audit_measurement() produces looks ok but may not have a good name
for the 'type'. Now in this case I would not want to 'break user space'.
The only change I was going to make was to what ima_parse_rule() produces.

The only change for now is separating the IMA policy rules from the
IMA-audit messages.
Richard, when the containerid is appended to the IMA-audit messages,
would we make the audit type name change then?

No, go ahead and make the change now. I'm expecting that the
containerid record will just be another auxiliary record and should not
affect you folks.

To summarize, we need to disambiguate the 1805, as both
ima_parse_rule() and ima_audit_measurement() are using the same number
with different formats.  The main usage of 1805 that we are aware of
is ima_audit_measurement().  Yet the "type=" name for
ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not
INTEGRITY_RULE.
option 1: breaks both uses
1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
option 2: breaks the most common usage
1805 - INTEGRITY_RULE - ima_parse_rule()
1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
option 3: leaves the most common usage with the wrong name, and breaks
the other less common usage
1805 - INTEGRITY_RULE - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
So option 3 is the best option?

Yes, I think so, but option 2 I would be willing to consider. I'd like
to get Paul and Steve's opinions on this.

Mimi

- RGB

--
Richard Guy Briggs <***@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

[Steve Grubb]

This post might be inappropriate. Click to display it.

[Stefan Berger]

Post by Steve Grubb
Hello Stefan,

Post by Richard Guy Briggs
audit_log_container_info() then releasing the local context. This
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'?
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?

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.

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

Post by Steve Grubb
As an aside, I wonder why audit_log_string() is in the API when it is a
helper to audit_log_untrustedstring() ? Without understanding the rules of
untrusted strings, it can't be used correctly without re-inventing
audit_log_untrustedstring() by hand.

Should some of the fields from INTEGRITY_PCR also appear in
INTEGRITY_RULE? If so, which ones?

pid, uid, auid, tty, session, subj, comm, exe, res. <- these are
required to be searchable

We could probably refactor the current integrity_audit_message() and
have ima_parse_rule() call into it to get those fields as well. I
suppose adding new fields to it wouldn't be considered breaking user
space?

The audit user space utilities pretty much expects those fields in that
order for any IMA originating events. You can add things like op or
cause before

We will call into audit_log_task, which will put the parameters into
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.

Post by Steve Grubb
system had audit events with the order I'm mentioning. For example, here's
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

Post by Steve Grubb
Thanks,
-Steve

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

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

[Richard Guy Briggs]

Post by Stefan Berger

Post by Steve Grubb
Hello Stefan,

Post by Richard Guy Briggs
audit_log_container_info() then releasing the local context. This
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'?
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?

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.

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

Post by Steve Grubb
As an aside, I wonder why audit_log_string() is in the API when it is a
helper to audit_log_untrustedstring() ? Without understanding the rules of
untrusted strings, it can't be used correctly without re-inventing
audit_log_untrustedstring() by hand.

Should some of the fields from INTEGRITY_PCR also appear in
INTEGRITY_RULE? If so, which ones?

pid, uid, auid, tty, session, subj, comm, exe, res. <- these are
required to be searchable

We could probably refactor the current integrity_audit_message() and
have ima_parse_rule() call into it to get those fields as well. I
suppose adding new fields to it wouldn't be considered breaking user
space?

The audit user space utilities pretty much expects those fields in that
order for any IMA originating events. You can add things like op or
cause before

We will call into audit_log_task, which will put the parameters into
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.

Post by Steve Grubb
system had audit events with the order I'm mentioning. For example, here's
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.

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.

Post by Stefan Berger
   Stefan

Post by Steve Grubb
-Steve

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

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

- RGB

--
Richard Guy Briggs <***@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

[Steve Grubb]

Post by Richard Guy Briggs

Post by Stefan Berger

Post by Steve Grubb
Hello Stefan,

Post by Richard Guy Briggs
audit_log_container_info() then releasing the local context.
This
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'?
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?

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.

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

Post by Steve Grubb
As an aside, I wonder why audit_log_string() is in the API when it is a
helper to audit_log_untrustedstring() ? Without understanding the
rules of untrusted strings, it can't be used correctly without
re-inventing audit_log_untrustedstring() by hand.

Should some of the fields from INTEGRITY_PCR also appear in
INTEGRITY_RULE? If so, which ones?

pid, uid, auid, tty, session, subj, comm, exe, res. <- these are
required to be searchable

We could probably refactor the current integrity_audit_message() and
have ima_parse_rule() call into it to get those fields as well. I
suppose adding new fields to it wouldn't be considered breaking user
space?

The audit user space utilities pretty much expects those fields in that
order for any IMA originating events. You can add things like op or
cause before

We will call into audit_log_task, which will put the parameters into
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.

Post by Steve Grubb
system had audit events with the order I'm mentioning. For example, here's
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.

type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0
ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
op="invalid_pcr" cause="open_writers" comm="ssh"
name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1
exe="/usr/bin/ssh" tty=tty2

This isn't necessary since they already covered in the already
connected SYSCALL record which duplicates even more information than is
already.

My logs don't show any syscall record being attached. Nor should it. This is
a simple event that should stand on its own.

-Steve

[Steve Grubb]

Post by Stefan Berger

Post by Steve Grubb

Should some of the fields from INTEGRITY_PCR also appear in
INTEGRITY_RULE? If so, which ones?

pid, uid, auid, tty, session, subj, comm, exe, res. <- these are
required to be searchable

We could probably refactor the current integrity_audit_message() and
have ima_parse_rule() call into it to get those fields as well. I
suppose adding new fields to it wouldn't be considered breaking user
space?

The audit user space utilities pretty much expects those fields in that
order for any IMA originating events. You can add things like op or
cause before

We will call into audit_log_task, which will put the parameters into
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.

Post by Steve Grubb
system had audit events with the order I'm mentioning. For example,
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.

type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0
ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
op="invalid_pcr" cause="open_writers" comm="ssh"
name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1
exe="/usr/bin/ssh" tty=tty2

It is safe to put them where they belong. The tools currently skip over tty
and exe if they would be present. So, nothing would get messed up. Its very
simple to add an "if" statement to check for these new fields and collect
them for searching. Also, "res" is traditionally the last field in any event.

Thanks,
-Steve

[Steve Grubb]

[..]

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it
to get
those fields as well. I suppose adding new fields to it wouldn't
be
considered breaking user space?

Changing the order of existing fields or inserting fields could
break
stuff and is strongly discouraged without a good reason, but
appending
fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more
standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and
stick
with that, abandoning the other format, renaming the less
standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

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

So do we want to change both? I thought that what
ima_audit_measurement() produces looks ok but may not have a good
name
for the 'type'. Now in this case I would not want to 'break user
space'.
The only change I was going to make was to what ima_parse_rule()
produces.

The only change for now is separating the IMA policy rules from the
IMA-audit messages.
Richard, when the containerid is appended to the IMA-audit messages,
would we make the audit type name change then?

No, go ahead and make the change now. I'm expecting that the
containerid record will just be another auxiliary record and should not
affect you folks.

To summarize, we need to disambiguate the 1805, as both
ima_parse_rule() and ima_audit_measurement() are using the same number
with different formats. The main usage of 1805 that we are aware of
is ima_audit_measurement(). Yet the "type=" name for
ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not
INTEGRITY_RULE.
option 1: breaks both uses
1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
option 2: breaks the most common usage
1805 - INTEGRITY_RULE - ima_parse_rule()
1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
option 3: leaves the most common usage with the wrong name, and breaks
the other less common usage
1805 - INTEGRITY_RULE - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
So option 3 is the best option?
From a user space perspective, I don't care as long the event is well formed

(No unnecessary untrusted string logging) and we have the required fields for
searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And the
object is identifiable in the event.

-Steve

[Stefan Berger]

[..]

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it
to get
those fields as well. I suppose adding new fields to it wouldn't
be
considered breaking user space?

Changing the order of existing fields or inserting fields could
break
stuff and is strongly discouraged without a good reason, but
appending
fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more
standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and
stick
with that, abandoning the other format, renaming the less
standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

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

So do we want to change both? I thought that what
ima_audit_measurement() produces looks ok but may not have a good
name
for the 'type'. Now in this case I would not want to 'break user
space'.
The only change I was going to make was to what ima_parse_rule()
produces.

The only change for now is separating the IMA policy rules from the
IMA-audit messages.
Richard, when the containerid is appended to the IMA-audit messages,
would we make the audit type name change then?

No, go ahead and make the change now. I'm expecting that the
containerid record will just be another auxiliary record and should not
affect you folks.

To summarize, we need to disambiguate the 1805, as both
ima_parse_rule() and ima_audit_measurement() are using the same number
with different formats. The main usage of 1805 that we are aware of
is ima_audit_measurement(). Yet the "type=" name for
ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not
INTEGRITY_RULE.
option 1: breaks both uses
1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
option 2: breaks the most common usage
1805 - INTEGRITY_RULE - ima_parse_rule()
1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
option 3: leaves the most common usage with the wrong name, and breaks
the other less common usage
1805 - INTEGRITY_RULE - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
So option 3 is the best option?

From a user space perspective, I don't care as long the event is well formed

Are you saying this because of the tools you referred to that require
proper ordering and all fields need to be available?

(No unnecessary untrusted string logging) and we have the required fields for
searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And the
object is identifiable in the event.

There's at least one documented user that showed the existing format...

https://www.fireeye.com/blog/threat-research/2016/11/extending_linux_exec.html

[Steve Grubb]

Hello Stefan,

Post by Stefan Berger

[..]

If so, which ones? We could probably refactor the current
integrity_audit_message() and have ima_parse_rule() call into it
to get those fields as well. I suppose adding new fields to it
wouldn't be considered breaking user space?

Changing the order of existing fields or inserting fields could
break stuff and is strongly discouraged without a good reason, but
appending fields is usually the right way to add information.
There are exceptions, and in this case, I'd pick the "more
standard" of the formats for AUDIT_INTEGRITY_RULE
(ima_audit_measurement?) and stick with that, abandoning the other
format, renaming the less standard version of the record
(ima_parse_rule?) and perhpas adopting that abandonned format for
the new record type while using current->audit_context.

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

So do we want to change both? I thought that what
ima_audit_measurement() produces looks ok but may not have a good
name for the 'type'. Now in this case I would not want to 'break user
space'. The only change I was going to make was to what
ima_parse_rule() produces.

The only change for now is separating the IMA policy rules from the
IMA-audit messages.
Richard, when the containerid is appended to the IMA-audit messages,
would we make the audit type name change then?

No, go ahead and make the change now. I'm expecting that the
containerid record will just be another auxiliary record and should not
affect you folks.

To summarize, we need to disambiguate the 1805, as both
ima_parse_rule() and ima_audit_measurement() are using the same number
with different formats. The main usage of 1805 that we are aware of
is ima_audit_measurement(). Yet the "type=" name for
ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not
INTEGRITY_RULE.
option 1: breaks both uses
1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
option 2: breaks the most common usage
1805 - INTEGRITY_RULE - ima_parse_rule()
1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
option 3: leaves the most common usage with the wrong name, and breaks
the other less common usage
1805 - INTEGRITY_RULE - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
So option 3 is the best option?

From a user space perspective, I don't care as long the event is well formed

Are you saying this because of the tools you referred to that require
proper ordering and all fields need to be available?

Its because the order was established a long time ago. User space tools still
expect that ordering.

Post by Stefan Berger

(No unnecessary untrusted string logging) and we have the required fields
for searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And
the object is identifiable in the event.

There's at least one documented user that showed the existing format...
https://www.fireeye.com/blog/threat-research/2016/11/extending_linux_exec.h
tml

Hmm. I see. If that event was ever sent to linux-audit mail list for
feedback, we would have had some discussion because it's not a proper event.
The order of the fields changed, new fields were added, untrusted string
logging is being used, field names are not documented in the field name
dictionary, existing field names are not being used, subject information is
incomplete, results is missing, and its not entirely clear what the object
is.

-Steve