implement privmode support in dash

Hello, this is a patch to add privmode support to dash. privmode attempts to drop privileges by default if the effective uid does not match the uid. This can be disabled with -p, or -o nopriv.

This matches the behaviour of bash since version 2.0 (released around 1996, see section 7 of the bash NOTES file at http://tiswww.case.edu/php/chet/bash/NOTES),
and pdksh since version 5.0.5 (which provides /bin/sh on many non-Linux systems). The reason this is useful is that improper or incorrect usage of system(3) or popen(3) in setuid root executables when dash is installed as /bin/sh is a very common security problem.

For example, here is one I just found in vmware-tools that manages to call popen("lsb_release") with effective uid zero:

$ cc -xc - -olsb_release<<<'main(){system("sh>`tty` 2>&1");}';PATH=.:$PATH vmware-mount
# whoami
root

I believe errors like this are generally *not* exploitable specifically because of privmode in bash and pdksh, with the exception of Linux distributions derived from Debian.

I realise changing default behaviour might be controversial for a project like dash, but as most (all?) non-Debian derived distributions use bash as /bin/sh and bash has behaved like this for nearly a decade, I expect the impact to be minimal. The benefit of this exploit mitigation should pay for itself within a few years of dodged root-exploit bullets.

You can see in Chet Ramey's notes that only two real programs have been documented to be affected over the last 10 years:

dip (A program for SLIP/PPP, as far as I know it is not packaged by Debian)
rbsmtp (A batch mailer program used with UUCP, AFAICT no longer packaged
but it was at some point)

Please note that although these bugs were "fixed" in Debian by removing privmode in bash, that actually made this a security issue and users were able to escalate privileges - in my opinion, a much worse outcome than the package not working properly.

In most cases, if a package does break with this change - then it was also possible to use that package to become root without permission, and it hasnt worked on any distribution or UNIX other than Debian for nearly 10 years.

It really is almost impossible to use system(3) or popen(3) safely in setuid programs, and privmode can at least help prevent turning those errors into root shells.

Here is a related blog post on the topic http://blog.cmpxchg8b.com/2013/08/security-debianisms.html

If you care about tracking vulnerabilities, the vmware issue is called CVE-2013-1662.

---

 src/dash.1    | 16 ++++++++++------
 src/main.c    | 17 +++++++++++++++++
 src/options.c |  2 ++
 src/options.h |  7 ++++---
 src/var.c     | 29 +++++++++++++++++++++++------
 src/var.h     |  1 +
 6 files changed, 57 insertions(+), 15 deletions(-)

index a9cb491..94fc642 100644
@@ -41,8 +41,8 @@
 .Sh SYNOPSIS
 .Nm
 .Bk -words
-.Op Fl aCefnuvxIimqVEb
-.Op Cm +aCefnuvxIimqVEb
+.Op Fl aCefnuvxIimqVEbp
+.Op Cm +aCefnuvxIimqVEbp
 .Ek
 .Bk -words
 .Op Fl o Ar option_name

@@ -54,8 +54,8 @@
 .Nm
 .Fl c
 .Bk -words
-.Op Fl aCefnuvxIimqVEb
-.Op Cm +aCefnuvxIimqVEb
+.Op Fl aCefnuvxIimqVEbp
+.Op Cm +aCefnuvxIimqVEbp
 .Ek
 .Bk -words
 .Op Fl o Ar option_name

@@ -68,8 +68,8 @@
 .Nm
 .Fl s
 .Bk -words
-.Op Fl aCefnuvxIimqVEb
-.Op Cm +aCefnuvxIimqVEb
+.Op Fl aCefnuvxIimqVEbp
+.Op Cm +aCefnuvxIimqVEbp
 .Ek
 .Bk -words
 .Op Fl o Ar option_name

@@ -257,6 +257,10 @@ if it has been set).
 .It Fl b Em notify
 Enable asynchronous notification of background job completion.
 (UNIMPLEMENTED for 4.4alpha)
+.It Fl p Em nopriv
+Do not attempt to reset effective uid if it does not match uid. This is not set
+by default to help avoid incorrect usage by setuid root programs via system(3) or
+popen(3).
 .El
 .Ss Lexical Structure
 The shell reads input in terms of lines from a file and breaks it up into

