[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: chown regression from coreutils 5.2.1 for execute-only files
From: |
Paul Eggert |
Subject: |
Re: chown regression from coreutils 5.2.1 for execute-only files |
Date: |
Mon, 02 Jan 2006 22:25:07 -0800 |
User-agent: |
Gnus/5.1007 (Gnus v5.10.7) Emacs/21.4 (gnu/linux) |
Following up on my own email
<http://lists.gnu.org/archive/html/bug-coreutils/2006-01/msg00006.html>
it appears that an lchown-based approach doesn't work as I'd hoped,
because fts doesn't easily tell me whether the file we're visiting is
actually a symlink. However, I can fix the bug reported in that email
without resorting to lchown, as follows, so I installed this. I
included the idea of the bug report in a new test case.
2006-01-02 Paul Eggert <address@hidden>
* src/chown-core.c (RC_do_ordinary_chown): New enum value.
(restricted_chown): Return it, if the file cannot be accessed due
to EPERM, or if no uid or gid are required, or if the file is
neither a directory nor a regular file. Rewrite to avoid gotos.
(change_file_owner): Handle RC_do_ordinary_chown case.
Rewrite to avoid gotos.
* tests/chgrp/basic: Make sure we can change the group of
inaccessible files.
Index: src/chown-core.c
===================================================================
RCS file: /fetish/cu/src/chown-core.c,v
retrieving revision 1.38
diff -p -u -r1.38 chown-core.c
--- src/chown-core.c 27 Dec 2005 07:59:00 -0000 1.38
+++ src/chown-core.c 3 Jan 2006 06:11:57 -0000
@@ -1,5 +1,5 @@
/* chown-core.c -- core functions for changing ownership.
- Copyright (C) 2000, 2002, 2003, 2004, 2005 Free Software Foundation.
+ Copyright (C) 2000, 2002, 2003, 2004, 2005, 2006 Free Software Foundation.
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -43,6 +43,10 @@ enum RCH_status
/* SAME_INODE check failed */
RC_inode_changed,
+ /* open/fchown isn't needed, isn't safe, or doesn't work due to
+ permissions problems; fall back on chown */
+ RC_do_ordinary_chown,
+
/* open, fstat, fchown, or close failed */
RC_error
};
@@ -159,10 +163,7 @@ describe_change (const char *file, enum
/* Change the owner and/or group of the FILE to UID and/or GID (safely)
only if REQUIRED_UID and REQUIRED_GID match the owner and group IDs
- of FILE. ORIG_ST must be the result of `stat'ing FILE. If this
- function ends up being called with a FILE that is a symlink and when
- we are *not* dereferencing symlinks, we'll detect the problem via
- the SAME_INODE test below.
+ of FILE. ORIG_ST must be the result of `stat'ing FILE.
The `safely' part above means that we can't simply use chown(2),
since FILE might be replaced with some other file between the time
@@ -172,14 +173,9 @@ describe_change (const char *file, enum
the preceding stat call, and only then, if appropriate (given the
required_uid and required_gid constraints) do we call fchown.
- A minor problem:
- This function fails when FILE cannot be opened, but chown/lchown have
- no such limitation. But this may not be a problem for chown(1),
- since chown is useful mainly to root, and since root seems to have
- no problem opening `unreadable' files (on Linux). However, this can
- cause trouble when non-root users apply chgrp to files they own but
- to which they have neither read nor write access. For now, that
- isn't a problem since chgrp doesn't have a --from=O:G option.
+ Return RC_do_ordinary_chown if we can't open FILE, or if FILE is a
+ special file that might have undesirable side effects when opening.
+ In this case the caller can use the less-safe ordinary chown.
Return one of the RCH_status values. */
@@ -192,27 +188,31 @@ restricted_chown (char const *file,
enum RCH_status status = RC_ok;
struct stat st;
int open_flags = O_NONBLOCK | O_NOCTTY;
+ int fd;
- int fd = open (file, O_RDONLY | open_flags);
- if (! (0 <= fd
- || (errno == EACCES
- && 0 <= (fd = open (file, O_WRONLY | open_flags)))))
- return RC_error;
+ if (required_uid == (uid_t) -1 && required_gid == (gid_t) -1)
+ return RC_do_ordinary_chown;
- if (fstat (fd, &st) != 0)
+ if (! S_ISREG (orig_st->st_mode))
{
- status = RC_error;
- goto Lose;
+ if (S_ISDIR (orig_st->st_mode))
+ open_flags |= O_DIRECTORY;
+ else
+ return RC_do_ordinary_chown;
}
- if ( ! SAME_INODE (*orig_st, st))
- {
- status = RC_inode_changed;
- goto Lose;
- }
+ fd = open (file, O_RDONLY | open_flags);
+ if (! (0 <= fd
+ || (errno == EACCES && S_ISREG (orig_st->st_mode)
+ && 0 <= (fd = open (file, O_WRONLY | open_flags)))))
+ return (errno == EACCES ? RC_do_ordinary_chown : RC_error);
- if ((required_uid == (uid_t) -1 || required_uid == st.st_uid)
- && (required_gid == (gid_t) -1 || required_gid == st.st_gid))
+ if (fstat (fd, &st) != 0)
+ status = RC_error;
+ else if (! SAME_INODE (*orig_st, st))
+ status = RC_inode_changed;
+ else if ((required_uid == (uid_t) -1 || required_uid == st.st_uid)
+ && (required_gid == (gid_t) -1 || required_gid == st.st_gid))
{
if (fchown (fd, uid, gid) == 0)
{
@@ -226,7 +226,6 @@ restricted_chown (char const *file,
}
}
- Lose:
{ /* FIXME: remove these curly braces when we assume C99. */
int saved_errno = errno;
close (fd);
@@ -346,43 +345,41 @@ change_file_owner (FTS *fts, FTSENT *ent
}
else
{
- if ( required_uid == (uid_t) -1 && required_gid == (gid_t) -1)
+ /* If possible, avoid a race condition with --from=O:G and without the
+ (-h) --no-dereference option. If fts's stat call determined
+ that the uid/gid of FILE matched the --from=O:G-selected
+ owner and group IDs, blindly using chown(2) here could lead
+ chown(1) or chgrp(1) mistakenly to dereference a *symlink*
+ to an arbitrary file that an attacker had moved into the
+ place of FILE during the window between the stat and
+ chown(2) calls. If FILE is a regular file or a directory
+ that can be opened, this race condition can be avoided safely. */
+
+ enum RCH_status err
+ = restricted_chown (file, file_stats, uid, gid,
+ required_uid, required_gid);
+ switch (err)
{
+ case RC_ok:
+ break;
+
+ case RC_do_ordinary_chown:
ok = (chown (file, uid, gid) == 0);
- }
- else
- {
- /* Avoid a race condition with --from=O:G and without the
- (-h) --no-dereference option. If fts' stat call determined
- that the uid/gid of FILE matched the --from=O:G-selected
- owner and group IDs, blindly using chown(2) here could lead
- chown(1) or chgrp(1) mistakenly to dereference a *symlink*
- to an arbitrary file that an attacker had moved into the
- place of FILE during the window between the stat and
- chown(2) calls. */
- enum RCH_status err
- = restricted_chown (file, file_stats, uid, gid,
- required_uid, required_gid);
- switch (err)
- {
- case RC_ok:
- ok = true;
- break;
-
- case RC_error:
- ok = false;
- break;
-
- case RC_inode_changed:
- /* FIXME: give a diagnostic in this case? */
- case RC_excluded:
- do_chown = false;
- ok = false;
- goto Skip_chown;
-
- default:
- abort ();
- }
+ break;
+
+ case RC_error:
+ ok = false;
+ break;
+
+ case RC_inode_changed:
+ /* FIXME: give a diagnostic in this case? */
+ case RC_excluded:
+ do_chown = false;
+ ok = false;
+ break;
+
+ default:
+ abort ();
}
}
@@ -393,15 +390,13 @@ change_file_owner (FTS *fts, FTSENT *ent
by some other user and operating on files in a directory
where M has write access. */
- if (!ok && ! chopt->force_silent)
+ if (do_chown && !ok && ! chopt->force_silent)
error (0, errno, (uid != (uid_t) -1
? _("changing ownership of %s")
: _("changing group of %s")),
quote (file_full_name));
}
- Skip_chown:;
-
if (chopt->verbosity != V_off)
{
bool changed =
Index: tests/chgrp/basic
===================================================================
RCS file: /fetish/cu/tests/chgrp/basic,v
retrieving revision 1.21
diff -p -u -r1.21 basic
--- tests/chgrp/basic 18 Oct 2005 09:14:36 -0000 1.21
+++ tests/chgrp/basic 3 Jan 2006 06:11:57 -0000
@@ -74,8 +74,15 @@ test "$VERBOSE" = yes && set +x
chgrp -R $g2 symlink
chown --from=:$g1 -c :$g2 f
+ # Make sure we can change the group of inaccessible files.
+ chmod a-r f
+ chown --from=:$g2 -c :$g1 f
+ chmod 0 f
+ chown --from=:$g1 -c :$g2 f
+
# chown() must not be optimized away even when
# the file's owner and group already have the desired value.
+ rm -f f g
touch f g
chgrp $g1 f g
chgrp $g2 g
@@ -115,6 +122,8 @@ changed ownership of `f' to :G2
changed group of `symlink' to G1
changed ownership of `f' to :G2
changed ownership of `f' to :G2
+changed ownership of `f' to :G1
+changed ownership of `f' to :G2
f
g
EOF