[SSSD] [PATCH] Fix TOCTOU race conditions by copying and removing directory trees

Jakub Hrozek jhrozek at redhat.com
Wed Jan 23 11:37:42 UTC 2013


[PATCH 1/2] TOOLS: Use openat/unlinkat when removing the homedir

The removal of a home directory is sensitive to concurrent modification
of the directory tree being removed and can unlink files outside the
directory tree.

[PATCH 2/2] TOOLS: Use file descriptor to avoid races when creating a
home directory

When creating a home directory, the destination tree can be modified in
various ways while it is being constructed because directory permissions
are set before populating the directory. This can lead to file creation
and permission changes outside the target directory tree, using hard links.

https://fedorahosted.org/sssd/ticket/1782

This security problem was assigned CVE-2013-0219
-------------- next part --------------
>From 00ea259098d95d16fbd63402a8bef0ed25517f81 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 12 Dec 2012 19:02:33 +0100
Subject: [PATCH 1/2] TOOLS: Use openat/unlinkat when removing the homedir

The removal of a home directory is sensitive to concurrent modification
of the directory tree being removed and can unlink files outside the
directory tree.

This security issue was assigned CVE-2013-0219

https://fedorahosted.org/sssd/ticket/1782
---
 src/tools/files.c | 83 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 42 deletions(-)

diff --git a/src/tools/files.c b/src/tools/files.c
index d80e3bd9a387f69cb7bf11dc2b09fe07a928911a..3d7b8d421a2a39e96d280ba629c3208dcf172f35 100644
--- a/src/tools/files.c
+++ b/src/tools/files.c
@@ -78,8 +78,9 @@ struct copy_ctx {
 /* wrapper in order not to create a temporary context in
  * every iteration */
 static int remove_tree_with_ctx(TALLOC_CTX *mem_ctx,
-                                dev_t parent_dev,
-                                const char *root);
+                                int parent_fd,
+                                const char *dir_name,
+                                dev_t parent_dev);
 
 int remove_tree(const char *root)
 {
@@ -91,7 +92,7 @@ int remove_tree(const char *root)
         return ENOMEM;
     }
 
-    ret = remove_tree_with_ctx(tmp_ctx, 0, root);
+    ret = remove_tree_with_ctx(tmp_ctx, AT_FDCWD, root, 0);
     talloc_free(tmp_ctx);
     return ret;
 }
@@ -102,75 +103,75 @@ int remove_tree(const char *root)
  * reach the top level remove_tree() again
  */
 static int remove_tree_with_ctx(TALLOC_CTX *mem_ctx,
-                                dev_t parent_dev,
-                                const char *root)
+                                int parent_fd,
+                                const char *dir_name,
+                                dev_t parent_dev)
 {
-    char *fullpath = NULL;
     struct dirent *result;
-    struct dirent direntp;
     struct stat statres;
     DIR *rootdir = NULL;
     int ret, err;
+    int dir_fd;
 
-    rootdir = opendir(root);
+    dir_fd = openat(parent_fd, dir_name,
+                    O_RDONLY | O_CLOEXEC | O_DIRECTORY | O_NOFOLLOW);
+    if (dir_fd == -1) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot open %s: [%d]: %s\n",
+              dir_name, ret, strerror(ret)));
+        return ret;
+    }
+
+    rootdir = fdopendir(dir_fd);
     if (rootdir == NULL) {
         ret = errno;
-        DEBUG(1, ("Cannot open directory %s [%d][%s]\n",
-                  root, ret, strerror(ret)));
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("Cannot open directory: [%d][%s]\n", ret, strerror(ret)));
+        close(dir_fd);
         goto fail;
     }
 
