[PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Tyler Hicks]

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

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

[Tyler Hicks]

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

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

[Tyler Hicks]

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

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

[Steve Grubb]

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

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

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 4154 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Steve Grubb]

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

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

[Paul Moore]

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

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Paul Moore]

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

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Andy Lutomirski]

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

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Paul Moore]

On Tue, Jan 3, 2017 at 12:56 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> 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?

One of the motivations for the above idea is to make it easier for
admins/users to customize the seccomp logging on their own systems,
it's not just to make devs lives easier.  I feel okay providing
guidance to an admin/user the involves setting a sysctl variable, I
can't say the same about asking them to write their own BPF ;)

-- 
paul moore
www.paul-moore.com

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 2872 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Paul Moore]

On Tue, Jan 3, 2017 at 8:31 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
> 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.

/me nods

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

Yeah, I'd much rather solve this problem generally; everybody has
their favorite action and I'd like to avoid solving the same problem
multiple times.

Sooo ... you want to take a whack at coding this up? ;)

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

Heh, so you've got audit built into the kernel but you're not using
it; that sounds "fun".

Anyway, I think the logging consolidation could still help you, if for
no other reason than everything is going through the same function at
that point.  We could do some other stuff there to handle the case
where audit is compiled, but auditd is not running ... we already have
some code in place to handle that for other reasons, check
kernel/audit.c for more information.  I'd still work on the other
stuff first and then we can add this in at the end of the patchset.

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

-- 
paul moore
www.paul-moore.com

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Kees Cook]

On Tue, Jan 3, 2017 at 11:42 AM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 3, 2017 at 8:31 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> 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.
>
> /me nods

I think this sounds fine too.

>>> 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.
>
> Yeah, I'd much rather solve this problem generally; everybody has
> their favorite action and I'd like to avoid solving the same problem
> multiple times.
>
> Sooo ... you want to take a whack at coding this up? ;)
>
>>> 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.
>
> Heh, so you've got audit built into the kernel but you're not using
> it; that sounds "fun".
>
> Anyway, I think the logging consolidation could still help you, if for
> no other reason than everything is going through the same function at
> that point.  We could do some other stuff there to handle the case
> where audit is compiled, but auditd is not running ... we already have
> some code in place to handle that for other reasons, check
> kernel/audit.c for more information.  I'd still work on the other
> stuff first and then we can add this in at the end of the patchset.

Yeah, I think the "should I report it?" threshold sysctl could just
check if audit is enabled...

I still wonder, though, isn't there a way to use auditctl to get all
the seccomp messages you need?

-Kees

>
>>> 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
>
> --
> paul moore
> www.paul-moore.com



-- 
Kees Cook
Nexus Security

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Steve Grubb]

On Tuesday, January 3, 2017 12:44:41 PM EST Kees Cook wrote:
> >> 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.
> > 
> > Heh, so you've got audit built into the kernel but you're not using
> > it; that sounds "fun".
> > 
> > Anyway, I think the logging consolidation could still help you, if for
> > no other reason than everything is going through the same function at
> > that point.  We could do some other stuff there to handle the case
> > where audit is compiled, but auditd is not running ... we already have
> > some code in place to handle that for other reasons, check
> > kernel/audit.c for more information.  I'd still work on the other
> > stuff first and then we can add this in at the end of the patchset.
> 
> Yeah, I think the "should I report it?" threshold sysctl could just
> check if audit is enabled...
> 
> I still wonder, though, isn't there a way to use auditctl to get all
> the seccomp messages you need?

If you do "auditctl -e 1" then auditing will be enabled and it will send 
events to syslog if the audit daemon is not running.

-Steve

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Paul Moore]

On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
> I still wonder, though, isn't there a way to use auditctl to get all
> the seccomp messages you need?

Not all of the seccomp actions are currently logged, that's one of the
problems (and the biggest at the moment).

-- 
paul moore
www.paul-moore.com

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Kees Cook]

On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>> I still wonder, though, isn't there a way to use auditctl to get all
>> the seccomp messages you need?
>
> Not all of the seccomp actions are currently logged, that's one of the
> problems (and the biggest at the moment).

