[PATCH] implement privmode support in dash

* [PATCH] implement privmode support in dash
@ 2013-08-22 17:59 Tavis Ormandy
  2013-08-22 19:59 ` Harald van Dijk
  2013-08-23  3:31 ` [oss-security] " Kurt Seifried
  0 siblings, 2 replies; 11+ messages in thread
From: Tavis Ormandy @ 2013-08-22 17:59 UTC (permalink / raw)
  To: dash; +Cc: oss-security

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.

Signed-off-by: Tavis Ormandy <taviso@google.com>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Open Source Security <oss-security@lists.openwall.com>

---

 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(-)

diff --git a/src/dash.1 b/src/dash.1
index a9cb491..94fc642 100644
--- a/src/dash.1
+++ b/src/dash.1
@@ -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
diff --git a/src/main.c b/src/main.c
index f79ad7d..7ca247d 100644
--- a/src/main.c
+++ b/src/main.c
@@ -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");
diff --git a/src/options.c b/src/options.c
index 6f381e6..99ac55b 100644
--- a/src/options.c
+++ b/src/options.c
@@ -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,
 };
diff --git a/src/options.h b/src/options.h
index 975fe33..9e90947 100644
--- a/src/options.h
+++ b/src/options.h
@@ -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];
diff --git a/src/var.c b/src/var.c
index dc90249..d04c4d9 100644
--- a/src/var.c
+++ b/src/var.c
@@ -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;
+	}
 }
 
 /*
diff --git a/src/var.h b/src/var.h
index 79ee71a..bb2175b 100644
--- a/src/var.h
+++ b/src/var.h
@@ -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);

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