Re: [RFC PATCH ghak90 (was ghak32) V3 00/10] audit: implement container identifier

[Stefan Berger]

On 06/06/2018 12:58 PM, Richard Guy Briggs wrote:



Implement kernel audit container identifier.


What tree does this series build upon as a base? I don't seem to find 
one with the necessary base patches applied.


    Stefan

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v3 4/4] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

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.

Since we defined a new message type we can now also pass the
audit_context and get an associated SYSCALL record. This now produces
the following records when parsing IMA policy's rules:

type=UNKNOWN[1807] msg=audit(1527888965.738:320): action=audit \
  func=MMAP_CHECK mask=MAY_EXEC res=1
type=UNKNOWN[1807] msg=audit(1527888965.738:320): action=audit \
  func=FILE_CHECK mask=MAY_READ res=1
type=SYSCALL msg=audit(1527888965.738:320): arch=c03e syscall=1 \
  success=yes exit=17 a0=1 a1=55bcfcca9030 a2=11 a3=7fcc1b55fb38 \
  items=0 ppid=1567 pid=1601 auid=0 uid=0 gid=0 euid=0 suid=0 \
  fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="echo" \
  exe="/usr/bin/echo" \
  subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)

Signed-off-by: Stefan Berger 
---
 include/uapi/linux/audit.h  | 1 +
 security/integrity/ima/ima_policy.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 65d9293f1fb8..cb358551376b 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -148,6 +148,7 @@
 #define AUDIT_INTEGRITY_PCR1804 /* PCR invalidation msgs */
 #define AUDIT_INTEGRITY_RULE   1805 /* policy rule */
 #define AUDIT_INTEGRITY_EVM_XATTR   1806 /* New EVM-covered xattr */
+#define AUDIT_INTEGRITY_POLICY_RULE 1807 /* 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 bc99713dfe57..f7230db217a7 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -652,8 +652,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry 
*entry)
bool uid_token;
int result = 0;
 
-   ab = integrity_audit_log_start(NULL, GFP_KERNEL,
-  AUDIT_INTEGRITY_RULE);
+   ab = integrity_audit_log_start(current->audit_context, GFP_KERNEL,
+  AUDIT_INTEGRITY_POLICY_RULE);
 
entry->uid = INVALID_UID;
entry->fowner = INVALID_UID;
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v3 2/4] ima: Use audit_log_format() rather than audit_log_string()

[Stefan Berger]

Remove the usage of audit_log_string() and replace it with
audit_log_format().

Signed-off-by: Stefan Berger 
Suggested-by: Steve Grubb 
Reviewed-by: Mimi Zohar 
Acked-by: Paul Moore 
---
 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 1d00db19d167..3fcf0935468c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -634,8 +634,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(_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@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v3 0/4] IMA: work on audit records produced by IMA

[Stefan Berger]

This series of patches cleans up some usages of the audit
subsystem's API by IMA. We also introduce a new record type
that IMA creates while parsing policy rules.

   Stefan

v2->v3:
 - reworked patch 4; pass current->audit_context rather than NULL

v1->v2:
 - dropped several patches that extended existing messages with missing
   fields
 - Using audit_log_task_info() for new record type in last patch
 - rebased on security-next; new message type is now 1807

Stefan Berger (4):
  ima: Call audit_log_string() rather than logging it untrusted
  ima: Use audit_log_format() rather than audit_log_string()
  ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set
  ima: Differentiate auditing policy rules from "audit" actions

 include/uapi/linux/audit.h   |  1 +
 security/integrity/ima/Kconfig   |  1 +
 security/integrity/ima/ima_policy.c  |  9 ++---
 security/integrity/integrity.h   | 15 +++
 security/integrity/integrity_audit.c |  6 +-
 5 files changed, 24 insertions(+), 8 deletions(-)

-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v3 3/4] ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set

[Stefan Berger]

If Integrity is not auditing, IMA shouldn't audit, either.

Signed-off-by: Stefan Berger 
---
 security/integrity/ima/Kconfig  |  1 +
 security/integrity/ima/ima_policy.c |  6 +-
 security/integrity/integrity.h  | 15 +++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 6a8f67714c83..94c2151331aa 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -12,6 +12,7 @@ config IMA