-    while (readdir_r(rootdir, &direntp, &result) == 0) {
-        if (result == NULL) {
-            /* End of directory */
-            break;
-        }
-
-        if (strcmp (direntp.d_name, ".") == 0 ||
-            strcmp (direntp.d_name, "..") == 0) {
+    while ((result = readdir(rootdir)) != NULL) {
+        if (strcmp(result->d_name, ".") == 0 ||
+            strcmp(result->d_name, "..") == 0) {
             continue;
         }
 
-        fullpath = talloc_asprintf(mem_ctx, "%s/%s", root, direntp.d_name);
-        if (fullpath == NULL) {
-            ret = ENOMEM;
-            goto fail;
-        }
-
-        ret = lstat(fullpath, &statres);
+        ret = fstatat(dir_fd, result->d_name,
+                      &statres, AT_SYMLINK_NOFOLLOW);
         if (ret != 0) {
             ret = errno;
-            DEBUG(1, ("Cannot stat %s: [%d][%s]\n",
-                      fullpath, ret, strerror(ret)));
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  ("stat failed: [%d][%s]\n", ret, strerror(ret)));
             goto fail;
         }
 
         if (S_ISDIR(statres.st_mode)) {
             /* if directory, recursively descend, but check if on the same FS */
             if (parent_dev && parent_dev != statres.st_dev) {
-                DEBUG(1, ("Directory %s is on different filesystem, "
-                          "will not follow\n", fullpath));
+                DEBUG(SSSDBG_CRIT_FAILURE,
+                      ("Directory %s is on different filesystem, "
+                       "will not follow\n"));
                 ret = EFAULT;
                 goto fail;
             }
 
-            ret = remove_tree_with_ctx(mem_ctx, statres.st_dev, fullpath);
+            ret = remove_tree_with_ctx(mem_ctx, dir_fd, result->d_name, statres.st_dev);
             if (ret != EOK) {
-                DEBUG(1, ("Removing subdirectory %s failed: [%d][%s]\n",
-                            fullpath, ret, strerror(ret)));
+                DEBUG(SSSDBG_CRIT_FAILURE,
+                      ("Removing subdirectory failed: [%d][%s]\n",
+                       ret, strerror(ret)));
                 goto fail;
             }
         } else {
-            ret = unlink(fullpath);
+            ret = unlinkat(dir_fd, result->d_name, 0);
             if (ret != 0) {
                 ret = errno;
-                DEBUG(1, ("Removing file %s failed: [%d][%s]\n",
-                          fullpath, ret, strerror(ret)));
+                DEBUG(SSSDBG_CRIT_FAILURE,
+                        ("Removing file failed: [%d][%s]\n", ret, strerror(ret)));
                 goto fail;
             }
         }
-
-        talloc_free(fullpath);
     }
 
     ret = closedir(rootdir);
@@ -180,19 +181,17 @@ static int remove_tree_with_ctx(TALLOC_CTX *mem_ctx,
         goto fail;
     }
 
-    ret = rmdir(root);
-    if (ret != 0) {
+    ret = unlinkat(parent_fd, dir_name, AT_REMOVEDIR);
+    if (ret == -1) {
         ret = errno;
-        goto fail;
     }
 
     ret = EOK;
-
 fail:
     if (rootdir) {  /* clean up on abnormal exit but retain return code */
         err = closedir(rootdir);
         if (err) {
-            DEBUG(1, ("closedir failed, bad dirp?\n"));
+            DEBUG(SSSDBG_CRIT_FAILURE, ("closedir failed, bad dirp?\n"));
         }
     }
     return ret;
-- 
1.8.1

-------------- next part --------------
>From ac27c8645d7f43fe3404cb1eabe0a710512a7da3 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 20 Jan 2013 20:27:05 +0100
Subject: [PATCH 2/2] TOOLS: Use file descriptor to avoid races when creating a
 home directory

When creating a home directory, the destination tree can be modified in
various ways while it is being constructed because directory permissions
are set before populating the directory. This can lead to file creation
and permission changes outside the target directory tree, using hard links.

This security problem was assigned CVE-2013-0219

https://fedorahosted.org/sssd/ticket/1782
---
 src/tests/files-tests.c |   6 +-
 src/tools/files.c       | 725 +++++++++++++++++++++++++-----------------------
 src/tools/tools_util.c  |  28 +-
 src/tools/tools_util.h  |   5 +-
 4 files changed, 387 insertions(+), 377 deletions(-)

diff --git a/src/tests/files-tests.c b/src/tests/files-tests.c
index 5d7f17e19668746b358d2bd57757dd24636acdcc..4225098c27e6b1492aeb2985eb821fe8ba05a4b9 100644
--- a/src/tests/files-tests.c
+++ b/src/tests/files-tests.c
@@ -183,7 +183,7 @@ START_TEST(test_simple_copy)
 
     /* and finally copy.. */
     DEBUG(5, ("Will copy from '%s' to '%s'\n", dir_path, dst_path));
-    ret = copy_tree(dir_path, dst_path, uid, gid);
+    ret = copy_tree(dir_path, dst_path, 0700, uid, gid);
     fail_unless(ret == EOK, "copy_tree failed\n");
 
     /* check if really copied */
@@ -225,7 +225,7 @@ START_TEST(test_copy_symlink)
 
     /* and finally copy.. */
     DEBUG(5, ("Will copy from '%s' to '%s'\n", dir_path, dst_path));
-    ret = copy_tree(dir_path, dst_path, uid, gid);
+    ret = copy_tree(dir_path, dst_path, 0700, uid, gid);
     fail_unless(ret == EOK, "copy_tree failed\n");
 
     /* check if really copied */
@@ -264,7 +264,7 @@ START_TEST(test_copy_node)
 
     /* and finally copy.. */
     DEBUG(5, ("Will copy from '%s' to '%s'\n", dir_path, dst_path));
-    ret = copy_tree(dir_path, dst_path, uid, gid);
+    ret = copy_tree(dir_path, dst_path, 0700, uid, gid);
     fail_unless(ret == EOK, "copy_tree failed\n");
 
     /* check if really copied */
diff --git a/src/tools/files.c b/src/tools/files.c
index 3d7b8d421a2a39e96d280ba629c3208dcf172f35..4a6625eae56af87ef3f1b0c42323026c2bd4cac5 100644
--- a/src/tools/files.c
+++ b/src/tools/files.c
@@ -66,13 +66,12 @@
 #include "util/util.h"
 #include "tools/tools_util.h"
 
-int copy_tree(const char *src_root, const char *dst_root,
-              uid_t uid, gid_t gid);
-
 struct copy_ctx {
     const char *src_orig;
     const char *dst_orig;
     dev_t       src_dev;
+    uid_t       uid;
+    gid_t       gid;
 };
 
 /* wrapper in order not to create a temporary context in
@@ -197,66 +196,13 @@ fail:
     return ret;
 }
 
-static int copy_dir(const char *src, const char *dst,
-                    const struct stat *statp, const struct timeval mt[2],
-                    uid_t uid, gid_t gid)
-{
-    int ret = 0;
-
-    /*
-     * Create a new target directory, make it owned by
-     * the user and then recursively copy that directory.
-     */
-    selinux_file_context(dst);
-
-    ret = mkdir(dst, statp->st_mode);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot mkdir directory '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    ret = chown(dst, uid, gid);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot chown directory '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    ret = chmod(dst, statp->st_mode);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot chmod directory '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    ret = copy_tree(src, dst, uid, gid);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot copy directory from '%s' to '%s': [%d][%s].\n",
-                  src, dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    ret = utimes(dst, mt);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot set utimes on a directory '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    return EOK;
-}
-
-static char *talloc_readlink(TALLOC_CTX *mem_ctx, const char *filename)
+static char *talloc_readlinkat(TALLOC_CTX *mem_ctx, int dir_fd,
+                               const char *filename)
 {
     size_t size = 1024;
     ssize_t nchars;
     char *buffer;
+    char *new_buffer;
 
     buffer = talloc_array(mem_ctx, char, size);
     if (!buffer) {
@@ -264,8 +210,9 @@ static char *talloc_readlink(TALLOC_CTX *mem_ctx, const char *filename)
     }
 
     while (1) {
-        nchars = readlink(filename, buffer, size);
+        nchars = readlinkat(dir_fd, filename, buffer, size);
         if (nchars < 0) {
+            talloc_free(buffer);
             return NULL;
         }
 
@@ -276,10 +223,12 @@ static char *talloc_readlink(TALLOC_CTX *mem_ctx, const char *filename)
 
         /* Try again with a bigger buffer */
         size *= 2;
-        buffer = talloc_realloc(mem_ctx, buffer, char, size);
-        if (!buffer) {
+        new_buffer = talloc_realloc(mem_ctx, buffer, char, size);
+        if (!new_buffer) {
+            talloc_free(buffer);
             return NULL;
         }
+        buffer = new_buffer;
     }
 
     /* readlink does not nul-terminate */
@@ -287,188 +236,174 @@ static char *talloc_readlink(TALLOC_CTX *mem_ctx, const char *filename)
     return buffer;
 }
 
-static int copy_symlink(struct copy_ctx *cctx,
-                        const char *src,
-                        const char *dst,
-                        const struct stat *statp,
-                        const struct timeval mt[],
-                        uid_t uid, gid_t gid)
+static int
+copy_symlink(int src_dir_fd,
+             int dst_dir_fd,
+             const char *file_name,
+             const char *full_path,
+             const struct stat *statp,
+             uid_t uid, gid_t gid)
 {
-    int ret;
-    char *oldlink;
-    char *tmp;
-    TALLOC_CTX *tmp_ctx = NULL;
+    char *buf;
+    errno_t ret;
+    struct timespec timebuf[2];
 
-    tmp_ctx = talloc_new(cctx);
-    if (!tmp_ctx) {
+    buf = talloc_readlinkat(NULL, src_dir_fd, file_name);
+    if (!buf) {
         return ENOMEM;
     }
 
-    /*
-     * Get the name of the file which the link points
-     * to.  If that name begins with the original
-     * source directory name, that part of the link
-     * name will be replaced with the original
-     * destination directory name.
-     */
-    oldlink = talloc_readlink(tmp_ctx, src);
-    if (oldlink == NULL) {
-        ret = ENOMEM;
-        goto done;
+    ret = selinux_file_context(full_path);
+    if (ret != 0) {
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              ("Failed to set SELinux context for [%s]\n", full_path));
+        /* Not fatal */
     }
 
-    /* If src was a link to an entry of the src_orig directory itself,
-     * create a link to the corresponding entry in the dst_orig
-     * directory.
-     * FIXME: This may change a relative link to an absolute link
-     */
-    if (strncmp(oldlink, cctx->src_orig, strlen(cctx->src_orig)) == 0) {
-        tmp = talloc_asprintf(tmp_ctx, "%s%s", cctx->dst_orig, oldlink + strlen(cctx->src_orig));
-        if (tmp == NULL) {
-            ret = ENOMEM;
-            goto done;
+    ret = symlinkat(buf, dst_dir_fd, file_name);
+    talloc_free(buf);
+    if (ret == -1) {
+        ret = errno;
+        if (ret == EEXIST) {
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  ("symlink pointing to already exists at '%s'\n", full_path));
+            return EOK;
         }
 
-        talloc_free(oldlink);
-        oldlink = tmp;
+        DEBUG(SSSDBG_CRIT_FAILURE, ("symlinkat failed: %s\n", strerror(ret)));
+        return ret;
     }
 
-    selinux_file_context(dst);
-
-    ret = symlink(oldlink, dst);
-    if (ret != 0) {
+    ret = fchownat(dst_dir_fd, file_name,
+                   uid, gid, AT_SYMLINK_NOFOLLOW);
+    if (ret == -1) {
         ret = errno;
-        DEBUG(1, ("symlink() failed on file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        goto done;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+             ("fchownat failed: %s\n", strerror(ret)));
+        return ret;
     }
 
-    ret = lchown(dst, uid, gid);
-    if (ret != 0) {
+    timebuf[0] = statp->st_atim;
+    timebuf[1] = statp->st_mtim;
+    ret = utimensat(dst_dir_fd, file_name, timebuf,
+                    AT_SYMLINK_NOFOLLOW);
+    if (ret == -1) {
         ret = errno;
-        DEBUG(1, ("lchown() failed on file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        goto done;
+        DEBUG(SSSDBG_MINOR_FAILURE, ("utimensat failed [%d]: %s\n",
+              ret, strerror(ret)));
+        /* Do not fail */
     }
 
-done:
-    talloc_free(tmp_ctx);
-    return ret;
+    return EOK;
 }
 
-static int copy_special(const char *dst,
+/* Create a special file named file_name under a directory with file
+ * descriptor dst_dir_fd. full_path is used for both setting SELinux
+ * context and logging. The node is owned by uid/gid and its mode
+ * and device number is read from statp.
+ */
+static int copy_special(int dst_dir_fd,
+                        const char *file_name,
+                        const char *full_path,
                         const struct stat *statp,
-                        const struct timeval mt[],
                         uid_t uid, gid_t gid)
 {
-    int ret = 0;
-
-    selinux_file_context(dst);
-
-    ret = mknod(dst, statp->st_mode & ~07777, statp->st_rdev);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot mknod special file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    ret = chown(dst, uid, gid);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot chown special file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    ret = chmod(dst, statp->st_mode & 07777);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot chmod special file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    ret = utimes(dst, mt);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot call utimes on special file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        return ret;
-    }
-
-    return EOK;
-}
-
-static int copy_file(const char *src,
-                     const char *dst,
-                     const struct stat *statp,
-                     const struct timeval mt[],
-                     uid_t uid, gid_t gid)
-{
     int ret;
-    int ifd = -1;
+    struct timespec timebuf[2];
+
+    ret = selinux_file_context(full_path);
+    if (ret != 0) {
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              ("Failed to set SELinux context for [%s]\n", full_path));
+        /* Not fatal */
+    }
+
+    ret = mknodat(dst_dir_fd, file_name, statp->st_mode & ~07777,
+                  statp->st_rdev);
+    if (ret != 0) {
+        ret = errno;
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("Cannot mknod special file '%s': [%d][%s].\n",
+              full_path, ret, strerror(ret)));
+        return ret;
+    }
+
+    ret = fchownat(dst_dir_fd, file_name, uid, gid, 0);
+    if (ret != 0) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("fchownat failed for '%s': [%d][%s]\n",
+              full_path, ret, strerror(ret)));
+        return ret;
+    }
+
+    ret = fchmodat(dst_dir_fd, file_name, statp->st_mode & 07777, 0);
+    if (ret != 0) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("fchmodat failed for '%s': [%d][%s]\n",
+              full_path, ret, strerror(ret)));
+        return ret;
+    }
+
+    timebuf[0] = statp->st_atim;
+    timebuf[1] = statp->st_mtim;
+    ret = utimensat(dst_dir_fd, file_name, timebuf, 0);
+    if (ret == -1) {
+        ret = errno;
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              ("utimensat failed for '%s': [%d][%s]\n",
+              full_path, ret, strerror(ret)));
+        /* Do not fail, this shouldn't be fatal */
+    }
+
+    return EOK;
+}
+
+/* Copy bytes from input file descriptor ifd into file named
+ * dst_named under directory with dest_dir_fd. Own the new file
+ * by uid/gid
+ */
+static int
+copy_file(int ifd,
+          int dest_dir_fd,
+          const char *file_name,
+          const char *full_path,
+          const struct stat *statp,
+          uid_t uid, gid_t gid)
+{
     int ofd = -1;
+    errno_t ret;
     char buf[1024];
     ssize_t cnt, written;
-    struct stat fstatbuf;
+    struct timespec timebuf[2];
 
-    ifd = open(src, O_RDONLY);
-    if (ifd < 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot open() source file '%s': [%d][%s].\n",
-                  src, ret, strerror(ret)));
-        goto fail;
-    }
-
-    ret = fstat(ifd, &fstatbuf);
+    ret = selinux_file_context(full_path);
     if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot fstat() source file '%s': [%d][%s].\n",
-                  src, ret, strerror(ret)));
-        goto fail;
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              ("Failed to set SELinux context for [%s]\n", full_path));
+        /* Not fatal */
     }
 
