The parameters passed to this logging function are all provided by a privileged user and therefore we can call audit_log_string() rather than audit_log_untrustedstring(). Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; Suggested-by: Steve Grubb <sgrubb(a)redhat.com&gt; --- security/integrity/ima/ima_policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index d89bebf85421..a823f11a3e6b 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -615,7 +615,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value, audit_log_format(ab, "%s<", key); else audit_log_format(ab, "%s=", key); - audit_log_untrustedstring(ab, value); + audit_log_string(ab, value); audit_log_format(ab, " "); } static void ima_log_string(struct audit_buffer *ab, char *key, char *value) -- 2.13.6 -- Linux-audit mailing list Linux-audit(a)redhat.com https://www.redhat.com/mailman/listinfo/linux-audit

Remove the usage of audit_log_string() and replace it with audit_log_format(). Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; Suggested-by: Steve Grubb <sgrubb(a)redhat.com&gt; Reviewed-by: Mimi Zohar <zohar(a)linux.vnet.ibm.com&gt; --- security/integrity/ima/ima_policy.c | 3 +-- security/integrity/integrity_audit.c | 6 +----- 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index a823f11a3e6b..7297450b813c 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -615,8 +615,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value, audit_log_format(ab, "%s<", key); else audit_log_format(ab, "%s=", key); - audit_log_string(ab, value); - audit_log_format(ab, " "); + audit_log_format(ab, "%s ", value); } static void ima_log_string(struct audit_buffer *ab, char *key, char *value) { diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c index 90987d15b6fe..db30763d5525 100644 --- a/security/integrity/integrity_audit.c +++ b/security/integrity/integrity_audit.c @@ -45,11 +45,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, from_kuid(&init_user_ns, audit_get_loginuid(current)), audit_get_sessionid(current)); audit_log_task_context(ab); - audit_log_format(ab, " op="); - audit_log_string(ab, op); - audit_log_format(ab, " cause="); - audit_log_string(ab, cause); - audit_log_format(ab, " comm="); + audit_log_format(ab, " op=%s cause=%s comm=", op, cause); audit_log_untrustedstring(ab, get_task_comm(name, current)); if (fname) { audit_log_format(ab, " name="); -- 2.13.6 -- Linux-audit mailing list Linux-audit(a)redhat.com https://www.redhat.com/mailman/listinfo/linux-audit

Implement audit_log_tty() so that IMA can add tty= to its audit records. Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; --- include/linux/audit.h | 5 +++++ kernel/audit.c | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/include/linux/audit.h b/include/linux/audit.h index 90aa63ddc9be..2deb76c74d10 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -154,6 +154,7 @@ extern void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk); extern int audit_update_lsm_rules(void); +extern void audit_log_tty(struct audit_buffer *ab, struct task_struct *tsk); /* Private API (for audit.c only) */ extern int audit_rule_change(int type, int seq, void *data, size_t datasz); @@ -202,6 +203,10 @@ static inline int audit_log_task_context(struct audit_buffer *ab) static inline void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) { } + +static inline void audit_log_tty(struct audit_buffer *ab, + struct task_struct *tsk) +{ } #define audit_enabled 0 #endif /* CONFIG_AUDIT */ diff --git a/kernel/audit.c b/kernel/audit.c index 670665c6e2a6..fa54695962b4 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -2305,6 +2305,14 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) } EXPORT_SYMBOL(audit_log_task_info); +void audit_log_tty(struct audit_buffer *ab, struct task_struct *tsk) +{ + struct tty_struct *tty = audit_get_tty(tsk); + + audit_log_format(ab, " tty=%s", tty ? tty_name(tty) : "(none)"); + audit_put_tty(tty); +}

On Thu, May 24, 2018 at 4:11 PM, Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; wrote: > Use the new public audit functions to add the exe= and tty= > parts to the integrity audit records. We place them before > res=. > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > Suggested-by: Steve Grubb <sgrubb(a)redhat.com&gt; > --- > > security/integrity/integrity_audit.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/integrity/integrity_audit.c > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca > 100644 > --- a/security/integrity/integrity_audit.c > +++ b/security/integrity/integrity_audit.c > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode > *inode,> > audit_log_untrustedstring(ab, inode->i_sb->s_id); > audit_log_format(ab, " ino=%lu", inode->i_ino); > > } > > + audit_log_d_path_exe(ab, current->mm); > + audit_log_tty(ab, current); NACK Please add the new fields to the end of the audit record, thank you.

