[PATCH V1] audit: log xattr args not covered by syscall record

[PATCH V1] audit: log xattr args not covered by syscall record

[Richard Guy Briggs]

The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
four arguments and only lists pointers of string values.  The xattr name
string, value string and flags (5th arg) are needed by audit given the
syscall's main purpose.

Add the auxiliary record AUDIT_XATTR (1336) to record the details not
available in the SYSCALL record including the name string, value string
and flags.

Notes about field names:
- name is too generic, use xattr precedent from ima
- val is already generic value field name
- flags used by mmap, xflags new name

Sample event with new record:
type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
type=SYSCALL msg=audit(05/07/2021 12:58:42.176:189) : arch=x86_64 syscall=fsetxattr success=yes exit=0 a0=0x3 a1=0x7fc2f055905f a2=0x7ffebd58ebb0 a3=0x14 items=1 ppid=526 pid=554 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=filecap exe=/usr/bin/filecap subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=cap-test

Link: https://github.com/linux-audit/audit-kernel/issues/39
Link: https://lore.kernel.org/r/604ceafd516b0785fea120f552d6336054d196af.1620414949.git.rgb@redhat.com
Suggested-by: Steve Grubb <sgrubb@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 fs/xattr.c                 |  2 ++
 include/linux/audit.h      | 10 +++++++++
 include/uapi/linux/audit.h |  1 +
 kernel/audit.h             |  5 +++++
 kernel/auditsc.c           | 45 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 63 insertions(+)

diff --git a/fs/xattr.c b/fs/xattr.c
index b3444e06cded..f2b6af1719fd 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -570,6 +570,7 @@ setxattr(struct user_namespace *mnt_userns, struct dentry *d,
 			posix_acl_fix_xattr_from_user(mnt_userns, kvalue, size);
 	}
 
+	audit_xattr(name, value, flags);
 	error = vfs_setxattr(mnt_userns, d, kname, kvalue, size, flags);
 out:
 	kvfree(kvalue);
@@ -816,6 +817,7 @@ removexattr(struct user_namespace *mnt_userns, struct dentry *d,
 	if (error < 0)
 		return error;
 
+	audit_xattr(name, "(null)", 0);
 	return vfs_removexattr(mnt_userns, d, kname);
 }
 
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 82b7c1116a85..784d34888c8a 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -404,6 +404,7 @@ extern void __audit_tk_injoffset(struct timespec64 offset);
 extern void __audit_ntp_log(const struct audit_ntp_data *ad);
 extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
 			      enum audit_nfcfgop op, gfp_t gfp);
+extern void __audit_xattr(const char *name, const char *value, int flags);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -547,6 +548,12 @@ static inline void audit_log_nfcfg(const char *name, u8 af,
 		__audit_log_nfcfg(name, af, nentries, op, gfp);
 }
 
+static inline void audit_xattr(const char *name, const char *value, int flags)
+{
+	if (!audit_dummy_context())
+		__audit_xattr(name, value, flags);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -677,6 +684,9 @@ static inline void audit_log_nfcfg(const char *name, u8 af,
 				   enum audit_nfcfgop op, gfp_t gfp)
 { }
 
+static inline void audit_xattr(const char *name, const char *value, int flags)
+{ }
+
 #define audit_n_rules 0
 #define audit_signals 0
 #endif /* CONFIG_AUDITSYSCALL */
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index cd2d8279a5e4..4477ff80a24d 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -118,6 +118,7 @@
 #define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
 #define AUDIT_BPF		1334	/* BPF subsystem */
 #define AUDIT_EVENT_LISTENER	1335	/* Task joined multicast read socket */
+#define AUDIT_XATTR		1336	/* xattr arguments */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff --git a/kernel/audit.h b/kernel/audit.h
index 1522e100fd17..9544284fce57 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -191,6 +191,11 @@ struct audit_context {
 		struct {
 			char			*name;
 		} module;
+		struct {
+			char			*name;
+			char			*value;
+			int			flags;
+		} xattr;
 	};
 	int fds[2];
 	struct audit_proctitle proctitle;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8bb9ac84d2fb..7f2b56136fa4 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -884,6 +884,7 @@ static inline void audit_free_module(struct audit_context *context)
 		context->module.name = NULL;
 	}
 }
+
 static inline void audit_free_names(struct audit_context *context)
 {
 	struct audit_names *n, *next;
@@ -915,6 +916,16 @@ static inline void audit_free_aux(struct audit_context *context)
 	}
 }
 
+static inline void audit_free_xattr(struct audit_context *context)
+{
+	if (context->type == AUDIT_XATTR) {
+		kfree(context->xattr.name);
+		context->xattr.name = NULL;
+		kfree(context->xattr.value);
+		context->xattr.value = NULL;
+	}
+}
+
 static inline struct audit_context *audit_alloc_context(enum audit_state state)
 {
 	struct audit_context *context;
@@ -969,6 +980,7 @@ int audit_alloc(struct task_struct *tsk)
 
 static inline void audit_free_context(struct audit_context *context)
 {
+	audit_free_xattr(context);
 	audit_free_module(context);
 	audit_free_names(context);
 	unroll_tree_refs(context, NULL, 0);
@@ -1317,6 +1329,20 @@ static void show_special(struct audit_context *context, int *call_panic)
 		} else
 			audit_log_format(ab, "(null)");
 
+		break;
+	case AUDIT_XATTR:
+		audit_log_format(ab, "xattr=");
+		if (context->xattr.name)
+			audit_log_untrustedstring(ab, context->xattr.name);
+		else
+			audit_log_format(ab, "(null)");
+		audit_log_format(ab, " val=");
+		if (context->xattr.value)
+			audit_log_untrustedstring(ab, context->xattr.value);
+		else
+			audit_log_format(ab, "(null)");
+		audit_log_format(ab, " xflags=0x%x", context->xattr.flags);
+
 		break;
 	}
 	audit_log_end(ab);
