On 05/17/2014 10:40 AM, Pádraig Brady wrote:
> On 05/17/2014 01:03 AM, Bernhard Voelker wrote:
>> On 05/16/2014 11:02 PM, Pádraig Brady wrote:
>>> Pushed.
>>
>> Sorry, a bit late ...
>>
>>> + /* 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], "/"))
>>
>> What about canonicalizing argv[optind] ?
>> Or do we want the ability to force chroot(2) like this:
>>
>> $ src/chroot / env pwd
>> /home/berny/git/coreutils
>>
>> $ src/chroot /. env pwd
>> src/chroot: cannot change root directory to /.: Operation not permitted
>>
>> Probably this might be bit confusing - while some other guys
>> might use this difference to check for superuser privileges ...
>> ... and fall over once we'll fix this.
>
> Yes I wasn't sure about that.
> Better to canonicalize for consistency I suppose.
> If one does want to chdir("/") that can be done externally and inherited.
I'll push the attached later.
thanks,
Pádraig.
>From 70c4ffe8489334953c75b4a812c549ed5b72f03e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
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
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
+ test "$curdir" = '/' && fail=1
+done
Exit $fail
--
1.7.7.6
