From 7aba6185d55e06db3f3ef18daa63baf3821e5030 Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Mon, 22 Feb 2010 23:32:11 +0100 Subject: [PATCH 1/6] Add a testcase for ACL with restrictive umask. Right now, Git creates unreadable pack files on non-shared repositories when the user has a umask of 077, even when the default ACLs for the directory would give read/write access to a specific user. Loose object files are created world-readable, which doesn't break ACLs, but isn't necessarily desirable. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- t/t1304-default-acl.sh | 67 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100755 t/t1304-default-acl.sh diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh new file mode 100755 index 0000000000..07dd6af99c --- /dev/null +++ b/t/t1304-default-acl.sh @@ -0,0 +1,67 @@ +#!/bin/sh +# +# Copyright (c) 2010 Matthieu Moy +# + +test_description='Test repository with default ACL' + +# Create the test repo with restrictive umask +# => this must come before . ./test-lib.sh +umask 077 + +. ./test-lib.sh + +# We need an arbitrary other user give permission to using ACLs. root +# is a good candidate: exists on all unices, and it has permission +# anyway, so we don't create a security hole running the testsuite. + +if ! setfacl -m u:root:rwx .; then + say "Skipping ACL tests: unable to use setfacl" + test_done +fi + +modebits () { + ls -l "$1" | sed -e 's|^\(..........\).*|\1|' +} + +check_perms_and_acl () { + actual=$(modebits "$1") && + case "$actual" in + -r--r-----*) + : happy + ;; + *) + echo "Got permission '$actual', expected '-r--r-----'" + false + ;; + esac && + getfacl "$1" > actual && + grep -q "user:root:rwx" actual && + grep -q "user:${LOGNAME}:rwx" actual && + grep -q "mask::r--" actual && + grep -q "group::---" actual || false +} + +dirs_to_set="./ .git/ .git/objects/ .git/objects/pack/" + +test_expect_success 'Setup test repo' ' + setfacl -m u:root:rwx $dirs_to_set && + setfacl -d -m u:"$LOGNAME":rwx $dirs_to_set && + setfacl -d -m u:root:rwx $dirs_to_set && + + touch file.txt && + git add file.txt && + git commit -m "init" +' + +test_expect_failure 'Objects creation does not break ACLs with restrictive umask' ' + # SHA1 for empty blob + check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 +' + +test_expect_failure 'git gc does not break ACLs with restrictive umask' ' + git gc && + check_perms_and_acl .git/objects/pack/*.pack +' + +test_done From 00787ed55adbc2350efa911bf0bdebf6ca08c095 Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Mon, 22 Feb 2010 23:32:12 +0100 Subject: [PATCH 2/6] Move gitmkstemps to path.c This function used to be only a compatibility function, but we're going to extend it and actually use it, so make it part of Git. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- Makefile | 1 - compat/mkstemps.c | 70 ----------------------------------------------- path.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 71 deletions(-) delete mode 100644 compat/mkstemps.c diff --git a/Makefile b/Makefile index 7bf2fca407..4387d4207f 100644 --- a/Makefile +++ b/Makefile @@ -1200,7 +1200,6 @@ ifdef NO_MKDTEMP endif ifdef NO_MKSTEMPS COMPAT_CFLAGS += -DNO_MKSTEMPS - COMPAT_OBJS += compat/mkstemps.o endif ifdef NO_UNSETENV COMPAT_CFLAGS += -DNO_UNSETENV diff --git a/compat/mkstemps.c b/compat/mkstemps.c deleted file mode 100644 index 14179c8e6d..0000000000 --- a/compat/mkstemps.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "../git-compat-util.h" - -/* Adapted from libiberty's mkstemp.c. */ - -#undef TMP_MAX -#define TMP_MAX 16384 - -int gitmkstemps(char *pattern, int suffix_len) -{ - static const char letters[] = - "abcdefghijklmnopqrstuvwxyz" - "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "0123456789"; - static const int num_letters = 62; - uint64_t value; - struct timeval tv; - char *template; - size_t len; - int fd, count; - - len = strlen(pattern); - - if (len < 6 + suffix_len) { - errno = EINVAL; - return -1; - } - - if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { - errno = EINVAL; - return -1; - } - - /* - * Replace pattern's XXXXXX characters with randomness. - * Try TMP_MAX different filenames. - */ - gettimeofday(&tv, NULL); - value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); - template = &pattern[len - 6 - suffix_len]; - for (count = 0; count < TMP_MAX; ++count) { - uint64_t v = value; - /* Fill in the random bits. */ - template[0] = letters[v % num_letters]; v /= num_letters; - template[1] = letters[v % num_letters]; v /= num_letters; - template[2] = letters[v % num_letters]; v /= num_letters; - template[3] = letters[v % num_letters]; v /= num_letters; - template[4] = letters[v % num_letters]; v /= num_letters; - template[5] = letters[v % num_letters]; v /= num_letters; - - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); - if (fd > 0) - return fd; - /* - * Fatal error (EPERM, ENOSPC etc). - * It doesn't make sense to loop. - */ - if (errno != EEXIST) - break; - /* - * This is a random value. It is only necessary that - * the next TMP_MAX values generated by adding 7777 to - * VALUE are different with (module 2^32). - */ - value += 7777; - } - /* We return the null string if we can't find a unique file name. */ - pattern[0] = '\0'; - errno = EINVAL; - return -1; -} diff --git a/path.c b/path.c index 79aa104712..ab2e3687ab 100644 --- a/path.c +++ b/path.c @@ -157,6 +157,75 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) return mkstemps(path, suffix_len); } +/* Adapted from libiberty's mkstemp.c. */ + +#undef TMP_MAX +#define TMP_MAX 16384 + +int gitmkstemps(char *pattern, int suffix_len) +{ + static const char letters[] = + "abcdefghijklmnopqrstuvwxyz" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "0123456789"; + static const int num_letters = 62; + uint64_t value; + struct timeval tv; + char *template; + size_t len; + int fd, count; + + len = strlen(pattern); + + if (len < 6 + suffix_len) { + errno = EINVAL; + return -1; + } + + if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) { + errno = EINVAL; + return -1; + } + + /* + * Replace pattern's XXXXXX characters with randomness. + * Try TMP_MAX different filenames. + */ + gettimeofday(&tv, NULL); + value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid(); + template = &pattern[len - 6 - suffix_len]; + for (count = 0; count < TMP_MAX; ++count) { + uint64_t v = value; + /* Fill in the random bits. */ + template[0] = letters[v % num_letters]; v /= num_letters; + template[1] = letters[v % num_letters]; v /= num_letters; + template[2] = letters[v % num_letters]; v /= num_letters; + template[3] = letters[v % num_letters]; v /= num_letters; + template[4] = letters[v % num_letters]; v /= num_letters; + template[5] = letters[v % num_letters]; v /= num_letters; + + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); + if (fd > 0) + return fd; + /* + * Fatal error (EPERM, ENOSPC etc). + * It doesn't make sense to loop. + */ + if (errno != EEXIST) + break; + /* + * This is a random value. It is only necessary that + * the next TMP_MAX values generated by adding 7777 to + * VALUE are different with (module 2^32). + */ + value += 7777; + } + /* We return the null string if we can't find a unique file name. */ + pattern[0] = '\0'; + errno = EINVAL; + return -1; +} + int validate_headref(const char *path) { struct stat st; From b862b61c03797fd00490bb8caf05be840b79c6cb Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Mon, 22 Feb 2010 23:32:13 +0100 Subject: [PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument. gitmkstemps emulates the behavior of mkstemps, which is usually used to create files in a shared directory like /tmp/, hence, it creates files with permission 0600. Add git_mkstemps_mode() that allows us to specify the desired mode, and make git_mkstemps() a wrapper that always uses 0600 to call it. Later we will use git_mkstemps_mode() when creating pack files. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- cache.h | 4 ++++ path.c | 15 +++++++++++++-- wrapper.c | 10 ++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/cache.h b/cache.h index d478eff1f3..0319637723 100644 --- a/cache.h +++ b/cache.h @@ -641,6 +641,10 @@ int git_mkstemp(char *path, size_t n, const char *template); int git_mkstemps(char *path, size_t n, const char *template, int suffix_len); +/* set default permissions by passing mode arguments to open(2) */ +int git_mkstemps_mode(char *pattern, int suffix_len, int mode); +int git_mkstemp_mode(char *pattern, int mode); + /* * NOTE NOTE NOTE!! * diff --git a/path.c b/path.c index ab2e3687ab..03d284ba8b 100644 --- a/path.c +++ b/path.c @@ -162,7 +162,7 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len) #undef TMP_MAX #define TMP_MAX 16384 -int gitmkstemps(char *pattern, int suffix_len) +int git_mkstemps_mode(char *pattern, int suffix_len, int mode) { static const char letters[] = "abcdefghijklmnopqrstuvwxyz" @@ -204,7 +204,7 @@ int gitmkstemps(char *pattern, int suffix_len) template[4] = letters[v % num_letters]; v /= num_letters; template[5] = letters[v % num_letters]; v /= num_letters; - fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600); + fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode); if (fd > 0) return fd; /* @@ -226,6 +226,17 @@ int gitmkstemps(char *pattern, int suffix_len) return -1; } +int git_mkstemp_mode(char *pattern, int mode) +{ + /* mkstemp is just mkstemps with no suffix */ + return git_mkstemps_mode(pattern, 0, mode); +} + +int gitmkstemps(char *pattern, int suffix_len) +{ + return git_mkstemps_mode(pattern, suffix_len, 0600); +} + int validate_headref(const char *path) { struct stat st; diff --git a/wrapper.c b/wrapper.c index 0e3e20a3fd..673762fde9 100644 --- a/wrapper.c +++ b/wrapper.c @@ -204,6 +204,16 @@ int xmkstemp(char *template) return fd; } +int xmkstemp_mode(char *template, int mode) +{ + int fd; + + fd = git_mkstemp_mode(template, mode); + if (fd < 0) + die_errno("Unable to create temporary file"); + return fd; +} + /* * zlib wrappers to make sure we don't silently miss errors * at init time. From f80c7ae8fe9c0f3ce93c96a2dccaba34e456e33a Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Mon, 22 Feb 2010 23:32:14 +0100 Subject: [PATCH 4/6] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later. We used to create 0600 files, and then use chmod to set the group and other permission bits to the umask. This usually has the same effect as a normal file creation with a umask. But in the presence of ACLs, the group permission plays the role of the ACL mask: the "g" bits of newly created files are chosen according to default ACL mask of the directory, not according to the umask, and doing a chmod() on these "g" bits affect the ACL's mask instead of actual group permission. In other words, creating files with 0600 and then doing a chmod to the umask creates files which are unreadable by users allowed in the default ACL. To create the files without breaking ACLs, we let the umask do it's job at the file's creation time, and get rid of the later chmod. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- builtin-pack-objects.c | 18 ++---------------- t/t1304-default-acl.sh | 2 +- wrapper.c | 10 +++++++--- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index e1d3adf405..539e75d56f 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -464,9 +464,6 @@ static int write_one(struct sha1file *f, return 1; } -/* forward declaration for write_pack_file */ -static int adjust_perm(const char *path, mode_t mode); - static void write_pack_file(void) { uint32_t i = 0, j; @@ -523,21 +520,17 @@ static void write_pack_file(void) } if (!pack_to_stdout) { - mode_t mode = umask(0); struct stat st; const char *idx_tmp_name; char tmpname[PATH_MAX]; - umask(mode); - mode = 0444 & ~mode; - idx_tmp_name = write_idx_file(NULL, written_list, nr_written, sha1); snprintf(tmpname, sizeof(tmpname), "%s-%s.pack", base_name, sha1_to_hex(sha1)); free_pack_by_name(tmpname); - if (adjust_perm(pack_tmp_name, mode)) + if (adjust_shared_perm(pack_tmp_name)) die_errno("unable to make temporary pack file readable"); if (rename(pack_tmp_name, tmpname)) die_errno("unable to rename temporary pack file"); @@ -565,7 +558,7 @@ static void write_pack_file(void) snprintf(tmpname, sizeof(tmpname), "%s-%s.idx", base_name, sha1_to_hex(sha1)); - if (adjust_perm(idx_tmp_name, mode)) + if (adjust_shared_perm(idx_tmp_name)) die_errno("unable to make temporary index file readable"); if (rename(idx_tmp_name, tmpname)) die_errno("unable to rename temporary index file"); @@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av) loosen_unused_packed_objects(&revs); } -static int adjust_perm(const char *path, mode_t mode) -{ - if (chmod(path, mode)) - return -1; - return adjust_shared_perm(path); -} - int cmd_pack_objects(int argc, const char **argv, const char *prefix) { int use_internal_rev_list = 0; diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh index 07dd6af99c..8472dbb44a 100755 --- a/t/t1304-default-acl.sh +++ b/t/t1304-default-acl.sh @@ -59,7 +59,7 @@ test_expect_failure 'Objects creation does not break ACLs with restrictive umask check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 ' -test_expect_failure 'git gc does not break ACLs with restrictive umask' ' +test_expect_success 'git gc does not break ACLs with restrictive umask' ' git gc && check_perms_and_acl .git/objects/pack/*.pack ' diff --git a/wrapper.c b/wrapper.c index 673762fde9..9c71b21242 100644 --- a/wrapper.c +++ b/wrapper.c @@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush) int odb_mkstemp(char *template, size_t limit, const char *pattern) { int fd; - + /* + * we let the umask do its job, don't try to be more + * restrictive except to remove write permission. + */ + int mode = 0444; snprintf(template, limit, "%s/%s", get_object_directory(), pattern); - fd = mkstemp(template); + fd = git_mkstemp_mode(template, mode); if (0 <= fd) return fd; @@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern) snprintf(template, limit, "%s/%s", get_object_directory(), pattern); safe_create_leading_directories(template); - return xmkstemp(template); + return xmkstemp_mode(template, mode); } int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1) From 1d9740cb324f7f5d798ecfc259dc213b244ad9b7 Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Mon, 22 Feb 2010 23:32:15 +0100 Subject: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL on exit. When reaching the end of git_mkstemps_mode, at least one call to open() has been done, and errno has been set accordingly. Setting errno is therefore not necessary, and actually harmfull since callers can't distinguish e.g. permanent failure from ENOENT, which can just mean that we need to create the containing directory. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- path.c | 1 - 1 file changed, 1 deletion(-) diff --git a/path.c b/path.c index 03d284ba8b..12ef731ace 100644 --- a/path.c +++ b/path.c @@ -222,7 +222,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) } /* We return the null string if we can't find a unique file name. */ pattern[0] = '\0'; - errno = EINVAL; return -1; } From 5256b006312e4d06e11b49a8b128e9e550e54f31 Mon Sep 17 00:00:00 2001 From: Matthieu Moy Date: Mon, 22 Feb 2010 23:32:16 +0100 Subject: [PATCH 6/6] Use git_mkstemp_mode instead of plain mkstemp to create object files We used to unnecessarily give the read permission to group and others, regardless of the umask, which isn't serious because the objects are still protected by their containing directory, but isn't necessary either. Signed-off-by: Matthieu Moy Signed-off-by: Junio C Hamano --- sha1_file.c | 6 +++--- t/t1304-default-acl.sh | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 657825e14e..3316f282c6 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2206,7 +2206,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename) } out: - if (set_shared_perm(filename, (S_IFREG|0444))) + if (adjust_shared_perm(filename)) return error("unable to set permission to '%s'", filename); return 0; } @@ -2262,7 +2262,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) } memcpy(buffer, filename, dirlen); strcpy(buffer + dirlen, "tmp_obj_XXXXXX"); - fd = mkstemp(buffer); + fd = git_mkstemp_mode(buffer, 0444); if (fd < 0 && dirlen && errno == ENOENT) { /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); @@ -2272,7 +2272,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Try again */ strcpy(buffer + dirlen - 1, "/tmp_obj_XXXXXX"); - fd = mkstemp(buffer); + fd = git_mkstemp_mode(buffer, 0444); } return fd; } diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh index 8472dbb44a..cc30be4a65 100755 --- a/t/t1304-default-acl.sh +++ b/t/t1304-default-acl.sh @@ -54,7 +54,7 @@ test_expect_success 'Setup test repo' ' git commit -m "init" ' -test_expect_failure 'Objects creation does not break ACLs with restrictive umask' ' +test_expect_success 'Objects creation does not break ACLs with restrictive umask' ' # SHA1 for empty blob check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 '