Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1549294 > unrolled thread

[PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions Tyler Hicks <tyhicks@canonical.com> - 2017-01-02 18:00 +0100
    [PATCH 1/2] seccomp: Allow for auditing functionality specific to return actions Tyler Hicks <tyhicks@canonical.com> - 2017-01-02 18:00 +0100
    [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values Tyler Hicks <tyhicks@canonical.com> - 2017-01-02 18:00 +0100
      Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values Steve Grubb <sgrubb@redhat.com> - 2017-01-02 18:30 +0100
        Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno  values Tyler Hicks <tyhicks@canonical.com> - 2017-01-02 18:50 +0100
          Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values Steve Grubb <sgrubb@redhat.com> - 2017-01-02 20:00 +0100
            Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values Paul Moore <paul@paul-moore.com> - 2017-01-03 00:00 +0100
    Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions Paul Moore <paul@paul-moore.com> - 2017-01-02 23:50 +0100
      Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions Andy Lutomirski <luto@amacapital.net> - 2017-01-03 07:00 +0100
      Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions Tyler Hicks <tyhicks@canonical.com> - 2017-01-03 14:40 +0100
    Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions Andy Lutomirski <luto@kernel.org> - 2017-01-03 07:00 +0100
      Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions Tyler Hicks <tyhicks@canonical.com> - 2017-01-03 15:00 +0100

#1549294 — [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

This patch set creates the basis for auditing information specific to a given
seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
errno value that will be returned to userspace.

Tyler

[toc] | [next] | [standalone]

#1549298 — [PATCH 1/2] seccomp: Allow for auditing functionality specific to return actions

This patch introduces the concept of auditing formats that are specific
to the action specified in a filter's return value. Initially, only
SECCOMP_RET_KILL has an auditing message that differs from other return
actions because it specifies the signal that is to be sent.

This patch causes a small functional change in that "sig=0" is not
printed when auditing seccomp actions other than SECCOMP_RET_KILL.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 include/linux/audit.h | 39 +++++++++++++++++++++++++++++++++------
 kernel/auditsc.c      | 19 +++++++++++++++----
 kernel/seccomp.c      |  6 +++---
 3 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index f51fca8d..8c588c3 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -85,6 +85,11 @@ struct audit_field {
 	u32				op;
 };
 
+struct audit_seccomp_info {
+	int		code;
+	long		signr;
+};
+
 extern int is_audit_feature_set(int which);
 
 extern int __init audit_register_class(int class, unsigned *list);
@@ -243,7 +248,8 @@ extern void __audit_file(const struct file *);
 extern void __audit_inode_child(struct inode *parent,
 				const struct dentry *dentry,
 				const unsigned char type);
-extern void __audit_seccomp(unsigned long syscall, long signr, int code);
+extern void __audit_seccomp(unsigned long syscall,
+			    struct audit_seccomp_info *info);
 extern void __audit_ptrace(struct task_struct *t);
 
 static inline bool audit_dummy_context(void)
@@ -313,14 +319,31 @@ static inline void audit_inode_child(struct inode *parent,
 }
 void audit_core_dumps(long signr);
 
-static inline void audit_seccomp(unsigned long syscall, long signr, int code)
+static inline void audit_seccomp_signal(unsigned long syscall, long signr,
+					int code)
 {
 	if (!audit_enabled)
 		return;
 
 	/* Force a record to be reported if a signal was delivered. */
-	if (signr || unlikely(!audit_dummy_context()))
-		__audit_seccomp(syscall, signr, code);
+	if (signr || unlikely(!audit_dummy_context())) {
+		struct audit_seccomp_info info = { .code = code,
+						   .signr = signr };
+
+		__audit_seccomp(syscall, &info);
+	}
+}
+
+static inline void audit_seccomp_common(unsigned long syscall, int code)
+{
+	if (!audit_enabled)
+		return;
+
+	if (code || unlikely(!audit_dummy_context())) {
+		struct audit_seccomp_info info = { .code = code };
+
+		__audit_seccomp(syscall, &info);
+	}
 }
 
 static inline void audit_ptrace(struct task_struct *t)
@@ -485,9 +508,13 @@ static inline void audit_inode_child(struct inode *parent,
 { }
 static inline void audit_core_dumps(long signr)
 { }
-static inline void __audit_seccomp(unsigned long syscall, long signr, int code)
+static inline void __audit_seccomp(unsigned long syscall,
+				   struct audit_seccomp_info *info);
+{ }
+static inline void audit_seccomp_signal(unsigned long syscall, long signr,
+					int code)
 { }
-static inline void audit_seccomp(unsigned long syscall, long signr, int code)
+static inline void audit_seccomp_common(unsigned long syscall, int code)
 { }
 static inline int auditsc_get_stamp(struct audit_context *ctx,
 			      struct timespec *t, unsigned int *serial)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index cf1fa43..b3472f2 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -74,6 +74,7 @@
 #include <linux/string.h>
 #include <linux/uaccess.h>
 #include <uapi/linux/limits.h>
+#include <uapi/linux/seccomp.h>
 
 #include "audit.h"
 
@@ -2415,7 +2416,7 @@ void audit_core_dumps(long signr)
 	audit_log_end(ab);
 }
 
-void __audit_seccomp(unsigned long syscall, long signr, int code)
+void __audit_seccomp(unsigned long syscall, struct audit_seccomp_info *info)
 {
 	struct audit_buffer *ab;
 
@@ -2423,9 +2424,19 @@ void __audit_seccomp(unsigned long syscall, long signr, int code)
 	if (unlikely(!ab))
 		return;
 	audit_log_task(ab);
-	audit_log_format(ab, " sig=%ld arch=%x syscall=%ld compat=%d ip=0x%lx code=0x%x",
-			 signr, syscall_get_arch(), syscall,
-			 in_compat_syscall(), KSTK_EIP(current), code);
+
+	switch (info->code) {
+	case SECCOMP_RET_KILL:
+		audit_log_format(ab, " sig=%ld", info->signr);
+		break;
+	default:
+		break;
+	}
+
+	audit_log_format(ab,
+			 " arch=%x syscall=%ld compat=%d ip=0x%lx code=0x%x",
+			 syscall_get_arch(), syscall, in_compat_syscall(),
+			 KSTK_EIP(current), info->code);
 	audit_log_end(ab);
 }
 
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f7ce79a..54c01b6 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -532,7 +532,7 @@ static void __secure_computing_strict(int this_syscall)
 #ifdef SECCOMP_DEBUG
 	dump_stack();
 #endif
-	audit_seccomp(this_syscall, SIGKILL, SECCOMP_RET_KILL);
+	audit_seccomp_signal(this_syscall, SIGKILL, SECCOMP_RET_KILL);
 	do_exit(SIGKILL);
 }
 
@@ -635,14 +635,14 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 
 	case SECCOMP_RET_KILL:
 	default:
-		audit_seccomp(this_syscall, SIGSYS, action);
+		audit_seccomp_signal(this_syscall, SIGSYS, action);
 		do_exit(SIGSYS);
 	}
 
 	unreachable();
 
 skip:
