From 248a67fb75a0d2c98f4f9935b7bb9e11382b2c78 Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 7 Aug 2017 18:18:09 +0200 Subject: [PATCH] free,stat: make NOEXEC pkill/pgrep/pidof uncovered another quirk: what about noexec's _process names_? Signed-off-by: Denys Vlasenko --- NOFORK_NOEXEC.lst | 18 ++++++++++-------- coreutils/stat.c | 2 +- libbb/vfork_daemon_rexec.c | 2 ++ procps/free.c | 7 +++++-- procps/pgrep.c | 6 +++++- procps/pidof.c | 4 ++++ shell/ash.c | 2 ++ shell/hush.c | 2 ++ 8 files changed, 31 insertions(+), 12 deletions(-) diff --git a/NOFORK_NOEXEC.lst b/NOFORK_NOEXEC.lst index 70f38d867..8ec3bdbe6 100644 --- a/NOFORK_NOEXEC.lst +++ b/NOFORK_NOEXEC.lst @@ -16,6 +16,8 @@ leak categories. Why can't be NOEXEC: suid: runs under different uid - must fork+exec +if it's important that /proc/PID/cmdline and comm are correct. + ("pkill sh" killing itself before it kills real "sh" is no fun) Why shouldn't be NOFORK/NOEXEC: rare: not started often enough to bother optimizing (example: poweroff) @@ -131,7 +133,7 @@ flash_unlock - hardware flashcp - hardware flock - spawner, changes state (file locks), let's play safe and not be noexec fold - noexec. runner -free - nofork candidate(struct globals, needs to close /proc/meminfo fd) +free - noexec. nofork candidate(struct globals, needs to close /proc/meminfo fd) freeramdisk - leaks: open+ioctl_or_perror_and_die fsck - interactive, longterm fsck.minix - needs ^C @@ -172,7 +174,7 @@ inotifyd - daemon insmod - noexec install - runner ionice - noexec. spawner -iostat - runner +iostat - longterm: "iostat 1" runs indefinitely ip - noexec candidate ipaddr - noexec candidate ipcalc - noexec candidate @@ -244,7 +246,7 @@ mv - noexec candidate, runner nameif - noexec. openlog(), leaks: config_open2+ioctl_or_perror_and_die nbd-client - noexec nc - runner -netstat - runner with -c +netstat - longterm with -c (continuous listing) nice - noexec. spawner nl - runner nmeter - longterm @@ -257,13 +259,13 @@ partprobe - noexec. leaks: open+ioctl_or_perror_and_die(BLKRRPART) passwd - suid paste - noexec. runner patch - needs ^C -pgrep - nofork candidate(xregcomp, procps_scan - are they ok?) -pidof - nofork candidate(uses find_pid_by_name, is that ok?) +pgrep - must fork+exec to get correct /proc/PID/cmdline and comm field +pidof - must fork+exec to get correct /proc/PID/cmdline and comm field ping - suid, longterm ping6 - suid, longterm pipe_progress - longterm pivot_root - NOFORK -pkill - nofork candidate(xregcomp, procps_scan - are they ok?) +pkill - must fork+exec to get correct /proc/PID/cmdline and comm field pmap - noexec candidate, leaks: open+xstrdup popmaildir - runner poweroff - rare @@ -329,7 +331,7 @@ sort - noexec. runner split - runner ssl_client - longterm start-stop-daemon - not noexec: uses bb_common_bufsiz1 -stat - nofork candidate(needs fewer allocs) +stat - noexec. nofork candidate(needs fewer allocs) strings - runner stty - noexec. nofork candidate: has no allocs or opens except xmove_fd(xopen("-F DEVICE"),STDIN). tcsetattr(STDIN) is not a problem: it would work the same across processes sharing this fd su - suid, spawner @@ -338,7 +340,7 @@ sum - runner sv - noexec. needs ^C (uses usleep(420000)) svc - noexec. needs ^C (uses usleep(420000)) svlogd - daemon -swapoff - rare +swapoff - longterm: may cause memory pressure, execing is beneficial swapon - rare switch_root - spawner, rare, changes state (oh yes), execing may be important to free binary's inode sync - NOFORK diff --git a/coreutils/stat.c b/coreutils/stat.c index 3b85808b5..4e926a908 100644 --- a/coreutils/stat.c +++ b/coreutils/stat.c @@ -36,7 +36,7 @@ //config: Without this, stat will not support the '-f' option to display //config: information about filesystem status. -//applet:IF_STAT(APPLET(stat, BB_DIR_BIN, BB_SUID_DROP)) +//applet:IF_STAT(APPLET_NOEXEC(stat, stat, BB_DIR_BIN, BB_SUID_DROP, stat)) //kbuild:lib-$(CONFIG_STAT) += stat.o diff --git a/libbb/vfork_daemon_rexec.c b/libbb/vfork_daemon_rexec.c index f84e678b5..50ecea762 100644 --- a/libbb/vfork_daemon_rexec.c +++ b/libbb/vfork_daemon_rexec.c @@ -175,6 +175,8 @@ int FAST_FUNC spawn_and_wait(char **argv) return wait4pid(rc); /* child */ +//TODO: prctl(PR_SET_NAME, (long)argv[0], 0, 0, 0);? [think pidof, pgrep, pkill] +//Rewrite /proc/PID/cmdline? (need to save argv0 and length at init for this to work!) /* reset some state and run without execing */ /* msg_eol = "\n"; - no caller needs this reinited yet */ diff --git a/procps/free.c b/procps/free.c index 618664e08..b57e4a322 100644 --- a/procps/free.c +++ b/procps/free.c @@ -15,7 +15,7 @@ //config: memory in the system, as well as the buffers used by the kernel. //config: The shared memory column should be ignored; it is obsolete. -//applet:IF_FREE(APPLET(free, BB_DIR_USR_BIN, BB_SUID_DROP)) +//applet:IF_FREE(APPLET_NOEXEC(free, free, BB_DIR_USR_BIN, BB_SUID_DROP, free)) //kbuild:lib-$(CONFIG_FREE) += free.o @@ -47,7 +47,10 @@ struct globals { #endif } FIX_ALIASING; #define G (*(struct globals*)bb_common_bufsiz1) -#define INIT_G() do { setup_common_bufsiz(); } while (0) +#define INIT_G() do { \ + setup_common_bufsiz(); \ + /* NB: noexec applet - globals not zeroed */ \ +} while (0) static unsigned long long scale(unsigned long d) diff --git a/procps/pgrep.c b/procps/pgrep.c index a3ca9e295..a16a6e959 100644 --- a/procps/pgrep.c +++ b/procps/pgrep.c @@ -18,9 +18,13 @@ //config: help //config: Send signals to processes by name. -//applet:IF_PGREP(APPLET(pgrep, BB_DIR_USR_BIN, BB_SUID_DROP)) +//applet:IF_PGREP(APPLET_ODDNAME(pgrep, pgrep, BB_DIR_USR_BIN, BB_SUID_DROP, pgrep)) // APPLET_ODDNAME:name main location suid_type help //applet:IF_PKILL(APPLET_ODDNAME(pkill, pgrep, BB_DIR_USR_BIN, BB_SUID_DROP, pkill)) +/* can't be noexec: can find _itself_ under wrong name, since after fork only, + * /proc/PID/cmdline and comm are wrong! Can fix comm (prctl(PR_SET_NAME)), + * but cmdline? + */ //kbuild:lib-$(CONFIG_PGREP) += pgrep.o //kbuild:lib-$(CONFIG_PKILL) += pgrep.o diff --git a/procps/pidof.c b/procps/pidof.c index 41247a02c..98d7949f8 100644 --- a/procps/pidof.c +++ b/procps/pidof.c @@ -30,6 +30,10 @@ //config: of the pidof, in other words the calling shell or shell script. //applet:IF_PIDOF(APPLET(pidof, BB_DIR_BIN, BB_SUID_DROP)) +/* can't be noexec: can find _itself_ under wrong name, since after fork only, + * /proc/PID/cmdline and comm are wrong! Can fix comm (prctl(PR_SET_NAME)), + * but cmdline? + */ //kbuild:lib-$(CONFIG_PIDOF) += pidof.o diff --git a/shell/ash.c b/shell/ash.c index e8f3ed26b..0a323e957 100644 --- a/shell/ash.c +++ b/shell/ash.c @@ -7803,6 +7803,8 @@ tryexec(IF_FEATURE_SH_STANDALONE(int applet_no,) const char *cmd, char **argv, c while (*envp) putenv(*envp++); popredir(/*drop:*/ 1); +//TODO: prctl(PR_SET_NAME, (long)argv[0], 0, 0, 0);? [think pidof, pgrep, pkill] +//Rewrite /proc/PID/cmdline? (need to save argv0 and length at init for this to work!) run_applet_no_and_exit(applet_no, cmd, argv); } /* re-exec ourselves with the new arguments */ diff --git a/shell/hush.c b/shell/hush.c index bb80f422c..b4fe7146b 100644 --- a/shell/hush.c +++ b/shell/hush.c @@ -7387,6 +7387,8 @@ static NOINLINE void pseudo_exec_argv(nommu_save_t *nommu_save, /* Without this, "rm -i FILE" can't be ^C'ed: */ switch_off_special_sigs(G.special_sig_mask); debug_printf_exec("running applet '%s'\n", argv[0]); +//TODO: prctl(PR_SET_NAME, (long)argv[0], 0, 0, 0);? [think pidof, pgrep, pkill] +//Rewrite /proc/PID/cmdline? (need to save argv0 and length at init for this to work!) run_applet_no_and_exit(a, argv[0], argv); } # endif -- 2.25.1