index: Check for valid paths before creating an index entry

src/index.c

@@ -756,19 +756,96 @@ void git_index_entry__init_from_stat(
 	entry->file_size = st->st_size;
 }
 
-static git_index_entry *index_entry_alloc(const char *path)
+/*
+ * We fundamentally don't like some paths: we don't want
+ * dot or dot-dot anywhere, and for obvious reasons don't
+ * want to recurse into ".git" either.
+ *
+ * Also, we don't want double slashes or slashes at the
+ * end that can make pathnames ambiguous.
+ */
+static int verify_dotfile(const char *rest)
+{
+	/*
+	 * The first character was '.', but that
+	 * has already been discarded, we now test
+	 * the rest.
+	 */
+
+	/* "." is not allowed */
+	if (*rest == '\0' || *rest == '/')
+		return -1;
+
+	switch (*rest) {
+		/*
+		 * ".git" followed by  NUL or slash is bad. This
+		 * shares the path end test with the ".." case.
+		 */
+		case 'g':
+		case 'G':
+			if (rest[1] != 'i' && rest[1] != 'I')
+				break;
+			if (rest[2] != 't' && rest[2] != 'T')
+				break;
+			rest += 2;
+			/* fallthrough */
+		case '.':
+			if (rest[1] == '\0' || rest[1] == '/')
+				return -1;
+	}
+	return 0;
+}
+
+static int verify_component(char c, const char *rest)
+{
+	if ((c == '.' && verify_dotfile(rest)) < 0 || c == '/' || c == '\0') {
+		giterr_set(GITERR_INDEX, "Invalid path component in index: '%c%s'", c, rest);
+		return -1;
+	}
+	return 0;
+}
+
+static int verify_path(const char *path)
+{
+	char c;
+
+	/* TODO: should we check this? */
+	/*
+	if (has_dos_drive_prefix(path))
+		return -1;
+	*/
+
+	c = *path++;
+	if (verify_component(c, path) < 0)
+		return -1;
+
+	while ((c = *path++) != '\0') {
+		if (c == '/') {
+			c = *path++;
+			if (verify_component(c, path) < 0)
+				return -1;
+		}
+	}
+	return 0;
+}
+
+static int index_entry_create(git_index_entry **out, const char *path)
 {
 	size_t pathlen = strlen(path);
-	struct entry_internal *entry =
-		git__calloc(sizeof(struct entry_internal) + pathlen + 1, 1);
-	if (!entry)
-		return NULL;
+	struct entry_internal *entry;
+
+	if (verify_path(path) < 0)
+		return -1;
+
+	entry = git__calloc(sizeof(struct entry_internal) + pathlen + 1, 1);
+	GITERR_CHECK_ALLOC(entry);
 
 	entry->pathlen = pathlen;
 	memcpy(entry->path, path, pathlen);
 	entry->entry.path = entry->path;
 
-	return (git_index_entry *)entry;
+	*out = (git_index_entry *)entry;
+	return 0;
 }
 
 static int index_entry_init(
@@ -784,14 +861,17 @@ static int index_entry_init(
 			"Could not initialize index entry. "
 			"Index is not backed up by an existing repository.");
 
+	if (index_entry_create(&entry, rel_path) < 0)
+		return -1;
+
 	/* write the blob to disk and get the oid and stat info */
 	error = git_blob__create_from_paths(
 		&oid, &st, INDEX_OWNER(index), NULL, rel_path, 0, true);
-	if (error < 0)
-		return error;
 
-	entry = index_entry_alloc(rel_path);
-	GITERR_CHECK_ALLOC(entry);
+	if (error < 0) {
+		index_entry_free(entry);
+		return error;
+	}
 
 	entry->id = oid;
 	git_index_entry__init_from_stat(entry, &st, !index->distrust_filemode);
@@ -856,11 +936,11 @@ static int index_entry_dup(git_index_entry **out, const git_index_entry *src)
 		return 0;
 	}
 
-	*out = entry = index_entry_alloc(src->path);
-	GITERR_CHECK_ALLOC(entry);
+	if (index_entry_create(&entry, src->path) < 0)
+		return -1;
 
 	index_entry_cpy(entry, src);
-
+	*out = entry;
 	return 0;
 }
 
