From 43761473c254b45883a64441dd0bc85a42f3645c Mon Sep 17 00:00:00 2001

From: Paul Moore <paul@paul-moore.com>

Date: Tue, 19 Jul 2016 17:42:57 -0400

Subject: [PATCH] audit: fix a double fetch in audit_log_single_execve_arg()

There is a double fetch problem in audit_log_single_execve_arg()

where we first check the execve(2) argumnets for any "bad" characters

which would require hex encoding and then re-fetch the arguments for

logging in the audit record[1].  Of course this leaves a window of

opportunity for an unsavory application to munge with the data.

This patch reworks things by only fetching the argument data once[2]

into a buffer where it is scanned and logged into the audit

records(s).  In addition to fixing the double fetch, this patch

improves on the original code in a few other ways: better handling

of large arguments which require encoding, stricter record length

checking, and some performance improvements (completely unverified,

but we got rid of some strlen() calls, that's got to be a good

thing).

As part of the development of this patch, I've also created a basic

regression test for the audit-testsuite, the test can be tracked on

GitHub at the following link:

 * https://github.com/linux-audit/audit-testsuite/issues/25

[1] If you pay careful attention, there is actually a triple fetch

problem due to a strnlen_user() call at the top of the function.

[2] This is a tiny white lie, we do make a call to strnlen_user()

prior to fetching the argument data.  I don't like it, but due to the

way the audit record is structured we really have no choice unless we

copy the entire argument at once (which would require a rather

wasteful allocation).  The good news is that with this patch the

kernel no longer relies on this strnlen_user() value for anything

beyond recording it in the log, we also update it with a trustworthy

value whenever possible.

Reported-by: Pengfei Wang <wpengfeinudt@gmail.com>

Cc: <stable@vger.kernel.org>

Signed-off-by: Paul Moore <paul@paul-moore.com>

---

 kernel/auditsc.c | 332 +++++++++++++++++++++++++++----------------------------

 1 file changed, 164 insertions(+), 168 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c

index aa3feec..c65af21 100644

--- a/kernel/auditsc.c

+++ b/kernel/auditsc.c

@@ -73,6 +73,7 @@

 #include <linux/compat.h>

 #include <linux/ctype.h>

 #include <linux/string.h>

+#include <linux/uaccess.h>

 #include <uapi/linux/limits.h>

 #include "audit.h"

@@ -82,7 +83,8 @@

 #define AUDITSC_SUCCESS 1

 #define AUDITSC_FAILURE 2

-/* no execve audit message should be longer than this (userspace limits) */

+/* no execve audit message should be longer than this (userspace limits),

+ * see the note near the top of audit_log_execve_info() about this value */

 #define MAX_EXECVE_AUDIT_LEN 7500

 /* max length to print of cmdline/proctitle value during audit */

@@ -992,184 +994,178 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,

 	return rc;

 }

-/*

- * to_send and len_sent accounting are very loose estimates.  We aren't

- * really worried about a hard cap to MAX_EXECVE_AUDIT_LEN so much as being

- * within about 500 bytes (next page boundary)

- *

- * why snprintf?  an int is up to 12 digits long.  if we just assumed when

- * logging that a[%d]= was going to be 16 characters long we would be wasting

- * space in every audit message.  In one 7500 byte message we can log up to

- * about 1000 min size arguments.  That comes down to about 50% waste of space

- * if we didn't do the snprintf to find out how long arg_num_len was.

- */

-static int audit_log_single_execve_arg(struct audit_context *context,

-					struct audit_buffer **ab,

-					int arg_num,

-					size_t *len_sent,

-					const char __user *p,

-					char *buf)

