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@xxxxxx> > --- > 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 >