@@ -2287,8 +2367,8 @@ static int read_tree_cb(
 	if (git_buf_joinpath(&path, root, tentry->filename) < 0)
 		return -1;
 
-	entry = index_entry_alloc(path.ptr);
-	GITERR_CHECK_ALLOC(entry);
+	if (index_entry_create(&entry, path.ptr) < 0)
+		return -1;
 
 	entry->mode = tentry->attr;
 	entry->id = tentry->oid;

tests/index/tests.c

@@ -309,31 +309,116 @@ void test_index_tests__add_bypath_to_a_bare_repository_returns_EBAREPO(void)
 	git_repository_free(bare_repo);
 }
 
+static void add_invalid_filename(git_repository *repo, const char *fn)
+{
+	git_index *index;
+	git_buf path = GIT_BUF_INIT;
+
+	cl_git_pass(git_repository_index(&index, repo));
+	cl_assert(git_index_entrycount(index) == 0);
+
+	git_buf_joinpath(&path, "./invalid", fn);
+
+	cl_git_mkfile(path.ptr, NULL);
+	cl_git_fail(git_index_add_bypath(index, fn));
+	cl_must_pass(p_unlink(path.ptr));
+
+	cl_assert(git_index_entrycount(index) == 0);
+
+	git_index_free(index);
+}
+
 /* Test that writing an invalid filename fails */
-void test_index_tests__write_invalid_filename(void)
+void test_index_tests__add_invalid_filename(void)
 {
 	git_repository *repo;
+
+	p_mkdir("invalid", 0700);
+
+	cl_git_pass(git_repository_init(&repo, "./invalid", 0));
+	cl_must_pass(p_mkdir("./invalid/subdir", 0777));
+
+	add_invalid_filename(repo, ".git/hello");
+	add_invalid_filename(repo, ".GIT/hello");
+	add_invalid_filename(repo, ".GiT/hello");
+	add_invalid_filename(repo, "./.git/hello");
+	add_invalid_filename(repo, "./foo");
+	add_invalid_filename(repo, "./bar");
+	add_invalid_filename(repo, "subdir/../bar");
+
+	git_repository_free(repo);
+
+	cl_fixture_cleanup("invalid");
+}
+
+static void replace_char(char *str, char in, char out)
+{
+	char *c = str;
+
+	while (*c++)
+		if (*c == in)
+			*c = out;
+}
+
+static void write_invalid_filename(git_repository *repo, const char *fn_orig)
+{
 	git_index *index;
 	git_oid expected;
+	const git_index_entry *entry;
+	git_buf path = GIT_BUF_INIT;
+	char *fn;
 
-	p_mkdir("read_tree", 0700);
-
-	cl_git_pass(git_repository_init(&repo, "./read_tree", 0));
 	cl_git_pass(git_repository_index(&index, repo));
-
 	cl_assert(git_index_entrycount(index) == 0);
 
-	cl_git_mkfile("./read_tree/.git/hello", NULL);
+	/*
+	 * Sneak a valid path into the index, we'll update it
+	 * to an invalid path when we try to write the index.
+	 */
+	fn = git__strdup(fn_orig);
+	replace_char(fn, '/', '_');
+
+	git_buf_joinpath(&path, "./invalid", fn);
+
+	cl_git_mkfile(path.ptr, NULL);
+
+	cl_git_pass(git_index_add_bypath(index, fn));
+
+	cl_assert(entry = git_index_get_bypath(index, fn, 0));
 
-	cl_git_pass(git_index_add_bypath(index, ".git/hello"));
+	/* kids, don't try this at home */
+	replace_char((char *)entry->path, '_', '/');
 
 	/* write-tree */
 	cl_git_fail(git_index_write_tree(&expected, index));
 
+	p_unlink(path.ptr);
+
+	cl_git_pass(git_index_remove_all(index, NULL, NULL, NULL));
 	git_index_free(index);
+	git__free(fn);
+}
+
+/* Test that writing an invalid filename fails */
+void test_index_tests__write_invalid_filename(void)
+{
+	git_repository *repo;
+
+	p_mkdir("invalid", 0700);
+
+	cl_git_pass(git_repository_init(&repo, "./invalid", 0));
+
+	write_invalid_filename(repo, ".git/hello");
+	write_invalid_filename(repo, ".GIT/hello");
+	write_invalid_filename(repo, ".GiT/hello");
+	write_invalid_filename(repo, "./.git/hello");
+	write_invalid_filename(repo, "./foo");
+	write_invalid_filename(repo, "./bar");
+	write_invalid_filename(repo, "foo/../bar");
+
 	git_repository_free(repo);
 
-	cl_fixture_cleanup("read_tree");
+	cl_fixture_cleanup("invalid");
 }
 
 void test_index_tests__remove_entry(void)