index f79ad7d..7ca247d 100644
@@ -97,11 +97,16 @@ main(int argc, char **argv)
 	struct jmploc jmploc;
 	struct stackmark smark;
 	int login;
+	uid_t uid;
+	gid_t gid;
 
 #ifdef __GLIBC__
 	dash_errno = __errno_location();
 #endif
 
+	uid = getuid();
+	gid = getgid();
+
 #if PROFILE
 	monitor(4, etext, profile_buf, sizeof profile_buf, 50);
 #endif

@@ -148,6 +153,18 @@ main(int argc, char **argv)
 	init();
 	setstackmark(&smark);
 	login = procargs(argc, argv);
+
+	/*
+	 * To limit bogus system(3) or popen(3) calls in setuid binaries, require
+	 * -p flag to work in this situation.
+	 */
+	if (!pflag && (uid != geteuid() || gid != getegid())) {
+		setuid(uid);
+		setgid(gid);
+		/* PS1 might need to be changed accordingly. */
+		choose_ps1();
+	}
+
 	if (login) {
 		state = 1;
 		read_profile("/etc/profile");

index 6f381e6..99ac55b 100644
@@ -79,6 +79,7 @@ static const char *const optnames[NOPTS] = {
 	"allexport",
 	"notify",
 	"nounset",
+	"nopriv",
 	"nolog",
 	"debug",
 };

@@ -99,6 +100,7 @@ const char optletters[NOPTS] = {
 	'a',
 	'b',
 	'u',
+	'p',
 	0,
 	0,
 };

index 975fe33..9e90947 100644
@@ -59,10 +59,11 @@ struct shparam {
 #define	aflag optlist[12]
 #define	bflag optlist[13]
 #define	uflag optlist[14]
-#define	nolog optlist[15]
-#define	debug optlist[16]
+#define	pflag optlist[15]
+#define	nolog optlist[16]
+#define	debug optlist[17]
 
-#define NOPTS	17
+#define NOPTS	18
 
 extern const char optletters[NOPTS];
 extern char optlist[NOPTS];

index dc90249..d04c4d9 100644
@@ -81,6 +81,9 @@ const char defifsvar[] = "IFS= \t\n";
 const char defifs[] = " \t\n";
 #endif
 
+const char defps1[] = "PS1=$ ";
+const char rootps1[] = "PS1=# ";
+
 int lineno;
 char linenovar[sizeof("LINENO=")+sizeof(int)*CHAR_BIT/3+1] = "LINENO=";
 

@@ -97,7 +100,7 @@ struct var varinit[] = {
 	{ 0,	VSTRFIXED|VTEXTFIXED|VUNSET,	"MAIL\0",	changemail },
 	{ 0,	VSTRFIXED|VTEXTFIXED|VUNSET,	"MAILPATH\0",	changemail },
 	{ 0,	VSTRFIXED|VTEXTFIXED,		defpathvar,	changepath },
-	{ 0,	VSTRFIXED|VTEXTFIXED,		"PS1=$ ",	0 },
+	{ 0,	VSTRFIXED|VTEXTFIXED,		defps1,	0 },
 	{ 0,	VSTRFIXED|VTEXTFIXED,		"PS2=> ",	0 },
 	{ 0,	VSTRFIXED|VTEXTFIXED,		"PS4=+ ",	0 },
 	{ 0,	VSTRFIXED|VTEXTFIXED,		"OPTIND=1",	getoptsreset },

@@ -178,11 +181,25 @@ initvar(void)
 		vp->next = *vpp;
 		*vpp = vp;
 	} while (++vp < end);
-	/*
-	 * PS1 depends on uid
-	 */
-	if (!geteuid())
-		vps1.text = "PS1=# ";
+
+	choose_ps1();
+}
+
+/*
+ * Modify PS1 to reflect uid, unless an exported var exists.
+ */
+
+void
+choose_ps1(void)
+{
+	if (vps1.flags & VEXPORT)
+		return;
+
+	if (!geteuid()) {
+		vps1.text = rootps1;
+	} else {
+		vps1.text = defps1;
+	}
 }
 
 /*

index 79ee71a..bb2175b 100644
@@ -139,6 +139,7 @@ extern char linenovar[];
 #define mpathset()	((vmpath.flags & VUNSET) == 0)
 
 void initvar(void);
+void choose_ps1(void);
 struct var *setvar(const char *name, const char *val, int flags);
 intmax_t setvarint(const char *, intmax_t, int);
 struct var *setvareq(char *s, int flags);

Hi Tavis,

Your approach definitely has my support (FWTW), but there are two aspects that surprised me, and are different from bash and FreeBSD's sh:

You named the option nopriv, while bash and FBSD use the name privileged. I think it is likely to confuse people if "bash -o privileged" and "dash -o nopriv" do the same thing, and that it would be better to match bash and give the option a positive name, such as "priv", or perhaps even match them exactly and use "privileged".

In bash and FBSD, after starting with -p, set +p can be used to drop privileges. With your patch, dash accepts set +p, but silently ignores it.

How does something like the attached, to be applied on top of your patch, look?

Cheers,
Harald

index 2a37381..82d00fb 100644
@@ -21,14 +21,14 @@ bin_PROGRAMS = dash
 dash_CFILES = \
 	alias.c arith_yacc.c arith_yylex.c cd.c error.c eval.c exec.c expand.c \
 	histedit.c input.c jobs.c mail.c main.c memalloc.c miscbltin.c \
-	mystring.c options.c parser.c redir.c show.c trap.c output.c \
+	mystring.c options.c parser.c priv.c redir.c show.c trap.c output.c \
 	bltin/printf.c system.c bltin/test.c bltin/times.c var.c
 dash_SOURCES = \
 	$(dash_CFILES) \
 	alias.h arith_yacc.h bltin/bltin.h cd.h error.h eval.h exec.h \
 	expand.h hetio.h \
 	init.h input.h jobs.h machdep.h mail.h main.h memalloc.h miscbltin.h \
-	myhistedit.h mystring.h options.h output.h parser.h redir.h shell.h \
+	myhistedit.h mystring.h options.h output.h parser.h priv.h redir.h shell.h \
 	show.h system.h trap.h var.h
 dash_LDADD = builtins.o init.o nodes.o signames.o syntax.o
 

index 94fc642..0f35748 100644
@@ -257,7 +257,7 @@ if it has been set).
 .It Fl b Em notify
 Enable asynchronous notification of background job completion.
 (UNIMPLEMENTED for 4.4alpha)
-.It Fl p Em nopriv
+.It Fl p Em priv
 Do not attempt to reset effective uid if it does not match uid. This is not set
 by default to help avoid incorrect usage by setuid root programs via system(3) or
 popen(3).

index e106848..69e5e07 100644
@@ -50,6 +50,7 @@
 #include "eval.h"
 #include "jobs.h"
 #include "input.h"
+#include "priv.h"
 #include "trap.h"
 #include "var.h"
 #include "show.h"

@@ -97,8 +98,6 @@ main(int argc, char **argv)
 	struct jmploc jmploc;
 	struct stackmark smark;
 	int login;
-	uid_t uid;
-	gid_t gid;
 
 #ifdef __GLIBC__
 	dash_errno = __errno_location();

@@ -154,17 +153,6 @@ main(int argc, char **argv)
 	setstackmark(&smark);
 	login = procargs(argc, argv);
 
-	/*
-	 * To limit bogus system(3) or popen(3) calls in setuid binaries, require
-	 * -p flag to work in this situation.
-	 */
-	if (!pflag && (uid != geteuid() || gid != getegid())) {
-		setuid(uid);
-		setgid(gid);
-		/* PS1 might need to be changed accordingly. */
-		choose_ps1();
-	}
-
 	if (login) {
 		state = 1;
 		read_profile("/etc/profile");

index 99ac55b..c640a41 100644
@@ -45,6 +45,7 @@
 #include "jobs.h"
 #include "input.h"
 #include "output.h"
+#include "priv.h"
 #include "trap.h"
 #include "var.h"
 #include "memalloc.h"

@@ -79,7 +80,7 @@ static const char *const optnames[NOPTS] = {
 	"allexport",
 	"notify",
 	"nounset",
-	"nopriv",
+	"priv",
 	"nolog",
 	"debug",
 };

@@ -184,6 +185,7 @@ optschanged(void)
 #ifdef DEBUG
 	opentrace();
 #endif
+	setprivileged(pflag);
 	setinteractive(iflag);
 #ifndef SMALL
 	histedit();

new file mode 100644
index 0000000..26346c0
@@ -0,0 +1,27 @@
+#include <unistd.h>
+
+#include "priv.h"
+#include "var.h"
+
+uid_t uid;
+gid_t gid;
+
+void setprivileged(int on)
+{
+	static int is_privileged = 1;
+	if (is_privileged == on)
+		return;
+
+	is_privileged = on;
+
+	/*
+	 * To limit bogus system(3) or popen(3) calls in setuid binaries, require
+	 * -p flag to work in this situation.
+	 */
+	if (!on && (uid != geteuid() || gid != getegid())) {
+		setuid(uid);
+		setgid(gid);
+		/* PS1 might need to be changed accordingly. */
+		choose_ps1();
+	}
+}

new file mode 100644
index 0000000..31b380b
@@ -0,0 +1,6 @@
+#include <unistd.h>
+
+extern uid_t uid;
+extern gid_t gid;
+
+void setprivileged(int on);

index cafe407..277777f 100644
@@ -192,13 +192,12 @@ initvar(void)
 void
 choose_ps1(void)
 {
-	if (vps1.flags & VEXPORT)
-		return;

Thanks Harald, those changes make sense to me.

Tavis.

I think there is no reason to deviate from other shells here. Therefore, please call it "privileged".

Jilles Tjoelker

Agreed.

This is logic duplicated from pdksh and bash, I'm slightly reluctant to do things differently, unless it's not going to get committed otherwise.

You can see some code snippets here:
http://blog.cmpxchg8b.com/2013/08/security-debianisms.html

I don't think this is true anymore, but I have no strong objection to adding it, so long as it's noted that bash and pdksh do not do this.

Tavis.

pdksh is only maintained by OpenBSD, afaik (mksh syncs regularly). The current code rather looks like this:

	if (f == FPRIVILEGED && oldval && !newval) {
		gid_t gid = getgid();

		setresgid(gid, gid, gid);
		setgroups(1, &gid);
		setresuid(ksheuid, ksheuid, ksheuid);
	} ...

jca | PGP: 0x06A11494 / 61DB D9A0 00A4 67CF 2A90  8961 6191 8FBF 06A1 1494

Also,

Tavis Ormandy <[email protected]> writes:

[...]

Just for reference, from mksh:

[...]

#ifdef SETUID_CAN_FAIL_WITH_EAGAIN
/* we don't need to check for other codes, EPERM won't happen */

#define DO_SETUID(func, argvec) do {					\
	if ((func argvec) && errno == EAGAIN)				\
		errorf("%s failed with EAGAIN, probably due to a"	\
		    " too low process limit; aborting", #func);		\
} while (/* CONSTCOND */ 0)

#else
#define DO_SETUID(func, argvec) func argvec
#endif

[...]

	  if (f == FPRIVILEGED && oldval && !newval) {
		/* Turning off -p? */

		/*XXX this can probably be optimised */
		kshegid = kshgid = getgid();
#if HAVE_SETRESUGID
		DO_SETUID(setresgid, (kshegid, kshegid, kshegid));
#if HAVE_SETGROUPS
		/* setgroups doesn't EAGAIN on Linux */
		setgroups(1, &kshegid);
#endif
		DO_SETUID(setresuid, (ksheuid, ksheuid, ksheuid));
#else
		/* seteuid, setegid, setgid don't EAGAIN on Linux */
		ksheuid = kshuid = getuid();
#ifndef MKSH__NO_SETEUGID
		seteuid(ksheuid);
#endif
		DO_SETUID(setuid, (ksheuid));
#ifndef MKSH__NO_SETEUGID
		setegid(kshegid);
#endif
		setgid(kshegid);
#endif
	} [...]

jca | PGP: 0x06A11494 / 61DB D9A0 00A4 67CF 2A90  8961 6191 8FBF 06A1 1494

[snip]

BTW it is just changed in cvs. Log message:

Commit ID: 10052176CB912FE954B
CVSROOT: /cvs
Module name: src
Changes by: [email protected] 2013/08/23 14:07:41
UTC

Modified files:
distrib/special/mksh: Makefile
bin/mksh : Build.sh Makefile check.t misc.c mksh.1 sh.h

Log message:
SECURITY: Unbreak “set +p”, broken by OpenBSD ksh change.

TODO: I am seriously considering following Chet and changing the way this works, by explicitly dropping privs unless the shell is run with -p. Every other shell does it like mksh, except Heirloom sh, which on the other hand doesn’t know any explicit set -p or set +p (though it doesn’t know set +foo for any foo either).

┌──┤ QUESTION: Do we need the ability to do this:
│ tg@blau:~ $ ./suidmksh -p -c 'whoami; set +p; whoami'
│ root
│ tg

If not, I’m seriously considering to drop set ±p as well, only parse -p on the command line, with +p being the default, and dropping FPRIVILEGED.

Thanks to RT for noticing and jilles for initial follow-up discussion, as well as Chet Ramey for doing the sane/secure thing instead of following Debian.

To generate a diff of this changeset, execute the following commands:
cvs -R rdiff -kk -upr1.71 -r1.72 src/distrib/special/mksh/Makefile cvs -R rdiff -kk -upr1.645 -r1.646 src/bin/mksh/Build.sh cvs -R rdiff -kk -upr1.124 -r1.125 src/bin/mksh/Makefile cvs -R rdiff -kk -upr1.630 -r1.631 src/bin/mksh/check.t cvs -R rdiff -kk -upr1.214 -r1.215 src/bin/mksh/misc.c
cvs -R rdiff -kk -upr1.320 -r1.321 src/bin/mksh/mksh.1
cvs -R rdiff -kk -upr1.668 -r1.669 src/bin/mksh/sh.h

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Do we need one for Debian as well? Seems like a strong maybe.

- --
Kurt Seifried Red Hat Security Response Team (SRT)
PGP: 0x5E267993 A90B F995 7350 148F 66BF 7554 160D 4553 5E26 7993 -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)

iQIcBAEBAgAGBQJSFtd3AAoJEBYNRVNeJnmTjWIQAI2N8A7Sej0z8VBHIqBewpR9 cmonEsKtutVAhAm08umBhYvyaaiSXB9dOazpFHttUhGGt6YdJIVlaXWuDTsX1JAp XdLPSzTwgxuGQ3mPwr22fwYTWfqutI+hiCd364kFpkecBKkHUu/5wWXQJBf7O7D1 J9i93jX1LJIN2zGJs3U1NqwClybP4rVEsEY0JHcZ/6ouZB8iQx0IKV8WvKELV7k/ MEnH9jXj1iflRECtX3y4lEyM1QKxi1ZsI88HSXsb6WSwVlEmBj/iMkUAzkkdddqc +bS7OFknUuf1cQVfpMaH3RLoRr2NUMaIK5l9XjZF4OZJSYrXZfM6qnC5vyZaLPGl eSmDz1HvjC07Ow/hXqVx1jYnPr4YvndtZNdQ76NYvt4m/RVAJHdLf2dHjEWhwZGm oYIXDrFFnmzlF/zI2NYthTRUV33Toae/GjBhkaZ5nDqUOD0uiWQnNhDCM/e3IEEi 2xgxXjmuHI9qlNjfYDySi6AKhHehMYbfB9g9SrBIrEunF/DoaOTgavdLjdINSYDD fLIapXhDGsiafJeB4WauMEHLF4SksXiix/5dE99gnqaOdXy3t2XZgZk80SheQQxq alhGRqSZv8hw1AJ4mFmfHFd4BZhCiP+4JGVzK1WNdzBinclCXuji/6fg5DzEXYrW wEF2wooBieoW6pbji8NZ
=qGEM
-----END PGP SIGNATURE-----

I think it would be a good idea, it seems similar to something like CVE-2009-2695 which was a mitigation being disabled.

Tavis.

Do you mean dash instead of Debian?

Florian Weimer / Red Hat Product Security Team