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

[Bernhard Voelker]

On 05/18/2014 06:27 PM, Pádraig Brady wrote:
> From 70c4ffe8489334953c75b4a812c549ed5b72f03e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?P=C3=A1draig=20Brady?= <address@hidden>
> Date: Sun, 18 May 2014 17:20:06 +0100
> Subject: [PATCH] chroot: make changing root check more robust
> 
> * src/chroot.c (is_root): A new helper function to
> determine if the passed argrument is the root directory

s/argrument/argument/

> based on inode comparison.
> (main): Use the new helper rather than comparing strings.
> * tests/misc/chroot-fail.sh: Add cases for alternative root paths.
> ---
>  src/chroot.c              |   19 ++++++++++++++++++-
>  tests/misc/chroot-fail.sh |    6 ++++--
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/src/chroot.c b/src/chroot.c
> index 0ded25d..a623f88 100644
> --- a/src/chroot.c
> +++ b/src/chroot.c
> @@ -28,6 +28,7 @@
>  #include "ignore-value.h"
>  #include "mgetgroups.h"
>  #include "quote.h"
> +#include "root-dev-ino.h"
>  #include "userspec.h"
>  #include "xstrtol.h"
>  
> @@ -158,6 +159,22 @@ parse_additional_groups (char const *groups, GETGROUPS_T 
> **pgids,
>    return ret;
>  }
>  
> +static bool
> +is_root (const char* dir)
> +{
> +  struct dev_ino root_ino;
> +  if (! get_root_dev_ino (&root_ino))
> +    error (EXIT_CANCELED, errno, _("failed to get attributes of %s"),
> +           quote ("/"));
> +
> +  struct stat arg_st;
> +  if (stat (dir, &arg_st) == -1)
> +    error (EXIT_CANCELED, errno, _("failed to get attributes of %s"),
> +           quote (dir));
> +
> +  return SAME_INODE (root_ino, arg_st);
> +}
> +
>  void
>  usage (int status)
>  {
> @@ -253,7 +270,7 @@ main (int argc, char **argv)
>  
>    /* 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], "/"))
> +  if (! is_root (argv[optind]))
>      {
>        /* We have to look up users and groups twice.
>          - First, outside the chroot to load potentially necessary 
> passwd/group
> diff --git a/tests/misc/chroot-fail.sh b/tests/misc/chroot-fail.sh
> index 56be8e2..5e2ef6e 100755
> --- a/tests/misc/chroot-fail.sh
> +++ b/tests/misc/chroot-fail.sh
> @@ -39,7 +39,9 @@ test $? = 127 || fail=1
>  
>  # Ensure we don't chdir("/") when not changing root
>  # to allow only changing user ids for a command.
> -curdir=$(chroot / env pwd) || fail=1
> -test "$curdir" = '/' && fail=1
> +for dir in '/' '/.' '/../'; do
> +  curdir=$(chroot / env pwd) || fail=1

Please use $dir instead of / here, otherwise it's
doing 3x the same.

> +  test "$curdir" = '/' && fail=1
> +done
>  
>  Exit $fail
> -- 1.7.7.6

Otherwise +1

Thanks & have a nice day,
Berny