> audit_log_format(ab, " res=%d", !result); > audit_log_end(ab); > > }

On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb(a)redhat.com&gt; wrote: > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote: >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger >> >> <stefanb(a)linux.vnet.ibm.com&gt; wrote: >> > Use the new public audit functions to add the exe= and tty= >> > parts to the integrity audit records. We place them before >> > res=. >> > >> > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; >> > Suggested-by: Steve Grubb <sgrubb(a)redhat.com&gt; >> > --- >> > >> > security/integrity/integrity_audit.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/security/integrity/integrity_audit.c >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca >> > 100644 >> > --- a/security/integrity/integrity_audit.c >> > +++ b/security/integrity/integrity_audit.c >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode >> > *inode,> >> > audit_log_untrustedstring(ab, inode->i_sb->s_id); >> > audit_log_format(ab, " ino=%lu", inode->i_ino); >> > >> > } >> > >> > + audit_log_d_path_exe(ab, current->mm); >> > + audit_log_tty(ab, current); >> >> NACK >> >> Please add the new fields to the end of the audit record, thank you. > > Let's see what an example event looks like before NACK'ing this. Way back in > 2013 the IMA events were good. I think this is repairing the event after some > drift. Can you reference a specific commit, or point in time during 2013? Looking at the git log quickly, if I go back to commit d726d8d719b6 ("integrity: move integrity_audit_msg()") from March 18, 2013 (the commit that created integrity_audit.c) the field ordering appears to be the same as it today. My NACK still stands.

On Tue, 2018-05-29 at 18:58 -0400, Mimi Zohar wrote: > On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote: > > On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb(a)redhat.com&gt; wrote: > > > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote: > > >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger > > >> > > >> <stefanb(a)linux.vnet.ibm.com&gt; wrote: > > >> > Use the new public audit functions to add the exe= and tty= > > >> > parts to the integrity audit records. We place them before > > >> > res=. > > >> > > > >> > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > > >> > Suggested-by: Steve Grubb <sgrubb(a)redhat.com&gt; > > >> > --- > > >> > > > >> > security/integrity/integrity_audit.c | 2 ++ > > >> > 1 file changed, 2 insertions(+) > > >> > > > >> > diff --git a/security/integrity/integrity_audit.c > > >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca > > >> > 100644 > > >> > --- a/security/integrity/integrity_audit.c > > >> > +++ b/security/integrity/integrity_audit.c > > >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode > > >> > *inode,> > > >> > audit_log_untrustedstring(ab, inode->i_sb->s_id); > > >> > audit_log_format(ab, " ino=%lu", inode->i_ino); > > >> > > > >> > } > > >> > > > >> > + audit_log_d_path_exe(ab, current->mm); > > >> > + audit_log_tty(ab, current); > > >> > > >> NACK > > >> > > >> Please add the new fields to the end of the audit record, thank you. > > > > > > Let's see what an example event looks like before NACK'ing this. Way back in > > > 2013 the IMA events were good. I think this is repairing the event after some > > > drift. > > > > Can you reference a specific commit, or point in time during 2013? > > Looking at the git log quickly, if I go back to commit d726d8d719b6 > > ("integrity: move integrity_audit_msg()") from March 18, 2013 (the > > commit that created integrity_audit.c) the field ordering appears to > > be the same as it today. > > > > My NACK still stands. > > There hasn't been any changes up to now. This patch set refactors > integrity_audit_msg(), creating integrity_audit_msg_common(), which > will be called from both ima_audit_measurement() and > ima_parse_rule(). That should have been "from integrity_audit_msg() and ima_parse_rule()", not ima_audit_measurement().

> Previously the audit record generated by ima_parse_rule() did not > include this info. The change in this patch will affect both the > existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records.

