From 10ecfa76491e4923988337b2e2243b05376b40de 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 There are a few reasons it's not a good idea to make .gitmodules a symlink, including: 1. It won't be portable to systems without symlinks. 2. It may behave inconsistently, since Git may look at this file in the index or a tree without bothering to resolve any symbolic links. We don't do this _yet_, but the config infrastructure is there and it's planned for the future. 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 it: 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 complications in (1) and (2), we're better off just outlawing it explicitly. 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 | 40 +++++++++++++++++++++++++++++++--------- 4 files changed, 37 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..aa99f1f797 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,12 @@ 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) && + (*rest == '\0' || is_dir_sep(*rest))) + return 0; + } break; case '.': if (rest[1] == '\0' || is_dir_sep(rest[1])) @@ -831,7 +840,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 +853,25 @@ 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)) + return 0; + } + } + if (protect_ntfs) { + if (is_ntfs_dotgit(path)) + return 0; + if (S_ISLNK(mode)) { + if (is_ntfs_dotgitmodules(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 +1188,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 && -- 2.39.2