* 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 related [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 related [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 related [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).