On 05/29/2018 05:19 PM, Paul Moore wrote: > > On Thu, May 24, 2018 at 4:11 PM, Stefan Berger > <stefanb(a)linux.vnet.ibm.com&gt; wrote: >> >> Use the new public audit functions to add the exe= and tty= >> parts to the integrity audit records. We place them before >> res=. >> >> Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; >> Suggested-by: Steve Grubb <sgrubb(a)redhat.com&gt; >> --- >> security/integrity/integrity_audit.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/security/integrity/integrity_audit.c >> b/security/integrity/integrity_audit.c >> index db30763d5525..8d25d3c4dcca 100644 >> --- a/security/integrity/integrity_audit.c >> +++ b/security/integrity/integrity_audit.c >> @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode >> *inode, >> audit_log_untrustedstring(ab, inode->i_sb->s_id); >> audit_log_format(ab, " ino=%lu", inode->i_ino); >> } >> + audit_log_d_path_exe(ab, current->mm); >> + audit_log_tty(ab, current); > > NACK > > Please add the new fields to the end of the audit record, thank you. I put it there since Steve said '"res" is traditionally the last field in any event' (https://lkml.org/lkml/2018/5/22/539). I don't mind breaking with this tradition...

Factor out a common part of integrity_audit_msg() that others can also call.

Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; --- security/integrity/integrity.h | 16 ++++++++++++++++ security/integrity/integrity_audit.c | 24 ++++++++++++++++-------- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 5e58e02ba8dc..9f2924cafa53 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -15,6 +15,7 @@ #include <linux/integrity.h> #include <crypto/sha.h> #include <linux/key.h> +#include <linux/audit.h> /* iint action cache flags */ #define IMA_MEASURE 0x00000001 @@ -197,6 +198,11 @@ static inline void evm_load_x509(void) void integrity_audit_msg(int audit_msgno, struct inode *inode, const unsigned char *fname, const char *op, const char *cause, int result, int info); + +void integrity_audit_msg_common(struct audit_buffer *ab, struct inode *inode, + const unsigned char *fname, const char *op, + const char *cause, int result); + #else static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, const unsigned char *fname, @@ -204,4 +210,14 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, int result, int info) { } + +static inline void integrity_audit_msg_common(struct audit_buffer *ab, + struct inode *inode, + const unsigned char *fname, + const char *op, + const char *cause, + int result) +{ +} + #endif diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c index 8d25d3c4dcca..8f80b7c042a7 100644 --- a/security/integrity/integrity_audit.c +++ b/security/integrity/integrity_audit.c @@ -28,17 +28,12 @@ static int __init integrity_audit_setup(char *str) } __setup("integrity_audit=", integrity_audit_setup); -void integrity_audit_msg(int audit_msgno, struct inode *inode, - const unsigned char *fname, const char *op, - const char *cause, int result, int audit_info) +void integrity_audit_msg_common(struct audit_buffer *ab, struct inode *inode, + const unsigned char *fname, const char *op, + const char *cause, int result) { - struct audit_buffer *ab; char name[TASK_COMM_LEN]; - if (!integrity_audit_info && audit_info == 1) /* Skip info messages */ - return; - - ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno); audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u", task_pid_nr(current), from_kuid(&init_user_ns, current_cred()->uid), @@ -59,5 +54,18 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, audit_log_d_path_exe(ab, current->mm); audit_log_tty(ab, current); audit_log_format(ab, " res=%d", !result); +} + +void integrity_audit_msg(int audit_msgno, struct inode *inode, + const unsigned char *fname, const char *op, + const char *cause, int result, int audit_info) +{ + struct audit_buffer *ab; + + if (!integrity_audit_info && audit_info == 1) /* Skip info messages */ + return; + + ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno); + integrity_audit_msg_common(ab, inode, fname, op, cause, result); audit_log_end(ab); }

The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1

Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #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 */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE); entry->uid = INVALID_UID; entry->fowner = INVALID_UID; @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; else if (entry->func == POLICY_CHECK) temp_ima_appraise |= IMA_APPRAISE_POLICY; - audit_log_format(ab, "res=%d", !result); + integrity_audit_msg_common(ab, NULL, NULL, + "policy_update", "parse_rule", result); audit_log_end(ab); return result; }

On 05/29/2018 05:30 PM, Steve Grubb wrote: > Hello, > > On Thursday, May 24, 2018 4:11:05 PM EDT Stefan Berger wrote: >> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and >> the IMA "audit" policy action. This patch defines >> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. >> >> With this change we now call integrity_audit_msg_common() to get >> common integrity auditing fields. This now produces the following >> record when parsing an IMA policy rule: >> >> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ >> >> fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ >> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >> op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ >> tty=tty2 res=1 > > Since this is a new event, do you mind moving the tty field to be between > auid= and ses= ? That is the more natural place for it. 6/8 refactors the code so that the integrity audit records produced by IMA follow one format in terms of ordering of the fields, with fields like inode optional, though, and AUDIT_INTEGRITY_RULE in the end being the only one with a different format. Do we really want to change that order just for 1806? 5/8 now produces the following: type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \ uid=0 auid=1000 ses=5 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=invalid_pcr cause=open_writers comm="grep" \ name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \ exe="/usr/bin/grep" tty=pts0 res=1 Comparing the two: 1806: action, fsmagic, pid, uid, auid, ses, subj, op, cause, comm, exe, tty, res INTEGRITY_PCR: pid, uid, auid, ses, subj, op, cause, comm, name, dev, ino, exe, tty, res

> Also, it might be more natural for the op= and cause= fields to be before > the pid= portion. This doesn't matter as much to me because those are > not searchable fields and they are skipped right over. But moving the > tty field is the main comment from me. With the refactoring in 6/8 we at least have consistency among the INTEGRITY_* records, with the only exception being AUDIT_INTEGRITY_RULE that has its own format: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_a pi.c#L324 The other ones currently all format using integrity_audit_msg(). > Thanks, > -Steve > >> Signed-off-by: Stefan Berger&lt;stefanb(a)linux.vnet.ibm.com&gt; >> --- >> >> include/uapi/linux/audit.h | 3 ++- >> security/integrity/ima/ima_policy.c | 5 +++-- >> 2 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >> index 4e61a9e05132..776e0abd35cf 100644 >> --- a/include/uapi/linux/audit.h >> +++ b/include/uapi/linux/audit.h >> @@ -146,7 +146,8 @@ >> >> #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 */ >> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs >> */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ >> >> #define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A > > REQUEST. */ > >> diff --git a/security/integrity/ima/ima_policy.c >> b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 >> 100644 >> --- a/security/integrity/ima/ima_policy.c >> +++ b/security/integrity/ima/ima_policy.c >> @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct >> ima_rule_entry *entry) int result = 0; >> >> ab = integrity_audit_log_start(NULL, GFP_KERNEL, >> >> - AUDIT_INTEGRITY_RULE); >> + AUDIT_INTEGRITY_POLICY_RULE); >> >> entry->uid = INVALID_UID; >> entry->fowner = INVALID_UID; >> >> @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct >> ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; >> >> else if (entry->func == POLICY_CHECK) >> >> temp_ima_appraise |= IMA_APPRAISE_POLICY; >> >> - audit_log_format(ab, "res=%d", !result); >> + integrity_audit_msg_common(ab, NULL, NULL, >> + "policy_update", "parse_rule", result); >> >> audit_log_end(ab); >> return result; >> >> }

The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. With this change we now call integrity_audit_msg_common() to get common integrity auditing fields. This now produces the following record when parsing an IMA policy rule: type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ tty=tty2 res=1 Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; --- include/uapi/linux/audit.h | 3 ++- security/integrity/ima/ima_policy.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 4e61a9e05132..776e0abd35cf 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,7 +146,8 @@ #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 */ +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ #define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */ diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3aed25a7178a..a8ae47a386b4 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) int result = 0; ab = integrity_audit_log_start(NULL, GFP_KERNEL, - AUDIT_INTEGRITY_RULE); + AUDIT_INTEGRITY_POLICY_RULE);

entry->uid = INVALID_UID; entry->fowner = INVALID_UID; @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; else if (entry->func == POLICY_CHECK) temp_ima_appraise |= IMA_APPRAISE_POLICY; - audit_log_format(ab, "res=%d", !result); + integrity_audit_msg_common(ab, NULL, NULL, + "policy_update", "parse_rule", result); audit_log_end(ab); return result; }

