[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: bug in chdir-safer
From: |
Paul Eggert |
Subject: |
Re: bug in chdir-safer |
Date: |
Sun, 25 Dec 2005 13:41:30 -0800 |
User-agent: |
Gnus/5.1007 (Gnus v5.10.7) Emacs/21.4 (gnu/linux) |
Jim Meyering <address@hidden> writes:
> * chdir-safer.c (chdir_no_follow): Remove unnecessary
> test of S_ISDIR (sb_init.st_mode).
I had noticed that problem along with some others with chdir-safer.c.
I installed the following patch to clean up the other issues I saw.
2005-12-25 Paul Eggert <address@hidden>
* chdir-safer.h (FCHMOD_SAFER_H): Remove: it was misnamed, and
wasn't needed anyay.
* chdir-safer.c (chdir_no_follow): Don't include stdio.h, assert.h,
fcntl--.h; not needed.
(O_DIRECTORY): Define if not already defined.
(chdir_no_follow): Revamp describing comment to match code more
closely. Redo use of internal vars to avoid lint complaints.
Work even if directory is writeable but not readable.
Open with O_DIRECTORY | O_NOCTTY, for benefit of hosts that
don't have O_NOFOLLOW. Use O_NONBLOCK (POSIX spelling) rather
than O_NDELAY. Don't bother invoking fstat if open does not
dereference symlink, since the result isn't used then.
Don't assume file descriptor is positive; it might be zero
now that we no longer include fcntl--.h (we don't need fcntl--.h
since we immediately close the descriptor).
Index: lib/chdir-safer.h
===================================================================
RCS file: /fetish/cu/lib/chdir-safer.h,v
retrieving revision 1.1
diff -p -u -r1.1 chdir-safer.h
--- lib/chdir-safer.h 21 Dec 2005 09:42:36 -0000 1.1
+++ lib/chdir-safer.h 25 Dec 2005 21:33:53 -0000
@@ -1,4 +1,5 @@
-/* like chdir(2), but safer, if possible
+/* much like chdir(2), but safer
+
Copyright (C) 2005 Free Software Foundation, Inc.
This program is free software; you can redistribute it and/or modify
@@ -17,9 +18,4 @@
/* Written by Jim Meyering. */
-#ifndef FCHMOD_SAFER_H
-# define FCHMOD_SAFER_H 1
-
int chdir_no_follow (char const *file);
-
-#endif /* FCHMOD_SAFER_H */
Index: lib/chdir-safer.c
===================================================================
RCS file: /fetish/cu/lib/chdir-safer.c,v
retrieving revision 1.4
diff -p -u -r1.4 chdir-safer.c
--- lib/chdir-safer.c 25 Dec 2005 17:33:57 -0000 1.4
+++ lib/chdir-safer.c 25 Dec 2005 21:33:53 -0000
@@ -1,4 +1,5 @@
-/* like chdir(2), but safer, if possible
+/* much like chdir(2), but safer
+
Copyright (C) 2005 Free Software Foundation, Inc.
This program is free software; you can redistribute it and/or modify
@@ -24,17 +25,16 @@
#include "chdir-safer.h"
#include <stdbool.h>
-#include <stdio.h>
-#include <assert.h>
#include <fcntl.h>
#include <errno.h>
-#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
-#include "fcntl--.h" /* for the open->open_safer mapping */
+#ifndef O_DIRECTORY
+# define O_DIRECTORY 0
+#endif
-#if !defined O_NOFOLLOW
+#ifndef O_NOFOLLOW
# define O_NOFOLLOW 0
#endif
@@ -42,51 +42,52 @@
((Stat_buf_1).st_ino == (Stat_buf_2).st_ino \
&& (Stat_buf_1).st_dev == (Stat_buf_2).st_dev)
-/* Just like chmod, but fail if DIR is a symbolic link.
- This can avoid a minor race condition between when a
- directory is created or stat'd and when we chdir into it.
-
- Note that this function fails (while chdir would succeed)
- if DIR cannot be opened with O_RDONLY. */
+/* Like chdir, but fail if DIR is a symbolic link to a directory (or
+ similar funny business), or if DIR is neither readable nor
+ writeable. This avoids a minor race condition between when a
+ directory is created or statted and when the process chdirs into
+ it. */
int
chdir_no_follow (char const *dir)
{
- int fail = -1;
- struct stat sb;
- struct stat sb_init;
- int saved_errno = 0;
- int fd;
-
- bool open_dereferences_symlink = ! O_NOFOLLOW;
-
- /* If open follows symlinks, lstat DIR, to get its device and
- inode numbers. */
- if (open_dereferences_symlink && lstat (dir, &sb_init) != 0)
- return fail;
-
- fd = open (dir, O_NOFOLLOW | O_RDONLY | O_NDELAY);
-
- if (0 <= fd
- && fstat (fd, &sb) == 0
- /* If DIR is a different directory, then someone is trying to do
- something nasty. However, the risk of such an attack is so low
- that it isn't worth a special diagnostic. Simply skip the fchdir
- and set errno (to the same value that open uses for symlinks with
- O_NOFOLLOW), so that the caller can report the failure. */
- && ( ! open_dereferences_symlink || SAME_INODE (sb_init, sb)
- || ((errno = ELOOP), 0))
- && fchdir (fd) == 0)
+ int result = 0;
+ int saved_errno;
+ int open_flags = O_DIRECTORY | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK;
+ int fd = open (dir, O_RDONLY | open_flags);
+ if (fd < 0)
{
- fail = 0;
+ if (errno == EACCES)
+ fd = open (dir, O_WRONLY | open_flags);
+ if (fd < 0)
+ return fd;
}
- else
+
+ /* If open follows symlinks, lstat DIR and fstat FD to ensure that
+ they are the same file; if they are different files, set errno to
+ ELOOP (the same value that open uses for symlinks with
+ O_NOFOLLOW) so the caller can report a failure. */
+ if (! O_NOFOLLOW)
{
- saved_errno = errno;
+ struct stat sb1;
+ struct stat sb2;
+
+ result = lstat (dir, &sb1);
+ if (result == 0)
+ {
+ result = fstat (fd, &sb2);
+ if (result == 0 && ! SAME_INODE (sb1, sb2))
+ {
+ errno = ELOOP;
+ result = -1;
+ }
+ }
}
- if (0 < fd)
- close (fd); /* Ignore any failure. */
+ if (result == 0)
+ result = fchdir (fd);
+ saved_errno = errno;
+ close (fd);
errno = saved_errno;
- return fail;
+ return result;
}
- bug in chdir-safer, Eric Blake, 2005/12/22
- Re: bug in chdir-safer, Eric Blake, 2005/12/22
- Re: bug in chdir-safer, Jim Meyering, 2005/12/23
- Re: bug in chdir-safer, Eric Blake, 2005/12/24
- Re: bug in chdir-safer, Jim Meyering, 2005/12/25
- Re: bug in chdir-safer,
Paul Eggert <=
- Re: bug in chdir-safer, Jim Meyering, 2005/12/26
- Re: bug in chdir-safer, Paul Eggert, 2005/12/26
- Re: bug in chdir-safer, Paul Eggert, 2005/12/27