Well... sort of. It all gets passed around, but the logic isn't very
obvious (or at least I always have to go look it up).

include/linux/audit.h:

#ifdef CONFIG_AUDITSYSCALL
...
static inline void audit_seccomp(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);
}
...
#else /* CONFIG_AUDITSYSCALL */

static inline void audit_seccomp(unsigned long syscall, long signr, int code)
{ }
...
#endif

kernel/seccomp.c:

        switch (action) {
        case SECCOMP_RET_ERRNO:
                /* Set low-order bits as an errno, capped at MAX_ERRNO. */
                if (data > MAX_ERRNO)
                        data = MAX_ERRNO;
                syscall_set_return_value(current, task_pt_regs(current),
                                         -data, 0);
                goto skip;
...
        case SECCOMP_RET_KILL:
        default:
                audit_seccomp(this_syscall, SIGSYS, action);
                do_exit(SIGSYS);
        }

        unreachable();

skip:
        audit_seccomp(this_syscall, 0, action);


Current state:

- if CONFIG_AUDITSYSCALL=n, then nothing is ever reported.

- if audit is disabled, nothing is ever reported.

- if a process isn't being specifically audited
(!audit_dummy_context()), only signals (RET_KILL) are reported.

- when being specifically audited, everything is reported.


So, shouldn't it be possible to specifically audit a process and
examine the resulting logs for the RET_* level one is interested in
("code=0x%x" in __audit_seccomp())?

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Paul Moore]

On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>> I still wonder, though, isn't there a way to use auditctl to get all
>>> the seccomp messages you need?
>>
>> Not all of the seccomp actions are currently logged, that's one of the
>> problems (and the biggest at the moment).
>
> Well... sort of. It all gets passed around, but the logic isn't very
> obvious (or at least I always have to go look it up).

Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
least one other action, but I can't remember which off the top of my
head)?

> include/linux/audit.h:
>
> #ifdef CONFIG_AUDITSYSCALL
> ...
> static inline void audit_seccomp(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);
> }
> ...
> #else /* CONFIG_AUDITSYSCALL */
>
> static inline void audit_seccomp(unsigned long syscall, long signr, int code)
> { }
> ...
> #endif
>
> kernel/seccomp.c:
>
>         switch (action) {
>         case SECCOMP_RET_ERRNO:
>                 /* Set low-order bits as an errno, capped at MAX_ERRNO. */
>                 if (data > MAX_ERRNO)
>                         data = MAX_ERRNO;
>                 syscall_set_return_value(current, task_pt_regs(current),
>                                          -data, 0);
>                 goto skip;
> ...
>         case SECCOMP_RET_KILL:
>         default:
>                 audit_seccomp(this_syscall, SIGSYS, action);
>                 do_exit(SIGSYS);
>         }
>
>         unreachable();
>
> skip:
>         audit_seccomp(this_syscall, 0, action);
>
>
> Current state:
>
> - if CONFIG_AUDITSYSCALL=n, then nothing is ever reported.
>
> - if audit is disabled, nothing is ever reported.
>
> - if a process isn't being specifically audited
> (!audit_dummy_context()), only signals (RET_KILL) are reported.
>
> - when being specifically audited, everything is reported.
>
>
> So, shouldn't it be possible to specifically audit a process and
> examine the resulting logs for the RET_* level one is interested in
> ("code=0x%x" in __audit_seccomp())?
>
> -Kees
>
> --
> Kees Cook
> Nexus Security



-- 
paul moore
www.paul-moore.com

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Kees Cook]

On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> I still wonder, though, isn't there a way to use auditctl to get all
>>>> the seccomp messages you need?
>>>
>>> Not all of the seccomp actions are currently logged, that's one of the
>>> problems (and the biggest at the moment).
>>
>> Well... sort of. It all gets passed around, but the logic isn't very
>> obvious (or at least I always have to go look it up).
>
> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
> least one other action, but I can't remember which off the top of my
> head)?

Sure, but if you're using audit, you don't need RET_ALLOW to be logged
because you'll get a full syscall log entry. Logging RET_ALLOW is
redundant and provides no new information, it seems to me.

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Paul Moore]

