Re: "command -p" does not correctly limit search to a safe PATH

[Harald van Dijk <harald@gigawatt.nl>]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

On 14/07/13 21:54, Harald van Dijk wrote:
> On 10/07/13 20:18, Craig Loomis wrote:
>>   Dash (0.5.7 and git master) does not implement 'command -p'
>> according to the standard, and opens an intriguing security hole to
>> anyone trying this scheme.
>>[...]
>> the path that 'command -p cmd' uses is a compiled-in constant
>> from dash's src/var.c:defpathvar, which starts with
>> "/usr/local/sbin:/usr/local/bin".

So, how about this, to be applied on top of my previous patch? It
defaults to using confstr() if available and reporting a hard error at
run time if that fails, but it can be configured to not use confstr(),
and/or fall back to a path specified at configuration time:

Use confstr() only, fail to configure if confstr is unavailable, fail at
runtime if confstr() does not return any path:
  configure --enable-confstr

Use confstr(), fail to configure if confstr is unavailable, fall back to
/usr/bin:/bin if confstr() does not return any path:
  configure --enable-confstr \
    --with-fallback-standard-path='"/usr/bin:/bin"'

Use confstr() if available, use /usr/bin:/bin if confstr() is
unavailable or does not return any path:
  configure \
    --with-fallback-standard-path='"/usr/bin:/bin"'

Do no use confstr(), always use /usr/bin:/bin:
  configure --disable-confstr \
    --with-fallback-standard-path='"/usr/bin:/bin"'

Feedback welcome, there are several parts of this that I am not sure
about. Also, defpathvar is no longer used outside of var.c, so I made it
static and removed the references in var.h.

Cheers,
Harald

[-- Attachment #2: dash-confstr.patch --]
[-- Type: text/x-patch, Size: 3938 bytes --]

commit 008118857bdb34ea885d76cbddbb56290706dcd2
Author: Harald van Dijk <harald@gigawatt.nl>
Date:   Fri Jul 19 23:29:55 2013 +0200

    command: use confstr() for -p to get a safe path

diff --git a/configure.ac b/configure.ac
index c6fb401..e595e99 100644
--- a/configure.ac
+++ b/configure.ac
@@ -40,6 +40,36 @@ AC_ARG_ENABLE(fnmatch, AS_HELP_STRING(--enable-fnmatch, \
 				      [Use fnmatch(3) from libc]))
 AC_ARG_ENABLE(glob, AS_HELP_STRING(--enable-glob, [Use glob(3) from libc]))
 
+AC_ARG_ENABLE([confstr],
+[AS_HELP_STRING([--enable-confstr], [Use confstr(3) from libc (default=auto)])],,
+[enable_confstr=auto])
+
+if test x"$enable_confstr" != x"no"; then
+	have_confstr=no
+	AC_CHECK_FUNCS([confstr],[have_confstr=yes])
+	have_decl_confstr=no
+	AC_CHECK_DECL([confstr],[have_decl_confstr=yes
+	AC_DEFINE([HAVE_DECL_CONFSTR], [1], [Define as 1 if you have a declaration of confstr()])])
+	have_decl_cs_path=no
+	AC_CHECK_DECL([_CS_PATH],[have_decl_cs_path=yes
+	AC_DEFINE([HAVE_DECL_CS_PATH], [1], [Define as 1 if you have a declaration of _CS_PATH])])
+
+	if test "$have_confstr:$have_decl_confstr:$have_decl_cs_path" != "yes:yes:yes"; then
+		if test x"$enable_confstr" = x"yes"; then
+			AC_MSG_ERROR([cannot use confstr])
+		fi
+	fi
+fi
+
+AC_ARG_WITH([fallback-standard-path],
+[AS_HELP_STRING([--with-fallback-standard-path=\"ARG\"],
+[use ARG for command -p path lookups if confstr does not return anything (default: none)])],,
+[with_fallback_standard_path=no])
+
+if test x"$with_fallback_standard_path" != x"no"; then
+	AC_DEFINE_UNQUOTED([FALLBACK_STANDARD_PATH], [$with_fallback_standard_path], [Define to the path to use for command -p path lookups (ignored if confstr() is used)])
+fi
+
 dnl Checks for libraries.
 
 dnl Checks for header files.
diff --git a/src/eval.c b/src/eval.c
index c7358a6..1dba165 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -644,7 +644,7 @@ parse_command_args(char **argv, const char **path)
 		do {
 			switch (c) {
 			case 'p':
-				*path = defpath;
+				*path = stdpath();
 				break;
 			default:
 				/* run 'typecmd' for other options */
diff --git a/src/exec.c b/src/exec.c
index e56e3f6..e32d48e 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -848,7 +848,7 @@ commandcmd(argc, argv)
 		else if (c == 'v')
 			verify |= VERIFY_BRIEF;
 		else if (c == 'p')
-			path = defpath;
+			path = stdpath();
 #ifdef DEBUG
 		else
 			abort();
@@ -860,3 +860,32 @@ commandcmd(argc, argv)
 
 	return 0;
 }
+
+const char *
+stdpath(void)
+{
+	static const char *path = NULL;
+
+#if HAVE_CONFSTR && HAVE_DECL_CONFSTR && HAVE_DECL_CS_PATH
+	if (path == NULL) {
+		size_t size = confstr(_CS_PATH, NULL, 0);
+		if (size != 0) {
+			char *newpath = ckmalloc(size);
+			confstr(_CS_PATH, newpath, size);
+			path = newpath;
+		}
+	}
+#endif
+
+#ifdef FALLBACK_STANDARD_PATH
+	if (path == NULL) {
+		path = FALLBACK_STANDARD_PATH;
+	}
+#endif
+
+	if (path == NULL) {
+		exerror(EXEXIT, "%s", "no path for standard utilities");
+	}
+
+	return path;
+}
diff --git a/src/exec.h b/src/exec.h
index 9ccb305..a3f9314 100644
--- a/src/exec.h
+++ b/src/exec.h
@@ -75,3 +75,4 @@ void defun(union node *);
 void unsetfunc(const char *);
 int typecmd(int, char **);
 int commandcmd(int, char **);
+const char *stdpath(void);
diff --git a/src/var.c b/src/var.c
index dc90249..653ba5b 100644
--- a/src/var.c
+++ b/src/var.c
@@ -73,7 +73,7 @@ struct localvar_list {
 
 MKINIT struct localvar_list *localvar_stack;
 
-const char defpathvar[] =
+static const char defpathvar[] =
 	"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin";
 #ifdef IFS_BROKEN
 const char defifsvar[] = "IFS= \t\n";
diff --git a/src/var.h b/src/var.h
index 79ee71a..71b85c9 100644
--- a/src/var.h
+++ b/src/var.h
@@ -106,8 +106,6 @@ extern const char defifsvar[];
 #else
 extern const char defifs[];
 #endif
-extern const char defpathvar[];
-#define defpath (defpathvar + 5)
 
 extern int lineno;
 extern char linenovar[];