select TCG_TIS if TCG_TPM && X86
select TCG_CRB if TCG_TPM && ACPI
select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES
+   select INTEGRITY_AUDIT if AUDIT
help
  The Trusted Computing Group(TCG) runtime Integrity
  Measurement Architecture(IMA) maintains a list of hash
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 3fcf0935468c..bc99713dfe57 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -628,6 +628,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
  bool (*rule_operator)(kuid_t, kuid_t))
 {
+   if (!ab)
+   return;
+
if (rule_operator == _gt)
audit_log_format(ab, "%s>", key);
else if (rule_operator == _lt)
@@ -649,7 +652,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry 
*entry)
bool uid_token;
int result = 0;
 
-   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
+   ab = integrity_audit_log_start(NULL, GFP_KERNEL,
+  AUDIT_INTEGRITY_RULE);
 
entry->uid = INVALID_UID;
entry->fowner = INVALID_UID;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0bb372eed62a..e60473b13a8d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* iint action cache flags */
 #define IMA_MEASURE0x0001
@@ -199,6 +200,13 @@ 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);
+
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+   return audit_log_start(ctx, gfp_mask, type);
+}
+
 #else
 static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
   const unsigned char *fname,
@@ -206,4 +214,11 @@ static inline void integrity_audit_msg(int audit_msgno, 
struct inode *inode,
   int result, int info)
 {
 }
+
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+   return NULL;
+}
+
 #endif
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v3 1/4] ima: Call audit_log_string() rather than logging it untrusted

[Stefan Berger]

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 
Suggested-by: Steve Grubb 
Acked-by: Paul Moore 
---
 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 8bbc18eb07eb..1d00db19d167 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -634,7 +634,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@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

On 06/01/2018 04:13 PM, Paul Moore wrote:

On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger
 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
 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 
---
 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_STATUS1802 /* 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_KERNEL2000/* 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_api.c#L323

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

Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

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
 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 
---
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_STATUS1802 /* 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_KERNEL2000/* 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_api.c#L323

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.





- RGB

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



--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v2 1/4] ima: Call audit_log_string() rather than logging it untrusted

[Stefan Berger]

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 
Suggested-by: Steve Grubb 
Acked-by: Paul Moore 
---
 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 8bbc18eb07eb..1d00db19d167 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -634,7 +634,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@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v2 2/4] ima: Use audit_log_format() rather than audit_log_string()

[Stefan Berger]

Remove the usage of audit_log_string() and replace it with
audit_log_format().

Signed-off-by: Stefan Berger 
Suggested-by: Steve Grubb 
Reviewed-by: Mimi Zohar 
Acked-by: Paul Moore 
---
 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 1d00db19d167..3fcf0935468c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -634,8 +634,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(_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@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v2 3/4] ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set

[Stefan Berger]

If Integrity is not auditing, IMA shouldn't audit, either.

Signed-off-by: Stefan Berger 
---
 security/integrity/ima/Kconfig  |  1 +
 security/integrity/ima/ima_policy.c |  6 +-
 security/integrity/integrity.h  | 15 +++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 6a8f67714c83..94c2151331aa 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -12,6 +12,7 @@ config IMA
select TCG_TIS if TCG_TPM && X86
select TCG_CRB if TCG_TPM && ACPI
select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES
+   select INTEGRITY_AUDIT if AUDIT
help
  The Trusted Computing Group(TCG) runtime Integrity
  Measurement Architecture(IMA) maintains a list of hash
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 3fcf0935468c..bc99713dfe57 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -628,6 +628,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
  bool (*rule_operator)(kuid_t, kuid_t))
 {
+   if (!ab)
+   return;
+
if (rule_operator == _gt)
audit_log_format(ab, "%s>", key);
else if (rule_operator == _lt)
@@ -649,7 +652,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry 
*entry)
bool uid_token;
int result = 0;
 
-   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
+   ab = integrity_audit_log_start(NULL, GFP_KERNEL,
+  AUDIT_INTEGRITY_RULE);
 
entry->uid = INVALID_UID;
entry->fowner = INVALID_UID;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 0bb372eed62a..e60473b13a8d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* iint action cache flags */
 #define IMA_MEASURE0x0001