-    if (statp->st_dev != fstatbuf.st_dev ||
-        statp->st_ino != fstatbuf.st_ino) {
-        DEBUG(1, ("File %s was modified between lstat and open.\n", src));
-        ret = EIO;
-        goto fail;
-    }
-
-    selinux_file_context(dst);
-
-    ofd = open(dst, O_WRONLY | O_CREAT | O_TRUNC, statp->st_mode & 07777);
-    if (ofd < 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot open() destination file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        goto fail;
-    }
-
-    ret = fchown(ofd, uid, gid);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot fchown() destination file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        goto fail;
-    }
-
-    ret = fchmod(ofd, statp->st_mode & 07777);
-    if (ret != 0) {
+    /* Start with absolutely restrictive permissions */
+    ofd = openat(dest_dir_fd, file_name,
+                 O_EXCL | O_CREAT | O_WRONLY | O_NOFOLLOW,
+                 0);
+    if (ofd < 0 && errno != EEXIST) {
         ret = errno;
-        DEBUG(1, ("Cannot fchmod() destination file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        goto fail;
+        DEBUG(SSSDBG_OP_FAILURE,
+               ("Cannot open() destination file '%s': [%d][%s].\n",
+               full_path, ret, strerror(ret)));
+        goto done;
     }
 
     while ((cnt = sss_atomic_read_s(ifd, buf, sizeof(buf))) != 0) {
         if (cnt == -1) {
             ret = errno;
             DEBUG(SSSDBG_CRIT_FAILURE,
-                  ("Cannot read() from source file '%s': [%d][%s].\n",
-                   src, ret, strerror(ret)));
-            goto fail;
+                  ("Cannot read() from source file: [%d][%s].\n",
+                   ret, strerror(ret)));
+            goto done;
         }
 
         errno = 0;
@@ -476,222 +411,324 @@ static int copy_file(const char *src,
         if (written == -1) {
             ret = errno;
             DEBUG(SSSDBG_CRIT_FAILURE,
-                  ("Cannot write() to destination file '%s': [%d][%s].\n",
-                   dst, ret, strerror(ret)));
-            goto fail;
+                  ("Cannot write() to destination file: [%d][%s].\n",
+                   ret, strerror(ret)));
+            goto done;
         }
 
         if (written != cnt) {
             DEBUG(SSSDBG_CRIT_FAILURE,
                   ("Wrote %d bytes, expected %d\n", written, cnt));
-            goto fail;
+            goto done;
         }
     }
 
-    ret = close(ifd);
-    ifd = -1;
-    if (ret != 0) {
+    /* Set the ownership; permissions are still
+     * restrictive. */
+    ret = fchown(ofd, uid, gid);
+    if (ret == -1 && errno != EPERM) {
         ret = errno;
-        DEBUG(1, ("Cannot close() source file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        goto fail;
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("Error changing owner of '%s': %s\n",
+              full_path, strerror(ret)));
+        goto done;
     }
 
-    ret = close(ofd);
-    ifd = -1;
-    if (ret != 0) {
+    /* Set the desired mode. */
+    ret = fchmod(ofd, statp->st_mode);
+    if (ret == -1) {
         ret = errno;
-        DEBUG(1, ("Cannot close() destination file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        goto fail;
+        DEBUG(SSSDBG_OP_FAILURE, ("Error changing owner of '%s': %s\n",
+              full_path, strerror(ret)));
+              goto done;
     }
 
-    ret = utimes(dst, mt);
-    if (ret != 0) {
+    timebuf[0] = statp->st_atim;
+    timebuf[1] = statp->st_mtim;
+    ret = futimens(ofd, timebuf);
+    if (ret == -1) {
         ret = errno;
-        DEBUG(1, ("Cannot call utimes() on destination file '%s': [%d][%s].\n",
-                  dst, ret, strerror(ret)));
-        goto fail;
+        DEBUG(SSSDBG_MINOR_FAILURE, ("futimens failed [%d]: %s\n",
+              ret, strerror(ret)));
+        /* Do not fail */
     }
 
-    return EOK;
+    close(ofd);
+    ofd = -1;
+    ret = EOK;
 
-    /* Reachable by jump only */
-fail:
-    if (ifd != -1) close(ifd);
+done:
     if (ofd != -1) close(ofd);
     return ret;
 }
 
-/*
- * The context is not freed in case of error
- * because this is a recursive function, will be freed when we
- * reach the top level copy_tree() again
- */
-static int copy_entry(struct copy_ctx *cctx,
-                      const char *src,
-                      const char *dst,
-                      uid_t uid,
-                      gid_t gid)
+static errno_t
+copy_dir(struct copy_ctx *cctx,
+         int src_dir_fd, const char *src_dir_path,
+         int dest_parent_fd, const char *dest_dir_name,
+         const char *dest_dir_path,
+         mode_t mode,
+         const struct stat *src_dir_stat);
+
+static errno_t
+copy_entry(struct copy_ctx *cctx,
+           int src_dir_fd,
+           const char *src_dir_path,
+           int dest_dir_fd,
+           const char *dest_dir_path,
+           const char *ent_name)
 {
-    int ret = EOK;
-    struct stat sb;
-    struct timeval mt[2];
+    char *src_ent_path = NULL;
+    char *dest_ent_path = NULL;
+    int ifd = -1;
+    errno_t ret;
+    struct stat st;
 
-    ret = lstat(src, &sb);
-    if (ret == -1) {
-        ret = errno;
-        DEBUG(1, ("Cannot lstat() the source file '%s': [%d][%s].\n",
-                  src, ret, strerror(ret)));
-        return ret;
+    /* Build the path of the source file or directory and its
+     * corresponding member in the new tree. */
+    src_ent_path = talloc_asprintf(cctx, "%s/%s", src_dir_path, ent_name);
+    dest_ent_path = talloc_asprintf(cctx, "%s/%s", dest_dir_path, ent_name);
+    if (!src_ent_path || !dest_ent_path) {
+        ret = ENOMEM;
+        goto done;
     }
 
-    mt[0].tv_sec  = sb.st_atime;
-    mt[0].tv_usec = 0;
-
-    mt[1].tv_sec  = sb.st_mtime;
-    mt[1].tv_usec = 0;
+    /* Open the input entry first, then we can fstat() it and be
+     * certain that it is still the same file.  O_NONBLOCK protects
+     * us against FIFOs and perhaps side-effects of the open() of a
+     * device file if there ever was one here, and doesn't matter
+     * for regular files or directories. */
+    ifd = openat(src_dir_fd, ent_name,
+                 O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK);
+    if (ifd == -1 && errno != ELOOP) {
+        /* openat error */
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE, ("openat failed on '%s': %s\n",
+              src_ent_path, strerror(ret)));
+        goto done;
+    } else if (ifd == -1 && errno == ELOOP) {
+        /* Should be a symlink.. */
+        ret = fstatat(src_dir_fd, ent_name, &st, AT_SYMLINK_NOFOLLOW);
+        if (ret == -1) {
+            ret = errno;
+            DEBUG(SSSDBG_CRIT_FAILURE, ("fstatat failed on '%s': %s\n",
+                  src_ent_path, strerror(ret)));
+            goto done;
+        }
 
-    if (S_ISLNK (sb.st_mode)) {
-        ret = copy_symlink(cctx, src, dst, &sb, mt, uid, gid);
+        /* Handle symlinks */
+        ret = copy_symlink(src_dir_fd, dest_dir_fd, ent_name,
+                           dest_ent_path, &st, cctx->uid, cctx->gid);
         if (ret != EOK) {
-            DEBUG(1, ("Cannot copy symlink '%s' to '%s': [%d][%s]\n",
-                      src, dst, ret, strerror(ret)));
+            DEBUG(SSSDBG_OP_FAILURE, ("Cannot copy '%s' to '%s'\n",
+                  src_ent_path, dest_ent_path));
         }
-        return ret;
+        goto done;
     }
 
-    if (S_ISDIR(sb.st_mode)) {
-        /* Check if we're still on the same FS */
-        if (sb.st_dev != cctx->src_dev) {
-            DEBUG(2, ("Will not descend to other FS\n"));
-            /* Skip this without error */
-            return EOK;
+    ret = fstat(ifd, &st);
+    if (ret != 0) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+                ("couldn't stat '%s': %s", src_ent_path, strerror(ret)));
+        goto done;
+    }
+
+    if (S_ISDIR(st.st_mode)) {
+        /* If it's a directory, descend into it. */
+        ret = copy_dir(cctx, ifd, src_ent_path,
+                       dest_dir_fd, ent_name,
+                       dest_ent_path, st.st_mode & 07777,
+                       &st);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_OP_FAILURE,
+                    ("Could recursively copy '%s' to '%s': %s\n",
+                    src_ent_path, dest_dir_fd, strerror(ret)));
+            goto done;
+        }
+    } else if (S_ISREG(st.st_mode)) {
+        /* Copy a regular file */
+        ret = copy_file(ifd, dest_dir_fd, ent_name, dest_ent_path,
+                        &st, cctx->uid, cctx->gid);
+        if (ret) {
+            DEBUG(SSSDBG_OP_FAILURE, ("Cannot copy '%s' to '%s'\n",
+                    src_ent_path, dest_ent_path));
+            goto done;
         }
-        return copy_dir(src, dst, &sb, mt, uid, gid);
-    } else if (!S_ISREG(sb.st_mode)) {
-        /*
-         * Deal with FIFOs and special files.  The user really
-         * shouldn't have any of these, but it seems like it
-         * would be nice to copy everything ...
-         */
-        return copy_special(dst, &sb, mt, uid, gid);
     } else {
-        /*
-         * Create the new file and copy the contents.  The new
-         * file will be owned by the provided UID and GID values.
-         */
-        return copy_file(src, dst, &sb, mt, uid, gid);
+        /* Copy a special file */
+        ret = copy_special(dest_dir_fd, ent_name, dest_ent_path,
+                           &st, cctx->uid, cctx->gid);
+        if (ret) {
+            DEBUG(SSSDBG_OP_FAILURE, ("Cannot copy '%s' to '%s'\n",
+                  src_ent_path, dest_ent_path));
+            goto done;
+        }
     }
 
+    ret = EOK;
+done:
+    talloc_free(src_ent_path);
+    talloc_free(dest_ent_path);
+    if (ifd != -1) close(ifd);
     return ret;
 }
 
-/*
- * The context is not freed in case of error
- * because this is a recursive function, will be freed when we
- * reach the top level copy_tree() again
- */
-static int copy_tree_ctx(struct copy_ctx *cctx,
-                         const char *src_root,
-                         const char *dst_root,
-                         uid_t uid,
-                         gid_t gid)
+static errno_t
+copy_dir(struct copy_ctx *cctx,
+         int src_dir_fd, const char *src_dir_path,
+         int dest_parent_fd, const char *dest_dir_name,
+         const char *dest_dir_path,
+         mode_t mode,
+         const struct stat *src_dir_stat)
 {
-    DIR *src_dir = NULL;
-    int ret, err;
-    struct dirent *result;
-    struct dirent direntp;
-    char *src_name, *dst_name;
-    TALLOC_CTX *tmp_ctx;
+    errno_t ret;
+    int dest_dir_fd = -1;
+    DIR *dir = NULL;
+    struct dirent *ent;
+    struct timespec timebuf[2];
 
-    tmp_ctx = talloc_new(cctx);
+    if (!dest_dir_path) {
+        return EINVAL;
+    }
+
+    dir = fdopendir(src_dir_fd);
+    if (dir == NULL) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("Error reading '%s': %s", src_dir_path, strerror(ret)));
+        goto done;
+    }
 
-    src_dir = opendir(src_root);
-    if (src_dir == NULL) {
+    /* Create the directory.  It starts owned by us (presumbaly root), with
+     * fairly restrictive permissions that still allow us to use the
+     * directory.
+     * */
+    errno = 0;
+    ret = mkdirat(dest_parent_fd, dest_dir_name, S_IRWXU);
+    if (ret == -1 && errno != EEXIST) {
         ret = errno;
-        DEBUG(1, ("Cannot open the source directory %s: [%d][%s].\n",
-                  src_root, ret, strerror(ret)));
-        goto fail;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("Error reading '%s': %s", dest_dir_path, strerror(ret)));
+        goto done;
     }
 
-    while (readdir_r(src_dir, &direntp, &result) == 0) {
-        if (result == NULL) {
-            /* End of directory */
-            break;
-        }
+    dest_dir_fd = openat(dest_parent_fd, dest_dir_name,
+                         O_RDONLY | O_CLOEXEC | O_DIRECTORY | O_NOFOLLOW);
+    if (dest_dir_fd == -1) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("Error opening '%s': %s", dest_dir_path, strerror(ret)));
+        goto done;
+    }
 
-        if (strcmp (direntp.d_name, ".") == 0 ||
-            strcmp (direntp.d_name, "..") == 0) {
+    while ((ent = readdir(dir)) != NULL) {
+        /* Iterate through each item in the directory. */
+        /* Skip over self and parent hard links. */
+        if (strcmp(ent->d_name, ".") == 0 ||
+            strcmp(ent->d_name, "..") == 0) {
             continue;
         }
 
-        /* build src and dst paths */
-        src_name = talloc_asprintf(tmp_ctx, "%s/%s", src_root, direntp.d_name);
-        dst_name = talloc_asprintf(tmp_ctx, "%s/%s", dst_root, direntp.d_name);
-        if (dst_name == NULL || src_name == NULL) {
-            ret = ENOMEM;
-            goto fail;
-        }
-
-        /* copy */
-        ret = copy_entry(cctx, src_name, dst_name, uid, gid);
+        ret = copy_entry(cctx,
+                         src_dir_fd, src_dir_path,
+                         dest_dir_fd, dest_dir_path,
+                         ent->d_name);
         if (ret != EOK) {
-            DEBUG(1, ("Cannot copy '%s' to '%s', error %d\n",
-                      src_name, dst_name, ret));
-            goto fail;
+            DEBUG(SSSDBG_OP_FAILURE, ("Could not copy [%s] to [%s]\n",
+                  src_dir_path, dest_dir_path));
+            goto done;
         }
-        talloc_free(src_name);
-        talloc_free(dst_name);
     }
 
-    ret = closedir(src_dir);
-    src_dir = NULL;
-    if (ret != 0) {
+    /* Set the ownership on the directory.  Permissions are still
+     * fairly restrictive. */
+    ret = fchown(dest_dir_fd, cctx->uid, cctx->gid);
+    if (ret == -1 && errno != EPERM) {
         ret = errno;
-        goto fail;
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("Error changing owner of '%s': %s",
+              dest_dir_path, strerror(ret)));
+        goto done;
+    }
+
+    /* Set the desired mode. Do this explicitly to preserve S_ISGID and
+     * other bits. Do this after chown, because chown is permitted to
+     * reset these bits. */
+    ret = fchmod(dest_dir_fd, mode);
+    if (ret == -1) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("Error setting mode of '%s': %s",
+              dest_dir_path, strerror(ret)));
+        goto done;
+    }
+
+    timebuf[0] = src_dir_stat->st_atim;
+    timebuf[1] = src_dir_stat->st_mtim;
+    futimens(dest_dir_fd, timebuf);
+    if (ret == -1) {
+        ret = errno;
+        DEBUG(SSSDBG_MINOR_FAILURE, ("futimens failed [%d]: %s\n",
+              ret, strerror(ret)));
+        /* Do not fail */
     }
 
     ret = EOK;
-fail:
-    if (src_dir) {  /* clean up on abnormal exit but retain return code */
-        err = closedir(src_dir);
-        if (err) {
-            DEBUG(1, ("closedir failed, bad dirp?\n"));
-        }
+done:
+    if (dir) closedir(dir);
+
+    if (dest_dir_fd != -1) {
+        close(dest_dir_fd);
     }
-    talloc_free(tmp_ctx);
     return ret;
 }
 
+/* NOTE:
+ * For several reasons, including the fact that we copy even special files
+ * (pipes, etc) from the skeleton directory, the skeldir needs to be trusted
+ */
 int copy_tree(const char *src_root, const char *dst_root,
-              uid_t uid, gid_t gid)
+              mode_t mode_root, uid_t uid, gid_t gid)
 {
     int ret = EOK;
     struct copy_ctx *cctx = NULL;
+    int fd = -1;
     struct stat s_src;
 
+    fd = open(src_root, O_RDONLY | O_CLOEXEC | O_DIRECTORY);
+    if (fd == -1) {
+        ret = errno;
+        goto fail;
+    }
+
+    ret = fstat(fd, &s_src);
+    if (ret == -1) {
+        ret = errno;
+        goto fail;
+    }
+
     cctx = talloc_zero(NULL, struct copy_ctx);
-
-    ret = lstat(src_root, &s_src);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot lstat the source directory '%s': [%d][%s]\n",
-                  src_root, ret, strerror(ret)));
+    if (!cctx) {
+        ret = ENOMEM;
         goto fail;
     }
 
     cctx->src_orig = src_root;
     cctx->dst_orig = dst_root;
     cctx->src_dev  = s_src.st_dev;
+    cctx->uid      = uid;
+    cctx->gid      = gid;
 
-    ret = copy_tree_ctx(cctx, src_root, dst_root, uid, gid);
+    ret = copy_dir(cctx, fd, src_root, AT_FDCWD,
+                   dst_root, dst_root, mode_root, &s_src);
     if (ret != EOK) {
-        DEBUG(1, ("copy_tree_ctx failed: [%d][%s]\n", ret, strerror(ret)));
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              ("copy_dir failed: [%d][%s]\n", ret, strerror(ret)));
         goto fail;
     }
 
 fail:
