* "command -p" does not correctly limit search to a safe PATH
@ 2013-07-10 18:18 Craig Loomis
2013-07-14 19:54 ` Harald van Dijk
0 siblings, 1 reply; 6+ messages in thread
From: Craig Loomis @ 2013-07-10 18:18 UTC (permalink / raw)
To: dash
I needed to bootstrap a safe shell environment, assuming only a
minimal "POSIX system" and expecting some peculiar ones (embedded
systems with unusual paths, etc).
One modern POSIX prescription to establish a safe PATH is fleshed
out in OpenGroup's IEEE 1003.1 at
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html
(and so 'man 1p command' on some Linux versions).
This boils down to (after setting IFS and cleaning out aliases and functions):
$ PATH="$(command -p getconf PATH):$PATH"
Which works as a bootstrap because:
- "command" is a shell builtin (per the standard)
- "command -p" limits its search to system-defined locations.
- "getconf PATH" uses confstr(3) to get the required
system-defined _CS_PATH.
The Standard's definition of paths is a bit fuzzy; a paragraph from
the Rationale helps:
"The -p option is present because it is useful to be able to ensure a
safe path search that finds all the standard utilities. This search
might not be identical to the one that occurs through one of the exec
functions (as defined in the System Interfaces volume of POSIX.1-2008)
when PATH is unset. At the very least, this feature is required to
allow the script to access the correct version of getconf so that the
value of the default path can be accurately retrieved."
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.
When using 'command -v' to simply print the path to an executable,
'-p' has no effect:
# Assume that an executable /tmp/evilbin/getconf exists
$ PATH=/tmp/evilbin:$PATH
$ command -v getconf
/tmp/evilbin/getconf
$ command -p -v getconf
/tmp/evilbin/getconf
Not good, but most people would call getconf directly. At first
glance that _appears_ to work better -- /tmp/evilbin/getconf is not
called:
$ command getconf PATH
/tmp/evilbin:/bin:/usr/bin
$ command -p getconf PATH
/usr/bin:/bin:/usr/sbin:/sbin
but there are two surprises:
a) the behavior of 'command -p cmd' and '$(command -p -v cmd)'
really should be the same, no?
b) 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". To me, that is both completely
unexpected and pretty scary -- /usr/local/bin is (very) often less
well secured or checked than, say, /bin:
# somehow, sometime, 'sudo make install' or a g+w /usr/local/ might get you:
$ cp evil_getconf /usr/local/bin/getconf
after which:
$ command -p getconf PATH
/tmp/evilbin:/bin:/usr/bin
Ouch.
src/exec.c and src/eval.c are a bit tricky (see the different
behavior between 'command cmd' and 'command -v cmd') and too
fundamental for me to stand behind a quick patch. Sorry.
FWIW, bash 4.1+ (and maybe earlier) has a posix compliant 'command
-p'. Fixed sometime after 3.2, from trying that version on OS X 10.8
and RHEL 5.9.
- craig
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: "command -p" does not correctly limit search to a safe PATH 2013-07-10 18:18 "command -p" does not correctly limit search to a safe PATH Craig Loomis @ 2013-07-14 19:54 ` Harald van Dijk 2013-07-19 21:49 ` Harald van Dijk 2014-09-26 8:44 ` Herbert Xu 0 siblings, 2 replies; 6+ messages in thread From: Harald van Dijk @ 2013-07-14 19:54 UTC (permalink / raw) To: Craig Loomis; +Cc: dash [-- Attachment #1: Type: text/plain, Size: 1617 bytes --] 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. > > When using 'command -v' to simply print the path to an executable, > '-p' has no effect: You're right. dash has never supported combining -p with -v, but back in 2005 this was seemingly accidentally changed from reporting a syntax error to silently ignoring the -p option, only about a month after dash moved to git. Making sure that -p is respected even when -v is used is easy enough, see attached patch. Tested even with explicit PATH overrides: PATH=/path/to/some/other/dash command -pv dash correctly outputs /bin/dash on my system. > 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". To me, that is both completely > unexpected and pretty scary -- /usr/local/bin is (very) often less > well secured or checked than, say, /bin: Agreed. However, IMO, it does make sense for defpathvar to start with /usr/local/*: it has two separate functions, it also serves as the default path (hence the name) when dash is started with no PATH set at all. I think fixing this should be done in a way so that command -p does not use defpathvar, not by changing defpathvar. bash uses the same confstr function for this that getconf uses, and it shouldn't be too much work to make dash use that too. If no one else comes up with a working patch or a better approach, I'll try to get that working. Cheers, Harald [-- Attachment #2: dash-command-pv.patch --] [-- Type: text/x-patch, Size: 1737 bytes --] commit 475e328589fd2e843c138d49fb96699a2a66151d Author: Harald van Dijk <harald@gigawatt.nl> Date: Sun Jul 14 21:23:01 2013 +0200 command: allow combining -p with -v diff --git a/src/exec.c b/src/exec.c index 79e2007..e56e3f6 100644 --- a/src/exec.c +++ b/src/exec.c @@ -96,7 +96,7 @@ STATIC void clearcmdentry(int); STATIC struct tblentry *cmdlookup(const char *, int); STATIC void delete_cmd_entry(void); STATIC void addcmdentry(char *, struct cmdentry *); -STATIC int describe_command(struct output *, char *, int); +STATIC int describe_command(struct output *, char *, const char *, int); /* @@ -727,21 +727,21 @@ typecmd(int argc, char **argv) int err = 0; for (i = 1; i < argc; i++) { - err |= describe_command(out1, argv[i], 1); + err |= describe_command(out1, argv[i], pathval(), 1); } return err; } STATIC int -describe_command(out, command, verbose) +describe_command(out, command, path, verbose) struct output *out; char *command; + const char *path; int verbose; { struct cmdentry entry; struct tblentry *cmdp; const struct alias *ap; - const char *path = pathval(); if (verbose) { outstr(command, out); @@ -840,20 +840,23 @@ commandcmd(argc, argv) VERIFY_BRIEF = 1, VERIFY_VERBOSE = 2, } verify = 0; + const char *path = pathval(); while ((c = nextopt("pvV")) != '\0') if (c == 'V') verify |= VERIFY_VERBOSE; else if (c == 'v') verify |= VERIFY_BRIEF; + else if (c == 'p') + path = defpath; #ifdef DEBUG - else if (c != 'p') + else abort(); #endif cmd = *argptr; if (verify && cmd) - return describe_command(out1, cmd, verify - VERIFY_BRIEF); + return describe_command(out1, cmd, path, verify - VERIFY_BRIEF); return 0; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: "command -p" does not correctly limit search to a safe PATH 2013-07-14 19:54 ` Harald van Dijk @ 2013-07-19 21:49 ` Harald van Dijk 2014-09-26 9:19 ` Herbert Xu 2014-09-26 8:44 ` Herbert Xu 1 sibling, 1 reply; 6+ messages in thread From: Harald van Dijk @ 2013-07-19 21:49 UTC (permalink / raw) To: Craig Loomis, dash [-- 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[]; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: "command -p" does not correctly limit search to a safe PATH 2013-07-19 21:49 ` Harald van Dijk @ 2014-09-26 9:19 ` Herbert Xu 2014-09-27 21:57 ` Jilles Tjoelker 0 siblings, 1 reply; 6+ messages in thread From: Herbert Xu @ 2014-09-26 9:19 UTC (permalink / raw) To: Harald van Dijk; +Cc: Craig Loomis, dash On Fri, Jul 19, 2013 at 09:49:31PM +0000, Harald van Dijk wrote: > > 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: Thanks for the patch. But until someone who needs this complexity steps up, I'm going to stick with the simpler version below: commit 842050da1c14d7dbe365cd750032fcd8eaaa1db2 Author: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri Sep 26 17:18:35 2014 +0800 [BUILTIN] Set command -p path to /usr/sbin:/usr/bin:/sbin:/bin Exclude /usr/local from command -p PATH. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/ChangeLog b/ChangeLog index eb3bbc3..ba67b6e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,7 @@ 2014-09-26 Herbert Xu <herbert@gondor.apana.org.au> * Small optimisation of command -pv change. + * Set command -p path to /usr/sbin:/usr/bin:/sbin:/bin. 2014-09-26 Harald van Dijk <harald@gigawatt.nl> diff --git a/src/var.h b/src/var.h index 79ee71a..872e2db 100644 --- a/src/var.h +++ b/src/var.h @@ -107,7 +107,7 @@ extern const char defifsvar[]; extern const char defifs[]; #endif extern const char defpathvar[]; -#define defpath (defpathvar + 5) +#define defpath (defpathvar + 36) extern int lineno; extern char linenovar[]; Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: "command -p" does not correctly limit search to a safe PATH 2014-09-26 9:19 ` Herbert Xu @ 2014-09-27 21:57 ` Jilles Tjoelker 0 siblings, 0 replies; 6+ messages in thread From: Jilles Tjoelker @ 2014-09-27 21:57 UTC (permalink / raw) To: Herbert Xu; +Cc: Harald van Dijk, Craig Loomis, dash On Fri, Sep 26, 2014 at 05:19:42PM +0800, Herbert Xu wrote: > On Fri, Jul 19, 2013 at 09:49:31PM +0000, Harald van Dijk wrote: > > > > 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: > Thanks for the patch. But until someone who needs this complexity > steps up, I'm going to stick with the simpler version below: > [snip] > diff --git a/src/var.h b/src/var.h > index 79ee71a..872e2db 100644 > --- a/src/var.h > +++ b/src/var.h > @@ -107,7 +107,7 @@ extern const char defifsvar[]; > extern const char defifs[]; > #endif > extern const char defpathvar[]; > -#define defpath (defpathvar + 5) > +#define defpath (defpathvar + 36) > > extern int lineno; > extern char linenovar[]; This needs a comment at the definition of defpathvar in var.c; otherwise, someone changing the default path will subtly break command -p without knowing. The number 36 is rather magic too, but it can be found back through git history. Alternatively, you could rely on the linker combining common string constant endings: put in some #define for "/usr/sbin:/usr/bin:/sbin:/bin" and make defpathvar a #define instead of a const array. -- Jilles Tjoelker ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: "command -p" does not correctly limit search to a safe PATH 2013-07-14 19:54 ` Harald van Dijk 2013-07-19 21:49 ` Harald van Dijk @ 2014-09-26 8:44 ` Herbert Xu 1 sibling, 0 replies; 6+ messages in thread From: Herbert Xu @ 2014-09-26 8:44 UTC (permalink / raw) To: Harald van Dijk; +Cc: Craig Loomis, dash On Sun, Jul 14, 2013 at 07:54:58PM +0000, Harald van Dijk wrote: > > commit 475e328589fd2e843c138d49fb96699a2a66151d > Author: Harald van Dijk <harald@gigawatt.nl> > Date: Sun Jul 14 21:23:01 2013 +0200 > > command: allow combining -p with -v Patch applied. Thanks a lot! I've also added a little optimisation patch on top to minimise the size impact. commit d813a572d94455f1d0d43ccad49edfacd13ee2e3 Author: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri Sep 26 16:42:18 2014 +0800 [BUILTIN] Small optimisation of command -pv change This patch moves the pathval call into the describe_command function and also eliminates an unnecessary branch when DEBUG is off. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/ChangeLog b/ChangeLog index 2fbc628..a8d5fa2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2014-09-26 Herbert Xu <herbert@gondor.apana.org.au> + + * Small optimisation to command -pv change. + 2014-09-26 Harald van Dijk <harald@gigawatt.nl> * command: allow combining -p with -v. diff --git a/src/exec.c b/src/exec.c index e56e3f6..ec0eadd 100644 --- a/src/exec.c +++ b/src/exec.c @@ -727,7 +727,7 @@ typecmd(int argc, char **argv) int err = 0; for (i = 1; i < argc; i++) { - err |= describe_command(out1, argv[i], pathval(), 1); + err |= describe_command(out1, argv[i], NULL, 1); } return err; } @@ -743,6 +743,8 @@ describe_command(out, command, path, verbose) struct tblentry *cmdp; const struct alias *ap; + path = path ?: pathval(); + if (verbose) { outstr(command, out); } @@ -840,19 +842,19 @@ commandcmd(argc, argv) VERIFY_BRIEF = 1, VERIFY_VERBOSE = 2, } verify = 0; - const char *path = pathval(); + const char *path = NULL; while ((c = nextopt("pvV")) != '\0') if (c == 'V') verify |= VERIFY_VERBOSE; else if (c == 'v') verify |= VERIFY_BRIEF; - else if (c == 'p') - path = defpath; #ifdef DEBUG - else + else if (c != 'p') abort(); #endif + else + path = defpath; cmd = *argptr; if (verify && cmd) Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-27 22:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-10 18:18 "command -p" does not correctly limit search to a safe PATH Craig Loomis 2013-07-14 19:54 ` Harald van Dijk 2013-07-19 21:49 ` Harald van Dijk 2014-09-26 9:19 ` Herbert Xu 2014-09-27 21:57 ` Jilles Tjoelker 2014-09-26 8:44 ` Herbert Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.