From b49c8339b21ee5a22c8e673c68331312a5d40fe4 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Tue, 19 Apr 2016 09:17:52 -0400 Subject: [PATCH 1/3] UTIL: Add secure copy function This is a precursor to supporting a static default configuration file. We need to be able to copy the default into the mutable location if the infopipe is asked to modify it. This patch opens both the source and destination files together in order to avoid time-of-check/time-of-use bugs. Reviewed-by: Jakub Hrozek --- src/tests/files-tests.c | 52 +++++++++++++++++- src/tools/files.c | 141 +++++++++++++++++++++++++++++++++++------------- src/tools/tools_util.h | 6 +++ 3 files changed, 159 insertions(+), 40 deletions(-) diff --git a/src/tests/files-tests.c b/src/tests/files-tests.c index 09df5cbd48ae056c7d089204f15c6d3b32d98477..596069e2858e07953b3d48f6b8015ce66e2dd423 100644 --- a/src/tests/files-tests.c +++ b/src/tests/files-tests.c @@ -36,6 +36,8 @@ #include "util/util.h" #include "tests/common.h" +#define TESTS_PATH "tp_" BASE_FILE_STEM + static char tpl_dir[] = "file-tests-dir-XXXXXX"; static char *dir_path; static char *dst_path; @@ -47,8 +49,9 @@ static void setup_files_test(void) { /* create a temporary directory that we fill with stuff later on */ test_ctx = talloc_new(NULL); - dir_path = mkdtemp(talloc_strdup(test_ctx, tpl_dir)); - dst_path = mkdtemp(talloc_strdup(test_ctx, tpl_dir)); + mkdir(TESTS_PATH, 0700); + dir_path = mkdtemp(talloc_asprintf(test_ctx, "%s/%s", TESTS_PATH, tpl_dir)); + dst_path = mkdtemp(talloc_asprintf(test_ctx, "%s/%s", TESTS_PATH, tpl_dir)); uid = getuid(); gid = getgid(); @@ -75,6 +78,7 @@ static void teardown_files_test(void) } } + rmdir(TESTS_PATH); /* clean up */ talloc_zfree(test_ctx); } @@ -199,6 +203,49 @@ START_TEST(test_simple_copy) } END_TEST +START_TEST(test_copy_file) +{ + TALLOC_CTX *tmp_ctx = talloc_new(test_ctx); + int ret; + char origpath[PATH_MAX+1]; + char *foo_path; + char *bar_path; + int fd = -1; + + errno = 0; + fail_unless(getcwd(origpath, PATH_MAX) == origpath, "Cannot getcwd\n"); + fail_unless(errno == 0, "Cannot getcwd\n"); + + /* create a file */ + ret = chdir(dir_path); + fail_if(ret == -1, "Cannot chdir1\n"); + + ret = create_simple_file("foo", "foo"); + fail_if(ret == -1, "Cannot create foo\n"); + foo_path = talloc_asprintf(tmp_ctx, "%s/foo", dir_path); + bar_path = talloc_asprintf(tmp_ctx, "%s/bar", dst_path); + + /* create a file */ + ret = chdir(origpath); + fail_if(ret == -1, "Cannot chdir1\n"); + + /* Copy this file to a new file */ + DEBUG(SSSDBG_FUNC_DATA, + "Will copy from 'foo' to 'bar'\n"); + ret = copy_file_secure(foo_path, bar_path, 0700, uid, gid, 0); + fail_unless(ret == EOK, "copy_file_secure failed\n"); + + /* check if really copied */ + ret = access(bar_path, F_OK); + fail_unless(ret == 0, "destination file 'bar' not there\n"); + + ret = check_and_open_readonly(bar_path, &fd, uid, gid, S_IFREG|S_IRWXU, 0); + fail_unless(ret == EOK, "Cannot open %s\n", bar_path); + close(fd); + talloc_free(tmp_ctx); +} +END_TEST + START_TEST(test_copy_symlink) { int ret; @@ -291,6 +338,7 @@ static Suite *files_suite(void) tcase_add_test(tc_files, test_remove_tree); tcase_add_test(tc_files, test_simple_copy); + tcase_add_test(tc_files, test_copy_file); tcase_add_test(tc_files, test_copy_symlink); tcase_add_test(tc_files, test_copy_node); suite_add_tcase(s, tc_files); diff --git a/src/tools/files.c b/src/tools/files.c index 5b3f9d103120aa5c06d9a453b279aca19258947d..012205f9e7b0dac60c2470ac67ff3f12bb45d3c0 100644 --- a/src/tools/files.c +++ b/src/tools/files.c @@ -351,42 +351,16 @@ copy_symlink(int src_dir_fd, 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) +copy_file_contents(int ifd, + int ofd, + mode_t mode, + uid_t uid, gid_t gid) { - int ofd = -1; errno_t ret; char buf[1024]; ssize_t cnt, written; - 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 */ - } - - /* 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(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; @@ -419,29 +393,71 @@ copy_file(int ifd, if (ret == -1 && errno != EPERM) { ret = errno; DEBUG(SSSDBG_OP_FAILURE, - "Error changing owner of '%s': %s\n", - full_path, strerror(ret)); + "Error changing owner: %s\n", + strerror(ret)); goto done; } /* Set the desired mode. */ - ret = fchmod(ofd, statp->st_mode); + ret = fchmod(ofd, mode); if (ret == -1) { ret = errno; - DEBUG(SSSDBG_OP_FAILURE, "Error changing owner of '%s': %s\n", - full_path, strerror(ret)); + DEBUG(SSSDBG_OP_FAILURE, "Error changing mode: %s\n", + strerror(ret)); goto done; } + ret = EOK; + +done: + return ret; +} + + +/* 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; + + 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 */ + } + + /* 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(SSSDBG_OP_FAILURE, + "Cannot open() destination file '%s': [%d][%s].\n", + full_path, ret, strerror(ret)); + goto done; + } + + ret = copy_file_contents(ifd, ofd, statp->st_mode, uid, gid); + if (ret != EOK) goto done; + + ret = sss_futime_set(ofd, statp); if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "sss_futime_set failed [%d]: %s\n", ret, strerror(ret)); /* Do not fail */ } - - close(ofd); - ofd = -1; ret = EOK; done: @@ -449,6 +465,55 @@ done: return ret; } +int +copy_file_secure(const char *src, + const char *dest, + mode_t mode, + uid_t uid, gid_t gid, + bool force) +{ + int ifd = -1; + int ofd = -1; + int dest_flags = 0; + errno_t ret; + + ret = selinux_file_context(dest); + if (ret != 0) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Failed to set SELinux context for [%s]\n", dest); + /* Not fatal */ + } + + /* Start with absolutely restrictive permissions */ + dest_flags = O_CREAT | O_WRONLY | O_NOFOLLOW; + if (!force) { + dest_flags |= O_EXCL; + } + + ofd = open(dest, dest_flags, mode); + if (ofd < 0) { + DEBUG(SSSDBG_OP_FAILURE, + "Cannot open() destination file '%s': [%d][%s].\n", + dest, errno, strerror(errno)); + goto done; + } + + ifd = sss_open_cloexec(src, O_RDONLY | O_NOFOLLOW, &ret); + if (ifd < 0) { + DEBUG(SSSDBG_OP_FAILURE, + "Cannot open() source file '%s': [%d][%s].\n", + src, ret, strerror(ret)); + goto done; + } + + ret = copy_file_contents(ifd, ofd, mode, uid, gid); + +done: + if (ifd != -1) close(ifd); + if (ofd != -1) close(ofd); + return ret; +} + static errno_t copy_dir(struct copy_ctx *cctx, int src_dir_fd, const char *src_dir_path, diff --git a/src/tools/tools_util.h b/src/tools/tools_util.h index c5990b012892a25b315d744a056861e7b2130410..f914e9a73b817873f18cd2c2ea70e830460e4539 100644 --- a/src/tools/tools_util.h +++ b/src/tools/tools_util.h @@ -119,6 +119,12 @@ int remove_tree(const char *root); int copy_tree(const char *src_root, const char *dst_root, mode_t mode_root, uid_t uid, gid_t gid); +int +copy_file_secure(const char *src, + const char *dest, + mode_t mode, + uid_t uid, gid_t gid, + bool force); /* from selinux.c */ int selinux_file_context(const char *dst_name); -- 2.4.11