On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>> I still wonder, though, isn't there a way to use auditctl to get all
>>>>> the seccomp messages you need?
>>>>
>>>> Not all of the seccomp actions are currently logged, that's one of the
>>>> problems (and the biggest at the moment).
>>>
>>> Well... sort of. It all gets passed around, but the logic isn't very
>>> obvious (or at least I always have to go look it up).
>>
>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
>> least one other action, but I can't remember which off the top of my
>> head)?
>
> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
> because you'll get a full syscall log entry. Logging RET_ALLOW is
> redundant and provides no new information, it seems to me.

I only bring this up as it might be a way to help solve the
SECCOMP_RET_AUDIT problem that Tyler mentioned.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Kees Cook]

On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>>> I still wonder, though, isn't there a way to use auditctl to get all
>>>>>> the seccomp messages you need?
>>>>>
>>>>> Not all of the seccomp actions are currently logged, that's one of the
>>>>> problems (and the biggest at the moment).
>>>>
>>>> Well... sort of. It all gets passed around, but the logic isn't very
>>>> obvious (or at least I always have to go look it up).
>>>
>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
>>> least one other action, but I can't remember which off the top of my
>>> head)?
>>
>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
>> because you'll get a full syscall log entry. Logging RET_ALLOW is
>> redundant and provides no new information, it seems to me.
>
> I only bring this up as it might be a way to help solve the
> SECCOMP_RET_AUDIT problem that Tyler mentioned.

So, I guess I want to understand why something like this doesn't work,
with no changes at all to the kernel:

Imaginary "seccomp-audit.c":

...
    pid = fork();
    if (pid) {
        char cmd[80];

        sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid);
        system(cmd);
        release...
     } else {
        wait for release...
        execv(argv[1], argv + 1);
     }
...

This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well
as all seccomp actions of any kind. (Down side is the need for root to
launch auditctl...)

Perhaps an improvement to this could be enabling audit when seccomp
syscall is seen? I can't tell if auditctl already has something to do
this ("start auditing this process and all children when syscall X is
performed").

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 3155 bytes --]

On 01/04/2017 04:44 AM, Kees Cook wrote:
> On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>>>> I still wonder, though, isn't there a way to use auditctl to get all
>>>>>>> the seccomp messages you need?
>>>>>>
>>>>>> Not all of the seccomp actions are currently logged, that's one of the
>>>>>> problems (and the biggest at the moment).
>>>>>
>>>>> Well... sort of. It all gets passed around, but the logic isn't very
>>>>> obvious (or at least I always have to go look it up).
>>>>
>>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
>>>> least one other action, but I can't remember which off the top of my
>>>> head)?
>>>
>>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
>>> because you'll get a full syscall log entry. Logging RET_ALLOW is
>>> redundant and provides no new information, it seems to me.
>>
>> I only bring this up as it might be a way to help solve the
>> SECCOMP_RET_AUDIT problem that Tyler mentioned.
> 
> So, I guess I want to understand why something like this doesn't work,
> with no changes at all to the kernel:
> 
> Imaginary "seccomp-audit.c":
> 
> ...
>     pid = fork();
>     if (pid) {
>         char cmd[80];
> 
>         sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid);
>         system(cmd);
>         release...
>      } else {
>         wait for release...
>         execv(argv[1], argv + 1);
>      }
> ...
> 
> This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well
> as all seccomp actions of any kind. (Down side is the need for root to
> launch auditctl...)

Hey Kees - Thanks for the suggestion!

Here are some of the reasons that it doesn't quite work:

1) We don't install/run auditd by default and would continue to prefer
not to in some situations where resources are tight.

2) We block a relatively small number of syscalls as compared to what
are allowed so auditing all syscalls is a really heavyweight solution.
That could be fixed with a better -S argument, though.

3) We sometimes only block certain arguments for a given syscall and
auditing all instances of the syscall could still be a heavyweight solution.

4) If the application spawns children processes, that rule doesn't audit
their syscalls. That can be fixed with ppid=%d but then grandchildren
pids are a problem.

