diff --git a/System/Process.hs b/System/Process.hs --- a/System/Process.hs +++ b/System/Process.hs @@ -404,8 +404,8 @@ -- @readProcess@, the forked process will be terminated and @readProcess@ will -- wait (block) until the process has been terminated. -- --- Output is returned strictly, so this is not suitable for --- interactive applications. +-- Output is returned strictly, so this is not suitable for launching processes +-- that require interaction over the standard file streams. -- -- This function throws an 'IOError' if the process 'ExitCode' is -- anything other than 'ExitSuccess'. If instead you want to get the diff --git a/System/Process/Common.hs b/System/Process/Common.hs --- a/System/Process/Common.hs +++ b/System/Process/Common.hs @@ -74,7 +74,7 @@ std_in :: StdStream, -- ^ How to determine stdin std_out :: StdStream, -- ^ How to determine stdout std_err :: StdStream, -- ^ How to determine stderr - close_fds :: Bool, -- ^ Close all file descriptors except stdin, stdout and stderr in the new process (on Windows, only works if std_in, std_out, and std_err are all Inherit) + close_fds :: Bool, -- ^ Close all file descriptors except stdin, stdout and stderr in the new process (on Windows, only works if std_in, std_out, and std_err are all Inherit). This implementation will call close an every fd from 3 to the maximum of open files, which can be slow for high maximum of open files. create_group :: Bool, -- ^ Create a new process group delegate_ctlc:: Bool, -- ^ Delegate control-C handling. Use this for interactive console processes to let them handle control-C themselves (see below for details). -- diff --git a/System/Process/Internals.hs b/System/Process/Internals.hs --- a/System/Process/Internals.hs +++ b/System/Process/Internals.hs @@ -46,6 +46,7 @@ #else pPrPr_disableITimers, c_execvpe, ignoreSignal, defaultSignal, + runInteractiveProcess_lock, #endif withFilePathException, withCEnvironment, translate, diff --git a/System/Process/Posix.hs b/System/Process/Posix.hs --- a/System/Process/Posix.hs +++ b/System/Process/Posix.hs @@ -17,6 +17,7 @@ , createPipeInternal , createPipeInternalFd , interruptProcessGroupOfInternal + , runInteractiveProcess_lock ) where import Control.Concurrent @@ -93,7 +94,7 @@ withCEnvironment :: [(String,String)] -> (Ptr CString -> IO a) -> IO a withCEnvironment envir act = let env' = map (\(name, val) -> name ++ ('=':val)) envir - in withMany withCString env' (\pEnv -> withArray0 nullPtr pEnv act) + in withMany withFilePath env' (\pEnv -> withArray0 nullPtr pEnv act) -- ----------------------------------------------------------------------------- -- POSIX runProcess with signal handling in the child @@ -138,10 +139,7 @@ when mb_delegate_ctlc startDelegateControlC - -- runInteractiveProcess() blocks signals around the fork(). - -- Since blocking/unblocking of signals is a global state - -- operation, we better ensure mutual exclusion of calls to - -- runInteractiveProcess(). + -- See the comment on runInteractiveProcess_lock proc_handle <- withMVar runInteractiveProcess_lock $ \_ -> c_runInteractiveProcess pargs pWorkDir pEnv fdin fdout fderr @@ -174,6 +172,15 @@ } {-# NOINLINE runInteractiveProcess_lock #-} +-- | 'runInteractiveProcess' blocks signals around the fork(). +-- Since blocking/unblocking of signals is a global state operation, we need to +-- ensure mutual exclusion of calls to 'runInteractiveProcess'. +-- This lock is exported so that other libraries which also need to fork() +-- (and also need to make the same global state changes) can protect their changes +-- with the same lock. +-- See https://github.com/haskell/process/pull/154. +-- +-- @since 1.6.6.0 runInteractiveProcess_lock :: MVar () runInteractiveProcess_lock = unsafePerformIO $ newMVar () diff --git a/cbits/runProcess.c b/cbits/runProcess.c --- a/cbits/runProcess.c +++ b/cbits/runProcess.c @@ -33,6 +33,10 @@ extern void blockUserSignals(void); extern void unblockUserSignals(void); +// These are arbitrarily chosen -- JP +#define forkSetgidFailed 124 +#define forkSetuidFailed 125 + // See #1593. The convention for the exit code when // exec() fails seems to be 127 (gleened from C's // system()), but there's no equivalent convention for @@ -40,9 +44,8 @@ #define forkChdirFailed 126 #define forkExecFailed 127 -// These are arbitrarily chosen -- JP -#define forkSetgidFailed 124 -#define forkSetuidFailed 125 +#define forkGetpwuidFailed 128 +#define forkInitgroupsFailed 129 __attribute__((__noreturn__)) static void childFailed(int pipe, int failCode) { @@ -182,6 +185,23 @@ } if ( childUser) { + // Using setuid properly first requires that we initgroups. + // However, to do this we must know the username of the user we are + // switching to. + struct passwd pw; + struct passwd *res = NULL; + int buf_len = sysconf(_SC_GETPW_R_SIZE_MAX); + char *buf = malloc(buf_len); + gid_t suppl_gid = childGroup ? *childGroup : getgid(); + if ( getpwuid_r(*childUser, &pw, buf, buf_len, &res) != 0) { + childFailed(forkCommunicationFds[1], forkGetpwuidFailed); + } + if ( res == NULL ) { + childFailed(forkCommunicationFds[1], forkGetpwuidFailed); + } + if ( initgroups(res->pw_name, suppl_gid) != 0) { + childFailed(forkCommunicationFds[1], forkInitgroupsFailed); + } if ( setuid( *childUser) != 0) { // ERROR childFailed(forkCommunicationFds[1], forkSetuidFailed); @@ -329,6 +349,13 @@ break; case forkSetuidFailed: *failed_doing = "runInteractiveProcess: setuid"; + break; + case forkGetpwuidFailed: + *failed_doing = "runInteractiveProcess: getpwuid"; + break; + case forkInitgroupsFailed: + *failed_doing = "runInteractiveProcess: initgroups"; + break; default: *failed_doing = "runInteractiveProcess: unknown"; break; diff --git a/changelog.md b/changelog.md --- a/changelog.md +++ b/changelog.md @@ -1,5 +1,17 @@ # Changelog for [`process` package](http://hackage.haskell.org/package/process) +## 1.6.6.0 *October 2019* + +* Fix a potential privilege escalation issue (or, more precisely, privileges + not being dropped when this was the user's intent) where the groups of the + spawning process's user would be incorrectly retained due to a missing call to + `initgroups` [#149]. +* Bug fix: Prevent stripping undecodable bytes from environment variables + when in a non-unicode locale. + [#152](https://github.com/haskell/process/issues/152) +* Expose `runInteractiveProcess_lock` in `System.Process.Internals` + [#154](https://github.com/haskell/process/pull/154) + ## 1.6.5.1 *June 2019* * Version bound bumps diff --git a/include/runProcess.h b/include/runProcess.h --- a/include/runProcess.h +++ b/include/runProcess.h @@ -21,6 +21,10 @@ #include #include +#if !(defined(_MSC_VER) || defined(__MINGW32__) || defined(_WIN32)) +#include +#include +#endif #ifdef HAVE_FCNTL_H #include diff --git a/process.cabal b/process.cabal --- a/process.cabal +++ b/process.cabal @@ -1,5 +1,5 @@ name: process -version: 1.6.5.1 +version: 1.6.6.0 -- NOTE: Don't forget to update ./changelog.md license: BSD3 license-file: LICENSE