From 97dc50aceb2cb3bce0eeab7c5a20711cb370e7b7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Sep 2019 15:08:56 +0200 Subject: cl_git_fail: do not report bogus error message When we expect a checkout operation to fail, but it succeeds, we actually do not want to see the error messages that were generated in the meantime for errors that were handled gracefully by the code (e.g. when an object could not be found in a pack: in this case, the next backend would have been given a chance to look up the object, and probably would have found it because the checkout succeeded, after all). Which means that in the specific case of `cl_git_fail()`, we actually want to clear the global error state _after_ evaluating the command: we know that any still-available error would be bogus, seeing as the command succeeded (unexpectedly). Signed-off-by: Johannes Schindelin --- tests/clar_libgit2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/clar_libgit2.h b/tests/clar_libgit2.h index c72d37db3..b0a069355 100644 --- a/tests/clar_libgit2.h +++ b/tests/clar_libgit2.h @@ -29,8 +29,8 @@ * calls that are supposed to fail! */ #define cl_git_fail(expr) do { \ - giterr_clear(); \ if ((expr) == 0) \ + giterr_clear(), \ cl_git_report_failure(0, 0, __FILE__, __LINE__, "Function call succeeded: " #expr); \ } while (0) -- cgit v1.2.1 From ca8a4cd363bff7c482bb434d018610ffaeb213bf Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Sep 2019 15:25:02 +0200 Subject: Protect against 8.3 "short name" attacks also on Linux/macOS The Windows Subsystem for Linux (WSL) is getting increasingly popular, in particular because it makes it _so_ easy to run Linux software on Windows' files, via the auto-mounted Windows drives (`C:\` is mapped to `/mnt/c/`, no need to set that up manually). Unfortunately, files/directories on the Windows drives can be accessed via their _short names_, if that feature is enabled (which it is on the `C:` drive by default). Which means that we have to safeguard even our Linux users against the short name attacks. Further, while the default options of CIFS/SMB-mounts seem to disallow accessing files on network shares via their short names on Linux/macOS, it _is_ possible to do so with the right options. So let's just safe-guard against short name attacks _everywhere_. Signed-off-by: Johannes Schindelin --- src/checkout.c | 2 +- tests/checkout/nasty.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/checkout.c b/src/checkout.c index debdbe95b..8567010ad 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -1273,7 +1273,7 @@ static int checkout_verify_paths( int action, git_diff_delta *delta) { - unsigned int flags = GIT_PATH_REJECT_WORKDIR_DEFAULTS; + unsigned int flags = GIT_PATH_REJECT_WORKDIR_DEFAULTS | GIT_PATH_REJECT_DOT_GIT_NTFS; if (action & CHECKOUT_ACTION__REMOVE) { if (!git_path_isvalid(repo, delta->old_file.path, delta->old_file.mode, flags)) { diff --git a/tests/checkout/nasty.c b/tests/checkout/nasty.c index d4d3c8fa4..96f717fc8 100644 --- a/tests/checkout/nasty.c +++ b/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 -- cgit v1.2.1 From 3595e237c98bcae11e655826d12f29361637510d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Sep 2019 14:32:05 +0200 Subject: Disallow NTFS Alternate Data Stream attacks, even on Linux/macOS A little-known feature of NTFS is that it offers to store metadata in so-called "Alternate Data Streams" (inspired by Apple's "resource forks") that are copied together with the file they are associated with. These Alternate Data Streams can be accessed via `::`. Directories, too, have Alternate Data Streams, and they even have a default stream type `$INDEX_ALLOCATION`. Which means that `abc/` and `abc::$INDEX_ALLOCATION/` are actually equivalent. This is of course another attack vector on the Git directory that we definitely want to prevent. On Windows, we already do this incidentally, by disallowing colons in file/directory names. While it looks as if files'/directories' Alternate Data Streams are not accessible in the Windows Subsystem for Linux, and neither via CIFS/SMB-mounted network shares in Linux, it _is_ possible to access them on SMB-mounted network shares on macOS. Therefore, let's go the extra mile and prevent this particular attack _everywhere_. To keep things simple, let's just disallow *any* Alternate Data Stream of `.git`. This is libgit2's variant of CVE-2019-1352. Signed-off-by: Johannes Schindelin --- src/path.c | 8 ++++++-- tests/checkout/nasty.c | 10 ++++++++++ .../objects/33/8190107c7ee7d8f5aa30061fc19b7d5ddcda86 | Bin 0 -> 55 bytes .../objects/97/c14994866466aeb73e769a6f34e07c7f4b53f7 | Bin 0 -> 65 bytes .../objects/b8/edf3ad62dbcbc983857a5bfee7b0181ee1a513 | Bin 0 -> 135 bytes .../nasty/.gitted/refs/heads/dotgit_alternate_data_stream | 1 + 6 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 tests/resources/nasty/.gitted/objects/33/8190107c7ee7d8f5aa30061fc19b7d5ddcda86 create mode 100644 tests/resources/nasty/.gitted/objects/97/c14994866466aeb73e769a6f34e07c7f4b53f7 create mode 100644 tests/resources/nasty/.gitted/objects/b8/edf3ad62dbcbc983857a5bfee7b0181ee1a513 create mode 100644 tests/resources/nasty/.gitted/refs/heads/dotgit_alternate_data_stream diff --git a/src/path.c b/src/path.c index 32cae8e5a..6f22b08e9 100644 --- a/src/path.c +++ b/src/path.c @@ -1609,8 +1609,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.' */ diff --git a/tests/checkout/nasty.c b/tests/checkout/nasty.c index 96f717fc8..2a602951b 100644 --- a/tests/checkout/nasty.c +++ b/tests/checkout/nasty.c @@ -273,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 } diff --git a/tests/resources/nasty/.gitted/objects/33/8190107c7ee7d8f5aa30061fc19b7d5ddcda86 b/tests/resources/nasty/.gitted/objects/33/8190107c7ee7d8f5aa30061fc19b7d5ddcda86 new file mode 100644 index 000000000..e539ccfec Binary files /dev/null and b/tests/resources/nasty/.gitted/objects/33/8190107c7ee7d8f5aa30061fc19b7d5ddcda86 differ diff --git a/tests/resources/nasty/.gitted/objects/97/c14994866466aeb73e769a6f34e07c7f4b53f7 b/tests/resources/nasty/.gitted/objects/97/c14994866466aeb73e769a6f34e07c7f4b53f7 new file mode 100644 index 000000000..9f7679917 Binary files /dev/null and b/tests/resources/nasty/.gitted/objects/97/c14994866466aeb73e769a6f34e07c7f4b53f7 differ diff --git a/tests/resources/nasty/.gitted/objects/b8/edf3ad62dbcbc983857a5bfee7b0181ee1a513 b/tests/resources/nasty/.gitted/objects/b8/edf3ad62dbcbc983857a5bfee7b0181ee1a513 new file mode 100644 index 000000000..bf446263c Binary files /dev/null and b/tests/resources/nasty/.gitted/objects/b8/edf3ad62dbcbc983857a5bfee7b0181ee1a513 differ diff --git a/tests/resources/nasty/.gitted/refs/heads/dotgit_alternate_data_stream b/tests/resources/nasty/.gitted/refs/heads/dotgit_alternate_data_stream new file mode 100644 index 000000000..ecdd340cd --- /dev/null +++ b/tests/resources/nasty/.gitted/refs/heads/dotgit_alternate_data_stream @@ -0,0 +1 @@ +b8edf3ad62dbcbc983857a5bfee7b0181ee1a513 -- cgit v1.2.1 From 4bae85c5b9f2c761827b86d3c82385487d60febe Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Sep 2019 16:33:18 +0200 Subject: path: also guard `.gitmodules` against NTFS Alternate Data Streams We just safe-guarded `.git` against NTFS Alternate Data Stream-related attack vectors, and now it is time to do the same for `.gitmodules`. Note: In the added regression test, we refrain from verifying all kinds of variations between short names and NTFS Alternate Data Streams: as the new code disallows _all_ Alternate Data Streams of `.gitmodules`, it is enough to test one in order to know that all of them are guarded against. Signed-off-by: Johannes Schindelin --- src/path.c | 2 +- tests/path/dotgit.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/path.c b/src/path.c index 6f22b08e9..508736532 100644 --- a/src/path.c +++ b/src/path.c @@ -1631,7 +1631,7 @@ GIT_INLINE(bool) only_spaces_and_dots(const char *path) const char *c = path; for (;; c++) { - if (*c == '\0') + if (*c == '\0' || *c == ':') return true; if (*c != ' ' && *c != '.') return false; diff --git a/tests/path/dotgit.c b/tests/path/dotgit.c index 20e585edb..425392403 100644 --- a/tests/path/dotgit.c +++ b/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)); } -- cgit v1.2.1 From 988757feefc77d7aa19f51a7671eb08759943a8d Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 17:47:31 +1100 Subject: path: rename function that detects end of filename The function `only_spaces_and_dots` used to detect the end of the filename on win32. Now we look at spaces and dots _before_ the end of the string _or_ a `:` character, which would signify a win32 alternate data stream. Thus, rename the function `ntfs_end_of_filename` to indicate that it detects the (virtual) end of a filename, that any further characters would be elided to the given path. --- src/path.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/path.c b/src/path.c index 508736532..2b8465f34 100644 --- a/src/path.c +++ b/src/path.c @@ -1626,7 +1626,16 @@ 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; @@ -1646,13 +1655,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++) { @@ -1674,7 +1683,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) -- cgit v1.2.1 From ba6e2bfba15541dcae55de309696a0b3d4b8cb97 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 18:49:23 +1100 Subject: test: improve badname verification test The name of the `write_invalid_filename` function suggests that we _want_ to write an invalid filename. Rename the function to show that we expect to _fail_ to write the invalid filename. --- tests/index/tests.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/index/tests.c b/tests/index/tests.c index ea8335b48..402a5aa5f 100644 --- a/tests/index/tests.c +++ b/tests/index/tests.c @@ -510,7 +510,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; @@ -559,13 +559,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); @@ -583,10 +583,10 @@ 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"); git_repository_free(repo); -- cgit v1.2.1 From ee3dbe1254f9537de4cc004421bd3d82aa0427cd Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 18:56:31 +1100 Subject: test: ensure index adds validate new protection rules Ensure that the new protection around .git::$INDEX_ALLOCATION rules are enabled for adding to the index when core.protectNTFS is set. --- tests/index/tests.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/index/tests.c b/tests/index/tests.c index 402a5aa5f..f1eff3cf5 100644 --- a/tests/index/tests.c +++ b/tests/index/tests.c @@ -527,6 +527,7 @@ static void assert_write_fails(git_repository *repo, const char *fn_orig) */ fn = git__strdup(fn_orig); replace_char(fn, '/', '_'); + replace_char(fn, ':', '!'); git_buf_joinpath(&path, "./invalid", fn); @@ -538,6 +539,7 @@ static void assert_write_fails(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)); @@ -587,6 +589,7 @@ void test_index_tests__honors_protect_filesystems(void) 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); -- cgit v1.2.1 From b437de92550367a926d4d0805db35e35b2762556 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 18:57:16 +1100 Subject: test: ensure treebuilder validate new protection rules Ensure that the new protection around .git::$INDEX_ALLOCATION rules are enabled for using the treebuilder when core.protectNTFS is set. --- tests/object/tree/write.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/object/tree/write.c b/tests/object/tree/write.c index a1ee03d6d..65b24752e 100644 --- a/tests/object/tree/write.c +++ b/tests/object/tree/write.c @@ -444,6 +444,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); } -- cgit v1.2.1 From 70edf887ad7d03713f0d88d23be58ee13f0c46fd Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 19:01:00 +1100 Subject: test: improve badname verification test The name of the `add_invalid_filename` function suggests that we _want_ to add an invalid filename. Rename the function to show that we expect to _fail_ to add the invalid filename. --- tests/index/tests.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/index/tests.c b/tests/index/tests.c index f1eff3cf5..51cac4c4e 100644 --- a/tests/index/tests.c +++ b/tests/index/tests.c @@ -452,7 +452,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; @@ -473,7 +473,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; @@ -488,13 +488,13 @@ 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); -- cgit v1.2.1 From dbee08cf0e1893a9095cdc7c0653f9bd0c9a7345 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 19:17:41 +1100 Subject: test: ensure we can't add a protected path Test that when we enable core.protectNTFS that we cannot add platform-specific invalid paths to the index. --- tests/index/tests.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/index/tests.c b/tests/index/tests.c index 51cac4c4e..a5bcacc36 100644 --- a/tests/index/tests.c +++ b/tests/index/tests.c @@ -501,6 +501,62 @@ void test_index_tests__cannot_add_invalid_filename(void) 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_free(&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); + + cl_fixture_cleanup("invalid"); +} + static void replace_char(char *str, char in, char out) { char *c = str; -- cgit v1.2.1 From 31541e5fd3800b82007f70ccd32d1fa078682a2c Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 19:24:59 +1100 Subject: path: protect NTFS everywhere Enable core.protectNTFS by default everywhere and in every codepath, not just on checkout. --- src/checkout.c | 2 +- src/path.c | 8 ++------ src/repository.h | 2 +- tests/index/tests.c | 15 +++++++++++++++ 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/checkout.c b/src/checkout.c index 8567010ad..debdbe95b 100644 --- a/src/checkout.c +++ b/src/checkout.c @@ -1273,7 +1273,7 @@ static int checkout_verify_paths( int action, git_diff_delta *delta) { - unsigned int flags = GIT_PATH_REJECT_WORKDIR_DEFAULTS | GIT_PATH_REJECT_DOT_GIT_NTFS; + unsigned int flags = GIT_PATH_REJECT_WORKDIR_DEFAULTS; if (action & CHECKOUT_ACTION__REMOVE) { if (!git_path_isvalid(repo, delta->old_file.path, delta->old_file.mode, flags)) { diff --git a/src/path.c b/src/path.c index 2b8465f34..3217fd7ce 100644 --- a/src/path.c +++ b/src/path.c @@ -1817,7 +1817,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; @@ -1826,16 +1826,12 @@ GIT_INLINE(unsigned int) dotgit_flags( protectHFS = 1; #endif -#ifdef GIT_WIN32 - protectNTFS = 1; -#endif - if (repo && !protectHFS) error = git_repository__cvar(&protectHFS, repo, GIT_CVAR_PROTECTHFS); if (!error && protectHFS) flags |= GIT_PATH_REJECT_DOT_GIT_HFS; - if (repo && !protectNTFS) + if (repo) error = git_repository__cvar(&protectNTFS, repo, GIT_CVAR_PROTECTNTFS); if (!error && protectNTFS) flags |= GIT_PATH_REJECT_DOT_GIT_NTFS; diff --git a/src/repository.h b/src/repository.h index fd6400cc1..65b38b5d6 100644 --- a/src/repository.h +++ b/src/repository.h @@ -110,7 +110,7 @@ typedef enum { /* core.protectHFS */ GIT_PROTECTHFS_DEFAULT = GIT_CVAR_FALSE, /* core.protectNTFS */ - GIT_PROTECTNTFS_DEFAULT = GIT_CVAR_FALSE, + GIT_PROTECTNTFS_DEFAULT = GIT_CVAR_TRUE, /* core.fsyncObjectFiles */ GIT_FSYNCOBJECTFILES_DEFAULT = GIT_CVAR_FALSE, } git_cvar_value; diff --git a/tests/index/tests.c b/tests/index/tests.c index a5bcacc36..e1e194e12 100644 --- a/tests/index/tests.c +++ b/tests/index/tests.c @@ -652,6 +652,21 @@ void test_index_tests__honors_protect_filesystems(void) 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__remove_entry(void) { git_repository *repo; -- cgit v1.2.1 From e4f1ff69680c1fb83418f14d736c35eb9d7f2121 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 23:23:02 +1100 Subject: tree: ensure we protect NTFS paths everywhere --- tests/object/tree/write.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/object/tree/write.c b/tests/object/tree/write.c index 65b24752e..bef5ff3f6 100644 --- a/tests/object/tree/write.c +++ b/tests/object/tree/write.c @@ -141,7 +141,7 @@ void test_object_tree_write__sorted_subtrees(void) cl_git_pass(git_treebuilder_new(&builder, g_repo, NULL)); for (i = 0; i < ARRAY_SIZE(entries); ++i) { - git_oid *id = entries[i].attr == GIT_FILEMODE_TREE ? &tid : &bid; + git_oid *id = entries[i].attr == GIT_FILEMODE_TREE ? &tid : &bid; cl_git_pass(git_treebuilder_insert(NULL, builder, entries[i].filename, id, entries[i].attr)); @@ -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)); -- cgit v1.2.1 From cf130e6de98e23f95ab32db91b1778deda0fe895 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 19:50:18 +1100 Subject: index: ensure that we respect core.protectNTFS=false Users may want to turn off core.protectNTFS, perhaps to import (and then repair) a broken tree. Ensure that core.protectNTFS=false is honored. --- tests/index/tests.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/index/tests.c b/tests/index/tests.c index e1e194e12..8ca4939fe 100644 --- a/tests/index/tests.c +++ b/tests/index/tests.c @@ -667,6 +667,26 @@ void test_index_tests__protectntfs_on_by_default(void) 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; -- cgit v1.2.1 From 6117dcd34a250a6a69f18cf07c8a1c62459ed694 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Tue, 3 Dec 2019 23:15:47 +1100 Subject: path: support non-ascii drive letters on dos Windows/DOS only supports drive letters that are alpha characters A-Z. However, you can `subst` any one-character as a drive letter, including numbers or even emoji. Test that we can identify emoji as drive letters. --- src/path.c | 38 ++++++++++++++++++++++++++++++-------- tests/path/core.c | 11 +++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/src/path.c b/src/path.c index 3217fd7ce..6d37abda1 100644 --- a/src/path.c +++ b/src/path.c @@ -21,7 +21,29 @@ #include #include -#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 @@ -260,11 +282,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? */ diff --git a/tests/path/core.c b/tests/path/core.c index 0ab41ea50..dcc85fb78 100644 --- a/tests/path/core.c +++ b/tests/path/core.c @@ -352,3 +352,14 @@ void test_path_core__join_unrooted(void) git_buf_free(&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/"); +} -- cgit v1.2.1 From b9c613387e61b74b4d59e07c79ad893f678916d0 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 10 Dec 2019 13:44:27 +0100 Subject: changelog: update for security release v0.27.10 --- CHANGELOG.md | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8398e5ab4..ddcbd8ed8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,63 @@ +v0.27.10 +-------- + +This is a security release fixing the following issues: + +- 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. + v0.27.9 ------- -- cgit v1.2.1 From 34c929572a635a6599dc8ff06a87c7c0a324caff Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 10 Dec 2019 13:45:29 +0100 Subject: version: bump version number to v0.27.10 --- include/git2/version.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/git2/version.h b/include/git2/version.h index 23d04173d..482487dba 100644 --- a/include/git2/version.h +++ b/include/git2/version.h @@ -7,10 +7,10 @@ #ifndef INCLUDE_git_version_h__ #define INCLUDE_git_version_h__ -#define LIBGIT2_VERSION "0.27.9" +#define LIBGIT2_VERSION "0.27.10" #define LIBGIT2_VER_MAJOR 0 #define LIBGIT2_VER_MINOR 27 -#define LIBGIT2_VER_REVISION 9 +#define LIBGIT2_VER_REVISION 10 #define LIBGIT2_VER_PATCH 0 #define LIBGIT2_SOVERSION 27 -- cgit v1.2.1