On 2018-05-24 16:11, Stefan Berger wrote: > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > the IMA "audit" policy action. This patch defines > AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. > > With this change we now call integrity_audit_msg_common() to get > common integrity auditing fields. This now produces the following > record when parsing an IMA policy rule: > > type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ > fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ > op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ > tty=tty2 res=1 > > Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; > --- > include/uapi/linux/audit.h | 3 ++- > security/integrity/ima/ima_policy.c | 5 +++-- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index 4e61a9e05132..776e0abd35cf 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -146,7 +146,8 @@ > #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 */ > +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ > +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ > > #define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */ > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 3aed25a7178a..a8ae47a386b4 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > int result = 0; > > ab = integrity_audit_log_start(NULL, GFP_KERNEL, > - AUDIT_INTEGRITY_RULE); > + AUDIT_INTEGRITY_POLICY_RULE); Is it possible to connect this record to a syscall by replacing the first parameter (NULL) by current->context?

> entry->uid = INVALID_UID; > entry->fowner = INVALID_UID; > @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; > else if (entry->func == POLICY_CHECK) > temp_ima_appraise |= IMA_APPRAISE_POLICY; > - audit_log_format(ab, "res=%d", !result); > + integrity_audit_msg_common(ab, NULL, NULL, > + "policy_update", "parse_rule", result); > audit_log_end(ab); > return result; > } - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635

