Add support for CRD user-session to operate setuid

remoting/host/linux/remoting_user_session.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // This file implements a wrapper to run the virtual me2me session within a
 // proper PAM session. It will generally be run as root and drop privileges to
 // the specified user before running the me2me session script.
 
+// Usage: user-session start [--foreground] [user]
+//
+// Options:
+//   --foreground  - Don't doemonize.

[Jamie, 2017/07/20 16:56:48]

daemonize

[rkjnsn, 2017/07/20 17:39:03]

Acknowledged.

+//   user          - Create a session for the specified user. Required when
+//                   running as root, not allowed when running as a normal user.
+
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/wait.h>
 #include <fcntl.h>
 #include <grp.h>
+#include <limits.h>
 #include <pwd.h>
 #include <unistd.h>
 
 #include <cerrno>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
 #include <ctime>
 
 #include <map>
 #include <memory>
 #include <string>
 #include <tuple>
 #include <utility>
 #include <vector>
 
 #include <security/pam_appl.h>
 
-#include "base/command_line.h"
 #include "base/environment.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/process/launch.h"
 #include "base/strings/string_piece.h"
 
 namespace {
 
 const char kPamName[] = "chrome-remote-desktop";
-
-const char kHelpSwitchName[] = "help";
-const char kQuestionSwitchName[] = "?";
-const char kUserSwitchName[] = "user";
-const char kScriptSwitchName[] = "me2me-script";
-const char kForegroundSwitchName[] = "foreground";
+const char kScriptName[] = "chrome-remote-desktop";
+const char kForegroundFlag[] = "--foreground";
 
 // This template will be formatted by strftime and then used by mkstemp
 const char kLogFileTemplate[] =
     "/tmp/chrome_remote_desktop_%Y%m%d_%H%M%S_XXXXXX";
 
 const char kUsageMessage[] =
-    "Usage: %s [options]\n"
-    "\n"
-    "Options:\n"
-    "  --help, -?               - Print this message.\n"
-    "  --user=<user>            - Create session as the specified user. "
-                                 "(Must run as root.)\n"
-    "  --me2me-script=<script>  - Location of the me2me python script "
-                                 "(required)\n"
-    "  --foreground             - Don't daemonize.\n";
+    "This program is not intended to be run by end users. To configure Chrome\n"
+    "Remote Desktop, please install the app from the Chrome Web Store:\n"
+    "https://chrome.google.com/remotedesktop\n";
 
-void PrintUsage(const base::FilePath& program_name) {
-  std::printf(kUsageMessage, program_name.MaybeAsASCII().c_str());
+void PrintUsage() {
+  std::fputs(kUsageMessage, stderr);
 }
 
 // Shell-escapes a single argument in a way that is compatible with various
 // different shells. Returns nullopt when argument contains a newline, which
 // can't be represented in a cross-shell fashion.
 base::Optional<std::string> ShellEscapeArgument(
     const base::StringPiece argument) {
   std::string result;
   for (char character : argument) {
     // csh in particular doesn't provide a good way to handle this
@@ skipping 158 matching lines @@
     }
   }
 
  private:
   pam_handle_t* pam_handle_ = nullptr;
   int last_return_code_ = PAM_SUCCESS;
 
   DISALLOW_COPY_AND_ASSIGN(PamHandle);
 };
 
+std::string FindScriptPath() {
+  std::string script_path(PATH_MAX, '\0');
+  ssize_t readlink_result;
+
+  readlink_result =
+      readlink("/proc/self/exe", &script_path[0], script_path.size());

[Lambros, 2017/07/20 01:42:49]

Use base::ReadSymbolicLink() from base/files/file_util.h ?

[Jamie, 2017/07/20 02:06:26]

This probably applies to a few of your comments, and it needs to be decided on a
case-by-case basis, but the rationale behind minimizing the use of library code
is that most of it doesn't normally run setuid root. We also want to minimize
the amount of code that gets pulled into this binary.

[rkjnsn, 2017/07/20 06:45:52]

When I was developing this patch, this happened before forking and thus I wanted
to limit the amount of external code run as root. Now that I have pushed it to
be after forking and dropping privileges, using base::ReadSymbolicLink() is
probably okay.

[rkjnsn, 2017/07/20 17:49:43]

Actually, looking at the implementation in file_util_posix.cc, it doesn't look
like ReadSymbolicLink checks if the link was too long for the buffer. PATH_MAX
is arbitrary and not actually enforced by anything, so that can lead to a
truncated result. It also discards all error information and just returns false
if something goes wrong.

[rkjnsn, 2017/07/21 00:44:25]

Okay, further testing indicates that Linux will return ENAMETOOLONG if the path
to the executable is longer that PATH_MAX, so using ReadSymbolicLink should be
okay.

+  PCHECK(readlink_result >= 0) << "readlink failed";

[Jamie, 2017/07/20 16:56:48]

Why PCHECK instead of CHECK here? I couldn't find any documentation on the
difference.

[rkjnsn, 2017/07/20 17:39:03]

PCHECK additionally prints the error message associated with errno.
See https://cs.chromium.org/chromium/src/base/logging.h?l=125

+  CHECK(static_cast<std::size_t>(readlink_result) < script_path.size())
+      << "binary path too long";
+  CHECK(script_path[0] == '/') << "path not absolute";
+
+  script_path.resize(readlink_result);
+
+  std::size_t last_slash = script_path.find_last_of('/');

[Lambros, 2017/07/20 01:42:49]

Use base::FilePath:: DirName() and Append() instead of raw string manipulations
here?

[rkjnsn, 2017/07/20 06:45:51]

Same here.

+  script_path.replace(script_path.begin() + last_slash + 1, script_path.end(),
+                      kScriptName);
+  return script_path;
+}
+
+// Execs the me2me script.
+// This function is called after forking and dropping privileges. It never
+// returns.
+void ChildProcess(base::EnvironmentMap environment,

[Lambros, 2017/07/20 01:42:49]

Please name the function as a verb.

[rkjnsn, 2017/07/20 06:45:51]

Acknowledged.

+                  const struct passwd* pwinfo) {
+  // By convention, a login shell is signified by preceeding the shell name in
+  // argv[0] with a '-'.
+  std::string shell_name =
+      '-' + base::FilePath(pwinfo->pw_shell).BaseName().value();
+
+  base::Optional<std::string> escaped_script_path =
+      ShellEscapeArgument(FindScriptPath());
+  CHECK(escaped_script_path) << "Could not escape script path";
+
+  std::string shell_arg =
+      *escaped_script_path + " --start --foreground --keep-parent-env";
+
+  environment["USER"] = pwinfo->pw_name;
+  environment["LOGNAME"] = pwinfo->pw_name;
+  environment["HOME"] = pwinfo->pw_dir;
+  environment["SHELL"] = pwinfo->pw_shell;
+  if (!environment.count("PATH")) {
+    environment["PATH"] = "/bin:/usr/bin";
+  }
+
+  std::vector<std::string> env_strings;
+  for (auto& env_var : environment) {

[Lambros, 2017/07/20 01:42:49]

const auto&

[rkjnsn, 2017/07/20 06:45:52]

Acknowledged.

+    env_strings.emplace_back(env_var.first + "=" + env_var.second);
+  }
+
+  std::vector<const char*> arg_ptrs = {shell_name.c_str(), "-c",
+                                       shell_arg.c_str(), nullptr};
+  std::vector<const char*> env_ptrs;
+  env_ptrs.reserve(env_strings.size() + 1);

[Lambros, 2017/07/20 01:42:49]

I don't know if we're allowed to malloc() in the child process?
Have a look at
https://cs.chromium.org/chromium/src/base/process/launch_posix.cc?type=cs&q=G...
where it calls reserve() in advance of doing the fork.

Might not matter to us (and maybe GNU glibc actually lets us malloc in the child
anyway)?

[rkjnsn, 2017/07/20 06:45:52]

I think this should be okay. As far as I understand it, the potential problem
occurs when the forking process has multiple threads. If the fork occurs while
another thread happens to be holding a lock, then that lock will never be
released in the child process, resulting in a deadlock if the child later tries
to obtain the lock. In a large application like Chromium, where there will be
many threads possibly allocating memory or calling POSIX functions, the only
safe way to ensure the child doesn't deadlock is to avoid calling anything that
might try to acquire a lock, which in practice means limiting oneself to
operations that are defined to be async-signal-safe.

Since we know we're the only thread in the process, we can be sure that the fork
is not going to occur in the middle of some lock-holding operation.

(As a side note, I believe glibc actually adds an atfork handler which will
obtain an allocator lock before forking and release it afterword, ensuring that
no other thread has an allocator lock during the fork. This only applies to the
system allocator, though, while I seem to recall that Chromium uses tcmalloc.)

+  for (auto& env_string : env_strings) {

[Lambros, 2017/07/20 01:42:50]

const auto&

[rkjnsn, 2017/07/20 06:45:52]

Acknowledged.

+    env_ptrs.push_back(env_string.c_str());
+  }
+  env_ptrs.push_back(nullptr);
+
+  execve(pwinfo->pw_shell, const_cast<char* const*>(arg_ptrs.data()),
+         const_cast<char* const*>(env_ptrs.data()));
+  PCHECK(false) << "failed to exec session script";

[Lambros, 2017/07/20 01:42:49]

Maybe log the filename (full path) of the script that failed?

[rkjnsn, 2017/07/20 06:45:51]

Acknowledged.

+}
+
 // Runs the me2me script in a PAM session. Exits the program on failure.
 // If chown_log is true, the owner and group of the file associated with stdout
-// will be changed to the target user.
-void ExecuteSession(std::string user, base::StringPiece script_path,
-                    bool chown_log) {
-  //////////////////////////////////////////////////////////////////////////////
-  // Set up the PAM session
-  //////////////////////////////////////////////////////////////////////////////
-
+// will be changed to the target user. If match_uid is not nullopt, this

[Lambros, 2017/07/20 01:42:49]

I think "present" or "provided" reads better than "not nullopt"?

[rkjnsn, 2017/07/20 06:45:51]

Agreed.

+// function will fail if the final user id does not match the one provided.
+void ExecuteSession(std::string user,
+                    bool chown_log,
+                    base::Optional<uid_t> match_uid) {
   PamHandle pam_handle(kPamName, user.c_str(), &kPamConversation);
   CHECK(pam_handle.IsInitialized()) << "Failed to initialize PAM";
 
   // Make sure the account is valid and enabled.
   pam_handle.CheckReturnCode(pam_handle.AccountManagement(0), "Account check");
 
   // PAM may remap the user at any stage.
   user = pam_handle.GetUser().value_or(std::move(user));
 
   // setcred explicitly does not handle user id or group membership, and
@@ skipping 13 matching lines @@
   // as done here, but it may be worth noting that `login` calls open_session
   // first.
   pam_handle.CheckReturnCode(pam_handle.SetCredentials(PAM_ESTABLISH_CRED),
                               "Set credentials");
 
   pam_handle.CheckReturnCode(pam_handle.OpenSession(0), "Open session");
 
   // The above may have remapped the user.
   user =  pam_handle.GetUser().value_or(std::move(user));
 
-  base::Optional<base::EnvironmentMap> pam_environment =
-      pam_handle.GetEnvironment();
-  CHECK(pam_environment) << "Failed to get environment from PAM";
-
-  //////////////////////////////////////////////////////////////////////////////
-  // Prepare to execute remoting session process
-  //////////////////////////////////////////////////////////////////////////////
-
-  // Callback to be run in child process after fork and before exec.
-  // chdir is called manually instead of using LaunchOptions.current_directory
-  // because it should take place after setuid. (This both makes sure the user
-  // has the proper permissions and also apparently avoids some obscure errors
-  // that can occur when accessing some network filesystems as the wrong user.)
-  class PreExecDelegate : public base::LaunchOptions::PreExecDelegate {
-   public:
-    void RunAsyncSafe() override {
-      // Use RAW_CHECK to avoid allocating post-fork.
-      RAW_CHECK(setuid(pwinfo_->pw_uid) == 0);
-      RAW_CHECK(chdir(pwinfo_->pw_dir) == 0);
-    }
-
-    PreExecDelegate(struct passwd* pwinfo) : pwinfo_(pwinfo) {}
-
-   private:
-    struct passwd* pwinfo_;
-  };
-
   // Fetch pwinfo again, as it may have been invalidated or the user name might
   // have been remapped.
   pwinfo = getpwnam(user.c_str());
   PCHECK(pwinfo != nullptr) << "getpwnam failed";
 
+  if (match_uid && pwinfo->pw_uid != *match_uid) {
+    LOG(FATAL) << "PAM remapped username to one with a different user ID.";
+  }
+
   if (chown_log) {
     int result = fchown(STDOUT_FILENO, pwinfo->pw_uid, pwinfo->pw_gid);
     PLOG_IF(WARNING, result != 0) << "Failed to change log file owner";
   }
 
-  PreExecDelegate pre_exec_delegate(pwinfo);
+  pid_t child_pid = fork();

[Lambros, 2017/07/20 01:42:49]

Why have you removed base::LaunchProcess() here?

[rkjnsn, 2017/07/20 06:45:52]

This is actually the key change for this version of the patch. Using
LaunchProcess requires us to figure out the exec arguments and environment
before forking, while we are still root. By removing LaunchProcess, we can move
all of our code to set environment variables, determine the script location, and
build the command line all in the child process after we have dropped
privileges. This significantly reduces the amount of code executed as root,
which is especially important in a setuid binary, as any exploitable bug in the
code that runs as root could be used for a privilege escalation attack.

+  PCHECK(child_pid >= 0) << "fork failed";
+  if (child_pid == 0) {
+    PCHECK(setuid(pwinfo->pw_uid) == 0) << "setuid failed";
+    PCHECK(chdir(pwinfo->pw_dir) == 0) << "chdir to $HOME failed";
+    base::Optional<base::EnvironmentMap> pam_environment =
+        pam_handle.GetEnvironment();
+    CHECK(pam_environment) << "Failed to get environment from PAM";
+    ChildProcess(std::move(*pam_environment), pwinfo);  // Never returns
+  } else {
+    // waidpid will return if the child is ptraced, so loop until the process

[Lambros, 2017/07/20 01:42:49]

waitpid

[rkjnsn, 2017/07/20 06:45:51]

Acknowledged.

+    // actually exits.

[Lambros, 2017/07/20 01:42:49]

Could this be a busy loop? Have a look at
https://cs.chromium.org/chromium/src/base/process/process_posix.cc?type=cs&q=...

to see how it's done there. They ramp up the usleep() but I guess a simple fixed
duration is good enough for us?

[rkjnsn, 2017/07/20 06:45:52]

I don't think so. In that code, they need to sleep because they are calling
waitpid with the WNOHANG option, which means it returns immediately even if the
child process is still running. It looks like they do that to implement timeout
functionality. Since we're not using WNOHANG here, waitpid should block until it
has something to report. (Unless a signal causes an EINTR, which I just realized
I'm not handling.)

+    int status;
+    do {
+      pid_t wait_result = waitpid(child_pid, &status, 0);
 
-  base::LaunchOptions launch_options;
+      // Die if wait fails so we don't close the PAM session while the child is
+      // still running.
+      PCHECK(wait_result >= 0) << "wait failed";
+    } while (!WIFEXITED(status) && !WIFSIGNALED(status));
 
-  // Required to allow suid binaries to function in the session.
-  launch_options.allow_new_privs = true;
+    if (WIFEXITED(status)) {
+      if (WEXITSTATUS(status) == EXIT_SUCCESS) {
+        LOG(INFO) << "Child exited successfully";
+      } else {
+        LOG(WARNING) << "Child exited with status " << WEXITSTATUS(status);
+      }
+    } else if (WIFSIGNALED(status)) {
+      LOG(WARNING) << "Child terminated by signal " << WTERMSIG(status);
+    }
 
-  launch_options.kill_on_parent_death = true;
-
-  launch_options.clear_environ = true;
-  launch_options.environ = std::move(*pam_environment);
-  launch_options.environ["USER"] = pwinfo->pw_name;
-  launch_options.environ["LOGNAME"] = pwinfo->pw_name;
-  launch_options.environ["HOME"] = pwinfo->pw_dir;
-  launch_options.environ["SHELL"] = pwinfo->pw_shell;
-  if (!launch_options.environ.count("PATH")) {
-    launch_options.environ["PATH"] = "/bin:/usr/bin";
+    // Best effort PAM cleanup
+    if (pam_handle.CloseSession(0) != PAM_SUCCESS) {
+      LOG(WARNING) << "Failed to close PAM session";
+    }
+    ignore_result(pam_handle.SetCredentials(PAM_DELETE_CRED));
   }
-
-  launch_options.pre_exec_delegate = &pre_exec_delegate;
-
-  // By convention, a login shell is signified by preceeding the shell name in
-  // argv[0] with a '-'.
-  base::CommandLine command_line(base::FilePath(
-      '-' + base::FilePath(pwinfo->pw_shell).BaseName().value()));
-
-  base::Optional<std::string> escaped_script_path =
-      ShellEscapeArgument(script_path);
-
-  CHECK(escaped_script_path) << "Could not escape script path";
-
-  command_line.AppendSwitch("-c");
-  command_line.AppendArg(*escaped_script_path +
-                         " --start --foreground --keep-parent-env");
-
-  // Tell LaunchProcess where to find the executable, since argv[0] doesn't
-  // point to it.
-  launch_options.real_path = base::FilePath(pwinfo->pw_shell);
-
-  //////////////////////////////////////////////////////////////////////////////
-  // We're ready to execute the remoting session
-  //////////////////////////////////////////////////////////////////////////////
-
-  base::Process child = base::LaunchProcess(command_line, launch_options);
-
-  if (child.IsValid()) {
-    int exit_code = 0;
-    // Die if wait fails so we don't close the PAM session while the child is
-    // still running.
-    CHECK(child.WaitForExit(&exit_code)) << "Failed to wait for child process";
-    LOG_IF(WARNING, exit_code != 0) << "Child did not exit normally";
-  }
-
-  // Best effort PAM cleanup
-  if (pam_handle.CloseSession(0) != PAM_SUCCESS) {
-    LOG(WARNING) << "Failed to close PAM session";
-  }
-  ignore_result(pam_handle.SetCredentials(PAM_DELETE_CRED));
 }
 
 // Opens a temp file for logging. Exits the program on failure.
 int OpenLogFile() {
   char logfile[265];
   std::time_t time = std::time(nullptr);
   CHECK_NE(time, (std::time_t)(-1));
   // Safe because we're single threaded
   std::tm* localtime = std::localtime(&time);
   CHECK_NE(std::strftime(logfile, sizeof(logfile), kLogFileTemplate, localtime),
            (std::size_t) 0);
 
   mode_t mode = umask(0177);
   int fd = mkstemp(logfile);
   PCHECK(fd != -1);
   umask(mode);
 
   return fd;
 }
 
+// Find the username for the current user. If either USER or LOGNAME is set to
+// a user matching our real user id, we return that. Otherwise, we use getpwuid
+// to attempt a reverse lookup. Note: It's possible to more than user with the

[Lambros, 2017/07/20 01:42:50]

.. to have more ..

Is this really true? I thought user IDs were unique on a system?

[rkjnsn, 2017/07/20 06:45:52]

Yes, I should probably word this differently. Having two different people with
the same user ID would be a very bad idea. However, what one can do is have
multiple passwd entries for the same user, distinguished by having different
user names, with potentially different group membership, home directories,
default shells, et cetera.

Here's an example of someone doing this in the real world:
https://askubuntu.com/a/427257 (One could probably argue that there's a better
way to solve that particular problem, but the point is that people do do it in
practice.)

+// same user id but different group membership, home directories, et cetera,
+// which is why we check USER and LOGNAME first.
+std::string FindCurrentUsername() {
+  uid_t real_uid = getuid();
+  struct passwd* pwinfo;
+  for (const char* var : {"USER", "LOGNAME"}) {
+    const char* value = getenv(var);
+    if (value) {
+      pwinfo = getpwnam(value);
+      // USER and LOGNAME can be overridden, so make sure the value is valid
+      // and matches the UID of the invoking user.
+      if (pwinfo && pwinfo->pw_uid == real_uid) {
+        return pwinfo->pw_name;
+      }
+    }
+  }
+  errno = 0;
+  pwinfo = getpwuid(real_uid);
+  PCHECK(pwinfo) << "getpwuid failed";
+  return pwinfo->pw_name;
+}
+
 // Daemonizes the process. Output is redirected to a file. Exits the program on
 // failure.
 //
 // This logic is mostly the same as daemonize() in linux_me2me_host.py. Log-
 // file redirection especially should be kept in sync. Note that this does
 // not currently wait for the host to start successfully before exiting the
 // parent process like the Python script does, as that functionality is
 // probably not useful at boot, where the wrapper is expected to be used. If
 // it turns out to be desired, it can be implemented by setting up a pipe and
 // passing a file descriptor to the Python script.
 void Daemonize() {
-
   int log_fd = OpenLogFile();
   int devnull_fd = open("/dev/null", O_RDONLY);
   PCHECK(devnull_fd != -1);
 
   PCHECK(dup2(devnull_fd, STDIN_FILENO) != -1);
   PCHECK(dup2(log_fd, STDOUT_FILENO) != -1);
   PCHECK(dup2(log_fd, STDERR_FILENO) != -1);
 
   // Close all file descriptors except stdio, including any we may have
   // inherited.
@@ skipping 24 matching lines @@
   // dropped privileges, so change to / to make sure we're not keeping any other
   // directory in use.
   PCHECK(chdir("/") == 0);
 
   // Done!
 }
 
 }  // namespace
 
 int main(int argc, char** argv) {
-  base::CommandLine::Init(argc, argv);
-
-  const base::CommandLine* command_line =
-      base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(kHelpSwitchName) ||
-      command_line->HasSwitch(kQuestionSwitchName)) {
-    PrintUsage(command_line->GetProgram());
-    std::exit(EXIT_SUCCESS);
-  }
-
-  base::FilePath script_path =
-      command_line->GetSwitchValuePath(kScriptSwitchName);
-  std::string user = command_line->GetSwitchValueNative(kUserSwitchName);
-  bool foreground = command_line->HasSwitch(kForegroundSwitchName);
-
-  if (script_path.empty()) {
-    std::fputs("The path to the me2me python script is required.\n", stderr);
+  if (argc < 2 || strcmp(argv[1], "start") != 0) {

[Lambros, 2017/07/20 01:42:50]

Why remove base::CommandLine() ? The code manipulating argc/argv directly is
much harder to read and check for correctness.

[rkjnsn, 2017/07/20 06:45:51]

This is something that specifically came up in security review. Command-line
parsing is not something that's usually security sensitive, as exploiting a bug
in command-line parsing doesn't generally let you do anything you couldn't do,
otherwise. In the case of of a setuid binary, however, a bug and command-line
parsing can lead to a privilege escalation vulnerability, especially given that
the user can pass whatever they want as the command-line. base::CommandLine has
a fair amount of code to support features we don't need for the wrapper, and
even if we audited it today, there's no guarantee that a bug won't get added,
tomorrow.

By using very basic command-line parsing, we hopefully reduce the attack surface
and make it more likely that future changes will receive the appropriate amount
of scrutiny.

I agree that manual parsing is harder to read, though. Let me know if you have
any suggestions for improving it.

+    PrintUsage();
     std::exit(EXIT_FAILURE);
   }
 
-  if (user.empty()) {
-    std::fputs("The target user must be specified.\n", stderr);
-    std::exit(EXIT_FAILURE);
+  // Skip initial args
+  argc -= 2;
+  argv += 2;
+
+  bool foreground = false;
+  if (argc >= 1 && strcmp(argv[0], kForegroundFlag) == 0) {
+    foreground = true;
+    argc -= 1;
+    argv += 1;
   }
 
+  if (argc > 1) {
+    std::fputs("Too many command-line arguments.\n", stderr);

[Jamie, 2017/07/20 16:56:48]

exit here?

[rkjnsn, 2017/07/20 17:39:03]

Doh!

+  }
+
+  uid_t real_uid = getuid();
+  std::string user;
+
+  // Note: This logic is security sensitive. It is imperative that a non-root
+  // user is not allowed to specify an arbitrary target user.
+  if (argc == 0) {
+    if (real_uid == 0) {
+      std::fputs("Target user must be specified when run as root.\n", stderr);
+      std::exit(EXIT_FAILURE);
+    }
+    user = FindCurrentUsername();
+  } else {
+    if (real_uid != 0) {
+      std::fputs("Target user may not be specified by non-root users.\n",
+                 stderr);
+      std::exit(EXIT_FAILURE);
+    }
+    user = argv[0];
+  }
+
+  bool chown_stdout = false;

[Lambros, 2017/07/20 01:42:49]

Why add a new flag?

If you prefer this new flag for readability, better to declare it just before
it's used:

bool chown_stdout = !foreground;
ExecuteSession(..chown_stdout..);

[rkjnsn, 2017/07/20 06:45:51]

I thought this was more readable, both because one can tell what the argument
does without referring to the declaration of ExecuteSession and because it makes
it more apparent why it should be the negation of foreground. You're way
probably works just as well if I keep the comment, though.

   if (!foreground) {
     Daemonize();
+    // Daemonizing redirects stdout to a log file, which we want to be owned by
+    // the target user.
+    chown_stdout = true;
   }
 
-  ExecuteSession(std::move(user), script_path.value(), !foreground);
+  ExecuteSession(std::move(user), chown_stdout,
+                 real_uid != 0 ? base::make_optional(real_uid) : base::nullopt);
 }