LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* PROBLEM: Linux kernel.core_pattern with pipes does argument splitting after template expansion
@ 2019-03-19  6:06 Paul Wise
  2019-05-20  9:01 ` [PATCH] coredump: Split pipe command whitespace before expanding template Paul Wise
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Wise @ 2019-03-19  6:06 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel, linux-kernel; +Cc: Jakub Wilk

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

The Linux kernel.core_pattern support for core dump handlers using the
pipe syntax does argument splitting after template expansion.

At minimum this bug could cause truncated values for the executable
name. This also means that the argument parsing for core dump handlers
is slightly more complicated because they have to deal with the fact
that an attacker that can control the executable name via %E and %e
could pass additional arguments, including command-line options, to the
handler. Usually this is easy to deal with by merging the remaining
arguments after an options termination indicator but it is very
unlikely that core dump handler implementers are aware of the issue.

Theoretically hostnames with %h could also be split up but in practice
they do not appear to be allowed to contain spaces.

Steps to reproduce:

   $ cat foo 
   #!/bin/sh
   printf "%s~" "$@" >> /var/log/core
   echo >> /var/log/core

   $ chmod a+rx foo

   $ sudo sysctl kernel.core_pattern="|`pwd`/foo %E"
   kernel.core_pattern = |/home/pabs/foo %E

   $ cp /bin/sleep 'sleep with spaces'

   $ ./sleep\ with\ spaces 55555 &
   [1] 16041

   $ kill -SEGV %1
   [1]+  Segmentation fault      (core dumped) ./sleep\ with\ spaces 55555

Incorrect results:

   $ cat /var/log/core
   !home!pabs!sleep~with~spaces~

Correct results:

   $ cat /var/log/core
   !home!pabs!sleep with spaces~

This was originally reported by Jakub Wilk <jwilk@jwilk.net>:
<20190312145043.jxjoj66kqssptolr@jwilk.net>
https://bugs.debian.org/924398

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] coredump: Split pipe command whitespace before expanding template
  2019-03-19  6:06 PROBLEM: Linux kernel.core_pattern with pipes does argument splitting after template expansion Paul Wise
@ 2019-05-20  9:01 ` Paul Wise
  2019-05-20 15:14   ` kbuild test robot
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Wise @ 2019-05-20  9:01 UTC (permalink / raw)
  To: Neil Horman, Alexander Viro, linux-fsdevel, linux-kernel
  Cc: Paul Wise, Jakub Wilk

Save the offsets of the start of each argument to avoid having to
update pointers to each argument after every corename krealloc and
to avoid having to duplicate the memory for the dump command.

Executable names containing spaces were previously being expanded from
%e or %E and then split in the middle of the filename. This is incorrect
behaviour since an argument list can represent arguments with spaces.

The splitting could lead to extra arguments being passed to the core dump
handler that it might have interpreted as options or ignored completely.

Core dump handlers that are not aware of this Linux kernel issue will be
using %e or %E without considering that it may be split and so they will
be vulnerable to processes with spaces in their names breaking their
argument list. If their internals are otherwise well written, such as
if they are written in shell but quote arguments, they will work better
after this change than before. If they are not well written, then there
is a slight chance of breakage depending on the details of the code but
they will already be fairly broken by the split filenames.

Core dump handlers that are aware of this Linux kernel issue will be
placing %e or %E as the last item in their core_pattern and then
aggregating all of the remaining arguments into one, separated by
spaces. Alternatively they will be obtaining the filename via other
methods. Both of these will be compatible with the new arrangement.

A side effect from this change is that unknown template types
(for example %z) result in an empty argument to the dump handler
instead of the argument being dropped. This is a desired change as:

It is easier for dump handlers to process empty arguments than dropped
ones, especially if they are written in shell or don't pass each template
item with a preceding command-line option in order to differentiate
between individual template types. Most core_patterns in the wild do not
use options so they can confuse different template types (especially
numeric ones) if an earlier one gets dropped in old kernels. If the
kernel introduces a new template type and a core_pattern uses it, the
core dump handler might not expect that the argument can be dropped in
old kernels.

For example, this can result in security issues when %d is dropped in old
kernels. This happened with the corekeeper package in Debian and resulted
in the interface between corekeeper and Linux having to be rewritten to
use command-line options to differentiate between template types.

The core_pattern for most core dump handlers is written by the handler
author who would generally not insert unknown template types so this
change should be compatible with all the core dump handlers that exist.

Reported-by: Jakub Wilk <jwilk@jwilk.net>
Reported-in: <20190312145043.jxjoj66kqssptolr@jwilk.net>
Reported-by: Paul Wise <pabs3@bonedaddy.net>
Reported-in: <c8b7ecb8508895bf4adb62a748e2ea2c71854597.camel@bonedaddy.net>
Suggestions-from: Jakub Wilk <jwilk@jwilk.net>
Signed-off-by: Paul Wise <pabs3@bonedaddy.net>
See-also: https://bugs.debian.org/924398
Fixes: commit 74aadce986052f20088c2678f589ea0e8d3a4b59
---
 fs/coredump.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index e42e17e55bfd..40c440efb5f4 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -7,6 +7,7 @@
 #include <linux/stat.h>
 #include <linux/fcntl.h>
 #include <linux/swap.h>
+#include <linux/ctype.h>
 #include <linux/string.h>
 #include <linux/init.h>
 #include <linux/pagemap.h>
@@ -187,11 +188,13 @@ static int cn_print_exe_file(struct core_name *cn)
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-static int format_corename(struct core_name *cn, struct coredump_params *cprm)
+static int format_corename(struct core_name *cn, struct coredump_params *cprm,
+			   size_t **argv, int *argc)
 {
 	const struct cred *cred = current_cred();
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
+	bool was_space = false;
 	int pid_in_pattern = 0;
 	int err = 0;
 
@@ -201,12 +204,36 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 		return -ENOMEM;
 	cn->corename[0] = '\0';
 
-	if (ispipe)
+	if (ispipe) {
+		/* sizeof(core_pattern) / 2 is the maximum number of args. */
+		int argvs = sizeof(core_pattern) / 2;
+		(*argvs) = kmalloc_array(argvs, sizeof(**argv), GFP_KERNEL);
+		if (!(*argv))
+			return -ENOMEM;
+		(*argv)[(*argc)++] = 0;
 		++pat_ptr;
+	}
 
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
+		/*
+		 * Split on spaces before doing template expansion so that
+		 * %e and %E don't get split if they have spaces in them
+		 */
+		if (ispipe) {
+			if (isspace(*pat_ptr)) {
+				was_space = true;
+				pat_ptr++;
+				continue;
+			} else if (was_space) {
+				was_space = false;
+				err = cn_printf(cn, "%c", '\0');
+				if (err)
+					return err;
+				(*argv)[(*argc)++] = cn->used;
+			}
+		}
 		if (*pat_ptr != '%') {
 			err = cn_printf(cn, "%c", *pat_ptr++);
 		} else {
@@ -546,6 +573,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	struct cred *cred;
 	int retval = 0;
 	int ispipe;
+	size_t *argv = NULL;
+	int argc = 0;
 	struct files_struct *displaced;
 	/* require nonrelative corefile path and be extra careful */
 	bool need_suid_safe = false;
@@ -592,9 +621,10 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 
 	old_cred = override_creds(cred);
 
-	ispipe = format_corename(&cn, &cprm);
+	ispipe = format_corename(&cn, &cprm, &argv, &argc);
 
 	if (ispipe) {
+		int argi;
 		int dump_count;
 		char **helper_argv;
 		struct subprocess_info *sub_info;
@@ -637,12 +667,16 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			goto fail_dropcount;
 		}
 
-		helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
+		helper_argv = kmalloc_array(argc + 1, sizeof(*helper_argv),
+					    GFP_KERNEL);
 		if (!helper_argv) {
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
 			goto fail_dropcount;
 		}
+		for (argi = 0; argi < argc; argi++)
+			helper_argv[argi] = cn.corename + argv[argi];
+		helper_argv[argi] = NULL;
 
 		retval = -ENOMEM;
 		sub_info = call_usermodehelper_setup(helper_argv[0],
@@ -652,7 +686,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			retval = call_usermodehelper_exec(sub_info,
 							  UMH_WAIT_EXEC);
 
-		argv_free(helper_argv);
+		kfree(helper_argv);
 		if (retval) {
 			printk(KERN_INFO "Core dump to |%s pipe failed\n",
 			       cn.corename);
@@ -766,6 +800,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	if (ispipe)
 		atomic_dec(&core_dump_count);
 fail_unlock:
+	kfree(argv);
 	kfree(cn.corename);
 	coredump_finish(mm, core_dumped);
 	revert_creds(old_cred);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] coredump: Split pipe command whitespace before expanding template
  2019-05-20  9:01 ` [PATCH] coredump: Split pipe command whitespace before expanding template Paul Wise