@@ -1742,6 +1768,7 @@ void __audit_syscall_exit(int success, long return_code)
 	context->in_syscall = 0;
 	context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
 
+	audit_free_xattr(context);
 	audit_free_module(context);
 	audit_free_names(context);
 	unroll_tree_refs(context, NULL, 0);
@@ -2536,6 +2563,24 @@ void __audit_log_kern_module(char *name)
 	context->type = AUDIT_KERN_MODULE;
 }
 
+void __audit_xattr(const char *name, const char *value, int flags)
+{
+	struct audit_context *context = audit_context();
+
+	context->type = AUDIT_XATTR;
+	context->xattr.flags = flags;
+	context->xattr.name = kstrdup(name, GFP_KERNEL);
+	if (!context->xattr.name)
+		goto out;
+	context->xattr.value = kstrdup(value, GFP_KERNEL);
+	if (!context->xattr.value)
+		goto out;
+	return;
+out:
+	kfree(context->xattr.name);
+	audit_log_lost("out of memory in __audit_xattr");
+}
+
 void __audit_fanotify(unsigned int response)
 {
 	audit_log(audit_context(), GFP_KERNEL,
-- 
2.27.0

Re: [PATCH V1] audit: log xattr args not covered by syscall record

[Casey Schaufler]

On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
> The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
> four arguments and only lists pointers of string values.  The xattr name
> string, value string and flags (5th arg) are needed by audit given the
> syscall's main purpose.
>
> Add the auxiliary record AUDIT_XATTR (1336) to record the details not
> available in the SYSCALL record including the name string, value string
> and flags.
>
> Notes about field names:
> - name is too generic, use xattr precedent from ima
> - val is already generic value field name
> - flags used by mmap, xflags new name
>
> Sample event with new record:
> type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
> type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
> type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
> type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0

Would it be sensible to break out the namespace from the attribute?

	attrspace="security" attrname="capability"

Why isn't val= quoted?

The attribute value can be a .jpg or worse. I could even see it being an eBPF
program (although That Would Be Wrong) so including it in an audit record could
be a bit of a problem.

It seems that you might want to leave it up to the LSMs to determine which xattr
values are audited. An SELinux system may want to see "security.selinux" values,
but it probably doesn't care about "security.SMACK64TRANSMUTE" values.

> type=SYSCALL msg=audit(05/07/2021 12:58:42.176:189) : arch=x86_64 syscall=fsetxattr success=yes exit=0 a0=0x3 a1=0x7fc2f055905f a2=0x7ffebd58ebb0 a3=0x14 items=1 ppid=526 pid=554 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=filecap exe=/usr/bin/filecap subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=cap-test
>
> Link: https://github.com/linux-audit/audit-kernel/issues/39
> Link: https://lore.kernel.org/r/604ceafd516b0785fea120f552d6336054d196af.1620414949.git.rgb@redhat.com
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  fs/xattr.c                 |  2 ++
>  include/linux/audit.h      | 10 +++++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/audit.h             |  5 +++++
>  kernel/auditsc.c           | 45 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 63 insertions(+)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index b3444e06cded..f2b6af1719fd 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -570,6 +570,7 @@ setxattr(struct user_namespace *mnt_userns, struct dentry *d,
>  			posix_acl_fix_xattr_from_user(mnt_userns, kvalue, size);
>  	}
>  
> +	audit_xattr(name, value, flags);
>  	error = vfs_setxattr(mnt_userns, d, kname, kvalue, size, flags);
>  out:
>  	kvfree(kvalue);
> @@ -816,6 +817,7 @@ removexattr(struct user_namespace *mnt_userns, struct dentry *d,
>  	if (error < 0)
>  		return error;
>  
> +	audit_xattr(name, "(null)", 0);
>  	return vfs_removexattr(mnt_userns, d, kname);
>  }
>  
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 82b7c1116a85..784d34888c8a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -404,6 +404,7 @@ extern void __audit_tk_injoffset(struct timespec64 offset);
>  extern void __audit_ntp_log(const struct audit_ntp_data *ad);
>  extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
>  			      enum audit_nfcfgop op, gfp_t gfp);
> +extern void __audit_xattr(const char *name, const char *value, int flags);
>  
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -547,6 +548,12 @@ static inline void audit_log_nfcfg(const char *name, u8 af,
>  		__audit_log_nfcfg(name, af, nentries, op, gfp);
>  }
>  
> +static inline void audit_xattr(const char *name, const char *value, int flags)
> +{
> +	if (!audit_dummy_context())
> +		__audit_xattr(name, value, flags);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -677,6 +684,9 @@ static inline void audit_log_nfcfg(const char *name, u8 af,
>  				   enum audit_nfcfgop op, gfp_t gfp)
>  { }
>  
> +static inline void audit_xattr(const char *name, const char *value, int flags)
> +{ }
> +
>  #define audit_n_rules 0
>  #define audit_signals 0
>  #endif /* CONFIG_AUDITSYSCALL */
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index cd2d8279a5e4..4477ff80a24d 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -118,6 +118,7 @@
>  #define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
>  #define AUDIT_BPF		1334	/* BPF subsystem */
>  #define AUDIT_EVENT_LISTENER	1335	/* Task joined multicast read socket */
> +#define AUDIT_XATTR		1336	/* xattr arguments */
>  
>  #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 1522e100fd17..9544284fce57 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -191,6 +191,11 @@ struct audit_context {
>  		struct {
>  			char			*name;
>  		} module;
> +		struct {
> +			char			*name;
> +			char			*value;
> +			int			flags;
> +		} xattr;
>  	};
>  	int fds[2];
>  	struct audit_proctitle proctitle;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8bb9ac84d2fb..7f2b56136fa4 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -884,6 +884,7 @@ static inline void audit_free_module(struct audit_context *context)
>  		context->module.name = NULL;
>  	}
>  }
> +
>  static inline void audit_free_names(struct audit_context *context)
>  {
>  	struct audit_names *n, *next;
> @@ -915,6 +916,16 @@ static inline void audit_free_aux(struct audit_context *context)
>  	}
>  }
>  
> +static inline void audit_free_xattr(struct audit_context *context)
> +{
> +	if (context->type == AUDIT_XATTR) {
> +		kfree(context->xattr.name);
> +		context->xattr.name = NULL;
> +		kfree(context->xattr.value);
> +		context->xattr.value = NULL;
> +	}
> +}
> +
>  static inline struct audit_context *audit_alloc_context(enum audit_state state)
>  {
>  	struct audit_context *context;
> @@ -969,6 +980,7 @@ int audit_alloc(struct task_struct *tsk)
>  
>  static inline void audit_free_context(struct audit_context *context)
>  {
> +	audit_free_xattr(context);
>  	audit_free_module(context);
>  	audit_free_names(context);
>  	unroll_tree_refs(context, NULL, 0);
> @@ -1317,6 +1329,20 @@ static void show_special(struct audit_context *context, int *call_panic)
>  		} else
>  			audit_log_format(ab, "(null)");
>  
> +		break;
> +	case AUDIT_XATTR:
> +		audit_log_format(ab, "xattr=");
> +		if (context->xattr.name)
> +			audit_log_untrustedstring(ab, context->xattr.name);
> +		else
> +			audit_log_format(ab, "(null)");
> +		audit_log_format(ab, " val=");
> +		if (context->xattr.value)
> +			audit_log_untrustedstring(ab, context->xattr.value);
> +		else
> +			audit_log_format(ab, "(null)");
> +		audit_log_format(ab, " xflags=0x%x", context->xattr.flags);
> +
>  		break;
>  	}
>  	audit_log_end(ab);
> @@ -1742,6 +1768,7 @@ void __audit_syscall_exit(int success, long return_code)
>  	context->in_syscall = 0;
>  	context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
>  
> +	audit_free_xattr(context);
>  	audit_free_module(context);
>  	audit_free_names(context);
>  	unroll_tree_refs(context, NULL, 0);
> @@ -2536,6 +2563,24 @@ void __audit_log_kern_module(char *name)
>  	context->type = AUDIT_KERN_MODULE;
>  }
>  
> +void __audit_xattr(const char *name, const char *value, int flags)
> +{
> +	struct audit_context *context = audit_context();
> +
> +	context->type = AUDIT_XATTR;
> +	context->xattr.flags = flags;
> +	context->xattr.name = kstrdup(name, GFP_KERNEL);
> +	if (!context->xattr.name)
> +		goto out;
> +	context->xattr.value = kstrdup(value, GFP_KERNEL);
> +	if (!context->xattr.value)
> +		goto out;
> +	return;
> +out:
> +	kfree(context->xattr.name);
> +	audit_log_lost("out of memory in __audit_xattr");
> +}
> +
>  void __audit_fanotify(unsigned int response)
>  {
>  	audit_log(audit_context(), GFP_KERNEL,

Re: [PATCH V1] audit: log xattr args not covered by syscall record

[Richard Guy Briggs]

On 2021-05-07 14:03, Casey Schaufler wrote:
> On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
> > The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
> > four arguments and only lists pointers of string values.  The xattr name
> > string, value string and flags (5th arg) are needed by audit given the
> > syscall's main purpose.
> >
> > Add the auxiliary record AUDIT_XATTR (1336) to record the details not
> > available in the SYSCALL record including the name string, value string
> > and flags.
> >
> > Notes about field names:
> > - name is too generic, use xattr precedent from ima
> > - val is already generic value field name
> > - flags used by mmap, xflags new name
> >
> > Sample event with new record:
> > type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
> > type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
> > type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
> > type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
> 
> Would it be sensible to break out the namespace from the attribute?
> 
> 	attrspace="security" attrname="capability"

Do xattrs always follow this nomenclature?  Or only the ones we care
about?

> Why isn't val= quoted?

Good question.  I guessed it should have been since it used
audit_log_untrustedstring(), but even the raw output is unquoted unless
it was converted by auditd to unquoted before being stored to disk due
to nothing offensive found in it since audit_log_n_string() does add
quotes.  (hmmm, bit of a run-on sentence there...)

> The attribute value can be a .jpg or worse. I could even see it being an eBPF
> program (although That Would Be Wrong) so including it in an audit record could
> be a bit of a problem.

In these cases it would almost certainly get caught by the control
character test audit_string_contains_control() in
audit_log_n_untrustedstring() called from audit_log_untrustedstring()
and deliver it as hex.

> It seems that you might want to leave it up to the LSMs to determine which xattr
> values are audited. An SELinux system may want to see "security.selinux" values,
> but it probably doesn't care about "security.SMACK64TRANSMUTE" values.

Are you suggesting that any that don't follow this nomenclature are
irrelevant or harmless at best and are not worth recording?

This sounds like you are thinking about your LSM stacking patchset that
issues a new record for each LSM attribute if there is more than one.
And any system that has multiple LSMs active should be able to indicate
all of interest.

> > type=SYSCALL msg=audit(05/07/2021 12:58:42.176:189) : arch=x86_64 syscall=fsetxattr success=yes exit=0 a0=0x3 a1=0x7fc2f055905f a2=0x7ffebd58ebb0 a3=0x14 items=1 ppid=526 pid=554 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=filecap exe=/usr/bin/filecap subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=cap-test
> >
> > Link: https://github.com/linux-audit/audit-kernel/issues/39
> > Link: https://lore.kernel.org/r/604ceafd516b0785fea120f552d6336054d196af.1620414949.git.rgb@redhat.com
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  fs/xattr.c                 |  2 ++
> >  include/linux/audit.h      | 10 +++++++++
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/audit.h             |  5 +++++
> >  kernel/auditsc.c           | 45 ++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 63 insertions(+)
> >
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index b3444e06cded..f2b6af1719fd 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -570,6 +570,7 @@ setxattr(struct user_namespace *mnt_userns, struct dentry *d,
> >  			posix_acl_fix_xattr_from_user(mnt_userns, kvalue, size);
> >  	}
> >  
> > +	audit_xattr(name, value, flags);
> >  	error = vfs_setxattr(mnt_userns, d, kname, kvalue, size, flags);
> >  out:
> >  	kvfree(kvalue);
> > @@ -816,6 +817,7 @@ removexattr(struct user_namespace *mnt_userns, struct dentry *d,
> >  	if (error < 0)
> >  		return error;
> >  
> > +	audit_xattr(name, "(null)", 0);
> >  	return vfs_removexattr(mnt_userns, d, kname);
> >  }
> >  
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 82b7c1116a85..784d34888c8a 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -404,6 +404,7 @@ extern void __audit_tk_injoffset(struct timespec64 offset);
> >  extern void __audit_ntp_log(const struct audit_ntp_data *ad);
> >  extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
> >  			      enum audit_nfcfgop op, gfp_t gfp);
> > +extern void __audit_xattr(const char *name, const char *value, int flags);
> >  
> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >  {
> > @@ -547,6 +548,12 @@ static inline void audit_log_nfcfg(const char *name, u8 af,
> >  		__audit_log_nfcfg(name, af, nentries, op, gfp);
> >  }
> >  
> > +static inline void audit_xattr(const char *name, const char *value, int flags)
> > +{
> > +	if (!audit_dummy_context())
> > +		__audit_xattr(name, value, flags);
> > +}
> > +
> >  extern int audit_n_rules;
> >  extern int audit_signals;
> >  #else /* CONFIG_AUDITSYSCALL */
> > @@ -677,6 +684,9 @@ static inline void audit_log_nfcfg(const char *name, u8 af,
> >  				   enum audit_nfcfgop op, gfp_t gfp)
> >  { }
> >  
> > +static inline void audit_xattr(const char *name, const char *value, int flags)
> > +{ }
> > +
> >  #define audit_n_rules 0
> >  #define audit_signals 0
> >  #endif /* CONFIG_AUDITSYSCALL */
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index cd2d8279a5e4..4477ff80a24d 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -118,6 +118,7 @@
> >  #define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
> >  #define AUDIT_BPF		1334	/* BPF subsystem */
> >  #define AUDIT_EVENT_LISTENER	1335	/* Task joined multicast read socket */
> > +#define AUDIT_XATTR		1336	/* xattr arguments */
> >  
> >  #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
> >  #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 1522e100fd17..9544284fce57 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -191,6 +191,11 @@ struct audit_context {
> >  		struct {
> >  			char			*name;
> >  		} module;
> > +		struct {
> > +			char			*name;
> > +			char			*value;
> > +			int			flags;
> > +		} xattr;
> >  	};
> >  	int fds[2];
> >  	struct audit_proctitle proctitle;
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 8bb9ac84d2fb..7f2b56136fa4 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -884,6 +884,7 @@ static inline void audit_free_module(struct audit_context *context)
> >  		context->module.name = NULL;
> >  	}
> >  }
> > +
> >  static inline void audit_free_names(struct audit_context *context)
> >  {
> >  	struct audit_names *n, *next;
> > @@ -915,6 +916,16 @@ static inline void audit_free_aux(struct audit_context *context)
> >  	}
> >  }
> >  
> > +static inline void audit_free_xattr(struct audit_context *context)
> > +{
> > +	if (context->type == AUDIT_XATTR) {
> > +		kfree(context->xattr.name);
> > +		context->xattr.name = NULL;
> > +		kfree(context->xattr.value);
> > +		context->xattr.value = NULL;
> > +	}
> > +}
> > +
> >  static inline struct audit_context *audit_alloc_context(enum audit_state state)
> >  {
> >  	struct audit_context *context;
> > @@ -969,6 +980,7 @@ int audit_alloc(struct task_struct *tsk)
> >  
> >  static inline void audit_free_context(struct audit_context *context)
> >  {
> > +	audit_free_xattr(context);
> >  	audit_free_module(context);
> >  	audit_free_names(context);
> >  	unroll_tree_refs(context, NULL, 0);
> > @@ -1317,6 +1329,20 @@ static void show_special(struct audit_context *context, int *call_panic)
> >  		} else
> >  			audit_log_format(ab, "(null)");
> >  
> > +		break;
> > +	case AUDIT_XATTR:
> > +		audit_log_format(ab, "xattr=");
> > +		if (context->xattr.name)
> > +			audit_log_untrustedstring(ab, context->xattr.name);
> > +		else
> > +			audit_log_format(ab, "(null)");
> > +		audit_log_format(ab, " val=");
> > +		if (context->xattr.value)
> > +			audit_log_untrustedstring(ab, context->xattr.value);
> > +		else
> > +			audit_log_format(ab, "(null)");
> > +		audit_log_format(ab, " xflags=0x%x", context->xattr.flags);
> > +
> >  		break;
> >  	}
> >  	audit_log_end(ab);
> > @@ -1742,6 +1768,7 @@ void __audit_syscall_exit(int success, long return_code)
> >  	context->in_syscall = 0;
> >  	context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
> >  
> > +	audit_free_xattr(context);
> >  	audit_free_module(context);
> >  	audit_free_names(context);
> >  	unroll_tree_refs(context, NULL, 0);
> > @@ -2536,6 +2563,24 @@ void __audit_log_kern_module(char *name)
> >  	context->type = AUDIT_KERN_MODULE;
> >  }
> >  
> > +void __audit_xattr(const char *name, const char *value, int flags)
> > +{
> > +	struct audit_context *context = audit_context();
> > +
> > +	context->type = AUDIT_XATTR;
> > +	context->xattr.flags = flags;
> > +	context->xattr.name = kstrdup(name, GFP_KERNEL);
> > +	if (!context->xattr.name)
> > +		goto out;
> > +	context->xattr.value = kstrdup(value, GFP_KERNEL);
> > +	if (!context->xattr.value)
> > +		goto out;
> > +	return;
> > +out:
> > +	kfree(context->xattr.name);
> > +	audit_log_lost("out of memory in __audit_xattr");
> > +}
> > +
> >  void __audit_fanotify(unsigned int response)
> >  {
> >  	audit_log(audit_context(), GFP_KERNEL,
> 
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://listman.redhat.com/mailman/listinfo/linux-audit

- RGB

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

Re: [PATCH V1] audit: log xattr args not covered by syscall record

[Casey Schaufler]

On 5/7/2021 6:54 PM, Richard Guy Briggs wrote:
> On 2021-05-07 14:03, Casey Schaufler wrote:
>> On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
>>> The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
>>> four arguments and only lists pointers of string values.  The xattr name
>>> string, value string and flags (5th arg) are needed by audit given the
>>> syscall's main purpose.
>>>
>>> Add the auxiliary record AUDIT_XATTR (1336) to record the details not
>>> available in the SYSCALL record including the name string, value string
>>> and flags.
>>>
>>> Notes about field names:
>>> - name is too generic, use xattr precedent from ima
>>> - val is already generic value field name
>>> - flags used by mmap, xflags new name
>>>
>>> Sample event with new record:
>>> type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
>>> type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
>>> type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
>>> type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
>> Would it be sensible to break out the namespace from the attribute?
>>
>> 	attrspace="security" attrname="capability"
> Do xattrs always follow this nomenclature?  Or only the ones we care
> about?

Xattrs always have a namespace (man 7 xattr) of "user", "trusted",
"system" or "security". It's possible that additional namespaces will
be created in the future, although it seems unlikely given that only
"security" is widely used today.

>> Why isn't val= quoted?
> Good question.  I guessed it should have been since it used
> audit_log_untrustedstring(), but even the raw output is unquoted unless
> it was converted by auditd to unquoted before being stored to disk due
> to nothing offensive found in it since audit_log_n_string() does add
> quotes.  (hmmm, bit of a run-on sentence there...)
>
>> The attribute value can be a .jpg or worse. I could even see it being an eBPF
>> program (although That Would Be Wrong) so including it in an audit record could
>> be a bit of a problem.
> In these cases it would almost certainly get caught by the control
> character test audit_string_contains_control() in
> audit_log_n_untrustedstring() called from audit_log_untrustedstring()
> and deliver it as hex.

In that case I'm more concerned with the potential size than with
quoting. One of original use cases proposed for xattrs (back in the
SGI Irix days) was to attach a bitmap to be used as the icon in file
browsers as an xattr. Another was to attach the build instructions
and source used to create a binary. None of that is information you'd
want to see in a audit record. On the other hand, if the xattr was an
eBPF program used to make access control decisions, you would want at
least a reference to it in the audit record. 

>> It seems that you might want to leave it up to the LSMs to determine which xattr
>> values are audited. An SELinux system may want to see "security.selinux" values,
>> but it probably doesn't care about "security.SMACK64TRANSMUTE" values.
> Are you suggesting that any that don't follow this nomenclature are
> irrelevant or harmless at best and are not worth recording?

Nope. I'm saying that unless an xattr is going to be used for security
related purposes it isn't security relevant and hence shouldn't go in
the audit trail. The "security" namespace indication may not be sufficient
to make that determination, although it's a pretty good indicator today.
As I pointed out, setting an attribute used by Smack on an SELinux system
is not security relevant.

There is nothing preventing an LSM from using "user" namespace attributes.
I could imagine a security policy that looks a user supplied attributes
(user.runasroot=never) and restricts when a program can be used. Thus, it
needs to be up to the LSM to decide if an attribute should be audited. We
have to leave ACLs as an exception, of course, because they don't (and in
their current form can't) use the LSM infrastructure.

> This sounds like you are thinking about your LSM stacking patchset that
> issues a new record for each LSM attribute if there is more than one.
> And any system that has multiple LSMs active should be able to indicate
> all of interest.

There's some of that, but realize that Smack already uses multiple xattrs,
none of which are "security.selinux". Logging setting of "security.SMACK64EXEC"
makes sense. Logging "security.MyDogHasNoNose" does not. On the other hand,
if Yama decided to start using that attribute you'd have to look for it as well.

Re: [PATCH V1] audit: log xattr args not covered by syscall record

[Paul Moore]

On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/7/2021 6:54 PM, Richard Guy Briggs wrote:
> > On 2021-05-07 14:03, Casey Schaufler wrote:
> >> On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
> >>> The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
> >>> four arguments and only lists pointers of string values.  The xattr name
> >>> string, value string and flags (5th arg) are needed by audit given the
> >>> syscall's main purpose.
> >>>
> >>> Add the auxiliary record AUDIT_XATTR (1336) to record the details not
> >>> available in the SYSCALL record including the name string, value string
> >>> and flags.
> >>>
> >>> Notes about field names:
> >>> - name is too generic, use xattr precedent from ima
> >>> - val is already generic value field name
> >>> - flags used by mmap, xflags new name
> >>>
> >>> Sample event with new record:
> >>> type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
> >>> type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
> >>> type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
> >>> type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
> >> Would it be sensible to break out the namespace from the attribute?
> >>
> >>      attrspace="security" attrname="capability"
> > Do xattrs always follow this nomenclature?  Or only the ones we care
> > about?
>
> Xattrs always have a namespace (man 7 xattr) of "user", "trusted",
> "system" or "security". It's possible that additional namespaces will
> be created in the future, although it seems unlikely given that only
> "security" is widely used today.

Why should audit care about separating the name into two distinct
fields, e.g. "attrspace" and "attrname", instead of just a single
"xattr" field with a value that follows the "namespace.attribute"
format that is commonly seen by userspace?

> >> Why isn't val= quoted?
> > Good question.  I guessed it should have been since it used
> > audit_log_untrustedstring(), but even the raw output is unquoted unless
> > it was converted by auditd to unquoted before being stored to disk due
> > to nothing offensive found in it since audit_log_n_string() does add
> > quotes.  (hmmm, bit of a run-on sentence there...)
> >
> >> The attribute value can be a .jpg or worse. I could even see it being an eBPF
> >> program (although That Would Be Wrong) so including it in an audit record could
> >> be a bit of a problem.
> > In these cases it would almost certainly get caught by the control
> > character test audit_string_contains_control() in
> > audit_log_n_untrustedstring() called from audit_log_untrustedstring()
> > and deliver it as hex.
>
> In that case I'm more concerned with the potential size than with
> quoting. One of original use cases proposed for xattrs (back in the
> SGI Irix days) was to attach a bitmap to be used as the icon in file
> browsers as an xattr. Another was to attach the build instructions
> and source used to create a binary. None of that is information you'd
> want to see in a audit record. On the other hand, if the xattr was an
> eBPF program used to make access control decisions, you would want at
> least a reference to it in the audit record.

It would be interesting to see how this code would handle arbitrarily
large xattr values, or at the very least large enough values to blow
up the audit record size.

As pointed out elsewhere in this thread, and brought up again above
(albeit indirectly), I'm guessing we don't really care about *all*
xattrs, just the "special" xattrs that are security relevant, in which
case I think we need to reconsider how we collect this data.

I think it would also be very good to see what requirements may be
driving this work.  Is it purely a "gee, this seems important" or is
it a hard requirement in a security certification for example.

-- 
paul moore
www.paul-moore.com

Re: [PATCH V1] audit: log xattr args not covered by syscall record

[Casey Schaufler]

On 5/10/2021 4:52 PM, Paul Moore wrote:
> On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 5/7/2021 6:54 PM, Richard Guy Briggs wrote:
>>> On 2021-05-07 14:03, Casey Schaufler wrote:
>>>> On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
>>>>> The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
>>>>> four arguments and only lists pointers of string values.  The xattr name
>>>>> string, value string and flags (5th arg) are needed by audit given the
>>>>> syscall's main purpose.
>>>>>
>>>>> Add the auxiliary record AUDIT_XATTR (1336) to record the details not
>>>>> available in the SYSCALL record including the name string, value string
>>>>> and flags.
>>>>>
>>>>> Notes about field names:
>>>>> - name is too generic, use xattr precedent from ima
>>>>> - val is already generic value field name
>>>>> - flags used by mmap, xflags new name
>>>>>
>>>>> Sample event with new record:
>>>>> type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
>>>>> type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
>>>>> type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
>>>>> type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
>>>> Would it be sensible to break out the namespace from the attribute?
>>>>
>>>>      attrspace="security" attrname="capability"
>>> Do xattrs always follow this nomenclature?  Or only the ones we care
>>> about?
>> Xattrs always have a namespace (man 7 xattr) of "user", "trusted",
>> "system" or "security". It's possible that additional namespaces will
>> be created in the future, although it seems unlikely given that only
>> "security" is widely used today.
> Why should audit care about separating the name into two distinct
> fields, e.g. "attrspace" and "attrname", instead of just a single
> "xattr" field with a value that follows the "namespace.attribute"
> format that is commonly seen by userspace?

I asked if it would be sensible. I don't much care myself.

>>>> Why isn't val= quoted?
>>> Good question.  I guessed it should have been since it used
>>> audit_log_untrustedstring(), but even the raw output is unquoted unless
>>> it was converted by auditd to unquoted before being stored to disk due
>>> to nothing offensive found in it since audit_log_n_string() does add
>>> quotes.  (hmmm, bit of a run-on sentence there...)
>>>
>>>> The attribute value can be a .jpg or worse. I could even see it being an eBPF
>>>> program (although That Would Be Wrong) so including it in an audit record could
>>>> be a bit of a problem.
>>> In these cases it would almost certainly get caught by the control
>>> character test audit_string_contains_control() in
>>> audit_log_n_untrustedstring() called from audit_log_untrustedstring()
>>> and deliver it as hex.
>> In that case I'm more concerned with the potential size than with
>> quoting. One of original use cases proposed for xattrs (back in the
>> SGI Irix days) was to attach a bitmap to be used as the icon in file
>> browsers as an xattr. Another was to attach the build instructions
>> and source used to create a binary. None of that is information you'd
>> want to see in a audit record. On the other hand, if the xattr was an
>> eBPF program used to make access control decisions, you would want at
>> least a reference to it in the audit record.
> It would be interesting to see how this code would handle arbitrarily
> large xattr values, or at the very least large enough values to blow
> up the audit record size.
>
> As pointed out elsewhere in this thread, and brought up again above
> (albeit indirectly), I'm guessing we don't really care about *all*
> xattrs, just the "special" xattrs that are security relevant, in which
> case I think we need to reconsider how we collect this data.

Right. And you can't know in advance which xattrs are relevant in the
face of "security=". We might want something like

	bool security_auditable_attribute(struct xattr *xattr)

which returns true if the passed xattr is one that an LSM in the stack
considers relevant. Much like security_ismaclabel(). I don't think that
we can just reuse security_ismaclabel() because there are xattrs that
are security relevant but are not MAC labels. SMACK64TRANSMUTE is one.
File capability sets are another. I also suggest passing the struct xattr
rather than the name because it seems reasonable that an LSM might
consider "user.execbyroot=never" relevant while "user.execbyroot=possible"
isn't.

> I think it would also be very good to see what requirements may be
> driving this work.  Is it purely a "gee, this seems important" or is
> it a hard requirement in a security certification for example.
>

Re: [PATCH V1] audit: log xattr args not covered by syscall record

[Paul Moore]

On Mon, May 10, 2021 at 8:37 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/10/2021 4:52 PM, Paul Moore wrote:
> > On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 5/7/2021 6:54 PM, Richard Guy Briggs wrote:
> >>> On 2021-05-07 14:03, Casey Schaufler wrote:
> >>>> On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
> >>>>> The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
> >>>>> four arguments and only lists pointers of string values.  The xattr name
> >>>>> string, value string and flags (5th arg) are needed by audit given the
> >>>>> syscall's main purpose.
> >>>>>
> >>>>> Add the auxiliary record AUDIT_XATTR (1336) to record the details not
> >>>>> available in the SYSCALL record including the name string, value string
> >>>>> and flags.
> >>>>>
> >>>>> Notes about field names:
> >>>>> - name is too generic, use xattr precedent from ima
> >>>>> - val is already generic value field name
> >>>>> - flags used by mmap, xflags new name
> >>>>>
> >>>>> Sample event with new record:
> >>>>> type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
> >>>>> type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
> >>>>> type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
> >>>>> type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
> >>>> Would it be sensible to break out the namespace from the attribute?
> >>>>
> >>>>      attrspace="security" attrname="capability"
> >>> Do xattrs always follow this nomenclature?  Or only the ones we care
> >>> about?
> >> Xattrs always have a namespace (man 7 xattr) of "user", "trusted",
> >> "system" or "security". It's possible that additional namespaces will
> >> be created in the future, although it seems unlikely given that only
> >> "security" is widely used today.
> > Why should audit care about separating the name into two distinct
> > fields, e.g. "attrspace" and "attrname", instead of just a single
> > "xattr" field with a value that follows the "namespace.attribute"
> > format that is commonly seen by userspace?
>
> I asked if it would be sensible. I don't much care myself.

I was *asking* a question - why would we want separate fields?  I
guess I thought there might be some reason for asking if it was
sensible; if not, I think I'd rather see it as a single field.

> >>>> Why isn't val= quoted?
> >>> Good question.  I guessed it should have been since it used
> >>> audit_log_untrustedstring(), but even the raw output is unquoted unless
> >>> it was converted by auditd to unquoted before being stored to disk due
> >>> to nothing offensive found in it since audit_log_n_string() does add
> >>> quotes.  (hmmm, bit of a run-on sentence there...)
> >>>
> >>>> The attribute value can be a .jpg or worse. I could even see it being an eBPF
> >>>> program (although That Would Be Wrong) so including it in an audit record could
> >>>> be a bit of a problem.
> >>> In these cases it would almost certainly get caught by the control
> >>> character test audit_string_contains_control() in
> >>> audit_log_n_untrustedstring() called from audit_log_untrustedstring()
> >>> and deliver it as hex.
> >> In that case I'm more concerned with the potential size than with
> >> quoting. One of original use cases proposed for xattrs (back in the
> >> SGI Irix days) was to attach a bitmap to be used as the icon in file
> >> browsers as an xattr. Another was to attach the build instructions
> >> and source used to create a binary. None of that is information you'd
> >> want to see in a audit record. On the other hand, if the xattr was an
> >> eBPF program used to make access control decisions, you would want at
> >> least a reference to it in the audit record.
> > It would be interesting to see how this code would handle arbitrarily
> > large xattr values, or at the very least large enough values to blow
> > up the audit record size.
> >
> > As pointed out elsewhere in this thread, and brought up again above
> > (albeit indirectly), I'm guessing we don't really care about *all*
> > xattrs, just the "special" xattrs that are security relevant, in which
> > case I think we need to reconsider how we collect this data.
>
> Right. And you can't know in advance which xattrs are relevant in the
> face of "security=". We might want something like
>
>         bool security_auditable_attribute(struct xattr *xattr)
>
> which returns true if the passed xattr is one that an LSM in the stack
> considers relevant. Much like security_ismaclabel(). I don't think that
> we can just reuse security_ismaclabel() because there are xattrs that
> are security relevant but are not MAC labels. SMACK64TRANSMUTE is one.
> File capability sets are another. I also suggest passing the struct xattr
> rather than the name because it seems reasonable that an LSM might
> consider "user.execbyroot=never" relevant while "user.execbyroot=possible"
> isn't.

Perhaps instead of having the audit code call into the LSM to
determine if an xattr is worth recording in the audit event, we leave
it to the LSMs themselves to add a record to the event?

-- 
paul moore
www.paul-moore.com

Re: [PATCH V1] audit: log xattr args not covered by syscall record

[Casey Schaufler]

On 5/10/2021 6:28 PM, Paul Moore wrote:
> On Mon, May 10, 2021 at 8:37 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 5/10/2021 4:52 PM, Paul Moore wrote:
>>> On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 5/7/2021 6:54 PM, Richard Guy Briggs wrote:
>>>>> On 2021-05-07 14:03, Casey Schaufler wrote:
>>>>>> On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
>>>>>>> The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
>>>>>>> four arguments and only lists pointers of string values.  The xattr name
>>>>>>> string, value string and flags (5th arg) are needed by audit given the
>>>>>>> syscall's main purpose.
>>>>>>>
>>>>>>> Add the auxiliary record AUDIT_XATTR (1336) to record the details not
>>>>>>> available in the SYSCALL record including the name string, value string
>>>>>>> and flags.
>>>>>>>
>>>>>>> Notes about field names:
>>>>>>> - name is too generic, use xattr precedent from ima
>>>>>>> - val is already generic value field name
>>>>>>> - flags used by mmap, xflags new name
>>>>>>>
>>>>>>> Sample event with new record:
>>>>>>> type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
>>>>>>> type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
>>>>>>> type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
>>>>>>> type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
>>>>>> Would it be sensible to break out the namespace from the attribute?
>>>>>>
>>>>>>      attrspace="security" attrname="capability"
>>>>> Do xattrs always follow this nomenclature?  Or only the ones we care
>>>>> about?
>>>> Xattrs always have a namespace (man 7 xattr) of "user", "trusted",
>>>> "system" or "security". It's possible that additional namespaces will
>>>> be created in the future, although it seems unlikely given that only
>>>> "security" is widely used today.
>>> Why should audit care about separating the name into two distinct
>>> fields, e.g. "attrspace" and "attrname", instead of just a single
>>> "xattr" field with a value that follows the "namespace.attribute"
>>> format that is commonly seen by userspace?
>> I asked if it would be sensible. I don't much care myself.
> I was *asking* a question - why would we want separate fields?  I
> guess I thought there might be some reason for asking if it was
> sensible; if not, I think I'd rather see it as a single field.

I thought that it might make searching records easier, but I'm
not the expert on that. One might filter on attrspace=security then
look at the attrname values. But that bikeshed can be either color.

>>>>>> Why isn't val= quoted?
>>>>> Good question.  I guessed it should have been since it used
>>>>> audit_log_untrustedstring(), but even the raw output is unquoted unless
>>>>> it was converted by auditd to unquoted before being stored to disk due
>>>>> to nothing offensive found in it since audit_log_n_string() does add
>>>>> quotes.  (hmmm, bit of a run-on sentence there...)
>>>>>
>>>>>> The attribute value can be a .jpg or worse. I could even see it being an eBPF
>>>>>> program (although That Would Be Wrong) so including it in an audit record could
>>>>>> be a bit of a problem.
>>>>> In these cases it would almost certainly get caught by the control
>>>>> character test audit_string_contains_control() in
>>>>> audit_log_n_untrustedstring() called from audit_log_untrustedstring()
>>>>> and deliver it as hex.
>>>> In that case I'm more concerned with the potential size than with
>>>> quoting. One of original use cases proposed for xattrs (back in the
>>>> SGI Irix days) was to attach a bitmap to be used as the icon in file
>>>> browsers as an xattr. Another was to attach the build instructions
>>>> and source used to create a binary. None of that is information you'd
>>>> want to see in a audit record. On the other hand, if the xattr was an
>>>> eBPF program used to make access control decisions, you would want at
>>>> least a reference to it in the audit record.
>>> It would be interesting to see how this code would handle arbitrarily
>>> large xattr values, or at the very least large enough values to blow
>>> up the audit record size.
>>>
>>> As pointed out elsewhere in this thread, and brought up again above
>>> (albeit indirectly), I'm guessing we don't really care about *all*
>>> xattrs, just the "special" xattrs that are security relevant, in which
>>> case I think we need to reconsider how we collect this data.
>> Right. And you can't know in advance which xattrs are relevant in the
>> face of "security=". We might want something like
>>
>>         bool security_auditable_attribute(struct xattr *xattr)
>>
>> which returns true if the passed xattr is one that an LSM in the stack
>> considers relevant. Much like security_ismaclabel(). I don't think that
>> we can just reuse security_ismaclabel() because there are xattrs that
>> are security relevant but are not MAC labels. SMACK64TRANSMUTE is one.
>> File capability sets are another. I also suggest passing the struct xattr
>> rather than the name because it seems reasonable that an LSM might
>> consider "user.execbyroot=never" relevant while "user.execbyroot=possible"
>> isn't.
> Perhaps instead of having the audit code call into the LSM to
> determine if an xattr is worth recording in the audit event, we leave
> it to the LSMs themselves to add a record to the event?

That would be even better. The LSM has all the information it needs.
No reason to add infrastructure.
 

Re: [PATCH V1] audit: log xattr args not covered by syscall record

[Paul Moore]

On Tue, May 11, 2021 at 10:00 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 5/10/2021 6:28 PM, Paul Moore wrote:
> > On Mon, May 10, 2021 at 8:37 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 5/10/2021 4:52 PM, Paul Moore wrote:
> >>> On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> On 5/7/2021 6:54 PM, Richard Guy Briggs wrote:
> >>>>> On 2021-05-07 14:03, Casey Schaufler wrote:
> >>>>>> On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
> >>>>>>> The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
> >>>>>>> four arguments and only lists pointers of string values.  The xattr name
> >>>>>>> string, value string and flags (5th arg) are needed by audit given the
> >>>>>>> syscall's main purpose.
> >>>>>>>
> >>>>>>> Add the auxiliary record AUDIT_XATTR (1336) to record the details not
> >>>>>>> available in the SYSCALL record including the name string, value string
> >>>>>>> and flags.
> >>>>>>>
> >>>>>>> Notes about field names:
> >>>>>>> - name is too generic, use xattr precedent from ima
> >>>>>>> - val is already generic value field name
> >>>>>>> - flags used by mmap, xflags new name
> >>>>>>>
> >>>>>>> Sample event with new record:
> >>>>>>> type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
> >>>>>>> type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
> >>>>>>> type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
> >>>>>>> type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
> >>>>>> Would it be sensible to break out the namespace from the attribute?
> >>>>>>
> >>>>>>      attrspace="security" attrname="capability"
> >>>>> Do xattrs always follow this nomenclature?  Or only the ones we care
> >>>>> about?
> >>>> Xattrs always have a namespace (man 7 xattr) of "user", "trusted",
> >>>> "system" or "security". It's possible that additional namespaces will
> >>>> be created in the future, although it seems unlikely given that only
> >>>> "security" is widely used today.
> >>> Why should audit care about separating the name into two distinct
> >>> fields, e.g. "attrspace" and "attrname", instead of just a single
> >>> "xattr" field with a value that follows the "namespace.attribute"
> >>> format that is commonly seen by userspace?
> >> I asked if it would be sensible. I don't much care myself.
> > I was *asking* a question - why would we want separate fields?  I
> > guess I thought there might be some reason for asking if it was
> > sensible; if not, I think I'd rather see it as a single field.
>
> I thought that it might make searching records easier, but I'm
> not the expert on that. One might filter on attrspace=security then
> look at the attrname values. But that bikeshed can be either color.

Yeah, understood.  My concern was that the xattr name (minus the
namespace) by itself isn't really useful; similar argument with just
the namespace.  If you are going to do a string match filter it really
shouldn't matter too much either way.

-- 
paul moore
www.paul-moore.com