Re: [patch] Avoid race in zf_mkdir

[Roman Perepelitsa <roman.perepelitsa@xxxxxxxxx>]

On Fri, Oct 9, 2020 at 11:27 PM Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
>
> The document linked by Matthew asserts that "mkdir -m mode" should behave "as if" chmod() is called after creating the directory.

This applies only when mkdir creates a directory but not when the
directory already existed prior to the call. Here's the relevant part:

   Each dir operand that names an existing directory shall be ignored
without error.

On Fri, Oct 9, 2020 at 11:40 PM Matthew Martin <phy1729@xxxxxxxxx> wrote:
>
> For a sufficiently well timed attacker, the target could be created and
> deleted so that this loop never exits.  Even if pathological, I don't
> think it should be possible for mkdir to loop forever.

Perhaps try N times instead of forever? The patch you've posted uses N
= 1 (which is already better than the existing code) but it can be any
other number.

Roman.

diff --git a/Src/Modules/files.c b/Src/Modules/files.c
index 6d20e38a8..5a58ad600 100644
--- a/Src/Modules/files.c
+++ b/Src/Modules/files.c
@@ -122,19 +122,29 @@ domkdir(char *nam, char *path, mode_t mode, int p)
 {
     int err;
     mode_t oumask;
+    struct stat st;
+    int n = 8;
     char const *rpath = unmeta(path);
 
-    if(p) {
-	struct stat st;
-
-	if(!stat(rpath, &st) && S_ISDIR(st.st_mode))
+    while(n--) {
+	oumask = umask(0);
+	err = mkdir(rpath, mode) ? errno : 0;
+	umask(oumask);
+	if (!err)
+	    return 0;
+	if(!p || err != EEXIST)
+	    break;
+	if(!stat(rpath, &st)) {
+	    if(errno == ENOENT)
+		continue;
+	    err = errno;
+	    break;
+	}
+	if(S_ISDIR(st.st_mode))
 	    return 0;
+	break;
     }
-    oumask = umask(0);
-    err = mkdir(rpath, mode) ? errno : 0;
-    umask(oumask);
-    if(!err)
-	return 0;
+
     zwarnnam(nam, "cannot make directory `%s': %e", path, err);
     return 1;
 }