5) Cleanup of the audit rule for an old pid, before the pid is reused,
could be difficult.

Tyler

> 
> Perhaps an improvement to this could be enabling audit when seccomp
> syscall is seen? I can't tell if auditctl already has something to do
> this ("start auditing this process and all children when syscall X is
> performed").
> 
> -Kees
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Richard Guy Briggs]

On 2017-01-04 08:58, Tyler Hicks wrote:
> On 01/04/2017 04:44 AM, Kees Cook wrote:
> > On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul@paul-moore.com> wrote:
> >> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
> >>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
> >>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
> >>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
> >>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
> >>>>>>> I still wonder, though, isn't there a way to use auditctl to get all
> >>>>>>> the seccomp messages you need?
> >>>>>>
> >>>>>> Not all of the seccomp actions are currently logged, that's one of the
> >>>>>> problems (and the biggest at the moment).
> >>>>>
> >>>>> Well... sort of. It all gets passed around, but the logic isn't very
> >>>>> obvious (or at least I always have to go look it up).
> >>>>
> >>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
> >>>> least one other action, but I can't remember which off the top of my
> >>>> head)?
> >>>
> >>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
> >>> because you'll get a full syscall log entry. Logging RET_ALLOW is
> >>> redundant and provides no new information, it seems to me.
> >>
> >> I only bring this up as it might be a way to help solve the
> >> SECCOMP_RET_AUDIT problem that Tyler mentioned.
> > 
> > So, I guess I want to understand why something like this doesn't work,
> > with no changes at all to the kernel:
> > 
> > Imaginary "seccomp-audit.c":
> > 
> > ...
> >     pid = fork();
> >     if (pid) {
> >         char cmd[80];
> > 
> >         sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid);
> >         system(cmd);
> >         release...
> >      } else {
> >         wait for release...
> >         execv(argv[1], argv + 1);
> >      }
> > ...
> > 
> > This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well
> > as all seccomp actions of any kind. (Down side is the need for root to
> > launch auditctl...)
> 
> Hey Kees - Thanks for the suggestion!
> 
> Here are some of the reasons that it doesn't quite work:
> 
> 1) We don't install/run auditd by default and would continue to prefer
> not to in some situations where resources are tight.
> 
> 2) We block a relatively small number of syscalls as compared to what
> are allowed so auditing all syscalls is a really heavyweight solution.
> That could be fixed with a better -S argument, though.
> 
> 3) We sometimes only block certain arguments for a given syscall and
> auditing all instances of the syscall could still be a heavyweight solution.
> 
> 4) If the application spawns children processes, that rule doesn't audit
> their syscalls. That can be fixed with ppid=%d but then grandchildren
> pids are a problem.

This patch that wasn't accepted upstream might be useful:
	https://www.redhat.com/archives/linux-audit/2015-August/msg00067.html
	https://www.redhat.com/archives/linux-audit/2015-August/msg00068.html

> 5) Cleanup of the audit rule for an old pid, before the pid is reused,
> could be difficult.
> 
> Tyler
> 
> > Perhaps an improvement to this could be enabling audit when seccomp
> > syscall is seen? I can't tell if auditctl already has something to do
> > this ("start auditing this process and all children when syscall X is
> > performed").
> > 
> > -Kees

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Kees Cook]

