Re: [PATCH] implement privmode support in dash

2013-08-23 Thread Jérémie Courrèges-Anglas 
Tavis Ormandy tav...@google.com writes:

 On Thu, Aug 22, 2013 at 1:35 PM, Jilles Tjoelker jil...@stack.nl wrote:
 I think there is no reason to deviate from other shells here. Therefore,
 please call it privileged.


 Agreed.

 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?

 [snip]
 + if (!on  (uid != geteuid() || gid != getegid())) {
 + setuid(uid);
 + setgid(gid);
 + /* PS1 might need to be changed accordingly. */
 + choose_ps1();
 + }
 +}

 This code tries to use setuid() and setgid() to drop all privilege,
 which is only correct if the privilege to be dropped is UID 0, or on BSD
 systems. It would be better to use setresuid() or setreuid(), and change
 the GID before changing the UID.

 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.

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);
} ...

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

 Apart from that, it is better to check the return value from setuid()
 and similar functions. In particular, some versions of Linux may fail
 setuid() for [EAGAIN], leaving the process running with the same
 privileges.

 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.

-- 
jca | PGP: 0x06A11494 / 61DB D9A0 00A4 67CF 2A90  8961 6191 8FBF 06A1 1494
--
To unsubscribe from this list: send the line unsubscribe dash in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] implement privmode support in dash

2013-08-23 Thread Roy 
On Fri, 23 Aug 2013 19:40:31 +0800, Jérémie Courrèges-Anglas  
jca+d...@wxcvbn.org wrote:




Also,

Tavis Ormandy tav...@google.com writes:

[...]


Apart from that, it is better to check the return value from setuid()
and similar functions. In particular, some versions of Linux may fail
setuid() for [EAGAIN], leaving the process running with the same
privileges.


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.


Just for reference, from mksh:



[snip]

BTW it is just changed in cvs. Log message:

Commit ID:  10052176CB912FE954B
CVSROOT:/cvs
Module name:src
Changes by: t...@herc.mirbsd.org2013/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

--
To unsubscribe from this list: send the line unsubscribe dash in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] implement privmode support in dash

2013-08-22 Thread Harald van Dijk 
On 22/08/13 19:59, Tavis Ormandy wrote:
 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.

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
diff --git a/src/Makefile.am b/src/Makefile.am
index 2a37381..82d00fb 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -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
 
diff --git a/src/dash.1 b/src/dash.1
index 94fc642..0f35748 100644
--- a/src/dash.1
+++ b/src/dash.1
@@ -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).
diff --git a/src/main.c b/src/main.c
index e106848..69e5e07 100644
--- a/src/main.c
+++ b/src/main.c
@@ -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);
diff --git a/src/options.c b/src/options.c
index 99ac55b..c640a41 100644
--- a/src/options.c
+++ b/src/options.c
@@ -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();
diff --git a/src/priv.c b/src/priv.c
new file mode 100644
index 000..26346c0
--- /dev/null
+++ b/src/priv.c
@@ -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();
+	}
+}
diff --git a/src/priv.h b/src/priv.h
new file mode 100644
index 000..31b380b
--- /dev/null
+++ b/src/priv.h
@@ -0,0 +1,6 @@
+#include unistd.h
+
+extern uid_t uid;
+extern gid_t gid;
+
+void setprivileged(int on);
diff --git a/src/var.c b/src/var.c
index cafe407..27f 100644
--- a/src/var.c
+++ b/src/var.c
@@ -192,13 +192,12 @@ initvar(void)
 void
 choose_ps1(void)
 {
-	if (vps1.flags  VEXPORT)
-		return;
-
 	if (!geteuid()) {
-		vps1.text = rootps1;
+		if (vps1.text == defps1)
+			vps1.text = rootps1;
 	} else {
-		vps1.text = defps1;
+		if (vps1.text

Re: [PATCH] implement privmode support in dash

2013-08-22 Thread Tavis Ormandy 
On Thu, Aug 22, 2013 at 1:35 PM, Jilles Tjoelker jil...@stack.nl wrote:
 I think there is no reason to deviate from other shells here. Therefore,
 please call it privileged.


Agreed.

 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?

 [snip]
 + if (!on  (uid != geteuid() || gid != getegid())) {
 + setuid(uid);
 + setgid(gid);
 + /* PS1 might need to be changed accordingly. */
 + choose_ps1();
 + }
 +}

 This code tries to use setuid() and setgid() to drop all privilege,
 which is only correct if the privilege to be dropped is UID 0, or on BSD
 systems. It would be better to use setresuid() or setreuid(), and change
 the GID before changing the UID.

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

 Apart from that, it is better to check the return value from setuid()
 and similar functions. In particular, some versions of Linux may fail
 setuid() for [EAGAIN], leaving the process running with the same
 privileges.

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.
--
To unsubscribe from this list: send the line unsubscribe dash in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] implement privmode support in dash

2013-08-22 Thread Jilles Tjoelker 
On Thu, Aug 22, 2013 at 09:59:36PM +0200, Harald van Dijk wrote:
 On 22/08/13 19:59, Tavis Ormandy wrote:
  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.

 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.

I think there is no reason to deviate from other shells here. Therefore,
please call it 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?

 [snip]
 + if (!on  (uid != geteuid() || gid != getegid())) {
 + setuid(uid);
 + setgid(gid);
 + /* PS1 might need to be changed accordingly. */
 + choose_ps1();
 + }
 +}

This code tries to use setuid() and setgid() to drop all privilege,
which is only correct if the privilege to be dropped is UID 0, or on BSD
systems. It would be better to use setresuid() or setreuid(), and change
the GID before changing the UID.

Apart from that, it is better to check the return value from setuid()
and similar functions. In particular, some versions of Linux may fail
setuid() for [EAGAIN], leaving the process running with the same
privileges.

-- 
Jilles Tjoelker
--
To unsubscribe from this list: send the line unsubscribe dash in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html