mingw: safeguard better against backslashes in file names #690

dscho · 2020-01-09T11:58:08Z

I investigated again, and I think that there are code paths involving make_transient_cache_entry() that might be vulnerable again after my recent change in 224c7d7 (mingw: only test index entries for backslashes, not tree entries, 2019-12-31).

This version should help with keeping Git for Windows' users safe.

In 224c7d7 (mingw: only test index entries for backslashes, not tree entries, 2019-12-31), we relaxed the check for backslashes in tree entries to check only index entries. However, the code change was incorrect: it was added to `add_index_entry_with_check()`, not to `add_index_entry()`, so under certain circumstances it was possible to side-step the protection. Besides, the description of that commit purported that all index entries would be checked when in fact they were only checked when being added to the index (there are code paths that do not do that, constructing "transient" index entries). In any case, it was pointed out in one insightful review at git-for-windows#2437 (comment) that it would be a much better idea to teach `verify_path()` to perform the check for a backslash. This is safer, even if it comes with two notable drawbacks: - `verify_path()` cannot say _what_ is wrong with the path, therefore the user will no longer be told that there was a backslash in the path, only that the path was invalid. - The `git apply` command also calls the `verify_path()` function, and might have been able to handle Windows-style paths (i.e. with backslashes instead of forward slashes). This will no longer be possible unless the user (temporarily) sets `core.protectNTFS=false`. Note that `git add <windows-path>` will _still_ work because `normalize_path_copy_len()` will convert the backslashes to forward slashes before hitting the code path that creates an index entry. The clear advantage is that `verify_path()`'s purpose is to check the validity of the file name, therefore we naturally tap into all the code paths that need safeguarding, also implicitly into future code paths. The benefits of that approach outweigh the downsides, so let's move the check from `add_index_entry_with_check()` to `verify_path()`. Signed-off-by: Johannes Schindelin <[email protected]>

dscho · 2020-01-09T13:29:28Z

/submit

gitgitgadget-git · 2020-01-09T13:30:51Z

Submitted as [email protected]

gitgitgadget-git · 2020-01-09T22:04:57Z

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi,

I forgot to say that I already included this patch into Git for Windows
v2.25.0-rc2.

Thanks,
Johannes

On Thu, 9 Jan 2020, Johannes Schindelin via GitGitGadget wrote:

> In 224c7d70fa1 (mingw: only test index entries for backslashes, not tree
> entries, 2019-12-31), we relaxed the check for backslashes in tree
> entries to check only index entries.
>
> However, the code change was incorrect: it was added to
> `add_index_entry_with_check()`, not to `add_index_entry()`, so under
> certain circumstances it was possible to side-step the protection.
>
> Besides, the description of that commit purported that all index entries
> would be checked when in fact they were only checked when being added to
> the index (there are code paths that do not do that, constructing
> "transient" index entries).
>
> In any case, it was pointed out in one insightful review at
> https://github.com/git-for-windows/git/pull/2437#issuecomment-566771835
> that it would be a much better idea to teach `verify_path()` to perform
> the check for a backslash. This is safer, even if it comes with two
> notable drawbacks:
>
> - `verify_path()` cannot say _what_ is wrong with the path, therefore
>   the user will no longer be told that there was a backslash in the
>   path, only that the path was invalid.
>
> - The `git apply` command also calls the `verify_path()` function, and
>   might have been able to handle Windows-style paths (i.e. with
>   backslashes instead of forward slashes). This will no longer be
>   possible unless the user (temporarily) sets `core.protectNTFS=false`.
>
> Note that `git add <windows-path>` will _still_ work because
> `normalize_path_copy_len()` will convert the backslashes to forward
> slashes before hitting the code path that creates an index entry.
>
> The clear advantage is that `verify_path()`'s purpose is to check the
> validity of the file name, therefore we naturally tap into all the code
> paths that need safeguarding, also implicitly into future code paths.
>
> The benefits of that approach outweigh the downsides, so let's move the
> check from `add_index_entry_with_check()` to `verify_path()`.
>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>     mingw: safeguard better against backslashes in file names
>
>     I investigated again, and I think that there are code paths involving
>     make_transient_cache_entry() that might be vulnerable again after my
>     recent change in 224c7d70fa1 (mingw: only test index entries for
>     backslashes, not tree entries, 2019-12-31).
>
>     This version should help with keeping Git for Windows' users safe.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-690%2Fdscho%2Fonly-error-on-backslash-in-index-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-690/dscho/only-error-on-backslash-in-index-v1
> Pull-Request: https://github.com/git/git/pull/690
>
>  read-cache.c               | 12 ++++++------
>  t/t7415-submodule-names.sh |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 737916ebd9..aa427c5c17 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -959,7 +959,7 @@ static int verify_dotfile(const char *rest, unsigned mode)
>
>  int verify_path(const char *path, unsigned mode)
>  {
> -	char c;
> +	char c = 0;
>
>  	if (has_dos_drive_prefix(path))
>  		return 0;
> @@ -974,6 +974,7 @@ int verify_path(const char *path, unsigned mode)
>  		if (is_dir_sep(c)) {
>  inside:
>  			if (protect_hfs) {
> +
>  				if (is_hfs_dotgit(path))
>  					return 0;
>  				if (S_ISLNK(mode)) {
> @@ -982,6 +983,10 @@ int verify_path(const char *path, unsigned mode)
>  				}
>  			}
>  			if (protect_ntfs) {
> +#ifdef GIT_WINDOWS_NATIVE
> +				if (c == '\\')
> +					return 0;
> +#endif
>  				if (is_ntfs_dotgit(path))
>  					return 0;
>  				if (S_ISLNK(mode)) {
> @@ -1278,11 +1283,6 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
>  	int skip_df_check = option & ADD_CACHE_SKIP_DFCHECK;
>  	int new_only = option & ADD_CACHE_NEW_ONLY;
>
> -#ifdef GIT_WINDOWS_NATIVE
> -	if (protect_ntfs && strchr(ce->name, '\\'))
> -		return error(_("filename in tree entry contains backslash: '%s'"), ce->name);
> -#endif
> -
>  	if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
>  		cache_tree_invalidate_path(istate, ce->name);
>
> diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
> index 7ae0dc8ff4..f70368bc2e 100755
> --- a/t/t7415-submodule-names.sh
> +++ b/t/t7415-submodule-names.sh
> @@ -209,7 +209,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
>  		hash="$(echo x | git hash-object -w --stdin)" &&
>  		test_must_fail git update-index --add \
>  			--cacheinfo 160000,$rev,d\\a 2>err &&
> -		test_i18ngrep backslash err &&
> +		test_i18ngrep "Invalid path" err &&
>  		git -c core.protectNTFS=false update-index --add \
>  			--cacheinfo 100644,$modules,.gitmodules \
>  			--cacheinfo 160000,$rev,c \
>
> base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0
> --
> gitgitgadget
>

gitgitgadget-git · 2020-01-10T20:52:42Z

This branch is now known as js/mingw-loosen-overstrict-tree-entry-checks.

gitgitgadget-git · 2020-01-10T20:52:43Z

This patch series was integrated into pu via 132d982.

gitgitgadget-git · 2020-01-10T22:21:13Z

This patch series was integrated into pu via f56e7dc.

gitgitgadget-git · 2020-01-10T22:21:14Z

This patch series was integrated into next via f43f0fe.

gitgitgadget-git · 2020-01-13T00:51:45Z

This patch series was integrated into pu via 1cf4836.

gitgitgadget-git · 2020-01-13T00:51:47Z

This patch series was integrated into next via f6adca8.

gitgitgadget-git · 2020-01-13T00:51:48Z

This patch series was integrated into master via 1cf4836.

gitgitgadget-git · 2020-01-13T00:51:50Z

Closed via 1cf4836.

An internal customer reported a segfault when running `git sparse-checkout set` with the `index.sparse` config enabled. I was unable to reproduce it locally, but with their help we debugged into the failing process and discovered the following stacktrace: ``` #0 0x00007ff6318fb7b0 in rehash (map=0x3dfb00d0440, newsize=1048576) at hashmap.c:125 #1 0x00007ff6318fbc66 in hashmap_add (map=0x3dfb00d0440, entry=0x3dfb5c58bc8) at hashmap.c:247 #2 0x00007ff631937a70 in hash_index_entry (istate=0x3dfb00d0400, ce=0x3dfb5c58bc8) at name-hash.c:122 #3 0x00007ff631938a2f in add_name_hash (istate=0x3dfb00d0400, ce=0x3dfb5c58bc8) at name-hash.c:638 #4 0x00007ff631a064de in set_index_entry (istate=0x3dfb00d0400, nr=8291, ce=0x3dfb5c58bc8) at sparse-index.c:255 #5 0x00007ff631a06692 in add_path_to_index (oid=0x5ff130, base=0x5ff580, path=0x3dfb4b725da "<redacted>", mode=33188, context=0x5ff570) at sparse-index.c:307 #6 0x00007ff631a3b48c in read_tree_at (r=0x7ff631c026a0 <the_repo>, tree=0x3dfb5b41f60, base=0x5ff580, depth=2, pathspec=0x5ff5a0, fn=0x7ff631a064e5 <add_path_to_index>, context=0x5ff570) at tree.c:46 #7 0x00007ff631a3b60b in read_tree_at (r=0x7ff631c026a0 <the_repo>, tree=0x3dfb5b41e80, base=0x5ff580, depth=1, pathspec=0x5ff5a0, fn=0x7ff631a064e5 <add_path_to_index>, context=0x5ff570) at tree.c:80 #8 0x00007ff631a3b60b in read_tree_at (r=0x7ff631c026a0 <the_repo>, tree=0x3dfb5b41ac8, base=0x5ff580, depth=0, pathspec=0x5ff5a0, fn=0x7ff631a064e5 <add_path_to_index>, context=0x5ff570) at tree.c:80 #9 0x00007ff631a06a95 in expand_index (istate=0x3dfb00d0100, pl=0x0) at sparse-index.c:422 #10 0x00007ff631a06cbd in ensure_full_index (istate=0x3dfb00d0100) at sparse-index.c:456 #11 0x00007ff631990d08 in index_name_stage_pos (istate=0x3dfb00d0100, name=0x3dfb0020080 "algorithm/levenshtein", namelen=21, stage=0, search_mode=EXPAND_SPARSE) at read-cache.c:556 #12 0x00007ff631990d6c in index_name_pos (istate=0x3dfb00d0100, name=0x3dfb0020080 "algorithm/levenshtein", namelen=21) at read-cache.c:566 #13 0x00007ff63180dbb5 in sanitize_paths (argc=185, argv=0x3dfb0030018, prefix=0x0, skip_checks=0) at builtin/sparse-checkout.c:756 #14 0x00007ff63180de50 in sparse_checkout_set (argc=185, argv=0x3dfb0030018, prefix=0x0) at builtin/sparse-checkout.c:860 #15 0x00007ff63180e6c5 in cmd_sparse_checkout (argc=186, argv=0x3dfb0030018, prefix=0x0) at builtin/sparse-checkout.c:1063 #16 0x00007ff6317234cb in run_builtin (p=0x7ff631ad9b38 <commands+2808>, argc=187, argv=0x3dfb0030018) at git.c:548 #17 0x00007ff6317239c0 in handle_builtin (argc=187, argv=0x3dfb0030018) at git.c:808 #18 0x00007ff631723c7d in run_argv (argcp=0x5ffdd0, argv=0x5ffd78) at git.c:877 #19 0x00007ff6317241d1 in cmd_main (argc=187, argv=0x3dfb0030018) at git.c:1017 #20 0x00007ff631838b60 in main (argc=190, argv=0x3dfb0030000) at common-main.c:64 ``` The very bottom of the stack being the `rehash()` method from `hashmap.c` as called within the `name-hash` API made me look at where these hashmaps were being used in the sparse index logic. These were being copied across indexes, which seems dangerous. Indeed, clearing these hashmaps and setting them as not initialized fixes the segfault. The second commit is a response to a test failure that happens in `t1092-sparse-checkout-compatibility.sh` where `git stash pop` starts to fail because the underlying `git checkout-index` process fails due to colliding files. Passing the `-f` flag appears to work, but it's unclear why this name-hash change causes that change in behavior.

gitgitgadget-git bot added the pu label Jan 10, 2020

gitgitgadget-git bot added the next label Jan 10, 2020

gitgitgadget-git bot added the master label Jan 13, 2020

gitgitgadget-git bot closed this Jan 13, 2020

dscho deleted the only-error-on-backslash-in-index branch January 13, 2020 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

mingw: safeguard better against backslashes in file names #690

mingw: safeguard better against backslashes in file names #690

Uh oh!

dscho commented Jan 9, 2020

Uh oh!

dscho commented Jan 9, 2020

Uh oh!

gitgitgadget-git bot commented Jan 9, 2020

Uh oh!

gitgitgadget-git bot commented Jan 9, 2020

Uh oh!

gitgitgadget-git bot commented Jan 10, 2020

Uh oh!

gitgitgadget-git bot commented Jan 10, 2020

Uh oh!

gitgitgadget-git bot commented Jan 10, 2020

Uh oh!

gitgitgadget-git bot commented Jan 10, 2020

Uh oh!

gitgitgadget-git bot commented Jan 13, 2020

Uh oh!

gitgitgadget-git bot commented Jan 13, 2020

Uh oh!

gitgitgadget-git bot commented Jan 13, 2020

Uh oh!

gitgitgadget-git bot commented Jan 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mingw: safeguard better against backslashes in file names #690

mingw: safeguard better against backslashes in file names #690

Uh oh!

Conversation

dscho commented Jan 9, 2020

Uh oh!

dscho commented Jan 9, 2020

Uh oh!

gitgitgadget-git bot commented Jan 9, 2020

Uh oh!

gitgitgadget-git bot commented Jan 9, 2020

Uh oh!

gitgitgadget-git bot commented Jan 10, 2020

Uh oh!

gitgitgadget-git bot commented Jan 10, 2020

Uh oh!

gitgitgadget-git bot commented Jan 10, 2020

Uh oh!

gitgitgadget-git bot commented Jan 10, 2020

Uh oh!

gitgitgadget-git bot commented Jan 13, 2020

Uh oh!

gitgitgadget-git bot commented Jan 13, 2020

Uh oh!

gitgitgadget-git bot commented Jan 13, 2020

Uh oh!

gitgitgadget-git bot commented Jan 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant