GNU bug report logs - #11108
chmod: fix symlink race condition

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 11108 in the body.
You can then email your comments to 11108 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] chmod: fix symlink race condition
Date: Tue, 27 Mar 2012 22:28:31 -0700

This fixes what I hope is an obvious race condition
that can occur if some other process substitutes a
symlink for a non-symlink while chmod is running.

=====
* src/chmod.c (process_file): Don't follow symlink if we
think the file is not a symlink.
---
 src/chmod.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/src/chmod.c b/src/chmod.c
index aa4ac77..2e1f1c7 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -268,7 +268,15 @@ process_file (FTS *fts, FTSENT *ent)
 
       if (! S_ISLNK (old_mode))
         {
-          if (chmodat (fts->fts_cwd_fd, file, new_mode) == 0)
+          /* Use any native support for AT_SYMLINK_NOFOLLOW, to avoid
+             following a symlink if there is a race.  */
+          #if HAVE_FCHMODAT || HAVE_LCHMOD
+          int follow_flag = AT_SYMLINK_NOFOLLOW;
+          #else
+          int follow_flag = 0;
+          #endif
+
+          if (fchmodat (fts->fts_cwd_fd, file, new_mode, follow_flag) == 0)
             chmod_succeeded = true;
           else
             {
-- 
1.7.6.5

Message #8 received at 11108 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 11108 <at> debbugs.gnu.org
Subject: Re: bug#11108: [PATCH] chmod: fix symlink race condition
Date: Wed, 28 Mar 2012 09:36:01 +0200

Paul Eggert wrote:
> This fixes what I hope is an obvious race condition
> that can occur if some other process substitutes a
> symlink for a non-symlink while chmod is running.

Good catch.  I'll bet that's exploitable by anyone who
can convince root to run "chmod -r ... DIR" on files they own.

The chmodat-introducing commit was v5.92-656-gc97a36e,
but the preceding use of chmod was just as vulnerable.
If you reference a commit in your log, please use "git describe"
output, not the bare-8-byte-SHA1 like we've done in the past.
While "git describe" output is not converted to a clickable link
by a released gitk, with the one in upcoming git-1.7.10, it is.

I presume you'll update NEWS, too, where you can say
[bug introduced in the beginning]
I've confirmed that the very first version of chmod.c has the same
problem: it calls stat, then calls chmod whenever !S_ISLNK.

I note also that this doesn't protect anyone who is using
a system that lacks both fchmodat and lchmod.
For that, we'd have to openat each file to get a file descriptor,
then fstat that FD to verify it's the same dev/ino as
found by the fts-run stat call, and only then, call fchmod.

> =====
> * src/chmod.c (process_file): Don't follow symlink if we
> think the file is not a symlink.
> ---
>  src/chmod.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/src/chmod.c b/src/chmod.c
> index aa4ac77..2e1f1c7 100644
> --- a/src/chmod.c
> +++ b/src/chmod.c
> @@ -268,7 +268,15 @@ process_file (FTS *fts, FTSENT *ent)
>
>        if (! S_ISLNK (old_mode))
>          {
> -          if (chmodat (fts->fts_cwd_fd, file, new_mode) == 0)
> +          /* Use any native support for AT_SYMLINK_NOFOLLOW, to avoid
> +             following a symlink if there is a race.  */
> +          #if HAVE_FCHMODAT || HAVE_LCHMOD
> +          int follow_flag = AT_SYMLINK_NOFOLLOW;
> +          #else
> +          int follow_flag = 0;
> +          #endif
> +
> +          if (fchmodat (fts->fts_cwd_fd, file, new_mode, follow_flag) == 0)
>              chmod_succeeded = true;
>            else
>              {

Message #11 received at 11108 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 11108 <at> debbugs.gnu.org
Subject: Re: bug#11108: [PATCH] chmod: fix symlink race condition
Date: Wed, 28 Mar 2012 11:11:29 -0700

On 03/28/2012 12:36 AM, Jim Meyering wrote:
> I presume you'll update NEWS, too, where you can say
> [bug introduced in the beginning]

Thanks, good point.  I did that in the version I just committed
to the master.

> I note also that this doesn't protect anyone who is using
> a system that lacks both fchmodat and lchmod.

Right; I put that in the NEWS entry.

There are still problems, in the sense that the attacker
can use a hard link to target any visible file on the same filesystem,
by using hard links; but this problem is unavoidable.

> we'd have to openat each file to get a file descriptor,
> then fstat that FD to verify it's the same dev/ino as
> found by the fts-run stat call, and only then, call fchmod.

This might be useful to close other (more-subtle) races
involving things like hard-link manipulation and chmod +X,
where the new mode depends on the old.  A general problem
with using 'open' for this sort of thing, though,
is that 'open' can have side effects on devices.  I wish
there was a variant of 'open' guaranteed to never
hang and never have side effects; then we could play this
sort of game more reliably.

Message #14 received at 11108 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 11108 <at> debbugs.gnu.org
Subject: Re: bug#11108: [PATCH] chmod: fix symlink race condition
Date: Wed, 28 Mar 2012 21:32:29 +0200

Paul Eggert wrote:
> On 03/28/2012 12:36 AM, Jim Meyering wrote:
>> I presume you'll update NEWS, too, where you can say
>> [bug introduced in the beginning]
>
> Thanks, good point.  I did that in the version I just committed
> to the master.
>
>> I note also that this doesn't protect anyone who is using
>> a system that lacks both fchmodat and lchmod.
>
> Right; I put that in the NEWS entry.
>
> There are still problems, in the sense that the attacker
> can use a hard link to target any visible file on the same filesystem,
> by using hard links; but this problem is unavoidable.
>
>> we'd have to openat each file to get a file descriptor,
>> then fstat that FD to verify it's the same dev/ino as
>> found by the fts-run stat call, and only then, call fchmod.
>
> This might be useful to close other (more-subtle) races
> involving things like hard-link manipulation and chmod +X,
> where the new mode depends on the old.  A general problem
> with using 'open' for this sort of thing, though,
> is that 'open' can have side effects on devices.  I wish
> there was a variant of 'open' guaranteed to never
> hang and never have side effects; then we could play this
> sort of game more reliably.

Oops.  I should not have suggested using open, since it cannot
work in general: it would fail for any file that is neither
readable nor writable.

Message #17 received at 11108 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 11108 <at> debbugs.gnu.org
Subject: Re: bug#11108: [PATCH] chmod: fix symlink race condition
Date: Wed, 28 Mar 2012 22:13:12 +0200

Paul Eggert wrote:
> On 03/28/2012 12:36 AM, Jim Meyering wrote:
>> I presume you'll update NEWS, too, where you can say
>> [bug introduced in the beginning]
>
> Thanks, good point.  I did that in the version I just committed
> to the master.

Rats:

    $ ./chmod u+w f
    ./chmod: changing permissions of 'f': Operation not supported

That fix introduces chmod failures on several important systems,
including my Fedora 17 desktop ;-)

I confess that I had not tested it, and had missed or forgotten
this part of the GNU/Linux/fchmodat documentation:

       AT_SYMLINK_NOFOLLOW
              If pathname is a symbolic link, do not dereference  it:  instead
              operate  on  the link itself.  This flag is not currently imple-
              mented.

The nixos/hydra build server is reporting failures, too:

  http://hydra.nixos.org/build/2341393
  http://hydra.nixos.org/build/2341397
  http://hydra.nixos.org/build/2341395

Message #20 received at 11108 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: 11108 <at> debbugs.gnu.org
Subject: Re: bug#11108: [PATCH] chmod: fix symlink race condition
Date: Wed, 28 Mar 2012 13:28:01 -0700

On 03/28/2012 01:13 PM, Jim Meyering wrote:
>     $ ./chmod u+w f
>     ./chmod: changing permissions of 'f': Operation not supported

Yeouch.  I undid the change for now.
Hmm, why did "make check" work for me?
I'll have to investigate later, alas.

Message #33 received at 11108-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 11108-done <at> debbugs.gnu.org
Subject: Re: bug#11108: [PATCH] chmod: fix symlink race condition
Date: Wed, 20 Mar 2024 19:01:22 +0000

On 28/03/2012 21:28, Paul Eggert wrote:
> On 03/28/2012 01:13 PM, Jim Meyering wrote:
>>      $ ./chmod u+w f
>>      ./chmod: changing permissions of 'f': Operation not supported
> 
> Yeouch.  I undid the change for now.
> Hmm, why did "make check" work for me?
> I'll have to investigate later, alas.

Patch for this pushed at:
https://git.sv.gnu.org/cgit/coreutils.git/commit/?id=v9.4-163-g425b8a2f5

Marking this as done.

cheers,
Pádraig.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.