From 8423fd828107b2d42f8cc6d9a47f3a4b80408dff Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 4 May 2018 20:03:35 -0400 Subject: [PATCH] verify_path: disallow symlinks in .gitmodules, etc There are a few reasons it's not a good idea to make .git* files symlinks, including: 1. It won't be portable to systems without symlinks. 2. It may behave inconsistently, since Git internally may look at these files from the index or a tree without bothering to resolve any symbolic links. So it may work in some settings (where we read from the filesystem) but not in others). With some clever code, we could make (2) work. And some people may not care about (1) if they only work on one platform. But there are a few security reasons to simply disallow symlinked meta-files: a. A symlinked .gitmodules file may circumvent any fsck checks of the content. b. Git may read and write from the on-disk file without sanity checking the symlink target. So for example, if you link ".gitmodules" to "../oops" and run "git submodule add", we'll write to the file "oops" outside the repository. Again, both of those are problems that _could_ be solved with sufficient code, but given the current inconsistent behavior and unportability, we're better off just outlawing it explicitly. We'll give the same treatment to .gitmodules, .gitignore, and .gitattributes. The latter two cannot be used to write outside the repository (we write them only as part of a checkout, where we are careful not to follow any symlinks). But they can still cause a "git clone && git log" combination to read arbitrary files outside the filesystem. There's _probably_ nothing too harmful you can do with that, but it seems questionable (and anyway, they suffer from the same portability and consistency problems). Note the slightly tricky call to verify_path() in update-index's update_one(). There we may not have a mode if we're not updating from the filesystem (e.g., we might just be removing the file). Passing "0" as the mode there works fine; since it's not a symlink, we'll just skip the extra checks. Signed-off-by: Jeff King --- apply.c | 4 ++-- builtin/update-index.c | 6 +++--- cache.h | 2 +- read-cache.c | 46 +++++++++++++++++++++++++++++++++--------- 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/apply.c b/apply.c index f8bf0bd932..b96d375595 100644 --- a/apply.c +++ b/apply.c @@ -3867,9 +3867,9 @@ static int check_unsafe_path(struct patch *patch) if (!patch->is_delete) new_name = patch->new_name; - if (old_name && !verify_path(old_name)) + if (old_name && !verify_path(old_name, patch->old_mode)) return error(_("invalid path '%s'"), old_name); - if (new_name && !verify_path(new_name)) + if (new_name && !verify_path(new_name, patch->new_mode)) return error(_("invalid path '%s'"), new_name); return 0; } diff --git a/builtin/update-index.c b/builtin/update-index.c index 8d152ded77..19216595fb 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -399,7 +399,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid, int size, len, option; struct cache_entry *ce; - if (!verify_path(path)) + if (!verify_path(path, mode)) return error("Invalid path '%s'", path); len = strlen(path); @@ -452,7 +452,7 @@ static void update_one(const char *path) stat_errno = errno; } /* else stat is valid */ - if (!verify_path(path)) { + if (!verify_path(path, st.st_mode)) { fprintf(stderr, "Ignoring path %s\n", path); return; } @@ -543,7 +543,7 @@ static void read_index_info(int nul_term_line) path_name = uq.buf; } - if (!verify_path(path_name)) { + if (!verify_path(path_name, mode)) { fprintf(stderr, "Ignoring path %s\n", path_name); continue; } diff --git a/cache.h b/cache.h index 041c0fb261..5a44f79e26 100644 --- a/cache.h +++ b/cache.h @@ -598,7 +598,7 @@ extern int read_index_unmerged(struct index_state *); extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags); extern int discard_index(struct index_state *); extern int unmerged_index(const struct index_state *); -extern int verify_path(const char *path); +extern int verify_path(const char *path, unsigned mode); extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change); extern int index_dir_exists(struct index_state *istate, const char *name, int namelen); extern void adjust_dirname_case(struct index_state *istate, char *name); diff --git a/read-cache.c b/read-cache.c index 333e0c5429..9de17d2b66 100644 --- a/read-cache.c +++ b/read-cache.c @@ -732,7 +732,7 @@ struct cache_entry *make_cache_entry(unsigned int mode, int size, len; struct cache_entry *ce, *ret; - if (!verify_path(path)) { + if (!verify_path(path, mode)) { error("Invalid path '%s'", path); return NULL; } @@ -796,7 +796,7 @@ int ce_same_name(const struct cache_entry *a, const struct cache_entry *b) * Also, we don't want double slashes or slashes at the * end that can make pathnames ambiguous. */ -static int verify_dotfile(const char *rest) +static int verify_dotfile(const char *rest, unsigned mode) { /* * The first character was '.', but that @@ -814,6 +814,9 @@ static int verify_dotfile(const char *rest) * case-insensitively here, even if ignore_case is not set. * This outlaws ".GIT" everywhere out of an abundance of caution, * since there's really no good reason to allow it. + * + * Once we've seen ".git", we can also find ".gitmodules", etc (also + * case-insensitively). */ case 'g': case 'G': @@ -823,6 +826,14 @@ static int verify_dotfile(const char *rest) break; if (rest[3] == '\0' || is_dir_sep(rest[3])) return 0; + if (S_ISLNK(mode)) { + rest += 3; + if ((skip_iprefix(rest, "modules", &rest) || + skip_iprefix(rest, "ignore", &rest) || + skip_iprefix(rest, "attributes", &rest)) && + (*rest == '\0' || is_dir_sep(*rest))) + return 0; + } break; case '.': if (rest[1] == '\0' || is_dir_sep(rest[1])) @@ -831,7 +842,7 @@ static int verify_dotfile(const char *rest) return 1; } -int verify_path(const char *path) +int verify_path(const char *path, unsigned mode) { char c; @@ -844,12 +855,29 @@ int verify_path(const char *path) return 1; if (is_dir_sep(c)) { inside: - if (protect_hfs && is_hfs_dotgit(path)) - return 0; - if (protect_ntfs && is_ntfs_dotgit(path)) - return 0; + if (protect_hfs) { + if (is_hfs_dotgit(path)) + return 0; + if (S_ISLNK(mode)) { + if (is_hfs_dotgitmodules(path) || + is_hfs_dotgitignore(path) || + is_hfs_dotgitattributes(path)) + return 0; + } + } + if (protect_ntfs) { + if (is_ntfs_dotgit(path)) + return 0; + if (S_ISLNK(mode)) { + if (is_ntfs_dotgitmodules(path) || + is_ntfs_dotgitignore(path) || + is_ntfs_dotgitattributes(path)) + return 0; + } + } + c = *path++; - if ((c == '.' && !verify_dotfile(path)) || + if ((c == '.' && !verify_dotfile(path, mode)) || is_dir_sep(c) || c == '\0') return 0; } @@ -1166,7 +1194,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e if (!ok_to_add) return -1; - if (!verify_path(ce->name)) + if (!verify_path(ce->name, ce->ce_mode)) return error("Invalid path '%s'", ce->name); if (!skip_df_check &&