On Tue, Jan 3, 2017 at 8:43 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-01-04 08:58, Tyler Hicks wrote:
>> On 01/04/2017 04:44 AM, Kees Cook wrote:
>> > On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul@paul-moore.com> wrote:
>> >> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
>> >>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
>> >>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>> >>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>> >>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>> >>>>>>> I still wonder, though, isn't there a way to use auditctl to get all
>> >>>>>>> the seccomp messages you need?
>> >>>>>>
>> >>>>>> Not all of the seccomp actions are currently logged, that's one of the
>> >>>>>> problems (and the biggest at the moment).
>> >>>>>
>> >>>>> Well... sort of. It all gets passed around, but the logic isn't very
>> >>>>> obvious (or at least I always have to go look it up).
>> >>>>
>> >>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
>> >>>> least one other action, but I can't remember which off the top of my
>> >>>> head)?
>> >>>
>> >>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
>> >>> because you'll get a full syscall log entry. Logging RET_ALLOW is
>> >>> redundant and provides no new information, it seems to me.
>> >>
>> >> I only bring this up as it might be a way to help solve the
>> >> SECCOMP_RET_AUDIT problem that Tyler mentioned.
>> >
>> > So, I guess I want to understand why something like this doesn't work,
>> > with no changes at all to the kernel:
>> >
>> > Imaginary "seccomp-audit.c":
>> >
>> > ...
>> >     pid = fork();
>> >     if (pid) {
>> >         char cmd[80];
>> >
>> >         sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid);
>> >         system(cmd);
>> >         release...
>> >      } else {
>> >         wait for release...
>> >         execv(argv[1], argv + 1);
>> >      }
>> > ...
>> >
>> > This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well
>> > as all seccomp actions of any kind. (Down side is the need for root to
>> > launch auditctl...)
>>
>> Hey Kees - Thanks for the suggestion!
>>
>> Here are some of the reasons that it doesn't quite work:
>>
>> 1) We don't install/run auditd by default and would continue to prefer
>> not to in some situations where resources are tight.

Strictly speaking, auditd isn't needed for auditctl, IIUC.

>> 2) We block a relatively small number of syscalls as compared to what
>> are allowed so auditing all syscalls is a really heavyweight solution.
>> That could be fixed with a better -S argument, though.

Yeah, it seems like there needs to be some kind of improvement there
on the audit side (I was thinking a better -F). The all-or-nothing
approach is way too big a hammer.

>> 3) We sometimes only block certain arguments for a given syscall and
>> auditing all instances of the syscall could still be a heavyweight solution.
>>
>> 4) If the application spawns children processes, that rule doesn't audit
>> their syscalls. That can be fixed with ppid=%d but then grandchildren
>> pids are a problem.
>
> This patch that wasn't accepted upstream might be useful:
>         https://www.redhat.com/archives/linux-audit/2015-August/msg00067.html
>         https://www.redhat.com/archives/linux-audit/2015-August/msg00068.html

I'd like this regardless. It's really difficult to audit trees of
processes before they launch. :)

>
>> 5) Cleanup of the audit rule for an old pid, before the pid is reused,
>> could be difficult.
>>
>> Tyler
>>
>> > Perhaps an improvement to this could be enabling audit when seccomp
>> > syscall is seen? I can't tell if auditctl already has something to do
>> > this ("start auditing this process and all children when syscall X is
>> > performed").
>> >
>> > -Kees
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Kernel Security Engineering, Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635

-Kees


-- 
Kees Cook
Nexus Security

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 4445 bytes --]

On 01/04/2017 02:42 AM, Paul Moore wrote:
> On Tue, Jan 3, 2017 at 8:31 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> 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.
> 
> /me nods
> 
>>> 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.
> 
> Yeah, I'd much rather solve this problem generally; everybody has
> their favorite action and I'd like to avoid solving the same problem
> multiple times.
> 
> Sooo ... you want to take a whack at coding this up? ;)

Yes, I can do that.

> 
>>> 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.
> 
> Heh, so you've got audit built into the kernel but you're not using
> it; that sounds "fun".

Users that need full auditing functionality can simply install auditd
but most users don't require it. Ubuntu has done it this way for many
years and the lack of seccomp auditing when auditd isn't running has
been the only problem that I can remember.

> Anyway, I think the logging consolidation could still help you, if for
> no other reason than everything is going through the same function at
> that point.  We could do some other stuff there to handle the case
> where audit is compiled, but auditd is not running ... we already have
> some code in place to handle that for other reasons, check
> kernel/audit.c for more information.  I'd still work on the other
> stuff first and then we can add this in at the end of the patchset.

/me nods. It is easy to continue to distro patch out the recently added
check for audit_enabled until a better solution for all distros can be
identified/upstreamed.

Thanks again!

Tyler

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Andy Lutomirski]

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?

Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 1888 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]