+    if (fd != -1) close(fd);
     reset_selinux_file_context();
     talloc_free(cctx);
     return ret;
diff --git a/src/tools/tools_util.c b/src/tools/tools_util.c
index 0151400d2c41a9879080a8e9fb2673676b85503d..731b2d04f4fa24d40de03e44c76bb86b40093e18 100644
--- a/src/tools/tools_util.c
+++ b/src/tools/tools_util.c
@@ -477,33 +477,7 @@ int create_homedir(const char *skeldir,
 
     selinux_file_context(homedir);
 
-    ret = mkdir(homedir, 0);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot create user's home directory: [%d][%s].\n",
-                  ret, strerror(ret)));
-        goto done;
-    }
-
-    ret = chown(homedir, uid, gid);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot chown user's home directory: [%d][%s].\n",
-                  ret, strerror(ret)));
-        goto done;
-    }
-
-    ret = chmod(homedir, 0777 & ~default_umask);
-    if (ret != 0) {
-        ret = errno;
-        DEBUG(1, ("Cannot chmod user's home directory: [%d][%s].\n",
-                  ret, strerror(ret)));
-        goto done;
-    }
-
-    reset_selinux_file_context();
-
-    ret = copy_tree(skeldir, homedir, uid, gid);
+    ret = copy_tree(skeldir, homedir, 0777 & ~default_umask, uid, gid);
     if (ret != EOK) {
         DEBUG(1, ("Cannot populate user's home directory: [%d][%s].\n",
                   ret, strerror(ret)));
diff --git a/src/tools/tools_util.h b/src/tools/tools_util.h
index 47bf3876e505193132e67287f3af6367bebbb3a7..50f5a51f712a88b66a6db9dd8d07ad18940db2c8 100644
--- a/src/tools/tools_util.h
+++ b/src/tools/tools_util.h
@@ -117,9 +117,8 @@ errno_t sss_mc_refresh_grouplist(struct tools_ctx *tctx,
 /* from files.c */
 int remove_tree(const char *root);
 
-int copy_tree(const char *src_root,
-              const char *dst_root,
-              uid_t uid, gid_t gid);
+int copy_tree(const char *src_root, const char *dst_root,
+              mode_t mode_root, uid_t uid, gid_t gid);
 
 /* from nscd.c */
 enum nscd_db {
-- 
1.8.1



More information about the sssd-devel mailing list