RFC: avoid chroot() call if not changing root dir

[Pádraig Brady]

I was considering changing the coreutils tests from using
the internal setuidgid test utility to chroot --user...
to give more coverage for the latter.

However I noticed that chroot changes directory to /
even when we're just using it to change user IDs
(by specifying / as the "new" root).

So I was wondering about the attached to avoid the
chdir("/") in this case?  The chdir() is done to enhance
security a bit, but also for consistency. If we'd not
actually changing root dir then there are no security implications,
but I'm a bit worried about the consistency angle.
Now --user is a newish feature anyway so I'm not
that worried about breaking old scripts that assume
the implicit chdir("/").

thanks,
Pádraig.

>From 9c1bad82852cec8403ead49f12f53280c468a2cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Tue, 13 May 2014 15:56:34 +0100
Subject: [PATCH] chroot: don't chdir() if not changing root

This allows chroot to use used as a light weight tool
to change user identification for a command,
while not changing the current working directory.

* src/chroot.c (main): If the same root is specified. i.e. '/'
then don't change the current working directory, and avoid the
overhead of the other redundant calls.
---
 src/chroot.c |   36 +++++++++++++++++++++---------------
 1 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index 8b08b84..a2debac 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -225,21 +225,27 @@ main (int argc, char **argv)
       usage (EXIT_CANCELED);
     }
 
-  /* We have to look up users and groups twice.
-     - First, outside the chroot to load potentially necessary passwd/group
-       parsing plugins (e.g. NSS);
-     - Second, inside chroot to redo the parsing in case IDs are different.  */
-  if (userspec)
-    ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL));
-  if (groups && *groups)
-    ignore_value (parse_additional_groups (groups, &out_gids, &n_gids, false));
-
-  if (chroot (argv[optind]) != 0)
-    error (EXIT_CANCELED, errno, _("cannot change root directory to %s"),
-           argv[optind]);
-
-  if (chdir ("/"))
-    error (EXIT_CANCELED, errno, _("cannot chdir to root directory"));
+  /* Only do chroot specific actions if actually changing root.
+     The main difference here is that we don't change working dir.  */
+  if (! STREQ (argv[optind], "/"))
+    {
+      /* We have to look up users and groups twice.
+        - First, outside the chroot to load potentially necessary passwd/group
+          parsing plugins (e.g. NSS);
+        - Second, inside chroot to redo parsing in case IDs are different.  */
+      if (userspec)
+        ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL));
+      if (groups && *groups)
+        ignore_value (parse_additional_groups (groups, &out_gids, &n_gids,
+                                               false));
+
+      if (chroot (argv[optind]) != 0)
+        error (EXIT_CANCELED, errno, _("cannot change root directory to %s"),
+               argv[optind]);
+
+      if (chdir ("/"))
+        error (EXIT_CANCELED, errno, _("cannot chdir to root directory"));
+    }
 
   if (argc == optind + 1)
     {
-- 
1.7.7.6