mingw: safeguard better against backslashes in file names

[dscho]

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.

[dscho]

/submit

[gitgitgadget-git]

Submitted as pull.690.git.git.1578576634678.gitgitgadget@gmail.com

[gitgitgadget-git]

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 <johannes.schindelin@gmx.de>
> ---
>     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]

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

[gitgitgadget-git]

This patch series was integrated into pu via 132d982.

[gitgitgadget-git]

This patch series was integrated into pu via f56e7dc.

[gitgitgadget-git]

This patch series was integrated into next via f43f0fe.

[gitgitgadget-git]

This patch series was integrated into pu via 1cf4836.

[gitgitgadget-git]

This patch series was integrated into next via f6adca8.

[gitgitgadget-git]

This patch series was integrated into master via 1cf4836.

[gitgitgadget-git]

Closed via 1cf4836.