@matheusbsbs / matheus-bagliano / commits / 10ecfa7649

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 <peff@peff.net>

Jeff King committed May 4, 2018 at 20:03 UTC 10ecfa76491e4923988337b2e2243b05376b40de

4 files changed +37 -15

apply.c

+2 -2

@@ -3867,9 +3867,9 @@ static int check_unsafe_path(struct patch *patch)

3867 if (!patch->is_delete)

3868 new_name = patch->new_name;

3869

3870 - if (old_name && !verify_path(old_name))

3870 + if (old_name && !verify_path(old_name, patch->old_mode))

3871 return error(_("invalid path '%s'"), old_name);

3872 - if (new_name && !verify_path(new_name))

3872 + if (new_name && !verify_path(new_name, patch->new_mode))

3873 return error(_("invalid path '%s'"), new_name);

3874 return 0;

3875 }

builtin/update-index.c

+3 -3

@@ -399,7 +399,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,

399 int size, len, option;

400 struct cache_entry *ce;

401

402 - if (!verify_path(path))

402 + if (!verify_path(path, mode))

403 return error("Invalid path '%s'", path);

404

405 len = strlen(path);

@@ -452,7 +452,7 @@ static void update_one(const char *path)

452 stat_errno = errno;

453 } /* else stat is valid */

454

455 - if (!verify_path(path)) {

455 + if (!verify_path(path, st.st_mode)) {

456 fprintf(stderr, "Ignoring path %s\n", path);

457 return;

458 }

@@ -543,7 +543,7 @@ static void read_index_info(int nul_term_line)

543 path_name = uq.buf;

544 }

545

546 - if (!verify_path(path_name)) {

546 + if (!verify_path(path_name, mode)) {

547 fprintf(stderr, "Ignoring path %s\n", path_name);

548 continue;

549 }

cache.h

+1 -1

@@ -598,7 +598,7 @@ extern int read_index_unmerged(struct index_state *);

598 extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);

599 extern int discard_index(struct index_state *);

600 extern int unmerged_index(const struct index_state *);

601 -extern int verify_path(const char *path);

601 +extern int verify_path(const char *path, unsigned mode);

602 extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);

603 extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);

604 extern void adjust_dirname_case(struct index_state *istate, char *name);

read-cache.c

+31 -9

@@ -732,7 +732,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,

732 int size, len;

733 struct cache_entry *ce, *ret;

734

735 - if (!verify_path(path)) {

735 + if (!verify_path(path, mode)) {

736 error("Invalid path '%s'", path);

737 return NULL;

738 }

@@ -796,7 +796,7 @@ int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)

796 * Also, we don't want double slashes or slashes at the

797 * end that can make pathnames ambiguous.

798 */

799 -static int verify_dotfile(const char *rest)

799 +static int verify_dotfile(const char *rest, unsigned mode)

800 {

801 /*

802 * The first character was '.', but that

@@ -814,6 +814,9 @@ static int verify_dotfile(const char *rest)

814 * case-insensitively here, even if ignore_case is not set.

815 * This outlaws ".GIT" everywhere out of an abundance of caution,

816 * since there's really no good reason to allow it.

817 + *

818 + * Once we've seen ".git", we can also find ".gitmodules", etc (also

819 + * case-insensitively).

820 */

821 case 'g':

822 case 'G':

@@ -823,6 +826,12 @@ static int verify_dotfile(const char *rest)

826 break;

827 if (rest[3] == '\0' || is_dir_sep(rest[3]))

828 return 0;

829 + if (S_ISLNK(mode)) {

830 + rest += 3;

831 + if (skip_iprefix(rest, "modules", &rest) &&

832 + (*rest == '\0' || is_dir_sep(*rest)))

833 + return 0;

834 + }

835 break;

836 case '.':

837 if (rest[1] == '\0' || is_dir_sep(rest[1]))

@@ -831,7 +840,7 @@ static int verify_dotfile(const char *rest)

840 return 1;

841 }

842

834 -int verify_path(const char *path)

843 +int verify_path(const char *path, unsigned mode)

844 {

845 char c;

846

@@ -844,12 +853,25 @@ int verify_path(const char *path)

853 return 1;

854 if (is_dir_sep(c)) {

855 inside:

847 - if (protect_hfs && is_hfs_dotgit(path))

848 - return 0;

849 - if (protect_ntfs && is_ntfs_dotgit(path))

850 - return 0;

856 + if (protect_hfs) {

857 + if (is_hfs_dotgit(path))

858 + return 0;

859 + if (S_ISLNK(mode)) {

860 + if (is_hfs_dotgitmodules(path))

861 + return 0;

862 + }

863 + }

864 + if (protect_ntfs) {

865 + if (is_ntfs_dotgit(path))

866 + return 0;

867 + if (S_ISLNK(mode)) {

868 + if (is_ntfs_dotgitmodules(path))

869 + return 0;

870 + }

871 + }

872 +

873 c = *path++;

852 - if ((c == '.' && !verify_dotfile(path)) ||

874 + if ((c == '.' && !verify_dotfile(path, mode)) ||

875 is_dir_sep(c) || c == '\0')

876 return 0;

877 }

@@ -1166,7 +1188,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e

1188

1189 if (!ok_to_add)

1190 return -1;

1169 - if (!verify_path(ce->name))

1191 + if (!verify_path(ce->name, ce->ce_mode))

1192 return error("Invalid path '%s'", ce->name);

1193

1194 if (!skip_df_check &&