Merge pull request #5331 from pks-t/security-fixes

Security fixes for master

docs/changelog.md

 v0.28 + 1
 ---------
 
+# Security Fixes
+
+- CVE-2019-1348: the fast-import stream command "feature
+  export-marks=path" allows writing to arbitrary file paths. As
+  libgit2 does not offer any interface for fast-import, it is not
+  susceptible to this vulnerability.
+
+- CVE-2019-1349: by using NTFS 8.3 short names, backslashes or
+  alternate filesystreams, it is possible to cause submodules to
+  be written into pre-existing directories during a recursive
+  clone using git. As libgit2 rejects cloning into non-empty
+  directories by default, it is not susceptible to this
+  vulnerability.
+
+- CVE-2019-1350: recursive clones may lead to arbitrary remote
+  code executing due to improper quoting of command line
+  arguments. As libgit2 uses libssh2, which does not require us
+  to perform command line parsing, it is not susceptible to this
+  vulnerability.
+
+- CVE-2019-1351: Windows provides the ability to substitute
+  drive letters with arbitrary letters, including multi-byte
+  Unicode letters. To fix any potential issues arising from
+  interpreting such paths as relative paths, we have extended
+  detection of DOS drive prefixes to accomodate for such cases.
+
+- CVE-2019-1352: by using NTFS-style alternative file streams for
+  the ".git" directory, it is possible to overwrite parts of the
+  repository. While this has been fixed in the past for Windows,
+  the same vulnerability may also exist on other systems that
+  write to NTFS filesystems. We now reject any paths starting
+  with ".git:" on all systems.
+
+- CVE-2019-1353: by using NTFS-style 8.3 short names, it was
+  possible to write to the ".git" directory and thus overwrite
+  parts of the repository, leading to possible remote code
+  execution. While this problem was already fixed in the past for
+  Windows, other systems accessing NTFS filesystems are
+  vulnerable to this issue too. We now enable NTFS protecions by
+  default on all systems to fix this attack vector.
+
+- CVE-2019-1354: on Windows, backslashes are not a valid part of
+  a filename but are instead interpreted as directory separators.
+  As other platforms allowed to use such paths, it was possible
+  to write such invalid entries into a Git repository and was
+  thus an attack vector to write into the ".git" dierctory. We
+  now reject any entries starting with ".git\" on all systems.
+
+- CVE-2019-1387: it is possible to let a submodule's git
+  directory point into a sibling's submodule directory, which may
+  result in overwriting parts of the Git repository and thus lead
+  to arbitrary command execution. As libgit2 doesn't provide any
+  way to do submodule clones natively, it is not susceptible to
+  this vulnerability. Users of libgit2 that have implemented
+  recursive submodule clones manually are encouraged to review
+  their implementation for this vulnerability.
+
 ### Breaking API changes
 
 * The "private" implementation details of the `git_cred` structure have been

src/path.c

@@ -21,7 +21,29 @@
 #include <stdio.h>
 #include <ctype.h>
 
-#define LOOKS_LIKE_DRIVE_PREFIX(S) (git__isalpha((S)[0]) && (S)[1] == ':')
+static int dos_drive_prefix_length(const char *path)
+{
+	int i;
+
+	/*
+	 * Does it start with an ASCII letter (i.e. highest bit not set),
+	 * followed by a colon?
+	 */
+	if (!(0x80 & (unsigned char)*path))
+		return *path && path[1] == ':' ? 2 : 0;
+
+	/*
+	 * While drive letters must be letters of the English alphabet, it is
+	 * possible to assign virtually _any_ Unicode character via `subst` as
+	 * a drive letter to "virtual drives". Even `1`, or `ä`. Or fun stuff
+	 * like this:
+	 *
+	 *	subst ֍: %USERPROFILE%\Desktop
+	 */
+	for (i = 1; i < 4 && (0x80 & (unsigned char)path[i]); i++)
+		; /* skip first UTF-8 character */
+	return path[i] == ':' ? i + 1 : 0;
+}
 
 #ifdef GIT_WIN32
 static bool looks_like_network_computer_name(const char *path, int pos)
@@ -123,11 +145,11 @@ static int win32_prefix_length(const char *path, int len)
 	GIT_UNUSED(len);
 #else
 	/*
-	 * Mimic unix behavior where '/.git' returns '/': 'C:/.git' will return
-	 * 'C:/' here
+	 * Mimic unix behavior where '/.git' returns '/': 'C:/.git'
+	 * will return 'C:/' here
 	 */