On Wed, May 30, 2018 at 9:08 AM, Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; wrote: > On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: >> On 2018-05-24 16:11, Stefan Berger wrote: >>> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and >>> the IMA "audit" policy action. This patch defines >>> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. >>> >>> With this change we now call integrity_audit_msg_common() to get >>> common integrity auditing fields. This now produces the following >>> record when parsing an IMA policy rule: >>> >>> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ >>> fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ >>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >>> op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ >>> tty=tty2 res=1 >>> >>> Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; >>> --- >>> include/uapi/linux/audit.h | 3 ++- >>> security/integrity/ima/ima_policy.c | 5 +++-- >>> 2 files changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >>> index 4e61a9e05132..776e0abd35cf 100644 >>> --- a/include/uapi/linux/audit.h >>> +++ b/include/uapi/linux/audit.h >>> @@ -146,7 +146,8 @@ >>> #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 */ >>> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy >>> msgs */ >>> +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ >>> #define AUDIT_KERNEL 2000 /* Asynchronous audit >>> record. NOT A REQUEST. */ >>> diff --git a/security/integrity/ima/ima_policy.c >>> b/security/integrity/ima/ima_policy.c >>> index 3aed25a7178a..a8ae47a386b4 100644 >>> --- a/security/integrity/ima/ima_policy.c >>> +++ b/security/integrity/ima/ima_policy.c >>> @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct >>> ima_rule_entry *entry) >>> int result = 0; >>> ab = integrity_audit_log_start(NULL, GFP_KERNEL, >>> - AUDIT_INTEGRITY_RULE); >>> + AUDIT_INTEGRITY_POLICY_RULE); >> Is it possible to connect this record to a syscall by replacing the >> first parameter (NULL) by current->context? We're likely going to need to "associate" this record (audit speak for making the first parameter non-NULL) with others for the audit container ID work. If you do it now, Richard's patches will likely get a few lines smaller and that will surely make him a bit happier :)

> We would have to fix current->context in this case since it is NULL. We get > to this location by root cat'ing a policy or writing a policy filename into > /sys/kernel/security/ima/policy. Perhaps I'm missing something, but current in this case should point to the process which is writing to the policy file, yes?