@ 2019-05-20 15:14   ` kbuild test robot
  2019-05-21  0:37     ` Paul Wise
  0 siblings, 1 reply; 6+ messages in thread
From: kbuild test robot @ 2019-05-20 15:14 UTC (permalink / raw)
  To: Paul Wise
  Cc: kbuild-all, Neil Horman, Alexander Viro, linux-fsdevel,
	linux-kernel, Paul Wise, Jakub Wilk

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

Hi Paul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.2-rc1 next-20190520]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Paul-Wise/coredump-Split-pipe-command-whitespace-before-expanding-template/20190520-212130
config: riscv-defconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/coredump.c: In function 'format_corename':
>> fs/coredump.c:210:4: error: invalid type argument of unary '*' (have 'int')
      (*argvs) = kmalloc_array(argvs, sizeof(**argv), GFP_KERNEL);
       ^~~~~~

vim +210 fs/coredump.c

   186	
   187	/* format_corename will inspect the pattern parameter, and output a
   188	 * name into corename, which must have space for at least
   189	 * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
   190	 */
   191	static int format_corename(struct core_name *cn, struct coredump_params *cprm,
   192				   size_t **argv, int *argc)
   193	{
   194		const struct cred *cred = current_cred();
   195		const char *pat_ptr = core_pattern;
   196		int ispipe = (*pat_ptr == '|');
   197		bool was_space = false;
   198		int pid_in_pattern = 0;
   199		int err = 0;
   200	
   201		cn->used = 0;
   202		cn->corename = NULL;
   203		if (expand_corename(cn, core_name_size))
   204			return -ENOMEM;
   205		cn->corename[0] = '\0';
   206	
   207		if (ispipe) {
   208			/* sizeof(core_pattern) / 2 is the maximum number of args. */
   209			int argvs = sizeof(core_pattern) / 2;
 > 210			(*argvs) = kmalloc_array(argvs, sizeof(**argv), GFP_KERNEL);
   211			if (!(*argv))
   212				return -ENOMEM;
   213			(*argv)[(*argc)++] = 0;
   214			++pat_ptr;
   215		}
   216	
   217		/* Repeat as long as we have more pattern to process and more output
   218		   space */
   219		while (*pat_ptr) {
   220			/*
   221			 * Split on spaces before doing template expansion so that
   222			 * %e and %E don't get split if they have spaces in them
   223			 */
   224			if (ispipe) {
   225				if (isspace(*pat_ptr)) {
   226					was_space = true;
   227					pat_ptr++;
   228					continue;
   229				} else if (was_space) {
   230					was_space = false;
   231					err = cn_printf(cn, "%c", '\0');
   232					if (err)
   233						return err;
   234					(*argv)[(*argc)++] = cn->used;
   235				}
   236			}
   237			if (*pat_ptr != '%') {
   238				err = cn_printf(cn, "%c", *pat_ptr++);
   239			} else {
   240				switch (*++pat_ptr) {
   241				/* single % at the end, drop that */
   242				case 0:
   243					goto out;
   244				/* Double percent, output one percent */
   245				case '%':
   246					err = cn_printf(cn, "%c", '%');
   247					break;
   248				/* pid */
   249				case 'p':
   250					pid_in_pattern = 1;
   251					err = cn_printf(cn, "%d",
   252						      task_tgid_vnr(current));
   253					break;
   254				/* global pid */
   255				case 'P':
   256					err = cn_printf(cn, "%d",
   257						      task_tgid_nr(current));
   258					break;
   259				case 'i':
   260					err = cn_printf(cn, "%d",
   261						      task_pid_vnr(current));
   262					break;
   263				case 'I':
   264					err = cn_printf(cn, "%d",
   265						      task_pid_nr(current));
   266					break;
   267				/* uid */
   268				case 'u':
   269					err = cn_printf(cn, "%u",
   270							from_kuid(&init_user_ns,
   271								  cred->uid));
   272					break;
   273				/* gid */
   274				case 'g':
   275					err = cn_printf(cn, "%u",
   276							from_kgid(&init_user_ns,
   277								  cred->gid));
   278					break;
   279				case 'd':
   280					err = cn_printf(cn, "%d",
   281						__get_dumpable(cprm->mm_flags));
   282					break;
   283				/* signal that caused the coredump */
   284				case 's':
   285					err = cn_printf(cn, "%d",
   286							cprm->siginfo->si_signo);
   287					break;
   288				/* UNIX time of coredump */
   289				case 't': {
   290					time64_t time;
   291	
   292					time = ktime_get_real_seconds();
   293					err = cn_printf(cn, "%lld", time);
   294					break;
   295				}
   296				/* hostname */
   297				case 'h':
   298					down_read(&uts_sem);
   299					err = cn_esc_printf(cn, "%s",
   300						      utsname()->nodename);
   301					up_read(&uts_sem);
   302					break;
   303				/* executable */
   304				case 'e':
   305					err = cn_esc_printf(cn, "%s", current->comm);
   306					break;
   307				case 'E':
   308					err = cn_print_exe_file(cn);
   309					break;
   310				/* core limit size */
   311				case 'c':
   312					err = cn_printf(cn, "%lu",
   313						      rlimit(RLIMIT_CORE));
   314					break;
   315				default:
   316					break;
   317				}
   318				++pat_ptr;
   319			}
   320	
   321			if (err)
   322				return err;
   323		}
   324	
   325	out:
   326		/* Backward compatibility with core_uses_pid:
   327		 *
   328		 * If core_pattern does not include a %p (as is the default)
   329		 * and core_uses_pid is set, then .%pid will be appended to
   330		 * the filename. Do not do this for piped commands. */
   331		if (!ispipe && !pid_in_pattern && core_uses_pid) {
   332			err = cn_printf(cn, ".%d", task_tgid_vnr(current));
   333			if (err)
   334				return err;
   335		}
   336		return ispipe;
   337	}
   338	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 17610 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] coredump: Split pipe command whitespace before expanding template
  2019-05-20 15:14   ` kbuild test robot
@ 2019-05-21  0:37     ` Paul Wise
  2019-05-28  5:11       ` [PATCH v3] " Paul Wise
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Wise @ 2019-05-21  0:37 UTC (permalink / raw)
  To: Neil Horman, Alexander Viro, linux-fsdevel, linux-kernel
  Cc: Paul Wise, Jakub Wilk

Save the offsets of the start of each argument to avoid having to
update pointers to each argument after every corename krealloc and
to avoid having to duplicate the memory for the dump command.

Executable names containing spaces were previously being expanded from
%e or %E and then split in the middle of the filename. This is incorrect
behaviour since an argument list can represent arguments with spaces.

The splitting could lead to extra arguments being passed to the core dump
handler that it might have interpreted as options or ignored completely.

Core dump handlers that are not aware of this Linux kernel issue will be
using %e or %E without considering that it may be split and so they will
be vulnerable to processes with spaces in their names breaking their
argument list. If their internals are otherwise well written, such as
if they are written in shell but quote arguments, they will work better
after this change than before. If they are not well written, then there
is a slight chance of breakage depending on the details of the code but
they will already be fairly broken by the split filenames.

Core dump handlers that are aware of this Linux kernel issue will be
placing %e or %E as the last item in their core_pattern and then
aggregating all of the remaining arguments into one, separated by
spaces. Alternatively they will be obtaining the filename via other
methods. Both of these will be compatible with the new arrangement.

A side effect from this change is that unknown template types
(for example %z) result in an empty argument to the dump handler
instead of the argument being dropped. This is a desired change as:

It is easier for dump handlers to process empty arguments than dropped
ones, especially if they are written in shell or don't pass each template
item with a preceding command-line option in order to differentiate
between individual template types. Most core_patterns in the wild do not
use options so they can confuse different template types (especially
numeric ones) if an earlier one gets dropped in old kernels. If the
kernel introduces a new template type and a core_pattern uses it, the
core dump handler might not expect that the argument can be dropped in
old kernels.

For example, this can result in security issues when %d is dropped in old
kernels. This happened with the corekeeper package in Debian and resulted
in the interface between corekeeper and Linux having to be rewritten to
use command-line options to differentiate between template types.

The core_pattern for most core dump handlers is written by the handler
author who would generally not insert unknown template types so this
change should be compatible with all the core dump handlers that exist.

Reported-by: Jakub Wilk <jwilk@jwilk.net>
Reported-in: <20190312145043.jxjoj66kqssptolr@jwilk.net>
Reported-by: Paul Wise <pabs3@bonedaddy.net>
Reported-in: <c8b7ecb8508895bf4adb62a748e2ea2c71854597.camel@bonedaddy.net>
Suggestions-from: Jakub Wilk <jwilk@jwilk.net>
Signed-off-by: Paul Wise <pabs3@bonedaddy.net>
See-also: https://bugs.debian.org/924398
Fixes: commit 74aadce986052f20088c2678f589ea0e8d3a4b59
---
 fs/coredump.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index e42e17e55bfd..c45582b5856c 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -7,6 +7,7 @@
 #include <linux/stat.h>
 #include <linux/fcntl.h>
 #include <linux/swap.h>
+#include <linux/ctype.h>
 #include <linux/string.h>
 #include <linux/init.h>
 #include <linux/pagemap.h>
@@ -187,11 +188,13 @@ static int cn_print_exe_file(struct core_name *cn)
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-static int format_corename(struct core_name *cn, struct coredump_params *cprm)
+static int format_corename(struct core_name *cn, struct coredump_params *cprm,
+			   size_t **argv, int *argc)
 {
 	const struct cred *cred = current_cred();
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
+	bool was_space = false;
 	int pid_in_pattern = 0;
 	int err = 0;
 
@@ -201,12 +204,36 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 		return -ENOMEM;
 	cn->corename[0] = '\0';
 
-	if (ispipe)
+	if (ispipe) {
+		/* sizeof(core_pattern) / 2 is the maximum number of args. */
+		int argvs = sizeof(core_pattern) / 2;
+		(*argv) = kmalloc_array(argvs, sizeof(**argv), GFP_KERNEL);
+		if (!(*argv))
+			return -ENOMEM;
+		(*argv)[(*argc)++] = 0;
 		++pat_ptr;
+	}
 
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
+		/*
+		 * Split on spaces before doing template expansion so that
+		 * %e and %E don't get split if they have spaces in them
+		 */
+		if (ispipe) {
+			if (isspace(*pat_ptr)) {
+				was_space = true;
+				pat_ptr++;
+				continue;
+			} else if (was_space) {
+				was_space = false;
+				err = cn_printf(cn, "%c", '\0');
+				if (err)
+					return err;
+				(*argv)[(*argc)++] = cn->used;
+			}
+		}
 		if (*pat_ptr != '%') {
 			err = cn_printf(cn, "%c", *pat_ptr++);
 		} else {
@@ -546,6 +573,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	struct cred *cred;
 	int retval = 0;
 	int ispipe;
+	size_t *argv = NULL;
+	int argc = 0;
 	struct files_struct *displaced;
 	/* require nonrelative corefile path and be extra careful */
 	bool need_suid_safe = false;
@@ -592,9 +621,10 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 
 	old_cred = override_creds(cred);
 
-	ispipe = format_corename(&cn, &cprm);
+	ispipe = format_corename(&cn, &cprm, &argv, &argc);
 
 	if (ispipe) {
+		int argi;
 		int dump_count;
 		char **helper_argv;
 		struct subprocess_info *sub_info;
@@ -637,12 +667,16 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			goto fail_dropcount;
 		}
 
-		helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
+		helper_argv = kmalloc_array(argc + 1, sizeof(*helper_argv),
+					    GFP_KERNEL);
 		if (!helper_argv) {
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
 			goto fail_dropcount;
 		}
+		for (argi = 0; argi < argc; argi++)
+			helper_argv[argi] = cn.corename + argv[argi];
+		helper_argv[argi] = NULL;
 
 		retval = -ENOMEM;
 		sub_info = call_usermodehelper_setup(helper_argv[0],
@@ -652,7 +686,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			retval = call_usermodehelper_exec(sub_info,
 							  UMH_WAIT_EXEC);
 
-		argv_free(helper_argv);
+		kfree(helper_argv);
 		if (retval) {
 			printk(KERN_INFO "Core dump to |%s pipe failed\n",
 			       cn.corename);
@@ -766,6 +800,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	if (ispipe)
 		atomic_dec(&core_dump_count);
 fail_unlock:
+	kfree(argv);
 	kfree(cn.corename);
 	coredump_finish(mm, core_dumped);
 	revert_creds(old_cred);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3] coredump: Split pipe command whitespace before expanding template
  2019-05-21  0:37     ` Paul Wise