-	if (len == 2 && LOOKS_LIKE_DRIVE_PREFIX(path))
-		return 2;
+	if (dos_drive_prefix_length(path) == len)
+		return len;
 
 	/*
 	 * Similarly checks if we're dealing with a network computer name
@@ -272,11 +294,11 @@ const char *git_path_topdir(const char *path)
 
 int git_path_root(const char *path)
 {
-	int offset = 0;
+	int offset = 0, prefix_len;
 
 	/* Does the root of the path look like a windows drive ? */
-	if (LOOKS_LIKE_DRIVE_PREFIX(path))
-		offset += 2;
+	if ((prefix_len = dos_drive_prefix_length(path)))
+		offset += prefix_len;
 
 #ifdef GIT_WIN32
 	/* Are we dealing with a windows network path? */
@@ -1624,8 +1646,12 @@ GIT_INLINE(bool) verify_dotgit_ntfs(git_repository *repo, const char *path, size
 	if (!start)
 		return true;
 
-	/* Reject paths like ".git\" */
-	if (path[start] == '\\')
+	/*
+	 * Reject paths that start with Windows-style directory separators
+	 * (".git\") or NTFS alternate streams (".git:") and could be used
+	 * to write to the ".git" directory on Windows platforms.
+	 */
+	if (path[start] == '\\' || path[start] == ':')
 		return false;
 
 	/* Reject paths like '.git ' or '.git.' */
@@ -1637,12 +1663,21 @@ GIT_INLINE(bool) verify_dotgit_ntfs(git_repository *repo, const char *path, size
 	return false;
 }
 
-GIT_INLINE(bool) only_spaces_and_dots(const char *path)
+/*
+ * Windows paths that end with spaces and/or dots are elided to the
+ * path without them for backward compatibility.  That is to say
+ * that opening file "foo ", "foo." or even "foo . . ." will all
+ * map to a filename of "foo".  This function identifies spaces and
+ * dots at the end of a filename, whether the proper end of the
+ * filename (end of string) or a colon (which would indicate a
+ * Windows alternate data stream.)
+ */
+GIT_INLINE(bool) ntfs_end_of_filename(const char *path)
 {
 	const char *c = path;
 
 	for (;; c++) {
-		if (*c == '\0')
+		if (*c == '\0' || *c == ':')
 			return true;
 		if (*c != ' ' && *c != '.')
 			return false;
@@ -1657,13 +1692,13 @@ GIT_INLINE(bool) verify_dotgit_ntfs_generic(const char *name, size_t len, const
 
 	if (name[0] == '.' && len >= dotgit_len &&
 	    !strncasecmp(name + 1, dotgit_name, dotgit_len)) {
-		return !only_spaces_and_dots(name + dotgit_len + 1);
+		return !ntfs_end_of_filename(name + dotgit_len + 1);
 	}
 
 	/* Detect the basic NTFS shortname with the first six chars */
 	if (!strncasecmp(name, dotgit_name, 6) && name[6] == '~' &&
 	    name[7] >= '1' && name[7] <= '4')
-		return !only_spaces_and_dots(name + 8);
+		return !ntfs_end_of_filename(name + 8);
 
 	/* Catch fallback names */
 	for (i = 0, saw_tilde = 0; i < 8; i++) {
@@ -1685,7 +1720,7 @@ GIT_INLINE(bool) verify_dotgit_ntfs_generic(const char *name, size_t len, const
 		}
 	}
 
-	return !only_spaces_and_dots(name + i);
+	return !ntfs_end_of_filename(name + i);
 }
 
 GIT_INLINE(bool) verify_char(unsigned char c, unsigned int flags)
@@ -1819,7 +1854,7 @@ GIT_INLINE(unsigned int) dotgit_flags(
 	git_repository *repo,
 	unsigned int flags)
 {
-	int protectHFS = 0, protectNTFS = 0;
+	int protectHFS = 0, protectNTFS = 1;
 	int error = 0;
 
 	flags |= GIT_PATH_REJECT_DOT_GIT_LITERAL;
@@ -1828,16 +1863,12 @@ GIT_INLINE(unsigned int) dotgit_flags(
 	protectHFS = 1;
 #endif
 
-#ifdef GIT_WIN32
-	protectNTFS = 1;
-#endif
-
 	if (repo && !protectHFS)
 		error = git_repository__configmap_lookup(&protectHFS, repo, GIT_CONFIGMAP_PROTECTHFS);
 	if (!error && protectHFS)
 		flags |= GIT_PATH_REJECT_DOT_GIT_HFS;
 
-	if (repo && !protectNTFS)
+	if (repo)
 		error = git_repository__configmap_lookup(&protectNTFS, repo, GIT_CONFIGMAP_PROTECTNTFS);
 	if (!error && protectNTFS)
 		flags |= GIT_PATH_REJECT_DOT_GIT_NTFS;

src/repository.h

@@ -113,7 +113,7 @@ typedef enum {
 	/* core.protectHFS */
 	GIT_PROTECTHFS_DEFAULT = GIT_CONFIGMAP_FALSE,
 	/* core.protectNTFS */
-	GIT_PROTECTNTFS_DEFAULT = GIT_CONFIGMAP_FALSE,
+	GIT_PROTECTNTFS_DEFAULT = GIT_CONFIGMAP_TRUE,
 	/* core.fsyncObjectFiles */
 	GIT_FSYNCOBJECTFILES_DEFAULT = GIT_CONFIGMAP_FALSE,
 } git_configmap_value;

tests/checkout/nasty.c

@@ -206,9 +206,8 @@ void test_checkout_nasty__dot_git_dot(void)
  */
 void test_checkout_nasty__git_tilde1(void)
 {
-#ifdef GIT_WIN32
 	test_checkout_fails("refs/heads/git_tilde1", ".git/foobar");
-#endif
+	test_checkout_fails("refs/heads/git_tilde1", "git~1/foobar");
 }
 
 /* A tree that contains an entry "git~2", when we have forced the short
@@ -274,6 +273,16 @@ void test_checkout_nasty__dot_git_colon_stuff(void)
 #endif
 }
 
+/* A tree that contains an entry ".git::$INDEX_ALLOCATION" because NTFS
+ * will interpret that as a synonym to ".git", even when mounted via SMB
+ * on macOS.
+ */
+void test_checkout_nasty__dotgit_alternate_data_stream(void)
+{
+	test_checkout_fails("refs/heads/dotgit_alternate_data_stream", ".git/dummy-file");
+	test_checkout_fails("refs/heads/dotgit_alternate_data_stream", ".git::$INDEX_ALLOCATION/dummy-file");
+}
+
 /* Trees that contains entries with a tree ".git" that contain
  * byte sequences:
  * { 0xe2, 0x80, 0x8c }

tests/clar_libgit2.h

@@ -29,8 +29,8 @@
  * calls that are supposed to fail!
  */
 #define cl_git_fail(expr) do { \
-	git_error_clear(); \
 	if ((expr) == 0) \
+		git_error_clear(), \
 		cl_git_report_failure(0, 0, __FILE__, __LINE__, "Function call succeeded: " #expr); \
 	} while (0)
 

tests/index/tests.c

@@ -541,7 +541,7 @@ 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)
+static void assert_add_bypath_fails(git_repository *repo, const char *fn)
 {
 	git_index *index;
 	git_buf path = GIT_BUF_INIT;
@@ -562,7 +562,7 @@ static void add_invalid_filename(git_repository *repo, const char *fn)
 }
 
 /* Test that writing an invalid filename fails */
-void test_index_tests__add_invalid_filename(void)
+void test_index_tests__cannot_add_invalid_filename(void)
 {
 	git_repository *repo;
 
@@ -577,13 +577,69 @@ void test_index_tests__add_invalid_filename(void)
 	if (!git_path_exists("./invalid/.GiT"))
 		cl_must_pass(p_mkdir("./invalid/.GiT", 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");
+	assert_add_bypath_fails(repo, ".git/hello");
+	assert_add_bypath_fails(repo, ".GIT/hello");
+	assert_add_bypath_fails(repo, ".GiT/hello");
+	assert_add_bypath_fails(repo, "./.git/hello");
+	assert_add_bypath_fails(repo, "./foo");
+	assert_add_bypath_fails(repo, "./bar");
+	assert_add_bypath_fails(repo, "subdir/../bar");
+
+	git_repository_free(repo);
+
+	cl_fixture_cleanup("invalid");
+}
+
+static void assert_add_fails(git_repository *repo, const char *fn)
+{
+	git_index *index;
+	git_buf path = GIT_BUF_INIT;
+	git_index_entry entry = {{0}};
+
+	cl_git_pass(git_repository_index(&index, repo));
+	cl_assert(git_index_entrycount(index) == 0);
+
+	entry.path = fn;
+	entry.mode = GIT_FILEMODE_BLOB;
+	cl_git_pass(git_oid_fromstr(&entry.id, "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"));
+
+	cl_git_fail(git_index_add(index, &entry));
+
+	cl_assert(git_index_entrycount(index) == 0);
+
+	git_buf_dispose(&path);
+	git_index_free(index);
+}
+
+/*
+ * Test that writing an invalid filename fails on filesystem
+ * specific protected names
+ */
+void test_index_tests__cannot_add_protected_invalid_filename(void)
+{
+	git_repository *repo;
+	git_index *index;
+
+	cl_must_pass(p_mkdir("invalid", 0700));
+
+	cl_git_pass(git_repository_init(&repo, "./invalid", 0));
+
+	/* add a file to the repository so we can reference it later */
+	cl_git_pass(git_repository_index(&index, repo));
+	cl_git_mkfile("invalid/dummy.txt", "");
+	cl_git_pass(git_index_add_bypath(index, "dummy.txt"));
+	cl_must_pass(p_unlink("invalid/dummy.txt"));
+	cl_git_pass(git_index_remove_bypath(index, "dummy.txt"));
+	git_index_free(index);
+
+	cl_repo_set_bool(repo, "core.protectHFS", true);
+	cl_repo_set_bool(repo, "core.protectNTFS", true);
+
+	assert_add_fails(repo, ".git./hello");
+	assert_add_fails(repo, ".git\xe2\x80\xad/hello");
+	assert_add_fails(repo, "git~1/hello");
+	assert_add_fails(repo, ".git\xe2\x81\xaf/hello");
+	assert_add_fails(repo, ".git::$INDEX_ALLOCATION/dummy-file");
 
 	git_repository_free(repo);
 
@@ -599,7 +655,7 @@ static void replace_char(char *str, char in, char out)
 			*c = out;
 }
 
-static void write_invalid_filename(git_repository *repo, const char *fn_orig)
+static void assert_write_fails(git_repository *repo, const char *fn_orig)
 {
 	git_index *index;
 	git_oid expected;
@@ -616,6 +672,7 @@ static void write_invalid_filename(git_repository *repo, const char *fn_orig)
 	 */
 	fn = git__strdup(fn_orig);
 	replace_char(fn, '/', '_');
+	replace_char(fn, ':', '!');
 
 	git_buf_joinpath(&path, "./invalid", fn);
 
@@ -627,6 +684,7 @@ static void write_invalid_filename(git_repository *repo, const char *fn_orig)
 
 	/* kids, don't try this at home */
 	replace_char((char *)entry->path, '_', '/');
+	replace_char((char *)entry->path, '!', ':');
 
 	/* write-tree */
 	cl_git_fail(git_index_write_tree(&expected, index));
@@ -672,13 +730,13 @@ void test_index_tests__write_invalid_filename(void)
 
 	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");
+	assert_write_fails(repo, ".git/hello");
+	assert_write_fails(repo, ".GIT/hello");
+	assert_write_fails(repo, ".GiT/hello");
+	assert_write_fails(repo, "./.git/hello");
+	assert_write_fails(repo, "./foo");
+	assert_write_fails(repo, "./bar");
+	assert_write_fails(repo, "foo/../bar");
 
 	git_repository_free(repo);
 
@@ -696,16 +754,52 @@ void test_index_tests__honors_protect_filesystems(void)
 	cl_repo_set_bool(repo, "core.protectHFS", true);
 	cl_repo_set_bool(repo, "core.protectNTFS", true);
 
-	write_invalid_filename(repo, ".git./hello");
-	write_invalid_filename(repo, ".git\xe2\x80\xad/hello");
-	write_invalid_filename(repo, "git~1/hello");
-	write_invalid_filename(repo, ".git\xe2\x81\xaf/hello");
+	assert_write_fails(repo, ".git./hello");
+	assert_write_fails(repo, ".git\xe2\x80\xad/hello");
+	assert_write_fails(repo, "git~1/hello");
+	assert_write_fails(repo, ".git\xe2\x81\xaf/hello");
+	assert_write_fails(repo, ".git::$INDEX_ALLOCATION/dummy-file");
 
 	git_repository_free(repo);
 
 	cl_fixture_cleanup("invalid");
 }
 
+void test_index_tests__protectntfs_on_by_default(void)
+{
+	git_repository *repo;
+
+	p_mkdir("invalid", 0700);
+
+	cl_git_pass(git_repository_init(&repo, "./invalid", 0));
+	assert_write_fails(repo, ".git./hello");
+	assert_write_fails(repo, "git~1/hello");
+
+	git_repository_free(repo);
+
+	cl_fixture_cleanup("invalid");
+}
+
+void test_index_tests__can_disable_protectntfs(void)
+{
+	git_repository *repo;
+	git_index *index;
+
+	cl_must_pass(p_mkdir("valid", 0700));
+	cl_git_rewritefile("valid/git~1", "steal the shortname");
+
+	cl_git_pass(git_repository_init(&repo, "./valid", 0));
+	cl_git_pass(git_repository_index(&index, repo));
+	cl_repo_set_bool(repo, "core.protectNTFS", false);
+
+	cl_git_pass(git_index_add_bypath(index, "git~1"));
+
+	git_index_free(index);
+	git_repository_free(repo);
+
+	cl_fixture_cleanup("valid");
+}
+
 void test_index_tests__remove_entry(void)
 {
 	git_repository *repo;

tests/object/tree/write.c

@@ -418,10 +418,8 @@ void test_object_tree_write__protect_filesystems(void)
 	 */
 	cl_git_pass(git_treebuilder_new(&builder, g_repo, NULL));
 
-#ifndef GIT_WIN32
-	cl_git_pass(git_treebuilder_insert(NULL, builder, ".git.", &bid, GIT_FILEMODE_BLOB));
-	cl_git_pass(git_treebuilder_insert(NULL, builder, "git~1", &bid, GIT_FILEMODE_BLOB));
-#endif
+	cl_git_fail(git_treebuilder_insert(NULL, builder, ".git.", &bid, GIT_FILEMODE_BLOB));
+	cl_git_fail(git_treebuilder_insert(NULL, builder, "git~1", &bid, GIT_FILEMODE_BLOB));
 
 #ifndef __APPLE__
 	cl_git_pass(git_treebuilder_insert(NULL, builder, ".git\xef\xbb\xbf", &bid, GIT_FILEMODE_BLOB));
@@ -444,6 +442,7 @@ void test_object_tree_write__protect_filesystems(void)
 
 	cl_git_fail(git_treebuilder_insert(NULL, builder, ".git\xef\xbb\xbf", &bid, GIT_FILEMODE_BLOB));
 	cl_git_fail(git_treebuilder_insert(NULL, builder, ".git\xe2\x80\xad", &bid, GIT_FILEMODE_BLOB));
+	cl_git_fail(git_treebuilder_insert(NULL, builder, ".git::$INDEX_ALLOCATION/dummy-file", &bid, GIT_FILEMODE_BLOB));
 
 	git_treebuilder_free(builder);
 }

tests/path/core.c

@@ -362,3 +362,14 @@ void test_path_core__join_unrooted(void)
 
 	git_buf_dispose(&out);
 }
+
+void test_path_core__join_unrooted_respects_funny_windows_roots(void)
+{
+	test_join_unrooted("💩:/foo/bar/foobar", 9, "bar/foobar", "💩:/foo");
+	test_join_unrooted("💩:/foo/bar/foobar", 13, "foobar", "💩:/foo/bar");
+	test_join_unrooted("💩:/foo", 5, "💩:/foo", "💩:/asdf");
+	test_join_unrooted("💩:/foo/bar", 5, "💩:/foo/bar", "💩:/asdf");
+	test_join_unrooted("💩:/foo/bar/foobar", 9, "💩:/foo/bar/foobar", "💩:/foo");
+	test_join_unrooted("💩:/foo/bar/foobar", 13, "💩:/foo/bar/foobar", "💩:/foo/bar");
+	test_join_unrooted("💩:/foo/bar/foobar", 9, "💩:/foo/bar/foobar", "💩:/foo/");
+}

tests/path/dotgit.c

@@ -116,4 +116,5 @@ void test_path_dotgit__dotgit_modules_symlink(void)
 	cl_assert_equal_b(true, git_path_isvalid(NULL, ".gitmodules", 0, GIT_PATH_REJECT_DOT_GIT_HFS|GIT_PATH_REJECT_DOT_GIT_NTFS));
 	cl_assert_equal_b(false, git_path_isvalid(NULL, ".gitmodules", S_IFLNK, GIT_PATH_REJECT_DOT_GIT_HFS));
 	cl_assert_equal_b(false, git_path_isvalid(NULL, ".gitmodules", S_IFLNK, GIT_PATH_REJECT_DOT_GIT_NTFS));
+	cl_assert_equal_b(false, git_path_isvalid(NULL, ".gitmodules . .::$DATA", S_IFLNK, GIT_PATH_REJECT_DOT_GIT_NTFS));
 }

tests/resources/nasty/.gitted/refs/heads/dotgit_alternate_data_stream

+b8edf3ad62dbcbc983857a5bfee7b0181ee1a513