+static void audit_log_execve_info(struct audit_context *context,

+				  struct audit_buffer **ab)

 {

-	char arg_num_len_buf[12];

-	const char __user *tmp_p = p;

-	/* how many digits are in arg_num? 5 is the length of ' a=""' */

-	size_t arg_num_len = snprintf(arg_num_len_buf, 12, "%d", arg_num) + 5;

-	size_t len, len_left, to_send;

-	size_t max_execve_audit_len = MAX_EXECVE_AUDIT_LEN;

-	unsigned int i, has_cntl = 0, too_long = 0;

-	int ret;

-

-	/* strnlen_user includes the null we don't want to send */

-	len_left = len = strnlen_user(p, MAX_ARG_STRLEN) - 1;

-

-	/*

-	 * We just created this mm, if we can't find the strings

-	 * we just copied into it something is _very_ wrong. Similar

-	 * for strings that are too long, we should not have created

-	 * any.

-	 */

-	if (WARN_ON_ONCE(len < 0 || len > MAX_ARG_STRLEN - 1)) {

-		send_sig(SIGKILL, current, 0);

-		return -1;

+	long len_max;

+	long len_rem;

+	long len_full;

+	long len_buf;

+	long len_abuf;

+	long len_tmp;

+	bool require_data;

+	bool encode;

+	unsigned int iter;

+	unsigned int arg;

+	char *buf_head;

+	char *buf;

+	const char __user *p = (const char __user *)current->mm->arg_start;

+

+	/* NOTE: this buffer needs to be large enough to hold all the non-arg

+	 *       data we put in the audit record for this argument (see the

+	 *       code below) ... at this point in time 96 is plenty */

+	char abuf[96];

+

+	/* NOTE: we set MAX_EXECVE_AUDIT_LEN to a rather arbitrary limit, the

+	 *       current value of 7500 is not as important as the fact that it

+	 *       is less than 8k, a setting of 7500 gives us plenty of wiggle

+	 *       room if we go over a little bit in the logging below */

+	WARN_ON_ONCE(MAX_EXECVE_AUDIT_LEN > 7500);

+	len_max = MAX_EXECVE_AUDIT_LEN;

+

+	/* scratch buffer to hold the userspace args */

+	buf_head = kmalloc(MAX_EXECVE_AUDIT_LEN + 1, GFP_KERNEL);

+	if (!buf_head) {

+		audit_panic("out of memory for argv string");

+		return;

 	}

+	buf = buf_head;

-	/* walk the whole argument looking for non-ascii chars */

+	audit_log_format(*ab, "argc=%d", context->execve.argc);

+

+	len_rem = len_max;

+	len_buf = 0;

+	len_full = 0;

+	require_data = true;

+	encode = false;

+	iter = 0;

+	arg = 0;

 	do {

-		if (len_left > MAX_EXECVE_AUDIT_LEN)

-			to_send = MAX_EXECVE_AUDIT_LEN;

-		else

-			to_send = len_left;

-		ret = copy_from_user(buf, tmp_p, to_send);

-		/*

-		 * There is no reason for this copy to be short. We just

-		 * copied them here, and the mm hasn't been exposed to user-

-		 * space yet.

-		 */

-		if (ret) {

-			WARN_ON(1);

-			send_sig(SIGKILL, current, 0);

-			return -1;

-		}

-		buf[to_send] = '\0';

-		has_cntl = audit_string_contains_control(buf, to_send);

-		if (has_cntl) {

-			/*

-			 * hex messages get logged as 2 bytes, so we can only

-			 * send half as much in each message

-			 */

-			max_execve_audit_len = MAX_EXECVE_AUDIT_LEN / 2;

-			break;

-		}

-		len_left -= to_send;

-		tmp_p += to_send;

-	} while (len_left > 0);

-

-	len_left = len;

-

-	if (len > max_execve_audit_len)

-		too_long = 1;

-

-	/* rewalk the argument actually logging the message */

-	for (i = 0; len_left > 0; i++) {

-		int room_left;

-

-		if (len_left > max_execve_audit_len)

-			to_send = max_execve_audit_len;

-		else

-			to_send = len_left;

-

-		/* do we have space left to send this argument in this ab? */

-		room_left = MAX_EXECVE_AUDIT_LEN - arg_num_len - *len_sent;

-		if (has_cntl)

-			room_left -= (to_send * 2);

-		else

-			room_left -= to_send;

-		if (room_left < 0) {

-			*len_sent = 0;

-			audit_log_end(*ab);

-			*ab = audit_log_start(context, GFP_KERNEL, AUDIT_EXECVE);

-			if (!*ab)

-				return 0;

-		}

+		/* NOTE: we don't ever want to trust this value for anything

+		 *       serious, but the audit record format insists we

+		 *       provide an argument length for really long arguments,

+		 *       e.g. > MAX_EXECVE_AUDIT_LEN, so we have no choice but

+		 *       to use strncpy_from_user() to obtain this value for

+		 *       recording in the log, although we don't use it

+		 *       anywhere here to avoid a double-fetch problem */

+		if (len_full == 0)

+			len_full = strnlen_user(p, MAX_ARG_STRLEN) - 1;

+

+		/* read more data from userspace */

+		if (require_data) {

+			/* can we make more room in the buffer? */

+			if (buf != buf_head) {

+				memmove(buf_head, buf, len_buf);

+				buf = buf_head;

+			}

+

+			/* fetch as much as we can of the argument */

+			len_tmp = strncpy_from_user(&buf_head[len_buf], p,

+						    len_max - len_buf);

+			if (len_tmp == -EFAULT) {

+				/* unable to copy from userspace */

+				send_sig(SIGKILL, current, 0);

+				goto out;

+			} else if (len_tmp == (len_max - len_buf)) {

+				/* buffer is not large enough */

+				require_data = true;

+				/* NOTE: if we are going to span multiple

+				 *       buffers force the encoding so we stand

+				 *       a chance at a sane len_full value and

+				 *       consistent record encoding */

+				encode = true;

+				len_full = len_full * 2;

+				p += len_tmp;

+			} else {

+				require_data = false;

+				if (!encode)

+					encode = audit_string_contains_control(

+								buf, len_tmp);

+				/* try to use a trusted value for len_full */

+				if (len_full < len_max)

+					len_full = (encode ?

+						    len_tmp * 2 : len_tmp);

+				p += len_tmp + 1;

+			}

+			len_buf += len_tmp;

+			buf_head[len_buf] = '\0';

-		/*

-		 * first record needs to say how long the original string was

-		 * so we can be sure nothing was lost.

-		 */

-		if ((i == 0) && (too_long))

-			audit_log_format(*ab, " a%d_len=%zu", arg_num,

-					 has_cntl ? 2*len : len);

-

-		/*

-		 * normally arguments are small enough to fit and we already

-		 * filled buf above when we checked for control characters

-		 * so don't bother with another copy_from_user

-		 */

-		if (len >= max_execve_audit_len)

-			ret = copy_from_user(buf, p, to_send);

-		else

-			ret = 0;

-		if (ret) {

-			WARN_ON(1);

-			send_sig(SIGKILL, current, 0);

-			return -1;

+			/* length of the buffer in the audit record? */

+			len_abuf = (encode ? len_buf * 2 : len_buf + 2);

 		}

-		buf[to_send] = '\0';

-

-		/* actually log it */

-		audit_log_format(*ab, " a%d", arg_num);

-		if (too_long)

-			audit_log_format(*ab, "[%d]", i);

-		audit_log_format(*ab, "=");

-		if (has_cntl)

-			audit_log_n_hex(*ab, buf, to_send);

-		else

-			audit_log_string(*ab, buf);

-

-		p += to_send;

-		len_left -= to_send;

-		*len_sent += arg_num_len;

-		if (has_cntl)

-			*len_sent += to_send * 2;

-		else

-			*len_sent += to_send;

-	}

-	/* include the null we didn't log */

-	return len + 1;

-}

-static void audit_log_execve_info(struct audit_context *context,

-				  struct audit_buffer **ab)

-{

-	int i, len;

-	size_t len_sent = 0;

-	const char __user *p;

-	char *buf;

+		/* write as much as we can to the audit log */

+		if (len_buf > 0) {

+			/* NOTE: some magic numbers here - basically if we

+			 *       can't fit a reasonable amount of data into the

+			 *       existing audit buffer, flush it and start with

+			 *       a new buffer */

+			if ((sizeof(abuf) + 8) > len_rem) {

+				len_rem = len_max;

+				audit_log_end(*ab);

+				*ab = audit_log_start(context,

+						      GFP_KERNEL, AUDIT_EXECVE);

+				if (!*ab)

+					goto out;

+			}

-	p = (const char __user *)current->mm->arg_start;

+			/* create the non-arg portion of the arg record */

+			len_tmp = 0;

+			if (require_data || (iter > 0) ||

+			    ((len_abuf + sizeof(abuf)) > len_rem)) {

+				if (iter == 0) {

+					len_tmp += snprintf(&abuf[len_tmp],

+							sizeof(abuf) - len_tmp,

+							" a%d_len=%lu",

+							arg, len_full);

+				}

+				len_tmp += snprintf(&abuf[len_tmp],

+						    sizeof(abuf) - len_tmp,

+						    " a%d[%d]=", arg, iter++);

+			} else

+				len_tmp += snprintf(&abuf[len_tmp],

+						    sizeof(abuf) - len_tmp,

+						    " a%d=", arg);

+			WARN_ON(len_tmp >= sizeof(abuf));

+			abuf[sizeof(abuf) - 1] = '\0';

+

+			/* log the arg in the audit record */

+			audit_log_format(*ab, "%s", abuf);

+			len_rem -= len_tmp;

+			len_tmp = len_buf;

+			if (encode) {

+				if (len_abuf > len_rem)

+					len_tmp = len_rem / 2; /* encoding */

+				audit_log_n_hex(*ab, buf, len_tmp);

+				len_rem -= len_tmp * 2;

+				len_abuf -= len_tmp * 2;

+			} else {

+				if (len_abuf > len_rem)

+					len_tmp = len_rem - 2; /* quotes */

+				audit_log_n_string(*ab, buf, len_tmp);

+				len_rem -= len_tmp + 2;

+				/* don't subtract the "2" because we still need

+				 * to add quotes to the remaining string */

+				len_abuf -= len_tmp;

+			}

+			len_buf -= len_tmp;

+			buf += len_tmp;

+		}

-	audit_log_format(*ab, "argc=%d", context->execve.argc);

+		/* ready to move to the next argument? */

+		if ((len_buf == 0) && !require_data) {

+			arg++;

+			iter = 0;

+			len_full = 0;

+			require_data = true;

+			encode = false;

+		}

+	} while (arg < context->execve.argc);

-	/*

-	 * we need some kernel buffer to hold the userspace args.  Just

-	 * allocate one big one rather than allocating one of the right size

-	 * for every single argument inside audit_log_single_execve_arg()

-	 * should be <8k allocation so should be pretty safe.

-	 */

-	buf = kmalloc(MAX_EXECVE_AUDIT_LEN + 1, GFP_KERNEL);

-	if (!buf) {

-		audit_panic("out of memory for argv string");

-		return;

-	}

+	/* NOTE: the caller handles the final audit_log_end() call */

-	for (i = 0; i < context->execve.argc; i++) {

-		len = audit_log_single_execve_arg(context, ab, i,

-						  &len_sent, p, buf);

-		if (len <= 0)

-			break;

-		p += len;

-	}

-	kfree(buf);

+out:

+	kfree(buf_head);

 }

 static void show_special(struct audit_context *context, int *call_panic)