@@ -199,6 +200,13 @@ 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);
+
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+   return audit_log_start(ctx, gfp_mask, type);
+}
+
 #else
 static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
   const unsigned char *fname,
@@ -206,4 +214,11 @@ static inline void integrity_audit_msg(int audit_msgno, 
struct inode *inode,
   int result, int info)
 {
 }
+
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+   return NULL;
+}
+
 #endif
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH v2 0/4] IMA: work on audit records produced by IMA

[Stefan Berger]

This series of patches cleans up some usages of the audit
subsystem's API by IMA. We also introduce a new record type
that IMA creates while parsing policy rules.

   Stefan

v1->v2:
 - dropped several patches that extended existing messages with missing
   fields
 - Using audit_log_task_info() for new record type in last patch
 - rebased on security-next; new message type is now 1807

Stefan Berger (4):
  ima: Call audit_log_string() rather than logging it untrusted
  ima: Use audit_log_format() rather than audit_log_string()
  ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set
  ima: Differentiate auditing policy rules from "audit" actions

 include/uapi/linux/audit.h   |  1 +
 security/integrity/ima/Kconfig   |  1 +
 security/integrity/ima/ima_policy.c  | 17 +
 security/integrity/integrity.h   | 15 +++
 security/integrity/integrity_audit.c |  6 +-
 5 files changed, 31 insertions(+), 9 deletions(-)

-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

On 05/30/2018 06:00 PM, Mimi Zohar wrote:

On Wed, 2018-05-30 at 17:49 -0400, Stefan Berger wrote:

So the other choice is to only keep patches 1,2, 6, and 7, so leave most
of the integrity audit messages untouched. Then only create a different
format for the new AUDIT_INTEGRITY_POLICY_RULE (current 8/8) that shares
(for consistency reasons) the same format with the existing integrity
audit messages but also misses tty= and exe= ?

Another option would be for the new AUDIT_INTEGRITY_POLICY_RULE to
call audit_log_task_info() similar to what ima_audit_measurement()
does.


Right. [That would mean keep 1,2, 7 and modify 8.] Is that the best 
solution?




Mimi



--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

On 05/30/2018 05:22 PM, Paul Moore wrote:

On Wed, May 30, 2018 at 9:08 AM, Stefan Berger
 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 
---
   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_STATUS1802 /* 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_KERNEL2000/* 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?


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.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

On 05/30/2018 12:27 PM, Steve Grubb wrote:

On Wednesday, May 30, 2018 11:25:05 AM EDT Stefan Berger wrote:

On 05/30/2018 11:15 AM, Steve Grubb wrote:

On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote:

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

OK. I guess go with it as is. It passes testing.

What about the position of 'res' field relative to the two new fields
'exe' and 'tty'?

res (results) is always the last field for every event. We have no events
where it is not the last field. I'd prefer to go with it as is. The events
pass my testing the way they are.


Do we want to keep them as shown or strictly append the
two new fields 'exe' and 'tty'?

I'd prefer the first option to keep things as expected.


Paul seems to request that they appear after 'res'.

I'd rather see them dropped, as useful as they could be, than to malform the
events.


Paul NACK'ed them since he wanted to have them added to the end. You 
seem to say it's ok to add them before 'res'. Not sure whether to drop 
them now since we are 'at it.'


   Stefan

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 3/8] audit: Implement audit_log_tty()

[Stefan Berger]

On 05/29/2018 05:07 PM, Paul Moore wrote:

On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
 wrote:


+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);
+}

Perhaps I missed it, but your IMA patches only ever call this to log
current's tty, yes?  If so, I would prefer if we dropped the
task_struct argument and always had audit_log_tty() use current.


Done.






--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

On 05/30/2018 11:15 AM, Steve Grubb wrote:

On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote:

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

OK. I guess go with it as is. It passes testing.


What about the position of 'res' field relative to the two new fields 
'exe' and 'tty'? Do we want to keep them as shown or strictly append the 
two new fields 'exe' and 'tty'? Paul seems to request that they appear 
after 'res'.


    Stefan



-Steve


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

   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;
   
   }






--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

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_api.c#L324

The other ones currently all format using integrity_audit_msg().



Thanks,
-Steve


Signed-off-by: Stefan Berger
---
  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_STATUS1802 /* 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;
  }





--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

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 
---
  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_STATUS1802 /* 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 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.








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



--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 6/8] integrity: Factor out common part of integrity_audit_msg()

[Stefan Berger]

On 05/29/2018 05:32 PM, Steve Grubb wrote:

On Thursday, May 24, 2018 4:11:03 PM EDT Stefan Berger wrote:

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

After all of these changes, do you mind sending an example event for testing/
review?


Adding example to 5/8 since this patch here doesn't change any records.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Stefan Berger]

On 05/29/2018 05:19 PM, Paul Moore wrote:

On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
 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 
Suggested-by: Steve Grubb 
---
  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...





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



--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 6/8] integrity: Factor out common part of integrity_audit_msg()

[Stefan Berger]

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

Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com>
---
 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 
 #include 
 #include 
+#include 
 
 /* iint action cache flags */
 #define IMA_MEASURE0x0001
@@ -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(_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);
 }
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 0/8] IMA: work on audit records produced by IMA

[Stefan Berger]

This series of patches cleans up some usages of the audit
subsystem's API by IMA and extends the audit subsystem's API
with API calls for adding new fields to the audit_buffer. Besides
that we extend the existing audit records created while parsing
IMA policy rules with fields that are common for audit records
produced by IMA. Besides that we introduce a new record type
that IMA creates while parsing policy rules.

   Stefan


Stefan Berger (8):
  ima: Call audit_log_string() rather than logging it untrusted
  ima: Use audit_log_format() rather than audit_log_string()
  audit: Implement audit_log_tty()
  audit: Allow others to call audit_log_d_path_exe()
  integrity: Add exe= and tty= before res= to integrity audits
  integrity: Factor out common part of integrity_audit_msg()
  ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set
  ima: Differentiate auditing policy rules from "audit" actions

 include/linux/audit.h| 10 ++
 include/uapi/linux/audit.h   |  3 ++-
 kernel/audit.c   |  8 
 security/integrity/ima/Kconfig   |  1 +
 security/integrity/ima/ima_policy.c  | 12 
 security/integrity/integrity.h   | 26 ++
 security/integrity/integrity_audit.c | 32 +++-
 7 files changed, 74 insertions(+), 18 deletions(-)

-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

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 <stef...@linux.vnet.ibm.com>
---
 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_PCR1804 /* 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;
 }
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 1/8] ima: Call audit_log_string() rather than logging it untrusted

[Stefan Berger]

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 <stef...@linux.vnet.ibm.com>
Suggested-by: Steve Grubb <sgr...@redhat.com>
---
 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@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Stefan Berger]

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 <stef...@linux.vnet.ibm.com>
Suggested-by: Steve Grubb <sgr...@redhat.com>
---
 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);
audit_log_format(ab, " res=%d", !result);
audit_log_end(ab);
 }
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 2/8] ima: Use audit_log_format() rather than audit_log_string()

[Stefan Berger]

Remove the usage of audit_log_string() and replace it with
audit_log_format().

Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com>
Suggested-by: Steve Grubb <sgr...@redhat.com>
Reviewed-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 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(_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@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 7/8] ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set

[Stefan Berger]

If Integrity is not auditing, IMA shouldn't audit, either.

Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com>
---
 security/integrity/ima/Kconfig  |  1 +
 security/integrity/ima/ima_policy.c |  6 +-
 security/integrity/integrity.h  | 10 ++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 6a8f67714c83..94c2151331aa 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -12,6 +12,7 @@ config IMA
select TCG_TIS if TCG_TPM && X86
select TCG_CRB if TCG_TPM && ACPI
select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES
+   select INTEGRITY_AUDIT if AUDIT
help
  The Trusted Computing Group(TCG) runtime Integrity
  Measurement Architecture(IMA) maintains a list of hash
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 7297450b813c..3aed25a7178a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -609,6 +609,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
  bool (*rule_operator)(kuid_t, kuid_t))
 {
+   if (!ab)
+   return;
+
if (rule_operator == _gt)
audit_log_format(ab, "%s>", key);
else if (rule_operator == _lt)
@@ -630,7 +633,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry 
*entry)
bool uid_token;
int result = 0;
 
-   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
+   ab = integrity_audit_log_start(NULL, GFP_KERNEL,
+  AUDIT_INTEGRITY_RULE);
 
entry->uid = INVALID_UID;
entry->fowner = INVALID_UID;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 9f2924cafa53..2afa266aea42 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -203,6 +203,11 @@ void integrity_audit_msg_common(struct audit_buffer *ab, 
struct inode *inode,
const unsigned char *fname, const char *op,
const char *cause, int result);
 
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+   return audit_log_start(ctx, gfp_mask, type);
+}
 #else
 static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
   const unsigned char *fname,
@@ -220,4 +225,9 @@ static inline void integrity_audit_msg_common(struct 
audit_buffer *ab,
 {
 }
 
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+   return NULL;
+}
 #endif
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 3/8] audit: Implement audit_log_tty()

[Stefan Berger]

Implement audit_log_tty() so that IMA can add tty= to its audit records.

Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com>
---
 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);
+}
+
 /**
  * audit_log_link_denied - report a link restriction denial
  * @operation: specific link operation
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 4/8] audit: Allow others to call audit_log_d_path_exe()

[Stefan Berger]

Add the prototype for audit_log_d_path_exe() so that it can be
called by IMA later in this series.

Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com>
Reviewed-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
 include/linux/audit.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2deb76c74d10..65eca0795308 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -144,6 +144,8 @@ extern void audit_log_untrustedstring(struct 
audit_buffer *ab,
 extern voidaudit_log_d_path(struct audit_buffer *ab,
 const char *prefix,
 const struct path *path);
+extern voidaudit_log_d_path_exe(struct audit_buffer *ab,
+struct mm_struct *mm);
 extern voidaudit_log_key(struct audit_buffer *ab,
  char *key);
 extern voidaudit_log_link_denied(const char *operation);
@@ -192,6 +194,9 @@ static inline void audit_log_d_path(struct audit_buffer *ab,
const char *prefix,
const struct path *path)
 { }
+static inline void audit_log_d_path_exe(struct audit_buffer *ab,
+   struct mm_struct *mm)
+{}
 static inline void audit_log_key(struct audit_buffer *ab, char *key)
 { }
 static inline void audit_log_link_denied(const char *string)
-- 
2.13.6

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

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

[Stefan Berger]

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

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

[Stefan Berger]

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




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




--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

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

[Stefan Berger]

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 = and named as listed:


https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events

https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv


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

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

[Stefan Berger]

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

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

[Stefan Berger]

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



--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

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

[Stefan Berger]

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 = and named as listed:


https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events

https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv


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 

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

[Stefan Berger]

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 

Re: [RFC PATCH V1 01/12] audit: add container id

[Stefan Berger]

On 04/18/2018 03:23 PM, Richard Guy Briggs wrote:

On 2018-04-18 14:45, Stefan Berger wrote:

On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:

On 2018-03-15 16:27, Stefan Berger wrote:

On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:

Implement the proc fs write to set the audit container ID of a process,
emitting an AUDIT_CONTAINER record to document the event.

This is a write from the container orchestrator task to a proc entry of
the form /proc/PID/containerid where PID is the process ID of the newly
created task that is to become the first task in a container, or an
additional task added to a container.

The write expects up to a u64 value (unset: 18446744073709551615).

This will produce a record such as this:
type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0

The "op" field indicates an initial set.  The "pid" to "ses" fields are
the orchestrator while the "opid" field is the object's PID, the process
being "contained".  Old and new container ID values are given in the
"contid" fields, while res indicates its success.

It is not permitted to self-set, unset or re-set the container ID.  A
child inherits its parent's container ID, but then can be set only once
after.

See: https://github.com/linux-audit/audit-kernel/issues/32


/* audit_rule_data supports filter rules with both integer and string
 * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4e0a4ac..0ee1e59 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
return rc;
}

+static int audit_set_containerid_perm(struct task_struct *task, u64 
containerid)
+{
+   struct task_struct *parent;
+   u64 pcontainerid, ccontainerid;
+   pid_t ppid;
+
+   /* Don't allow to set our own containerid */
+   if (current == task)
+   return -EPERM;
+   /* Don't allow the containerid to be unset */
+   if (!cid_valid(containerid))
+   return -EINVAL;
+   /* if we don't have caps, reject */
+   if (!capable(CAP_AUDIT_CONTROL))
+   return -EPERM;
+   /* if containerid is unset, allow */
+   if (!audit_containerid_set(task))
+   return 0;

I am wondering whether there should be a check for the target process that
will receive the containerid to not have CAP_SYS_ADMIN that would otherwise
allow it to arbitrarily unshare()/clone() and leave the set of namespaces
that may make up the container whose containerid we assign here?

This is a reasonable question.  This has been debated and I understood
the conclusion was that without a clear definition of a "container", the
task still remains in that container that just now has more
sub-namespaces (in the case of hierarchical namespaces), we don't want
to restrict it in such a way and that allows it to create nested
containers.  I see setns being more problematic if it could switch to
another existing namespace that was set up by the orchestrator for a
different container.  The coming v2 patchset acknowledges this situation
with the network namespace being potentially shared by multiple
containers.

Are you going to post v2 soon? We would like to build on top of it for IMA
namespacing and auditing inside of IMA namespaces.

I don't know if it addresses your specific needs, but V2 was posted on
March 16th along with userspace patches:
https://www.redhat.com/archives/linux-audit/2018-March/msg00110.html
https://www.redhat.com/archives/linux-audit/2018-March/msg00124.html

V3 is pending.
Thanks. I hadn't actually looked at primarily due to the ghak and ghau 
in the title. Whatever these may mean.


Does V2 or will V3 prevent a privileged process to setns() to a whole 
different set of namespaces and still be audited with that initial 
container id ?


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH V1 01/12] audit: add container id

[Stefan Berger]

On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:

On 2018-03-15 16:27, Stefan Berger wrote:

On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:

Implement the proc fs write to set the audit container ID of a process,
emitting an AUDIT_CONTAINER record to document the event.

This is a write from the container orchestrator task to a proc entry of
the form /proc/PID/containerid where PID is the process ID of the newly
created task that is to become the first task in a container, or an
additional task added to a container.

The write expects up to a u64 value (unset: 18446744073709551615).

This will produce a record such as this:
type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0

The "op" field indicates an initial set.  The "pid" to "ses" fields are
the orchestrator while the "opid" field is the object's PID, the process
being "contained".  Old and new container ID values are given in the
"contid" fields, while res indicates its success.

It is not permitted to self-set, unset or re-set the container ID.  A
child inherits its parent's container ID, but then can be set only once
after.

See: https://github.com/linux-audit/audit-kernel/issues/32


   /* audit_rule_data supports filter rules with both integer and string
* fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4e0a4ac..0ee1e59 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
return rc;
   }

+static int audit_set_containerid_perm(struct task_struct *task, u64 
containerid)
+{
+   struct task_struct *parent;
+   u64 pcontainerid, ccontainerid;
+   pid_t ppid;
+
+   /* Don't allow to set our own containerid */
+   if (current == task)
+   return -EPERM;
+   /* Don't allow the containerid to be unset */
+   if (!cid_valid(containerid))
+   return -EINVAL;
+   /* if we don't have caps, reject */
+   if (!capable(CAP_AUDIT_CONTROL))
+   return -EPERM;
+   /* if containerid is unset, allow */
+   if (!audit_containerid_set(task))
+   return 0;

I am wondering whether there should be a check for the target process that
will receive the containerid to not have CAP_SYS_ADMIN that would otherwise
allow it to arbitrarily unshare()/clone() and leave the set of namespaces
that may make up the container whose containerid we assign here?

This is a reasonable question.  This has been debated and I understood
the conclusion was that without a clear definition of a "container", the
task still remains in that container that just now has more
sub-namespaces (in the case of hierarchical namespaces), we don't want
to restrict it in such a way and that allows it to create nested
containers.  I see setns being more problematic if it could switch to
another existing namespace that was set up by the orchestrator for a
different container.  The coming v2 patchset acknowledges this situation
with the network namespace being potentially shared by multiple
containers.


Are you going to post v2 soon? We would like to build on top of it for 
IMA namespacing and auditing inside of IMA namespaces.


   Stefan

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH V1 01/12] audit: add container id

