Subject: Re: bug#21534: Bug in mkdir?!
From: Paul Eggert <[email-masked]>
To: Sebastian Unger <[email-masked]>, [email-masked]
Date: Tue, 22 Sep 2015 20:09:35 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Sebastian Unger wrote:
> Why is it trying to open the directory
> in the first place?

Security.

Apparently POSIX doesn't allow this level of paranoia for mkdir -p, so I removed 
it in the attached Gnulib patch, and this should appear in the next coreutils 
release.

A filesystem that doesn't let you read your own directory that you just created 
is likely to run into other problems like this -- i.e., the practice may 
introduce more security problems than it closes.  But I digress.
From 1f869b77a7e1bb871d58d30edd2ea04a9ac04d32 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Tue, 22 Sep 2015 20:04:13 -0700
Subject: [PATCH] savewd: remove SAVEWD_CHDIR_READABLE

It was problematic in the light of file systems that ignore umask.
Problem reported by Sebastian Ungar in: http://bugs.gnu.org/21534
* NEWS: Document this.
* lib/mkancesdirs.c (mkancesdirs): MAKE_DIR now returns 0 if
successful, -1 (setting errno) on failure, rather than something
more complicated than that.
* lib/mkdir-p.c (make_dir_parents):
Do not use SAVEWD_CHDIR_READABLE.
* lib/savewd.c (savewd_chdir):
Remove support for SAVEWD_CHDIR_READABLE.
* lib/savewd.h (SAVEWD_CHDIR_READABLE): Remove.
---
 ChangeLog         | 13 +++++++++++++
 NEWS              |  2 ++
 lib/mkancesdirs.c | 24 ++++++------------------
 lib/mkdir-p.c     |  4 +---
 lib/savewd.c      |  2 +-
 lib/savewd.h      |  9 ++-------
 6 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 62458b2..1bcc6f1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,18 @@
 2015-09-22  Paul Eggert  <[email-masked]>
 
+	savewd: remove SAVEWD_CHDIR_READABLE
+	It was problematic in the light of file systems that ignore umask.
+	Problem reported by Sebastian Ungar in: http://bugs.gnu.org/21534
+	* NEWS: Document this.
+	* lib/mkancesdirs.c (mkancesdirs): MAKE_DIR now returns 0 if
+	successful, -1 (setting errno) on failure, rather than something
+	more complicated than that.
+	* lib/mkdir-p.c (make_dir_parents):
+	Do not use SAVEWD_CHDIR_READABLE.
+	* lib/savewd.c (savewd_chdir):
+	Remove support for SAVEWD_CHDIR_READABLE.
+	* lib/savewd.h (SAVEWD_CHDIR_READABLE): Remove.
+
 	c-ctype: port better to EBCDIC
 	Problems reported by Daniel Richard G. in
 	http://lists.gnu.org/archive/html/bug-gnulib/2015-09/msg00020.html
diff --git a/NEWS b/NEWS
index b294edd..b54a992 100644
--- a/NEWS
+++ b/NEWS
@@ -42,6 +42,8 @@ User visible incompatible changes
 
 Date        Modules         Changes
 
+2015-09-22  savewd          SAVEWD_CHDIR_READABLE constant removed.
+
 2015-07-24  fprintftime     Exported functions' time zone arguments are now of
             strftime        type timezone_t (with NULL denoting UTC) instead of
                             type int (with nonzero denoting UTC).  These
diff --git a/lib/mkancesdirs.c b/lib/mkancesdirs.c
index 91ed694..2747d4a 100644
--- a/lib/mkancesdirs.c
+++ b/lib/mkancesdirs.c
@@ -42,11 +42,9 @@
 
    Create any ancestor directories that don't already exist, by
    invoking MAKE_DIR (FILE, COMPONENT, MAKE_DIR_ARG).  This function
-   should return 0 if successful and the resulting directory is
-   readable, 1 if successful but the resulting directory might not be
-   readable, -1 (setting errno) otherwise.  If COMPONENT is relative,
-   it is relative to the temporary working directory, which may differ
-   from *WD.
+   should return 0 if successful, -1 (setting errno) otherwise.  If
+   COMPONENT is relative, it is relative to the temporary working
+   directory, which may differ from *WD.
 
    Ordinarily MAKE_DIR is executed with the working directory changed
    to reflect the already-made prefix, and mkancesdirs returns with
@@ -112,20 +110,10 @@ mkancesdirs (char *file, struct savewd *wd,
             if (sep - component == 2
                 && component[0] == '.' && component[1] == '.')
               made_dir = false;
+            else if (make_dir (file, component, make_dir_arg) < 0)
+              make_dir_errno = errno;
             else
-              switch (make_dir (file, component, make_dir_arg))
-                {
-                case -1:
-                  make_dir_errno = errno;
-                  break;
-
-                case 0:
-                  savewd_chdir_options |= SAVEWD_CHDIR_READABLE;
-                  /* Fall through.  */
-                case 1:
-                  made_dir = true;
-                  break;
-                }
+              made_dir = true;
 
             if (made_dir)
               savewd_chdir_options |= SAVEWD_CHDIR_NOFOLLOW;
diff --git a/lib/mkdir-p.c b/lib/mkdir-p.c
index e55b1dd..e518272 100644
--- a/lib/mkdir-p.c
+++ b/lib/mkdir-p.c
@@ -136,9 +136,7 @@ make_dir_parents (char *dir,
               announce (dir, options);
               preserve_existing = (keep_owner & keep_special_mode_bits
                                    & umask_must_be_ok);
-              savewd_chdir_options |=
-                (SAVEWD_CHDIR_NOFOLLOW
-                 | (mode & S_IRUSR ? SAVEWD_CHDIR_READABLE : 0));
+              savewd_chdir_options |= SAVEWD_CHDIR_NOFOLLOW;
             }
           else
             {
diff --git a/lib/savewd.c b/lib/savewd.c
index 16f56a8..f835074 100644
--- a/lib/savewd.c
+++ b/lib/savewd.c
@@ -116,7 +116,7 @@ savewd_chdir (struct savewd *wd, char const *dir, int options,
           open_result[1] = errno;
         }
 
-      if (fd < 0 && (errno != EACCES || (options & SAVEWD_CHDIR_READABLE)))
+      if (fd < 0 && errno != EACCES)
         result = -1;
     }
 
diff --git a/lib/savewd.h b/lib/savewd.h
index bb442b3..69b3718 100644
--- a/lib/savewd.h
+++ b/lib/savewd.h
@@ -82,20 +82,15 @@ savewd_init (struct savewd *wd)
 }

-/* Options for savewd_chdir.  */
+/* Options for savewd_chdir.  Can be ORed together.  */
 enum
   {
     /* Do not follow symbolic links, if supported.  */
     SAVEWD_CHDIR_NOFOLLOW = 1,
 
-    /* The directory should be readable, so fail if it happens to be
-       discovered that the directory is not readable.  (Unreadable
-       directories are not necessarily diagnosed, though.)  */
-    SAVEWD_CHDIR_READABLE = 2,
-
     /* Do not chdir if the directory is readable; simply succeed
        without invoking chdir if the directory was opened.  */
-    SAVEWD_CHDIR_SKIP_READABLE = 4
+    SAVEWD_CHDIR_SKIP_READABLE = 2
   };
 
 /* Change the directory, and if successful, record into *WD the fact
-- 
2.1.0