@ 2019-05-28  5:11       ` Paul Wise
  2019-06-06 13:44         ` Neil Horman
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Wise @ 2019-05-28  5:11 UTC (permalink / raw)
  To: Neil Horman, Alexander Viro, linux-fsdevel, linux-kernel, Andrew Morton
  Cc: Paul Wise, Jakub Wilk

Save the offsets of the start of each argument to avoid having to
update pointers to each argument after every corename krealloc and
to avoid having to duplicate the memory for the dump command.

Executable names containing spaces were previously being expanded from
%e or %E and then split in the middle of the filename. This is incorrect
behaviour since an argument list can represent arguments with spaces.

The splitting could lead to extra arguments being passed to the core dump
handler that it might have interpreted as options or ignored completely.

Core dump handlers that are not aware of this Linux kernel issue will be
using %e or %E without considering that it may be split and so they will
be vulnerable to processes with spaces in their names breaking their
argument list. If their internals are otherwise well written, such as
if they are written in shell but quote arguments, they will work better
after this change than before. If they are not well written, then there
is a slight chance of breakage depending on the details of the code but
they will already be fairly broken by the split filenames.

Core dump handlers that are aware of this Linux kernel issue will be
placing %e or %E as the last item in their core_pattern and then
aggregating all of the remaining arguments into one, separated by
spaces. Alternatively they will be obtaining the filename via other
methods. Both of these will be compatible with the new arrangement.

A side effect from this change is that unknown template types
(for example %z) result in an empty argument to the dump handler
instead of the argument being dropped. This is a desired change as:

It is easier for dump handlers to process empty arguments than dropped
ones, especially if they are written in shell or don't pass each template
item with a preceding command-line option in order to differentiate
between individual template types. Most core_patterns in the wild do not
use options so they can confuse different template types (especially
numeric ones) if an earlier one gets dropped in old kernels. If the
kernel introduces a new template type and a core_pattern uses it, the
core dump handler might not expect that the argument can be dropped in
old kernels.

For example, this can result in security issues when %d is dropped in old
kernels. This happened with the corekeeper package in Debian and resulted
in the interface between corekeeper and Linux having to be rewritten to
use command-line options to differentiate between template types.

The core_pattern for most core dump handlers is written by the handler
author who would generally not insert unknown template types so this
change should be compatible with all the core dump handlers that exist.

Fixes: 74aadce98605
Reported-by: Jakub Wilk <jwilk@jwilk.net>
Reported-in: https://bugs.debian.org/924398
Reported-by: Paul Wise <pabs3@bonedaddy.net>
Reported-in: https://lore.kernel.org/linux-fsdevel/c8b7ecb8508895bf4adb62a748e2ea2c71854597.camel@bonedaddy.net/
Suggested-by: Jakub Wilk <jwilk@jwilk.net>
Signed-off-by: Paul Wise <pabs3@bonedaddy.net>
---
 fs/coredump.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

Changelog:
v3 Adjust footer fields, drop obvious comment
v2 Fix build failure due to typo after variable renaming

diff --git a/fs/coredump.c b/fs/coredump.c
index e42e17e55bfd..b1ea7dfbd149 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -7,6 +7,7 @@
 #include <linux/stat.h>
 #include <linux/fcntl.h>
 #include <linux/swap.h>
+#include <linux/ctype.h>
 #include <linux/string.h>
 #include <linux/init.h>
 #include <linux/pagemap.h>
@@ -187,11 +188,13 @@ static int cn_print_exe_file(struct core_name *cn)
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-static int format_corename(struct core_name *cn, struct coredump_params *cprm)
+static int format_corename(struct core_name *cn, struct coredump_params *cprm,
+			   size_t **argv, int *argc)
 {
 	const struct cred *cred = current_cred();
 	const char *pat_ptr = core_pattern;
 	int ispipe = (*pat_ptr == '|');
+	bool was_space = false;
 	int pid_in_pattern = 0;
 	int err = 0;
 
@@ -201,12 +204,35 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 		return -ENOMEM;
 	cn->corename[0] = '\0';
 
-	if (ispipe)
+	if (ispipe) {
+		int argvs = sizeof(core_pattern) / 2;
+		(*argv) = kmalloc_array(argvs, sizeof(**argv), GFP_KERNEL);
+		if (!(*argv))
+			return -ENOMEM;
+		(*argv)[(*argc)++] = 0;
 		++pat_ptr;
+	}
 
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
+		/*
+		 * Split on spaces before doing template expansion so that
+		 * %e and %E don't get split if they have spaces in them
+		 */
+		if (ispipe) {
+			if (isspace(*pat_ptr)) {
+				was_space = true;
+				pat_ptr++;
+				continue;
+			} else if (was_space) {
+				was_space = false;
+				err = cn_printf(cn, "%c", '\0');
+				if (err)
+					return err;
+				(*argv)[(*argc)++] = cn->used;
+			}
+		}
 		if (*pat_ptr != '%') {
 			err = cn_printf(cn, "%c", *pat_ptr++);
 		} else {
@@ -546,6 +572,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	struct cred *cred;
 	int retval = 0;
 	int ispipe;
+	size_t *argv = NULL;
+	int argc = 0;
 	struct files_struct *displaced;
 	/* require nonrelative corefile path and be extra careful */
 	bool need_suid_safe = false;
@@ -592,9 +620,10 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 
 	old_cred = override_creds(cred);
 
-	ispipe = format_corename(&cn, &cprm);
+	ispipe = format_corename(&cn, &cprm, &argv, &argc);
 
 	if (ispipe) {
+		int argi;
 		int dump_count;
 		char **helper_argv;
 		struct subprocess_info *sub_info;
@@ -637,12 +666,16 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			goto fail_dropcount;
 		}
 
-		helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
+		helper_argv = kmalloc_array(argc + 1, sizeof(*helper_argv),
+					    GFP_KERNEL);
 		if (!helper_argv) {
 			printk(KERN_WARNING "%s failed to allocate memory\n",
 			       __func__);
 			goto fail_dropcount;
 		}
+		for (argi = 0; argi < argc; argi++)
+			helper_argv[argi] = cn.corename + argv[argi];
+		helper_argv[argi] = NULL;
 
 		retval = -ENOMEM;
 		sub_info = call_usermodehelper_setup(helper_argv[0],
@@ -652,7 +685,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			retval = call_usermodehelper_exec(sub_info,
 							  UMH_WAIT_EXEC);
 
-		argv_free(helper_argv);
+		kfree(helper_argv);
 		if (retval) {
 			printk(KERN_INFO "Core dump to |%s pipe failed\n",
 			       cn.corename);
@@ -766,6 +799,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	if (ispipe)
 		atomic_dec(&core_dump_count);
 fail_unlock:
+	kfree(argv);
 	kfree(cn.corename);
 	coredump_finish(mm, core_dumped);
 	revert_creds(old_cred);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] coredump: Split pipe command whitespace before expanding template
  2019-05-28  5:11       ` [PATCH v3] " Paul Wise
@ 2019-06-06 13:44         ` Neil Horman
  0 siblings, 0 replies; 6+ messages in thread
From: Neil Horman @ 2019-06-06 13:44 UTC (permalink / raw)
  To: Paul Wise
  Cc: Alexander Viro, linux-fsdevel, linux-kernel, Andrew Morton, Jakub Wilk

On Tue, May 28, 2019 at 01:11:42PM +0800, Paul Wise wrote:
> Save the offsets of the start of each argument to avoid having to
> update pointers to each argument after every corename krealloc and
> to avoid having to duplicate the memory for the dump command.
> 
> Executable names containing spaces were previously being expanded from
> %e or %E and then split in the middle of the filename. This is incorrect
> behaviour since an argument list can represent arguments with spaces.
> 
> The splitting could lead to extra arguments being passed to the core dump
> handler that it might have interpreted as options or ignored completely.
> 
> Core dump handlers that are not aware of this Linux kernel issue will be
> using %e or %E without considering that it may be split and so they will
> be vulnerable to processes with spaces in their names breaking their
> argument list. If their internals are otherwise well written, such as
> if they are written in shell but quote arguments, they will work better
> after this change than before. If they are not well written, then there
> is a slight chance of breakage depending on the details of the code but
> they will already be fairly broken by the split filenames.
> 
> Core dump handlers that are aware of this Linux kernel issue will be
> placing %e or %E as the last item in their core_pattern and then
> aggregating all of the remaining arguments into one, separated by
> spaces. Alternatively they will be obtaining the filename via other
> methods. Both of these will be compatible with the new arrangement.
> 
> A side effect from this change is that unknown template types
> (for example %z) result in an empty argument to the dump handler
> instead of the argument being dropped. This is a desired change as:
> 
> It is easier for dump handlers to process empty arguments than dropped
> ones, especially if they are written in shell or don't pass each template
> item with a preceding command-line option in order to differentiate
> between individual template types. Most core_patterns in the wild do not
> use options so they can confuse different template types (especially
> numeric ones) if an earlier one gets dropped in old kernels. If the
> kernel introduces a new template type and a core_pattern uses it, the
> core dump handler might not expect that the argument can be dropped in
> old kernels.
> 
> For example, this can result in security issues when %d is dropped in old
> kernels. This happened with the corekeeper package in Debian and resulted
> in the interface between corekeeper and Linux having to be rewritten to
> use command-line options to differentiate between template types.
> 
> The core_pattern for most core dump handlers is written by the handler
> author who would generally not insert unknown template types so this
> change should be compatible with all the core dump handlers that exist.
> 
> Fixes: 74aadce98605
> Reported-by: Jakub Wilk <jwilk@jwilk.net>
> Reported-in: https://bugs.debian.org/924398
> Reported-by: Paul Wise <pabs3@bonedaddy.net>
> Reported-in: https://lore.kernel.org/linux-fsdevel/c8b7ecb8508895bf4adb62a748e2ea2c71854597.camel@bonedaddy.net/
> Suggested-by: Jakub Wilk <jwilk@jwilk.net>
> Signed-off-by: Paul Wise <pabs3@bonedaddy.net>
> ---
>  fs/coredump.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> Changelog:
> v3 Adjust footer fields, drop obvious comment
> v2 Fix build failure due to typo after variable renaming
> 
> diff --git a/fs/coredump.c b/fs/coredump.c
> index e42e17e55bfd..b1ea7dfbd149 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -7,6 +7,7 @@
>  #include <linux/stat.h>
>  #include <linux/fcntl.h>
>  #include <linux/swap.h>
> +#include <linux/ctype.h>
>  #include <linux/string.h>
>  #include <linux/init.h>
>  #include <linux/pagemap.h>
> @@ -187,11 +188,13 @@ static int cn_print_exe_file(struct core_name *cn)
>   * name into corename, which must have space for at least
>   * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
>   */
> -static int format_corename(struct core_name *cn, struct coredump_params *cprm)
> +static int format_corename(struct core_name *cn, struct coredump_params *cprm,
> +			   size_t **argv, int *argc)
>  {
>  	const struct cred *cred = current_cred();
>  	const char *pat_ptr = core_pattern;
>  	int ispipe = (*pat_ptr == '|');
> +	bool was_space = false;
>  	int pid_in_pattern = 0;
>  	int err = 0;
>  
> @@ -201,12 +204,35 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  		return -ENOMEM;
>  	cn->corename[0] = '\0';
>  
> -	if (ispipe)
> +	if (ispipe) {
> +		int argvs = sizeof(core_pattern) / 2;
> +		(*argv) = kmalloc_array(argvs, sizeof(**argv), GFP_KERNEL);
> +		if (!(*argv))
> +			return -ENOMEM;
> +		(*argv)[(*argc)++] = 0;
>  		++pat_ptr;
> +	}
>  
>  	/* Repeat as long as we have more pattern to process and more output
>  	   space */
>  	while (*pat_ptr) {
> +		/*
> +		 * Split on spaces before doing template expansion so that
> +		 * %e and %E don't get split if they have spaces in them
> +		 */
> +		if (ispipe) {
> +			if (isspace(*pat_ptr)) {
> +				was_space = true;
> +				pat_ptr++;
> +				continue;
> +			} else if (was_space) {
> +				was_space = false;
> +				err = cn_printf(cn, "%c", '\0');
> +				if (err)
> +					return err;
> +				(*argv)[(*argc)++] = cn->used;
> +			}
> +		}
>  		if (*pat_ptr != '%') {
>  			err = cn_printf(cn, "%c", *pat_ptr++);
>  		} else {
> @@ -546,6 +572,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  	struct cred *cred;
>  	int retval = 0;
>  	int ispipe;
> +	size_t *argv = NULL;
> +	int argc = 0;
>  	struct files_struct *displaced;
>  	/* require nonrelative corefile path and be extra careful */
>  	bool need_suid_safe = false;
> @@ -592,9 +620,10 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  
>  	old_cred = override_creds(cred);
>  
> -	ispipe = format_corename(&cn, &cprm);
> +	ispipe = format_corename(&cn, &cprm, &argv, &argc);
>  
>  	if (ispipe) {
> +		int argi;
>  		int dump_count;
>  		char **helper_argv;
>  		struct subprocess_info *sub_info;
> @@ -637,12 +666,16 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  			goto fail_dropcount;
>  		}
>  
> -		helper_argv = argv_split(GFP_KERNEL, cn.corename, NULL);
> +		helper_argv = kmalloc_array(argc + 1, sizeof(*helper_argv),
> +					    GFP_KERNEL);
>  		if (!helper_argv) {
>  			printk(KERN_WARNING "%s failed to allocate memory\n",
>  			       __func__);
>  			goto fail_dropcount;
>  		}
> +		for (argi = 0; argi < argc; argi++)
> +			helper_argv[argi] = cn.corename + argv[argi];
> +		helper_argv[argi] = NULL;
>  
>  		retval = -ENOMEM;
>  		sub_info = call_usermodehelper_setup(helper_argv[0],
> @@ -652,7 +685,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  			retval = call_usermodehelper_exec(sub_info,
>  							  UMH_WAIT_EXEC);
>  
> -		argv_free(helper_argv);
> +		kfree(helper_argv);
>  		if (retval) {
>  			printk(KERN_INFO "Core dump to |%s pipe failed\n",
>  			       cn.corename);
> @@ -766,6 +799,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  	if (ispipe)
>  		atomic_dec(&core_dump_count);
>  fail_unlock:
> +	kfree(argv);
>  	kfree(cn.corename);
>  	coredump_finish(mm, core_dumped);
>  	revert_creds(old_cred);
> -- 
> 2.20.1
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-06-06 13:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-19  6:06 PROBLEM: Linux kernel.core_pattern with pipes does argument splitting after template expansion Paul Wise
2019-05-20  9:01 ` [PATCH] coredump: Split pipe command whitespace before expanding template Paul Wise
2019-05-20 15:14   ` kbuild test robot
2019-05-21  0:37     ` Paul Wise
2019-05-28  5:11       ` [PATCH v3] " Paul Wise
2019-06-06 13:44         ` Neil Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).