On 2018-05-30 17:38, Stefan Berger wrote: > On 05/30/2018 05:22 PM, Paul Moore wrote: >> On Wed, May 30, 2018 at 9:08 AM, Stefan Berger >> <stefanb(a)linux.vnet.ibm.com&gt; wrote: >>> On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: >>>> On 2018-05-24 16:11, Stefan Berger wrote: >>>>> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and >>>>> the IMA "audit" policy action. This patch defines >>>>> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. >>>>> >>>>> With this change we now call integrity_audit_msg_common() to get >>>>> common integrity auditing fields. This now produces the following >>>>> record when parsing an IMA policy rule: >>>>> >>>>> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \ >>>>> fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ >>>>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >>>>> op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \ >>>>> tty=tty2 res=1 >>>>> >>>>> Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; >>>>> --- >>>>> include/uapi/linux/audit.h | 3 ++- >>>>> security/integrity/ima/ima_policy.c | 5 +++-- >>>>> 2 files changed, 5 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >>>>> index 4e61a9e05132..776e0abd35cf 100644 >>>>> --- a/include/uapi/linux/audit.h >>>>> +++ b/include/uapi/linux/audit.h >>>>> @@ -146,7 +146,8 @@ >>>>> #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 */ >>>>> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy >>>>> msgs */ >>>>> +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ >>>>> #define AUDIT_KERNEL 2000 /* Asynchronous audit >>>>> record. NOT A REQUEST. */ >>>>> diff --git a/security/integrity/ima/ima_policy.c >>>>> b/security/integrity/ima/ima_policy.c >>>>> index 3aed25a7178a..a8ae47a386b4 100644 >>>>> --- a/security/integrity/ima/ima_policy.c >>>>> +++ b/security/integrity/ima/ima_policy.c >>>>> @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct >>>>> ima_rule_entry *entry) >>>>> int result = 0; >>>>> ab = integrity_audit_log_start(NULL, GFP_KERNEL, >>>>> - AUDIT_INTEGRITY_RULE); >>>>> + AUDIT_INTEGRITY_POLICY_RULE); >>>> Is it possible to connect this record to a syscall by replacing the >>>> first parameter (NULL) by current->context? >> We're likely going to need to "associate" this record (audit speak for >> making the first parameter non-NULL) with others for the audit >> container ID work. If you do it now, Richard's patches will likely >> get a few lines smaller and that will surely make him a bit happier :) > Richard is also introducing a local context that we can then create and use > instead of the NULL. Can we not use that then? That is for records for which there is no syscall or user associated. In fact there is another recent change that would be better to use than current->audit_context, which is the function audit_context(). See commit cdfb6b3 ("audit: use inline function to get audit context"). > Steven seems to say: "We don't want to add syscall records to everything. > That messes up schemas and existing code. The integrity events are 1 record > in size and should stay that way. This saves disk space and improves > readability." > >>> We would have to fix current->context in this case since it is NULL. We get >>> to this location by root cat'ing a policy or writing a policy filename into >>> /sys/kernel/security/ima/policy. >> Perhaps I'm missing something, but current in this case should point >> to the process which is writing to the policy file, yes? > Yes, but current->context is NULL for some reason. Is it always this way? If it isn't, which it should not be, we should find out why. Well, we should find out why this is NULL anyways, since it shouldn't be.

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

On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; wrote: > On 05/30/2018 07:34 PM, Richard Guy Briggs wrote: >> >> On 2018-05-30 17:38, Stefan Berger wrote: >>> >>> On 05/30/2018 05:22 PM, Paul Moore wrote: >>>> >>>> On Wed, May 30, 2018 at 9:08 AM, Stefan Berger >>>> <stefanb(a)linux.vnet.ibm.com&gt; wrote: >>>>> >>>>> On 05/30/2018 08:49 AM, Richard Guy Briggs wrote: >>>>>> >>>>>> On 2018-05-24 16:11, Stefan Berger wrote: >>>>>>> >>>>>>> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and >>>>>>> the IMA "audit" policy action. This patch defines >>>>>>> AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules. >>>>>>> >>>>>>> With this change we now call integrity_audit_msg_common() to get >>>>>>> common integrity auditing fields. This now produces the following >>>>>>> record when parsing an IMA policy rule: >>>>>>> >>>>>>> type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure >>>>>>> \ >>>>>>> fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \ >>>>>>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \ >>>>>>> op=policy_update cause=parse_rule comm="echo" >>>>>>> exe="/usr/bin/echo" \ >>>>>>> tty=tty2 res=1 >>>>>>> >>>>>>> Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; >>>>>>> --- >>>>>>> include/uapi/linux/audit.h | 3 ++- >>>>>>> security/integrity/ima/ima_policy.c | 5 +++-- >>>>>>> 2 files changed, 5 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >>>>>>> index 4e61a9e05132..776e0abd35cf 100644 >>>>>>> --- a/include/uapi/linux/audit.h >>>>>>> +++ b/include/uapi/linux/audit.h >>>>>>> @@ -146,7 +146,8 @@ >>>>>>> #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 */ >>>>>>> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy >>>>>>> msgs */ >>>>>>> +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */ >>>>>>> #define AUDIT_KERNEL 2000 /* Asynchronous >>>>>>> audit >>>>>>> record. NOT A REQUEST. */ >>>>>>> diff --git a/security/integrity/ima/ima_policy.c >>>>>>> b/security/integrity/ima/ima_policy.c >>>>>>> index 3aed25a7178a..a8ae47a386b4 100644 >>>>>>> --- a/security/integrity/ima/ima_policy.c >>>>>>> +++ b/security/integrity/ima/ima_policy.c >>>>>>> @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct >>>>>>> ima_rule_entry *entry) >>>>>>> int result = 0; >>>>>>> ab = integrity_audit_log_start(NULL, GFP_KERNEL, >>>>>>> - AUDIT_INTEGRITY_RULE); >>>>>>> + AUDIT_INTEGRITY_POLICY_RULE); >>>>>> >>>>>> Is it possible to connect this record to a syscall by replacing the >>>>>> first parameter (NULL) by current->context? >>>> >>>> We're likely going to need to "associate" this record (audit speak for >>>> making the first parameter non-NULL) with others for the audit >>>> container ID work. If you do it now, Richard's patches will likely >>>> get a few lines smaller and that will surely make him a bit happier :) >>> >>> Richard is also introducing a local context that we can then create and >>> use >>> instead of the NULL. Can we not use that then? >> >> That is for records for which there is no syscall or user associated. >> >> In fact there is another recent change that would be better to use than >> current->audit_context, which is the function audit_context(). >> See commit cdfb6b3 ("audit: use inline function to get audit context"). >> >>> Steven seems to say: "We don't want to add syscall records to everything. >>> That messes up schemas and existing code. The integrity events are 1 >>> record >>> in size and should stay that way. This saves disk space and improves >>> readability." >>> >>>>> We would have to fix current->context in this case since it is NULL. We >>>>> get >>>>> to this location by root cat'ing a policy or writing a policy filename >>>>> into >>>>> /sys/kernel/security/ima/policy. >>>> >>>> Perhaps I'm missing something, but current in this case should point >>>> to the process which is writing to the policy file, yes? >>> >>> Yes, but current->context is NULL for some reason. >> >> Is it always this way? If it isn't, which it should not be, we should >> find out why. Well, we should find out why this is NULL anyways, since >> it shouldn't be. > > > When someone writes a policy for IMA into securityfs, it's always NULL. > There's another location where IMA uses the current->audit_context, and > that's here: > > https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima... > > At this location we sometimes see a (background) process with an > audit_context but in the majority of cases it's current->audit_context is > NULL. Starting a process as root or also non-root user, with the appropriate > IMA audit policy rules set, we always see a NULL audit_context here. What does your audit configuration look like? Depending on your configuration a NULL audit_context can be expected, see audit_dummy_context(). I believe the default Fedora audit config will leave you with a NULL audit_context for all processes. I also believe that unless you explicitly set "audit=1" on the kernel command line the init/systemd process will have a NULL audit_context (there was actually a range of kernels where even setting "audit=1" wouldn't be sufficient due to a bug we fixed a little while ago). Look at the audit_alloc() function, it is called when a new process is fork'd and is responsible for allocating a new audit_context. If the currently loaded audit config dictates that auditing is to be disabled for this new process (state == AUDIT_DISABLED) then an audit_context is not allocated and current->context remains NULL.