-	audit_seccomp(this_syscall, 0, action);
+	audit_seccomp_common(this_syscall, action);
 	return -1;
 }
 #else
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1549299 — [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

Generate audit records for SECCOMP_RET_ERRNO actions, which were
previously not audited.

Additionally, include the errno value that will be set in the audit
message.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 include/linux/audit.h | 19 ++++++++++++++++++-
 kernel/auditsc.c      |  3 +++
 kernel/seccomp.c      |  4 +++-
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 8c588c3..6815812 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -87,7 +87,10 @@ struct audit_field {
 
 struct audit_seccomp_info {
 	int		code;
-	long		signr;
+	union {
+		int	errno;
+		long	signr;
+	};
 };
 
 extern int is_audit_feature_set(int which);
@@ -319,6 +322,20 @@ static inline void audit_inode_child(struct inode *parent,
 }
 void audit_core_dumps(long signr);
 
+static inline void audit_seccomp_errno(unsigned long syscall, int errno,
+				       int code)
+{
+	if (!audit_enabled)
+		return;
+
+	if (errno || unlikely(!audit_dummy_context())) {
+		struct audit_seccomp_info info = { .code = code,
+						   .errno = errno };
+
+		__audit_seccomp(syscall, &info);
+	}
+}
+
 static inline void audit_seccomp_signal(unsigned long syscall, long signr,
 					int code)
 {
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index b3472f2..db5fc9d 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2426,6 +2426,9 @@ void __audit_seccomp(unsigned long syscall, struct audit_seccomp_info *info)
 	audit_log_task(ab);
 
 	switch (info->code) {
+	case SECCOMP_RET_ERRNO:
+		audit_log_format(ab, " errno=%d", info->errno);
+		break;
 	case SECCOMP_RET_KILL:
 		audit_log_format(ab, " sig=%ld", info->signr);
 		break;
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 54c01b6..e99c566 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -576,9 +576,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 		/* Set low-order bits as an errno, capped at MAX_ERRNO. */
 		if (data > MAX_ERRNO)
 			data = MAX_ERRNO;
+
+		audit_seccomp_errno(this_syscall, data, action);
 		syscall_set_return_value(current, task_pt_regs(current),
 					 -data, 0);
-		goto skip;
+		return -1;
 
 	case SECCOMP_RET_TRAP:
 		/* Show the handler the original registers. */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1549319 — Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote:
> Generate audit records for SECCOMP_RET_ERRNO actions, which were
> previously not audited.
> 
> Additionally, include the errno value that will be set in the audit
> message.
> 
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  include/linux/audit.h | 19 ++++++++++++++++++-
>  kernel/auditsc.c      |  3 +++
>  kernel/seccomp.c      |  4 +++-
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 8c588c3..6815812 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -87,7 +87,10 @@ struct audit_field {
> 
>  struct audit_seccomp_info {
>  	int		code;
> -	long		signr;
> +	union {
> +		int	errno;
> +		long	signr;
> +	};
>  };
> 
>  extern int is_audit_feature_set(int which);
> @@ -319,6 +322,20 @@ static inline void audit_inode_child(struct inode
> *parent, }
>  void audit_core_dumps(long signr);
> 
> +static inline void audit_seccomp_errno(unsigned long syscall, int errno,
> +				       int code)
> +{
> +	if (!audit_enabled)
> +		return;
> +
> +	if (errno || unlikely(!audit_dummy_context())) {
> +		struct audit_seccomp_info info = { .code = code,
> +						   .errno = errno };
> +
> +		__audit_seccomp(syscall, &info);
> +	}
> +}
> +
>  static inline void audit_seccomp_signal(unsigned long syscall, long signr,
>  					int code)
>  {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b3472f2..db5fc9d 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2426,6 +2426,9 @@ void __audit_seccomp(unsigned long syscall, struct
> audit_seccomp_info *info) audit_log_task(ab);
> 
>  	switch (info->code) {
> +	case SECCOMP_RET_ERRNO:
> +		audit_log_format(ab, " errno=%d", info->errno);
> +		break;

"exit" is the field name that syscalls use to return errno to user space. I'd
rather not see another field created that maps to the same thing. You can check
the translation with the auformat utility:

http://people.redhat.com/sgrubb/files/auformat.tar.gz

$ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat "%EXIT\n"

Also, I am working to normalize all the records. That mean every event record
of the same type has the same fields, in the same order, with the same
representation. I would think "exit" could be added to the current record after
syscall so that its ordered similarly to a syscall record.

-Steve

>  	case SECCOMP_RET_KILL:
>  		audit_log_format(ab, " sig=%ld", info->signr);
>  		break;
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 54c01b6..e99c566 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -576,9 +576,11 @@ static int __seccomp_filter(int this_syscall, const
> struct seccomp_data *sd, /* Set low-order bits as an errno, capped at
> MAX_ERRNO. */
>  		if (data > MAX_ERRNO)
>  			data = MAX_ERRNO;
> +
> +		audit_seccomp_errno(this_syscall, data, action);
>  		syscall_set_return_value(current, task_pt_regs(current),
>  					 -data, 0);
> -		goto skip;
> +		return -1;
> 
>  	case SECCOMP_RET_TRAP:
>  		/* Show the handler the original registers. */

[toc] | [prev] | [next] | [standalone]

#1549326 — Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

[Multipart message — attachments visible in raw view] — view raw

On 2017-01-02 12:20:53, Steve Grubb wrote:
> On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote:
> > Generate audit records for SECCOMP_RET_ERRNO actions, which were
> > previously not audited.
> > 
> > Additionally, include the errno value that will be set in the audit
> > message.
> > 
> > Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> > ---
> >  include/linux/audit.h | 19 ++++++++++++++++++-
> >  kernel/auditsc.c      |  3 +++
> >  kernel/seccomp.c      |  4 +++-
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 8c588c3..6815812 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -87,7 +87,10 @@ struct audit_field {
> > 
> >  struct audit_seccomp_info {
> >  	int		code;
> > -	long		signr;
> > +	union {
> > +		int	errno;
> > +		long	signr;
> > +	};
> >  };
> > 
> >  extern int is_audit_feature_set(int which);
> > @@ -319,6 +322,20 @@ static inline void audit_inode_child(struct inode
> > *parent, }
> >  void audit_core_dumps(long signr);
> > 
> > +static inline void audit_seccomp_errno(unsigned long syscall, int errno,
> > +				       int code)
> > +{
> > +	if (!audit_enabled)
> > +		return;
> > +
> > +	if (errno || unlikely(!audit_dummy_context())) {
> > +		struct audit_seccomp_info info = { .code = code,
> > +						   .errno = errno };
> > +
> > +		__audit_seccomp(syscall, &info);
> > +	}
> > +}
> > +
> >  static inline void audit_seccomp_signal(unsigned long syscall, long signr,
> >  					int code)
> >  {
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index b3472f2..db5fc9d 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2426,6 +2426,9 @@ void __audit_seccomp(unsigned long syscall, struct
> > audit_seccomp_info *info) audit_log_task(ab);
> > 
> >  	switch (info->code) {
> > +	case SECCOMP_RET_ERRNO:
> > +		audit_log_format(ab, " errno=%d", info->errno);
> > +		break;
> 
> "exit" is the field name that syscalls use to return errno to user space. I'd
> rather not see another field created that maps to the same thing. You can check
> the translation with the auformat utility:

Thanks for having a look at the field name I was using. Although I
prefer "errno" over "exit" in terms of clarity, I agree that it makes
sense to be consistent with the field names across record types. "exit"
works for me.

> 
> http://people.redhat.com/sgrubb/files/auformat.tar.gz
> 
> $ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat "%EXIT\n"
> 
> Also, I am working to normalize all the records. That mean every event record
> of the same type has the same fields, in the same order, with the same
> representation. I would think "exit" could be added to the current record after
> syscall so that its ordered similarly to a syscall record.

This patch goes against your normalization efforts in more ways than
just the placement of the "exit" field. If the action is
SECCOMP_RET_KILL, a "sig" field is present but if the action is
SECCOMP_RET_ERRNO, the "sig" field will not be present but the "errno"
field will be present. This happens all within the AUDIT_SECCOMP record
type. How would you suggest normalizing AUDIT_SECCOMP records for
different seccomp return actions?

Tyler

> 
> -Steve
> 
> >  	case SECCOMP_RET_KILL:
> >  		audit_log_format(ab, " sig=%ld", info->signr);
> >  		break;
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 54c01b6..e99c566 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -576,9 +576,11 @@ static int __seccomp_filter(int this_syscall, const
> > struct seccomp_data *sd, /* Set low-order bits as an errno, capped at
> > MAX_ERRNO. */
> >  		if (data > MAX_ERRNO)
> >  			data = MAX_ERRNO;
> > +
> > +		audit_seccomp_errno(this_syscall, data, action);
> >  		syscall_set_return_value(current, task_pt_regs(current),
> >  					 -data, 0);
> > -		goto skip;
> > +		return -1;
> > 
> >  	case SECCOMP_RET_TRAP:
> >  		/* Show the handler the original registers. */
> 
> 

[toc] | [prev] | [next] | [standalone]

#1549360 — Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

On Monday, January 2, 2017 5:42:47 PM EST Tyler Hicks wrote:
> On 2017-01-02 12:20:53, Steve Grubb wrote:
> > On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote:
> > > Generate audit records for SECCOMP_RET_ERRNO actions, which were
> > > previously not audited.
> > > 
> > > Additionally, include the errno value that will be set in the audit
> > > message.
> > > 
> > > Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> > > ---
> > > 
> > >  include/linux/audit.h | 19 ++++++++++++++++++-
> > >  kernel/auditsc.c      |  3 +++
> > >  kernel/seccomp.c      |  4 +++-
> > >  3 files changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 8c588c3..6815812 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -87,7 +87,10 @@ struct audit_field {
> > > 
> > >  struct audit_seccomp_info {
> > >  
> > >  	int		code;
> > > 
> > > -	long		signr;
> > > +	union {
> > > +		int	errno;
> > > +		long	signr;
> > > +	};
> > > 
> > >  };
> > >  
> > >  extern int is_audit_feature_set(int which);
> > > 
> > > @@ -319,6 +322,20 @@ static inline void audit_inode_child(struct inode
> > > *parent, }
> > > 
> > >  void audit_core_dumps(long signr);
> > > 
> > > +static inline void audit_seccomp_errno(unsigned long syscall, int
> > > errno,
> > > +				       int code)
> > > +{
> > > +	if (!audit_enabled)
> > > +		return;
> > > +
> > > +	if (errno || unlikely(!audit_dummy_context())) {
> > > +		struct audit_seccomp_info info = { .code = code,
> > > +						   .errno = errno };
> > > +
> > > +		__audit_seccomp(syscall, &info);
> > > +	}
> > > +}
> > > +
> > > 
> > >  static inline void audit_seccomp_signal(unsigned long syscall, long
> > >  signr,
> > >  
> > >  					int code)
> > >  
> > >  {
> > > 
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index b3472f2..db5fc9d 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -2426,6 +2426,9 @@ void __audit_seccomp(unsigned long syscall, struct
> > > audit_seccomp_info *info) audit_log_task(ab);
> > > 
> > >  	switch (info->code) {
> > > 
> > > +	case SECCOMP_RET_ERRNO:
> > > +		audit_log_format(ab, " errno=%d", info->errno);
> > > +		break;
> > 
> > "exit" is the field name that syscalls use to return errno to user space.
> > I'd rather not see another field created that maps to the same thing. You
> > can check
> > the translation with the auformat utility:
> Thanks for having a look at the field name I was using. Although I
> prefer "errno" over "exit" in terms of clarity, I agree that it makes
> sense to be consistent with the field names across record types. "exit"
> works for me.
> 
> > http://people.redhat.com/sgrubb/files/auformat.tar.gz
> > 
> > $ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat
> > "%EXIT\n"
> > 
> > Also, I am working to normalize all the records. That mean every event
> > record of the same type has the same fields, in the same order, with the
> > same representation. I would think "exit" could be added to the current
> > record after syscall so that its ordered similarly to a syscall record.
> 
> This patch goes against your normalization efforts in more ways than
> just the placement of the "exit" field. If the action is
> SECCOMP_RET_KILL, a "sig" field is present but if the action is
> SECCOMP_RET_ERRNO, the "sig" field will not be present but the "errno"
> field will be present. This happens all within the AUDIT_SECCOMP record
> type. How would you suggest normalizing AUDIT_SECCOMP records for
> different seccomp return actions?

Typically when the layout has to change, we just give it a new record type.

 -Steve

 
> > >  	case SECCOMP_RET_KILL:
> > >  		audit_log_format(ab, " sig=%ld", info->signr);
> > >  		break;
> > > 
> > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > index 54c01b6..e99c566 100644
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -576,9 +576,11 @@ static int __seccomp_filter(int this_syscall, const
> > > struct seccomp_data *sd, /* Set low-order bits as an errno, capped at
> > > MAX_ERRNO. */
> > > 
> > >  		if (data > MAX_ERRNO)
> > >  		
> > >  			data = MAX_ERRNO;
> > > 
> > > +
> > > +		audit_seccomp_errno(this_syscall, data, action);
> > > 
> > >  		syscall_set_return_value(current, task_pt_regs(current),
> > >  		
> > >  					 -data, 0);
> > > 
> > > -		goto skip;
> > > +		return -1;
> > > 
> > >  	case SECCOMP_RET_TRAP:
> > >  		/* Show the handler the original registers. */

[toc] | [prev] | [next] | [standalone]

#1549442 — Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

On Mon, Jan 2, 2017 at 1:49 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Monday, January 2, 2017 5:42:47 PM EST Tyler Hicks wrote:
>> On 2017-01-02 12:20:53, Steve Grubb wrote:
>> > On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote:

...

>> Thanks for having a look at the field name I was using. Although I
>> prefer "errno" over "exit" in terms of clarity, I agree that it makes
>> sense to be consistent with the field names across record types. "exit"
>> works for me.

FWIW, we have a nice (searchable due to GitHub CSV magic) audit field
database at the link below.  I will admit that it may be a bit crusty
in places, but we are making a new effort to keep it updated, if you
notice anything wrong, send email and/or a PR.

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

>> > http://people.redhat.com/sgrubb/files/auformat.tar.gz
>> >
>> > $ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat
>> > "%EXIT\n"
>> >
>> > Also, I am working to normalize all the records. That mean every event
>> > record of the same type has the same fields, in the same order, with the
>> > same representation. I would think "exit" could be added to the current
>> > record after syscall so that its ordered similarly to a syscall record.
>>
>> This patch goes against your normalization efforts in more ways than
>> just the placement of the "exit" field. If the action is
>> SECCOMP_RET_KILL, a "sig" field is present but if the action is
>> SECCOMP_RET_ERRNO, the "sig" field will not be present but the "errno"
>> field will be present. This happens all within the AUDIT_SECCOMP record
>> type. How would you suggest normalizing AUDIT_SECCOMP records for
>> different seccomp return actions?
>
> Typically when the layout has to change, we just give it a new record type.

I'm going to be very loathe to accept any new record types that *only*
reorder fields; if you need to add a new field, simply add it to the
end of the record.  From my perspective new record types are really
only an option if we need to remove a field that is bogus/confusing or
some other similar case that is not easily solved.  New record types
are a last resort.

-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [next] | [standalone]

#1549437

On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
> This patch set creates the basis for auditing information specific to a given
> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
> errno value that will be returned to userspace.

I'm replying to this patchset posting because it his my inbox first,
but my comments here apply to both this patchset and the other
seccomp/audit patchset you posted.

In my experience, we have two or three problems (the count varies
depending on perspective) when it comes to seccomp filter reporting:

1. Inability to log all filter actions.
2. Inability to selectively enable filtering; e.g. devs want noisy
logging, users want relative quiet.
3. Consistent behavior with audit enabled and disabled.

My current thinking - forgive me, this has been kicking around in my
head for the better part of six months (longer?) and I haven't
attempted to code it up - is to create a sysctl knob for a system wide
seccomp logging threshold that would be applied to the high 16-bits of
*every* triggered action: if the action was at/below the threshold a
record would be emitted, otherwise silence.  This should resolve
problems #1 and #2, and the code should be relatively straightforward
and small.

As part of the code above, I expect that all seccomp logging would get
routed through a single logging function (sort of like a better
implementation of the existing audit_seccomp()) that would check the
threshold and trigger the logging if needed.  This function could be
augmented to check for CONFIG_AUDIT and in the case where audit was
not built into the kernel, a simple printk could be used to log the
seccomp event; solving problem #3.

We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel
that is important (I personally waffle on this), but I think that is
independent of the ideas above.

Thoughts?

-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [next] | [standalone]

#1549546

On Mon, Jan 2, 2017 at 2:47 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch set creates the basis for auditing information specific to a given
>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>> errno value that will be returned to userspace.
>
> I'm replying to this patchset posting because it his my inbox first,
> but my comments here apply to both this patchset and the other
> seccomp/audit patchset you posted.
>
> In my experience, we have two or three problems (the count varies
> depending on perspective) when it comes to seccomp filter reporting:
>
> 1. Inability to log all filter actions.
> 2. Inability to selectively enable filtering; e.g. devs want noisy
> logging, users want relative quiet.
> 3. Consistent behavior with audit enabled and disabled.
>
> My current thinking - forgive me, this has been kicking around in my
> head for the better part of six months (longer?) and I haven't
> attempted to code it up - is to create a sysctl knob for a system wide
> seccomp logging threshold that would be applied to the high 16-bits of
> *every* triggered action: if the action was at/below the threshold a
> record would be emitted, otherwise silence.  This should resolve
> problems #1 and #2, and the code should be relatively straightforward
> and small.
>
> As part of the code above, I expect that all seccomp logging would get
> routed through a single logging function (sort of like a better
> implementation of the existing audit_seccomp()) that would check the
> threshold and trigger the logging if needed.  This function could be
> augmented to check for CONFIG_AUDIT and in the case where audit was
> not built into the kernel, a simple printk could be used to log the
> seccomp event; solving problem #3.

Would this not be doable with a seccomp tracepoint and a BPF filter?

--Andy

[toc] | [prev] | [next] | [standalone]

#1549777

[Multipart message — attachments visible in raw view] — view raw

On 01/02/2017 04:47 PM, Paul Moore wrote:
> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch set creates the basis for auditing information specific to a given
>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>> errno value that will be returned to userspace.
> 
> I'm replying to this patchset posting because it his my inbox first,
> but my comments here apply to both this patchset and the other
> seccomp/audit patchset you posted.
> 
> In my experience, we have two or three problems (the count varies
> depending on perspective) when it comes to seccomp filter reporting:
> 
> 1. Inability to log all filter actions.
> 2. Inability to selectively enable filtering; e.g. devs want noisy
> logging, users want relative quiet.
> 3. Consistent behavior with audit enabled and disabled.

Agreed. Those three logging issues are what have been nagging me the most.

> My current thinking - forgive me, this has been kicking around in my
> head for the better part of six months (longer?) and I haven't
> attempted to code it up - is to create a sysctl knob for a system wide
> seccomp logging threshold that would be applied to the high 16-bits of
> *every* triggered action: if the action was at/below the threshold a
> record would be emitted, otherwise silence.  This should resolve
> problems #1 and #2, and the code should be relatively straightforward
> and small.

I like that idea quite a bit. To be completely honest, for #1, I
personally only care about logging SECCOMP_RET_ERRNO actions but this
idea solves it in a nice and general way.

> As part of the code above, I expect that all seccomp logging would get
> routed through a single logging function (sort of like a better
> implementation of the existing audit_seccomp()) that would check the
> threshold and trigger the logging if needed.  This function could be
> augmented to check for CONFIG_AUDIT and in the case where audit was
> not built into the kernel, a simple printk could be used to log the
> seccomp event; solving problem #3.

That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we
build with CONFIG_AUDIT enabled but don't ship auditd by default so
audit_enabled is false. In that default configuration, we still want
seccomp audit messages to be printk'ed. I'll need to figure out how to
cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is
enabled and audit_enabled is false.

> We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel
> that is important (I personally waffle on this), but I think that is
> independent of the ideas above.

I agree that it is independent but SECCOMP_RET_AUDIT would still be
important to Ubuntu.

Tyler

[toc] | [prev] | [next] | [standalone]

#1549547

On Mon, Jan 2, 2017 at 8:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
> This patch set creates the basis for auditing information specific to a given
> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
> errno value that will be returned to userspace.
>

Not that I'm opposed to the idea, but what's the intended purpose?

[toc] | [prev] | [next] | [standalone]

#1549786

[Multipart message — attachments visible in raw view] — view raw

On 01/02/2017 11:57 PM, Andy Lutomirski wrote:
> On Mon, Jan 2, 2017 at 8:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch set creates the basis for auditing information specific to a given
>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>> errno value that will be returned to userspace.
>>
> 
> Not that I'm opposed to the idea, but what's the intended purpose?

Ubuntu has a security sandbox, which includes seccomp as a part of the
confinement strategy, that we're using to confine untrusted third-party
applications. Today, we're using SECCOMP_RET_KILL as the default action
when the applications make a call to a syscall that is not allowed by
the sandbox. It is great from a security perspective but not so great
from the perspective of the application developer as their application
(or in some cases, an interpretor) may work fine without the illegal
syscall but it doesn't get the chance to because it is killed.

In the near future, we want to switch over to using SECCOMP_RET_ERRNO
(the errno is still TBD) as the default action to improve the
application developer experience. The largest remaining blocker is that
there are no audit messages when a SECCOMP_RET_ERRNO action is taken.
Therefore, we can't suggest (to the application developer or to the
user) which sandbox knobs need to be turned to better suite their
application, we can't let the application developer know that a syscall
they're using is illegal outside of them having to debug an odd errno
value, and we can't let the user know of a potentially subverted process
that's under confinement of the sandbox. All of that could be addressed
if SECCOMP_RET_ERRNO actions generated audit messages.

I hope that helps to understand the use case.

Tyler

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web