[PATCH 0/2] Fix auditsc DoS and move it to staging

[PATCH 0/2] Fix auditsc DoS and move it to staging

[Andy Lutomirski]

CONFIG_AUDITSYSCALL is awful.  Patch 2 enumerates some reasons.

Patch 1 fixes a nasty DoS and possible information leak.  It should
be applied and backported.

Patch 2 is optional.  I leave it to other peoples' judgment.

Andy Lutomirski (2):
  auditsc: audit_krule mask accesses need bounds checking
  audit: Move CONFIG_AUDITSYSCALL into staging and update help text

 init/Kconfig     | 13 ++++++++-----
 kernel/auditsc.c | 27 ++++++++++++++++++---------
 2 files changed, 26 insertions(+), 14 deletions(-)

-- 
1.9.3

[PATCH 1/2] auditsc: audit_krule mask accesses need bounds checking

[Andy Lutomirski]

Fixes an easy DoS and possible information disclosure.

This does nothing about the broken state of x32 auditing.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 kernel/auditsc.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f251a5e..7ccd9db 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
 	return AUDIT_BUILD_CONTEXT;
 }
 
+static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
+{
+	int word, bit;
+
+	if (val > 0xffffffff)
+		return false;
+
+	word = AUDIT_WORD(val);
+	if (word >= AUDIT_BITMASK_SIZE)
+		return false;
+
+	bit = AUDIT_BIT(val);
+
+	return rule->mask[word] & bit;
+}
+
 /* At syscall entry and exit time, this filter is called if the
  * audit_state is not low enough that auditing cannot take place, but is
  * also not high enough that we already know we have to write an audit
@@ -745,11 +761,8 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
 
 	rcu_read_lock();
 	if (!list_empty(list)) {
-		int word = AUDIT_WORD(ctx->major);
-		int bit  = AUDIT_BIT(ctx->major);
-
 		list_for_each_entry_rcu(e, list, list) {
-			if ((e->rule.mask[word] & bit) == bit &&
+			if (audit_in_mask(&e->rule, ctx->major) &&
 			    audit_filter_rules(tsk, &e->rule, ctx, NULL,
 					       &state, false)) {
 				rcu_read_unlock();
@@ -769,20 +782,16 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
 static int audit_filter_inode_name(struct task_struct *tsk,
 				   struct audit_names *n,
 				   struct audit_context *ctx) {
-	int word, bit;
 	int h = audit_hash_ino((u32)n->ino);
 	struct list_head *list = &audit_inode_hash[h];
 	struct audit_entry *e;
 	enum audit_state state;
 
-	word = AUDIT_WORD(ctx->major);
-	bit  = AUDIT_BIT(ctx->major);
-
 	if (list_empty(list))
 		return 0;
 
 	list_for_each_entry_rcu(e, list, list) {
-		if ((e->rule.mask[word] & bit) == bit &&
+		if (audit_in_mask(&e->rule, ctx->major) &&
 		    audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
 			ctx->current_state = state;
 			return 1;
-- 
1.9.3

[PATCH 2/2] audit: Move CONFIG_AUDITSYSCALL into staging and update help text

[Andy Lutomirski]

Here are some issues with the code:
 - It thinks that syscalls have four arguments.
 - It's a performance disaster.
 - It assumes that syscall numbers are between 0 and 2048.
 - It's unclear whether it's supposed to be reliable.
 - It's broken on things like x32.
 - It can't support ARM OABI.
 - Its approach to memory allocation is terrifying.

I considered marking it BROKEN, but that might be too harsh.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 init/Kconfig | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 9d3585b..4584f8a 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -296,13 +296,16 @@ config HAVE_ARCH_AUDITSYSCALL
 	bool
 
 config AUDITSYSCALL
-	bool "Enable system-call auditing support"
-	depends on AUDIT && HAVE_ARCH_AUDITSYSCALL
+	bool "Enable system-call auditing support (not recommended)"
+	depends on AUDIT && HAVE_ARCH_AUDITSYSCALL && STAGING
 	default y if SECURITY_SELINUX
 	help
-	  Enable low-overhead system-call auditing infrastructure that
-	  can be used independently or with another kernel subsystem,
-	  such as SELinux.
+	  Enable system-call auditing infrastructure that can be used
+	  independently or with another kernel subsystem, such as
+	  SELinux.
+
+	  AUDITSYSCALL has serious performance and correctness issues.
+	  Use it with extreme caution.
 
 config AUDIT_WATCH
 	def_bool y
-- 
1.9.3

Re: [PATCH 2/2] audit: Move CONFIG_AUDITSYSCALL into staging and update help text

[Greg KH]

On Wed, May 28, 2014 at 03:21:20PM -0700, Andy Lutomirski wrote:
> Here are some issues with the code:
>  - It thinks that syscalls have four arguments.
>  - It's a performance disaster.
>  - It assumes that syscall numbers are between 0 and 2048.
>  - It's unclear whether it's supposed to be reliable.
>  - It's broken on things like x32.
>  - It can't support ARM OABI.
>  - Its approach to memory allocation is terrifying.
> 
> I considered marking it BROKEN, but that might be too harsh.
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  init/Kconfig | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 9d3585b..4584f8a 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -296,13 +296,16 @@ config HAVE_ARCH_AUDITSYSCALL
>  	bool
>  
>  config AUDITSYSCALL
> -	bool "Enable system-call auditing support"
> -	depends on AUDIT && HAVE_ARCH_AUDITSYSCALL
> +	bool "Enable system-call auditing support (not recommended)"
> +	depends on AUDIT && HAVE_ARCH_AUDITSYSCALL && STAGING

As it doesn't actually move any code into drivers/staging/, and I have
no TODO list that needs to be resolved in order to get it out of staging
(other than your list above), I'd prefer it not to take on the STAGING
mark.

But BROKEN is fine with me, that should wake people up to fix it or just
drop it :)

thanks,

greg k-h