[Stefan Berger]

On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:

Implement the proc fs write to set the audit container ID of a process,
emitting an AUDIT_CONTAINER record to document the event.

This is a write from the container orchestrator task to a proc entry of
the form /proc/PID/containerid where PID is the process ID of the newly
created task that is to become the first task in a container, or an
additional task added to a container.

The write expects up to a u64 value (unset: 18446744073709551615).

This will produce a record such as this:
type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0

The "op" field indicates an initial set.  The "pid" to "ses" fields are
the orchestrator while the "opid" field is the object's PID, the process
being "contained".  Old and new container ID values are given in the
"contid" fields, while res indicates its success.

It is not permitted to self-set, unset or re-set the container ID.  A
child inherits its parent's container ID, but then can be set only once
after.

See: https://github.com/linux-audit/audit-kernel/issues/32


  /* audit_rule_data supports filter rules with both integer and string
   * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4e0a4ac..0ee1e59 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
return rc;
  }

+static int audit_set_containerid_perm(struct task_struct *task, u64 
containerid)
+{
+   struct task_struct *parent;
+   u64 pcontainerid, ccontainerid;
+   pid_t ppid;
+
+   /* Don't allow to set our own containerid */
+   if (current == task)
+   return -EPERM;
+   /* Don't allow the containerid to be unset */
+   if (!cid_valid(containerid))
+   return -EINVAL;
+   /* if we don't have caps, reject */
+   if (!capable(CAP_AUDIT_CONTROL))
+   return -EPERM;
+   /* if containerid is unset, allow */
+   if (!audit_containerid_set(task))
+   return 0;


I am wondering whether there should be a check for the target process 
that will receive the containerid to not have CAP_SYS_ADMIN that would 
otherwise allow it to arbitrarily unshare()/clone() and leave the set of 
namespaces that may make up the container whose containerid we assign here?



+   /* it is already set, and not inherited from the parent, reject */
+   ccontainerid = audit_get_containerid(task);
+   rcu_read_lock();
+   parent = rcu_dereference(task->real_parent);
+   rcu_read_unlock();
+   task_lock(parent);
+   pcontainerid = audit_get_containerid(parent);
+   ppid = task_tgid_nr(parent);

ppid not needed...


+   task_unlock(parent);
+   if (ccontainerid != pcontainerid)
+   return -EPERM;
+   return 0;
+}
+
+static void audit_log_set_containerid(struct task_struct *task, u64 
oldcontainerid,
+ u64 containerid, int rc)
+{
+   struct audit_buffer *ab;
+   uid_t uid;
+   struct tty_struct *tty;
+
+   if (!audit_enabled)
+   return;
+
+   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONTAINER);
+   if (!ab)
+   return;
+
+   uid = from_kuid(_user_ns, task_uid(current));
+   tty = audit_get_tty(current);
+
+   audit_log_format(ab, "op=set pid=%d uid=%u", task_tgid_nr(current), 
uid);
+   audit_log_task_context(ab);
+   audit_log_format(ab, " auid=%u tty=%s ses=%u opid=%d old-contid=%llu 
contid=%llu res=%d",
+from_kuid(_user_ns, audit_get_loginuid(current)),
+tty ? tty_name(tty) : "(none)", 
audit_get_sessionid(current),
+task_tgid_nr(task), oldcontainerid, containerid, !rc);
+
+   audit_put_tty(tty);
+   audit_log_end(ab);
+}
+
+/**
+ * audit_set_containerid - set current task's audit_context containerid
+ * @containerid: containerid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_containerid_write().
+ */
+int audit_set_containerid(struct task_struct *task, u64 containerid)
+{
+   u64 oldcontainerid;
+   int rc;
+
+   oldcontainerid = audit_get_containerid(task);
+
+   rc = audit_set_containerid_perm(task, containerid);
+   if (!rc) {
+   task_lock(task);
+   task->containerid = containerid;
+   task_unlock(task);
+   }
+
+   audit_log_set_containerid(task, oldcontainerid, containerid, rc);
+   return rc;
+}
+
  /**
   * __audit_mq_open - record audit data for a POSIX MQ open
   * @oflag: open flag



--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit