Merge branch 'jk/submodule-name-verify-fix' into jk/submodule-name-verify-fsck

* jk/submodule-name-verify-fix:
  verify_path: disallow symlinks in .gitmodules
  update-index: stat updated files earlier
  verify_path: drop clever fallthrough
  skip_prefix: add icase-insensitive variant
  is_{hfs,ntfs}_dotgitmodules: add tests
  path: match NTFS short names for more .git files
  is_hfs_dotgit: match other .git files
  is_ntfs_dotgit: use a size_t for traversing string
  submodule-config: verify submodule names as paths

Note that this includes two bits of evil-merge:

 - there's a new call to verify_path() that doesn't actually
   have a mode available. It should be OK to pass "0" here,
   since we're just manipulating the untracked cache, not an
   actual index entry.

 - the lstat() in builtin/update-index.c:update_one() needs
   to be updated to handle the fsmonitor case (without this
   it still behaves correctly, but does an unnecessary
   lstat).
This commit is contained in:
Jeff King 2018-05-15 10:15:18 -04:00
commit eedd5949f5
16 changed files with 474 additions and 42 deletions

View file

@ -3860,9 +3860,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;
}

View file

@ -1817,6 +1817,29 @@ static int is_active(int argc, const char **argv, const char *prefix)
return !is_submodule_active(the_repository, argv[1]);
}
/*
* Exit non-zero if any of the submodule names given on the command line is
* invalid. If no names are given, filter stdin to print only valid names
* (which is primarily intended for testing).
*/
static int check_name(int argc, const char **argv, const char *prefix)
{
if (argc > 1) {
while (*++argv) {
if (check_submodule_name(*argv) < 0)
return 1;
}
} else {
struct strbuf buf = STRBUF_INIT;
while (strbuf_getline(&buf, stdin) != EOF) {
if (!check_submodule_name(buf.buf))
printf("%s\n", buf.buf);
}
strbuf_release(&buf);
}
return 0;
}
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@ -1842,6 +1865,7 @@ static struct cmd_struct commands[] = {
{"push-check", push_check, 0},
{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
{"is-active", is_active, 0},
{"check-name", check_name, 0},
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)

View file

@ -364,10 +364,9 @@ static int process_directory(const char *path, int len, struct stat *st)
return error("%s: is a directory - add files inside instead", path);
}
static int process_path(const char *path)
static int process_path(const char *path, struct stat *st, int stat_errno)
{
int pos, len;
struct stat st;
const struct cache_entry *ce;
len = strlen(path);
@ -391,13 +390,13 @@ static int process_path(const char *path)
* First things first: get the stat information, to decide
* what to do about the pathname!
*/
if (lstat(path, &st) < 0)
return process_lstat_error(path, errno);
if (stat_errno)
return process_lstat_error(path, stat_errno);
if (S_ISDIR(st.st_mode))
return process_directory(path, len, &st);
if (S_ISDIR(st->st_mode))
return process_directory(path, len, st);
return add_one_path(ce, path, len, &st);
return add_one_path(ce, path, len, st);
}
static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
@ -406,7 +405,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);
@ -449,7 +448,18 @@ static void chmod_path(char flip, const char *path)
static void update_one(const char *path)
{
if (!verify_path(path)) {
int stat_errno = 0;
struct stat st;
if (mark_valid_only || mark_skip_worktree_only || force_remove ||
mark_fsmonitor_only)
st.st_mode = 0;
else if (lstat(path, &st) < 0) {
st.st_mode = 0;
stat_errno = errno;
} /* else stat is valid */
if (!verify_path(path, st.st_mode)) {
fprintf(stderr, "Ignoring path %s\n", path);
return;
}
@ -475,7 +485,7 @@ static void update_one(const char *path)
report("remove '%s'", path);
return;
}
if (process_path(path))
if (process_path(path, &st, stat_errno))
die("Unable to process path %s", path);
report("add '%s'", path);
}
@ -545,7 +555,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;
}

12
cache.h
View file

@ -634,7 +634,7 @@ extern int unmerged_index(const struct index_state *);
*/
extern int index_has_changes(struct strbuf *sb);
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);
@ -1165,7 +1165,15 @@ int normalize_path_copy(char *dst, const char *src);
int longest_ancestor_length(const char *path, struct string_list *prefixes);
char *strip_path_suffix(const char *path, const char *suffix);
int daemon_avoid_alias(const char *path);
extern int is_ntfs_dotgit(const char *name);
/*
* These functions match their is_hfs_dotgit() counterparts; see utf8.h for
* details.
*/
int is_ntfs_dotgit(const char *name);
int is_ntfs_dotgitmodules(const char *name);
int is_ntfs_dotgitignore(const char *name);
int is_ntfs_dotgitattributes(const char *name);
/*
* Returns true iff "str" could be confused as a command-line option when

2
dir.c
View file

@ -2992,7 +2992,7 @@ void untracked_cache_invalidate_path(struct index_state *istate,
{
if (!istate->untracked || !istate->untracked->root)
return;
if (!safe_path && !verify_path(path))
if (!safe_path && !verify_path(path, 0))
return;
invalidate_one_component(istate->untracked, istate->untracked->root,
path, strlen(path));

View file

@ -1001,6 +1001,23 @@ static inline int sane_iscase(int x, int is_lower)
return (x & 0x20) == 0;
}
/*
* Like skip_prefix, but compare case-insensitively. Note that the comparison
* is done via tolower(), so it is strictly ASCII (no multi-byte characters or
* locale-specific conversions).
*/
static inline int skip_iprefix(const char *str, const char *prefix,
const char **out)
{
do {
if (!*prefix) {
*out = str;
return 1;
}
} while (tolower(*str++) == tolower(*prefix++));
return 0;
}
static inline int strtoul_ui(char const *s, int base, unsigned int *result)
{
unsigned long ul;

View file

@ -229,6 +229,11 @@ Use -f if you really want to add it." >&2
sm_name="$sm_path"
fi
if ! git submodule--helper check-name "$sm_name"
then
die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
fi
# perhaps the path exists and is already a git repo, else clone it
if test -e "$sm_path"
then

86
path.c
View file

@ -1305,7 +1305,7 @@ static int only_spaces_and_periods(const char *path, size_t len, size_t skip)
int is_ntfs_dotgit(const char *name)
{
int len;
size_t len;
for (len = 0; ; len++)
if (!name[len] || name[len] == '\\' || is_dir_sep(name[len])) {
@ -1322,6 +1322,90 @@ int is_ntfs_dotgit(const char *name)
}
}
static int is_ntfs_dot_generic(const char *name,
const char *dotgit_name,
size_t len,
const char *dotgit_ntfs_shortname_prefix)
{
int saw_tilde;
size_t i;
if ((name[0] == '.' && !strncasecmp(name + 1, dotgit_name, len))) {
i = len + 1;
only_spaces_and_periods:
for (;;) {
char c = name[i++];
if (!c)
return 1;
if (c != ' ' && c != '.')
return 0;
}
}
/*
* Is it a regular NTFS short name, i.e. shortened to 6 characters,
* followed by ~1, ... ~4?
*/
if (!strncasecmp(name, dotgit_name, 6) && name[6] == '~' &&
name[7] >= '1' && name[7] <= '4') {
i = 8;
goto only_spaces_and_periods;
}
/*
* Is it a fall-back NTFS short name (for details, see
* https://en.wikipedia.org/wiki/8.3_filename?
*/
for (i = 0, saw_tilde = 0; i < 8; i++)
if (name[i] == '\0')
return 0;
else if (saw_tilde) {
if (name[i] < '0' || name[i] > '9')
return 0;
} else if (name[i] == '~') {
if (name[++i] < '1' || name[i] > '9')
return 0;
saw_tilde = 1;
} else if (i >= 6)
return 0;
else if (name[i] < 0) {
/*
* We know our needles contain only ASCII, so we clamp
* here to make the results of tolower() sane.
*/
return 0;
} else if (tolower(name[i]) != dotgit_ntfs_shortname_prefix[i])
return 0;
goto only_spaces_and_periods;
}
/*
* Inline helper to make sure compiler resolves strlen() on literals at
* compile time.
*/
static inline int is_ntfs_dot_str(const char *name, const char *dotgit_name,
const char *dotgit_ntfs_shortname_prefix)
{
return is_ntfs_dot_generic(name, dotgit_name, strlen(dotgit_name),
dotgit_ntfs_shortname_prefix);
}
int is_ntfs_dotgitmodules(const char *name)
{
return is_ntfs_dot_str(name, "gitmodules", "gi7eba");
}
int is_ntfs_dotgitignore(const char *name)
{
return is_ntfs_dot_str(name, "gitignore", "gi250a");
}
int is_ntfs_dotgitattributes(const char *name)
{
return is_ntfs_dot_str(name, "gitattributes", "gi7d29");
}
int looks_like_command_line_option(const char *str)
{
return str && str[0] == '-';

View file

@ -752,7 +752,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;
}
@ -817,7 +817,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
@ -831,8 +831,13 @@ static int verify_dotfile(const char *rest)
switch (*rest) {
/*
* ".git" followed by NUL or slash is bad. This
* shares the path end test with the ".." case.
* ".git" followed by NUL or slash is bad. Note that we match
* 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':
@ -840,8 +845,15 @@ static int verify_dotfile(const char *rest)
break;
if (rest[2] != 't' && rest[2] != 'T')
break;
rest += 2;
/* fallthrough */
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]))
return 0;
@ -849,7 +861,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;
@ -862,12 +874,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;
}
@ -1184,7 +1209,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 &&

