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