[Pkg-shadow-devel] Bug#623199: useradd: posix_spawn() error=ENOENT
Jonathan Nieder
jrnieder at gmail.com
Thu Aug 18 22:05:25 UTC 2011
Hi Nicolas,
Nicolas François wrote:
> There could be two options:
> * test if nscd exists and is executable
> * get rid of posix_spawn, replace by fork & exec abd report errors,
> except for ENOENT
Thanks for looking into this. As you might have guessed, my first
impression was to dislike both options:
* testing for nscd before running it seems redundant and there is a
time-of-check/time-of-use race.
* posix_spawn behaves better than fork/exec on machines with poor
support for copy-on-write. On glibc/linux, it uses vfork when
possible, to be nice to the scheduler.
[...]
> When WIFEXITED(status) && WEXITSTATUS(status) == 127 any error may have
> occurred. There is IMO no robust way to find out what was the exec error
> when fork succeeded.
Thinking longer, I agree. To be robust, nscd_flush_cache() should
succeed when nscd is not present (because not installed) but fail when
executing it failed (for example, because of insufficient memory).
While we have survived so far without the latter, that was just a bug.
I don't know what the security impact of not flushing the cache would
be. I can't imagine it's good.
>> * Is it intended that "nscd" is passed as argv[1] rather than argv[0]?
>
> Yes, argv[1] is just a name and could be anything.
> argv[0] is the executable.
Strange. nscd(8) says nothing about this.
# nscd -i groups
sent invalidate(groups) request, exiting
# nscd nscd -i groups
sent invalidate(groups) request, exiting
>> * Why not use the PATH (posix_spawnp) to discover where "nscd" is, to
>> allow the admin to install a copy to /usr/local/sbin/nscd?
>
> The goal is to get the system's nscd executed and avoid any mean from the
> user to execute another program (some of the shadow utils are or may be
> setuid root).
>
> One option could be to have it configurable (e.g. in /etc/login.defs or
> /etc/default/useradd)
Okay, I suppose this can wait until someone needs it. :) Ideally
setuid programs should be sanitizing their own $PATH.
>> * Is it safe to run nscd with an empty environment (in particular
>> without $PATH)? Is the environment scrubbing intended?
>
> This is intentional, as above, to avoid any interaction between the caller
> and nscd.
Makes sense.
> My current intent is to replace the posix_spawn call, but do not intent to
> change the other points.
Sure, seems sensible. Here's a rough patch, only compile-tested.
It steals a fork/exec routine from userdel. Maybe it can save some
time.
lib/Makefile.am | 2 +
lib/nscd.c | 37 ++++++++++++--------------
lib/spawn.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
lib/spawn.h | 7 +++++
src/userdel.c | 18 ++++--------
5 files changed, 108 insertions(+), 32 deletions(-)
diff --git c/lib/Makefile.am w/lib/Makefile.am
index a04623cd..48da50ed 100644
--- c/lib/Makefile.am
+++ w/lib/Makefile.am
@@ -48,6 +48,8 @@ libshadow_la_SOURCES = \
shadowio.c \
shadowio.h \
shadowmem.c \
+ spawn.c \
+ spawn.h \
utent.c
if WITH_TCB
diff --git c/lib/nscd.c w/lib/nscd.c
index d7fa2d58..8aa94046 100644
--- c/lib/nscd.c
+++ w/lib/nscd.c
@@ -16,7 +16,9 @@
#include <errno.h>
#include <sys/wait.h>
#include <sys/types.h>
+#include "exitcodes.h"
#include "defines.h"
+#include "spawn.h"
#include "nscd.h"
#define MSG_NSCD_FLUSH_CACHE_FAILED "Failed to flush the nscd cache.\n"
@@ -26,37 +28,32 @@
*/
int nscd_flush_cache (const char *service)
{
- pid_t pid, termpid;
- int err, status;
- char *spawnedArgs[] = {"/usr/sbin/nscd", "nscd", "-i", service, NULL};
- char *spawnedEnv[] = {NULL};
+ pid_t pid;
+ int err, status, code;
+ const char *spawnedArgs[] = {"/usr/sbin/nscd", "nscd", "-i", service, NULL};
+ const char *spawnedEnv[] = {NULL};
- /* spawn process */
- err = posix_spawn (&pid, spawnedArgs[0], NULL, NULL,
- spawnedArgs, spawnedEnv);
- if(0 != err)
+ err = run_command (spawnedArgs[0], spawnedArgs, spawnedEnv, &status);
+ if (0 != err)
{
+ /* run_command writes its own more detailed message. */
(void) fputs (_(MSG_NSCD_FLUSH_CACHE_FAILED), stderr);
- (void) fprintf (stderr, "posix_spawn() error=%d\n", err);
return -1;
}
-
- /* Wait for the spawned process to exit */
- termpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0));
- if (-1 == termpid)
+ code = WIFEXITED (status) ? WEXITSTATUS (status)
+ : (WTERMSIG (status) + 128);
+ if (code == E_CMD_NOTFOUND)
{
- (void) fputs (_(MSG_NSCD_FLUSH_CACHE_FAILED), stderr);
- perror("waitpid");
- return -1;
+ /* nscd is not installed, or it is installed but uses an
+ interpreter that is missing. Probably the former. */
+ return 0;
}
- else if (termpid != pid)
+ if (code != 0)
{
+ (void) fprintf (stderr, "nscd exited with status %d", code);
(void) fputs (_(MSG_NSCD_FLUSH_CACHE_FAILED), stderr);
- (void) fprintf (stderr, "waitpid returned %ld != %ld\n",
- (long int) termpid, (long int) pid);
return -1;
}
-
return 0;
}
#else /* USE_NSCD */
diff --git c/lib/spawn.c w/lib/spawn.c
new file mode 100644
index 00000000..afce926a
--- /dev/null
+++ w/lib/spawn.c
@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2011, Jonathan Nieder
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE FREEBSD
+ * PROJECT OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <errno.h>
+#include "exitcodes.h"
+#include "spawn.h"
+
+extern char **environ;
+
+int run_command (const char *cmd, const char *argv[], const char *envp[],
+ int *status)
+{
+ pid_t pid, wpid;
+
+ if (!envp)
+ envp = (const char **)environ;
+ pid = fork ();
+ if (pid == 0) {
+ execve (cmd, (char * const *) argv, (char * const *) envp);
+ if (errno == ENOENT)
+ exit (E_CMD_NOTFOUND);
+ perror(cmd);
+ exit (E_CMD_NOEXEC);
+ } else if ((pid_t)-1 == pid) {
+ int saved_errno = errno;
+ perror ("fork");
+ errno = saved_errno;
+ return -1;
+ }
+
+ do {
+ wpid = waitpid (pid, status, 0);
+ } while ((pid_t)-1 == wpid && errno == EINTR);
+
+ if ((pid_t)-1 == wpid) {
+ int saved_errno = errno;
+ perror ("waitpid");
+ return -1;
+ } else if (wpid != pid) {
+ (void) fprintf (stderr, "waitpid returned %ld != %ld\n",
+ (long int) wpid, (long int) pid);
+ errno = ECHILD;
+ return -1;
+ }
+ return 0;
+}
diff --git c/lib/spawn.h w/lib/spawn.h
new file mode 100644
index 00000000..f975d2ce
--- /dev/null
+++ w/lib/spawn.h
@@ -0,0 +1,7 @@
+#ifndef _SPAWN_H
+#define _SPAWN_H
+
+extern int run_command (const char *cmd, const char *argv[],
+ const char *envp[], int *status);
+
+#endif
diff --git c/src/userdel.c w/src/userdel.c
index 857db04e..374d07e8 100644
--- c/src/userdel.c
+++ w/src/userdel.c
@@ -59,6 +59,7 @@
#ifdef SHADOWGRP
#include "sgroupio.h"
#endif /* SHADOWGRP */
+#include "spawn.h"
#ifdef WITH_TCB
#include <tcb.h>
#include "tcbfuncs.h"
@@ -628,6 +629,7 @@ static void update_user (void)
static void user_cancel (const char *user)
{
const char *cmd;
+ const char *argv[3];
pid_t pid, wpid;
int status;
@@ -635,18 +637,10 @@ static void user_cancel (const char *user)
if (NULL == cmd) {
return;
}
- pid = fork ();
- if (pid == 0) {
- execl (cmd, cmd, user, (char *) 0);
- perror (cmd);
- exit (errno == ENOENT ? E_CMD_NOTFOUND : E_CMD_NOEXEC);
- } else if ((pid_t)-1 == pid) {
- perror ("fork");
- return;
- }
- do {
- wpid = wait (&status);
- } while ((wpid != pid) && ((pid_t)-1 != wpid));
+ argv[0] = cmd;
+ argv[1] = user;
+ argv[2] = (char *)0;
+ (void) run_command (cmd, argv, NULL, &status);
}
#ifdef EXTRA_CHECK_HOME_DIR
More information about the Pkg-shadow-devel
mailing list