View file

@ -190,6 +190,31 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
return NULL;
}
int check_submodule_name(const char *name)
{
/* Disallow empty names */
if (!*name)
return -1;
/*
* Look for '..' as a path component. Check both '/' and '\\' as
* separators rather than is_dir_sep(), because we want the name rules
* to be consistent across platforms.
*/
goto in_component; /* always start inside component */
while (*name) {
char c = *name++;
if (c == '/' || c == '\\') {
in_component:
if (name[0] == '.' && name[1] == '.' &&
(!name[2] || name[2] == '/' || name[2] == '\\'))
return -1;
}
}
return 0;
}
static int name_and_item_from_var(const char *var, struct strbuf *name,
struct strbuf *item)
{
@ -201,6 +226,12 @@ static int name_and_item_from_var(const char *var, struct strbuf *name,
return 0;
strbuf_add(name, subsection, subsection_len);
if (check_submodule_name(name->buf) < 0) {
warning(_("ignoring suspicious submodule name: %s"), name->buf);
strbuf_release(name);
return 0;
}
strbuf_addstr(item, key);
return 1;

View file

@ -48,4 +48,11 @@ extern const struct submodule *submodule_from_cache(struct repository *repo,
const char *key);
extern void submodule_free(void);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
* (e.g., that may be found in the subsection of a .gitmodules file) and -1
* otherwise.
*/
int check_submodule_name(const char *name);
#endif /* SUBMODULE_CONFIG_H */

View file

@ -1,5 +1,6 @@
#include "cache.h"
#include "string-list.h"
#include "utf8.h"
/*
* A "string_list_each_func_t" function that normalizes an entry from
@ -170,6 +171,11 @@ static struct test_data dirname_data[] = {
{ NULL, NULL }
};
static int is_dotgitmodules(const char *path)
{
return is_hfs_dotgitmodules(path) || is_ntfs_dotgitmodules(path);
}
int cmd_main(int argc, const char **argv)
{
if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
@ -270,6 +276,20 @@ int cmd_main(int argc, const char **argv)
if (argc == 2 && !strcmp(argv[1], "dirname"))
return test_function(dirname_data, posix_dirname, argv[1]);
if (argc > 2 && !strcmp(argv[1], "is_dotgitmodules")) {
int res = 0, expect = 1, i;
for (i = 2; i < argc; i++)
if (!strcmp("--not", argv[i]))
expect = !expect;
else if (expect != is_dotgitmodules(argv[i]))
res = error("'%s' is %s.gitmodules", argv[i],
expect ? "not " : "");
else
fprintf(stderr, "ok: '%s' is %s.gitmodules\n",
argv[i], expect ? "" : "not ");
return !!res;
}
fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
argv[1] ? argv[1] : "(there was none)");
return 1;

View file

@ -349,4 +349,90 @@ test_submodule_relative_url "(null)" "ssh://hostname:22/repo" "../subrepo" "ssh:
test_submodule_relative_url "(null)" "user@host:path/to/repo" "../subrepo" "user@host:path/to/subrepo"
test_submodule_relative_url "(null)" "user@host:repo" "../subrepo" "user@host:subrepo"
test_expect_success 'match .gitmodules' '
test-path-utils is_dotgitmodules \
.gitmodules \
\
.git${u200c}modules \
\
.Gitmodules \
.gitmoduleS \
\
".gitmodules " \
".gitmodules." \
".gitmodules " \
".gitmodules. " \
".gitmodules ." \
".gitmodules.." \
".gitmodules " \
".gitmodules. " \
".gitmodules . " \
".gitmodules ." \
\
".Gitmodules " \
".Gitmodules." \
".Gitmodules " \
".Gitmodules. " \
".Gitmodules ." \
".Gitmodules.." \
".Gitmodules " \
".Gitmodules. " \
".Gitmodules . " \
".Gitmodules ." \
\
GITMOD~1 \
gitmod~1 \
GITMOD~2 \
gitmod~3 \
GITMOD~4 \
\
"GITMOD~1 " \
"gitmod~2." \
"GITMOD~3 " \
"gitmod~4. " \
"GITMOD~1 ." \
"gitmod~2 " \
"GITMOD~3. " \
"gitmod~4 . " \
\
GI7EBA~1 \
gi7eba~9 \
\
GI7EB~10 \
GI7EB~11 \
GI7EB~99 \
GI7EB~10 \
GI7E~100 \
GI7E~101 \
GI7E~999 \
~1000000 \
~9999999 \
\
--not \
".gitmodules x" \
".gitmodules .x" \
\
" .gitmodules" \
\
..gitmodules \
\
gitmodules \
\
.gitmodule \
\
".gitmodules x " \
".gitmodules .x" \
\
GI7EBA~ \
GI7EBA~0 \
GI7EBA~~1 \
GI7EBA~X \
Gx7EBA~1 \
GI7EBX~1 \
\
GI7EB~1 \
GI7EB~01 \
GI7EB~1X
'
test_done

76
t/t7415-submodule-names.sh Executable file
View file

@ -0,0 +1,76 @@
#!/bin/sh
test_description='check handling of .. in submodule names
Exercise the name-checking function on a variety of names, and then give a
real-world setup that confirms we catch this in practice.
'
. ./test-lib.sh
test_expect_success 'check names' '
cat >expect <<-\EOF &&
valid
valid/with/paths
EOF
git submodule--helper check-name >actual <<-\EOF &&
valid
valid/with/paths
../foo
/../foo
..\foo
\..\foo
foo/..
foo/../
foo\..
foo\..\
foo/../bar
EOF
test_cmp expect actual
'
test_expect_success 'create innocent subrepo' '
git init innocent &&
git -C innocent commit --allow-empty -m foo
'
test_expect_success 'submodule add refuses invalid names' '
test_must_fail \
git submodule add --name ../../modules/evil "$PWD/innocent" evil
'
test_expect_success 'add evil submodule' '
git submodule add "$PWD/innocent" evil &&
mkdir modules &&
cp -r .git/modules/evil modules &&
write_script modules/evil/hooks/post-checkout <<-\EOF &&
echo >&2 "RUNNING POST CHECKOUT"
EOF
git config -f .gitmodules submodule.evil.update checkout &&
git config -f .gitmodules --rename-section \
submodule.evil submodule.../../modules/evil &&
git add modules &&
git commit -am evil
'
# This step seems like it shouldn't be necessary, since the payload is
# contained entirely in the evil submodule. But due to the vagaries of the
# submodule code, checking out the evil module will fail unless ".git/modules"
# exists. Adding another submodule (with a name that sorts before "evil") is an
# easy way to make sure this is the case in the victim clone.
test_expect_success 'add other submodule' '
git submodule add "$PWD/innocent" another-module &&
git add another-module &&
git commit -am another
'
test_expect_success 'clone evil superproject' '
git clone --recurse-submodules . victim >output 2>&1 &&
! grep "RUNNING POST CHECKOUT" output
'
test_done

58
utf8.c
View file

@ -620,28 +620,33 @@ static ucs_char_t next_hfs_char(const char **in)
}
}
int is_hfs_dotgit(const char *path)
static int is_hfs_dot_generic(const char *path,
const char *needle, size_t needle_len)
{
ucs_char_t c;
c = next_hfs_char(&path);
if (c != '.')
return 0;
c = next_hfs_char(&path);
/*
* there's a great deal of other case-folding that occurs
* in HFS+, but this is enough to catch anything that will
* convert to ".git"
* in HFS+, but this is enough to catch our fairly vanilla
* hard-coded needles.
*/
if (c != 'g' && c != 'G')
return 0;
c = next_hfs_char(&path);
if (c != 'i' && c != 'I')
return 0;
c = next_hfs_char(&path);
if (c != 't' && c != 'T')
return 0;
for (; needle_len > 0; needle++, needle_len--) {
c = next_hfs_char(&path);
/*
* We know our needles contain only ASCII, so we clamp here to
* make the results of tolower() sane.
*/
if (c > 127)
return 0;
if (tolower(c) != *needle)
return 0;
}
c = next_hfs_char(&path);
if (c && !is_dir_sep(c))
return 0;
@ -649,6 +654,35 @@ int is_hfs_dotgit(const char *path)
return 1;
}
/*
* Inline wrapper to make sure the compiler resolves strlen() on literals at
* compile time.
*/
static inline int is_hfs_dot_str(const char *path, const char *needle)
{
return is_hfs_dot_generic(path, needle, strlen(needle));
}
int is_hfs_dotgit(const char *path)
{
return is_hfs_dot_str(path, "git");
}
int is_hfs_dotgitmodules(const char *path)
{
return is_hfs_dot_str(path, "gitmodules");
}
int is_hfs_dotgitignore(const char *path)
{
return is_hfs_dot_str(path, "gitignore");
}
int is_hfs_dotgitattributes(const char *path)
{
return is_hfs_dot_str(path, "gitattributes");
}
const char utf8_bom[] = "\357\273\277";
int skip_utf8_bom(char **text, size_t len)

5
utf8.h
View file

@ -52,8 +52,13 @@ int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
* The path should be NUL-terminated, but we will match variants of both ".git\0"
* and ".git/..." (but _not_ ".../.git"). This makes it suitable for both fsck
* and verify_path().
*
* Likewise, the is_hfs_dotgitfoo() variants look for ".gitfoo".
*/
int is_hfs_dotgit(const char *path);
int is_hfs_dotgitmodules(const char *path);
int is_hfs_dotgitignore(const char *path);
int is_hfs_dotgitattributes(const char *path);
typedef enum {
ALIGN_LEFT,