Disallow writing, but not fetching commits with file names containing backslashes

As of Git for Windows, v2.24.1(2). cloning a repository that contained a file with a backslash in its name some time in the past, the command will succeed but at the same time print errors like this:

error: filename in tree entry contains backslash: '\'

A corresponding git fetch will also show those errors, but fail.

The reason is that v2.24.1 is much more strict about backslashes in tree entries than earlier versions. The intention was actually to prevent checking out such files, though: if there was a mistake in a repository long ago that has been fixed long since, there is actually no reason why we should require the history to be rewritten.

This fixes https://github.com/git-for-windows/git/issues/2435.

The idea of this patch is to stop complaining about tree entries, and focus instead on the index: whenever a file is added to the index, we do not want any backslashes in the file name on Windows.

As before, this check is only performed on Windows, and only under core.protectNTFS. On other platforms, even if core.protectNTFS is turned on, the backslash is not a directory separator, therefore the Operating System's syscalls will (should?) refuse to create files on NTFS with backslashes in their file name.

I would appreciate reviews with a particular eye on keeping users safe: I am not 100% certain that all relevant file writes go through the index (I think that they all go through the index, but I might have well missed a corner case).

Johannes Schindelin (1):
mingw: only test index entries for backslashes, not tree entries

 read-cache.c               | 5 +++++
 t/t7415-submodule-names.sh | 7 ++++---
 tree-walk.c                | 6 ------
 3 files changed, 9 insertions(+), 9 deletions(-)

base-commit: 12029dc57db23baef008e77db1909367599210ee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-682%2Fdscho%2Fmingw-only-error-on-backslash-in-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-682/dscho/mingw-only-error-on-backslash-in-index-v1
Pull-Request: https://github.com/git/git/pull/682

gitgitgadget

From: Johannes Schindelin <[email protected]>

During a clone of a repository that contained a file with a backslash in its name in the past, as of v2.24.1(2), Git for Windows prints errors like this:

error: filename in tree entry contains backslash: '\'

While the clone still succeeds, a similar error prevents the equivalent git fetch operation, which is inconsistent.

Arguably, this is the wrong layer for that error, anyway: As long as the user never checks out the files whose names contain backslashes, there should not be any problem in the first place.

So let's instead prevent such files to be added to the index.

This addresses https://github.com/git-for-windows/git/issues/2435

---
 read-cache.c               | 5 +++++
 t/t7415-submodule-names.sh | 7 ++++---
 tree-walk.c                | 6 ------
 3 files changed, 9 insertions(+), 9 deletions(-)

index ad0b48c84d..737916ebd9 100644
@@ -1278,6 +1278,11 @@ 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);
 

index 905a557585..7ae0dc8ff4 100755
@@ -207,6 +207,9 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
 			git hash-object -w --stdin)" &&
 		rev="$(git rev-parse --verify HEAD)" &&
 		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 &&
 		git -c core.protectNTFS=false update-index --add \
 			--cacheinfo 100644,$modules,.gitmodules \
 			--cacheinfo 160000,$rev,c \

@@ -214,9 +217,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
 			--cacheinfo 100644,$hash,d./a/x \
 			--cacheinfo 100644,$hash,d./a/..git &&
 		test_tick &&
-		git -c core.protectNTFS=false commit -m "module" &&
-		test_must_fail git show HEAD: 2>err &&
-		test_i18ngrep backslash err
+		git -c core.protectNTFS=false commit -m "module"
 	) &&
 	test_must_fail git -c core.protectNTFS=false \
 		clone --recurse-submodules squatting squatting-clone 2>err &&

index b3d162051f..d5a8e096a6 100644
@@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
 		strbuf_addstr(err, _("empty filename in tree entry"));
 		return -1;
 	}
-#ifdef GIT_WINDOWS_NATIVE
-	if (protect_ntfs && strchr(path, '\\')) {
-		strbuf_addf(err, _("filename in tree entry contains backslash: '%s'"), path);
-		return -1;
-	}
-#endif
 	len = strlen(path) + 1;
 
 	/* Initialize the descriptor entry */

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Yes, inconsistent is bad and it is puzzling. I would have expected, if this gate on the transport layer is desirable, such a check would be implemented as a part of transfer.fsckObjects and covered equally by fetch and clone codepaths. What went wrong to allow "clone" to go through while stopping "fetch"? Can you describe the root cause of the difference in the log message?

I do agree that rejecting these tree objects that has a slash in its path component is probably wrong. A GIT_WINDOWS_NATIVE box should be able to host a bare repository on it, and users on machines that are OK with paths that Windows may not like should be able to interact with it, by pushing to it, fetching from it, and updating the repository on that Windows box by going there and fetching from elsewhere. Rejecting these names at the object validity level means Git on Windows would be incompatible with Git elsewhere.

And It hink the same logic apply to those names like prn, con, nul, etc. How are the users protected from them? We should prevent these names from entering the index the same way, shouldn't we?

... and loosen the check that (incorrectly) gets triggered from what codepaths in "git fetch" (but not from "git clone")?

	if (protect_ntfs && invalid_name_on_windows(ce->name))

"Johannes Schindelin via GitGitGadget" <[email protected]>

There are peripheral commands that do not use the index at all, such as "archive"; piping "git archive" output to unarchiver that writes into the filesystem would be a way. But I do not think that qualifies as an attack vector you are looking for.

Hi,

Hm. The choice of right layer depends on what repositories in the wild contain. If there are none containing filenames with '\', then fsck et al would be an appropriate layer for this. With hindsight, it was not a good idea to support this kind of filename.

However, between the lines of this commit messages I sense that there *are* repositories in the wild using these kinds of filenames.

Can you say more about that? What repositories are affected? Do they contain such filenames at HEAD or only in their history? If someone wants to check out a revision with such filenames, what should happen?

Hi Junio,

My bad, I should have root-caused this better.

Turns out that this inconsistency is only in Git for Windows v2.24.1(2) but not in current master of Git, so I simply struck that part from the commit message.

I rephrased it to:

So let's loosen the requirements: we now leave tree entries with backslashes in their file names alone, but we do require any entries that are added to the Git index to contain no backslashes on Windows.

I decided to perform those checks at yet another layer: when trying to create new files. My idea was that I would want to catch even things like git config -f LPT1 ... (LPT1 is a reserved name on Windows, you cannot create a file with that name).

Obviously, I cannot handle the backslash in the same code path, as e.g. git config -f C:\Users\me\.gitconfig ... is totally valid.

Ciao,
Dscho

Hi Junio,

Yes, I thought about git archive, too. The thing is, I could imagine legitimate use cases where a user wants to generate e.g. a .zip on Windows, with the intention to unpack it on Linux. In such a case, we would not want to prevent said .zip from being generated.

I also briefly considered the scripts that write some trees into a temporary location, but those scripts typically use git read-tree -u -m with a temporary index, i.e. they _do_ go through the index, still.

Thank you for your thoughts about this issue,
Dscho

Hi Jonathan,

Yes: https://github.com/zephyrproject-rtos/civetweb contains history where at some stage, by mistake, a directory was called \. It has been fixed a long time ago, but users obviously still want to be able to clone it ;-)

I added this paragraph to the commit message:

Note: just as before, the check is guarded by core.protectNTFS (to allow overriding the check by toggling that config setting), and it is _only_ performed on Windows, as the backslash is not a directory separator elsewhere, even when writing to NTFS-formatted volumes.

Does that clarify the issue enough?
Dscho

Hi,

[...]

Thanks.

What should and does happen if someone tries to check out an offending revision?

[...]

It helps: that tells me why the check is only performed on Windows.

Since this only affects Windows, please only take this review as data about how someone read the patch. The patch doesn't make non Windows specific code any *less* maintainable, and as a general presumption I assume that the Git for Windows maintainer knows best about what is needed for maintainability of Windows-specific code.

But the commit message and docs still don't describe what the desired behavior is. For example, should I be able to check out a revision with a backslash in it (e.g. via Git skipping the offending entry), or is the intended behavior for it to error out and prevent checking out such versions of code?

Is there anything we can or should do to prevent people checking in new examples of paths with backslash in them (on all platforms)?

Thanks,
Jonathan

I obviously won't dictate what should happen on Windows, but I think the overall principle for paths recorded in a tree object that can be problematic on some of the platforms ought to be:

* fsck and transfer.fsckobjects should be taught to notice offending characteristics (e.g. has a backslash in it, is one of the "reserved names" on some platform like LPT1).

* if paths with the offending characteristics are *so* obviously useless in real life and are possible only in a crafted path that is only useful to attack users, the check in fsck should default to "reject" to help the disease spread via hosting sites.

* otherwise, the check should be to "warn" but not "reject", so that projects can keep using paths that may problematic on platforms that do not matter to them.

I think LPT1 and friends fall into the "warning is fine" category, and a path component that contains a backslash would fall into the "this is an attack, just reject" category.

I guess I should have stepped back a bit.

In the message I am responding to, I focused solely on how tree objects that originate elsewhere should be treated, but there are two more data paths we need to worry about:

* A new path gets introduced to the system, via "update-index", "add", etc., to the index and then "write-tree" to form tree objects in the local repository.

* A path in the index, either created locally via "update-index", "add", etc., or read from a tree object, gets written to the local filesystem, resulting in an attempt to create a path that the local filesystem cannot represent (or worse---behaves badly, like "sending random garbage to the printer").

I think we should apply the same principle as the one I outlined for the tree objects. The fsckobjects mechanism may not be reusable to catch violations in add_index_entry_with_check() as-is, but we need to aim for as much reuse of logic and code as possible so that our checks at various layers all behave consistently.

Thanks.

We are in -rc so there is no real rush, but I take these to mean that I should just leave this loose end hanging untied, and wait for an updated version to replace it sometime early next year.

Thanks and happy new year.

Hi Junio & Jonathan,

Agree. This is on my radar, but so far not too-high priority, as the fsck checks are not (yet?) standard practice in PR builds (and warnings on the server side are prone to be ignored).

I don't think that reserved names such as aux, nor names containing backslashes should be rejected _always_. While I cannot think of _any_ instance where I would want to have a backslash in a file name, I am sure that just like aux.c, there _must_ be somebody out there who thought of a file name that contains a backslash and makes at least some sort of sense.

Yes, it should be "warn".

Right, that's what I had in mind when I wrote this patch. The path gets added to the index, we detect a backslash, and on Windows (under core.protectNTFS) fail with an error.

Happily, my patch seems to catch this code path, too: when reading from a tree object into the index, we use add_index_entry() (called via various code paths in the unpack_trees() machinery). That's exactly the patched function.

Or maybe you know of a code path in the unpack_trees() machinery that does _not_ go through add_index_entry()? I would be very interested to learn about such code paths.

I am afraid that we won't be able to reuse code paths for checking the backslash here, but for reserved names I am planning on refactoring the code accordingly.

Thanks,
Dscho

As of Git for Windows, v2.24.1(2). cloning a repository that contained a file with a backslash in its name some time in the past, the command will succeed but at the same time print errors like this:

error: filename in tree entry contains backslash: '\'

A corresponding git fetch will also show those errors, but fail.

The reason is that v2.24.1 is much more strict about backslashes in tree entries than earlier versions. The intention was actually to prevent checking out such files, though: if there was a mistake in a repository long ago that has been fixed long since, there is actually no reason why we should require the history to be rewritten.

This fixes https://github.com/git-for-windows/git/issues/2435.

The idea of this patch is to stop complaining about tree entries, and focus instead on the index: whenever a file is added to the index, we do not want any backslashes in the file name on Windows.

As before, this check is only performed on Windows, and only under core.protectNTFS. On other platforms, even if core.protectNTFS is turned on, the backslash is not a directory separator, therefore the Operating System's syscalls will (should?) refuse to create files on NTFS with backslashes in their file name.

I would appreciate reviews with a particular eye on keeping users safe: I am not 100% certain that all relevant file writes go through the index (I think that they all go through the index, but I might have well missed a corner case).

Changes since v1:

* Clarified the commit message (what is the goal, explain that the requirement is now loosened, explain why the code is still GIT_WINDOWS_NATIVE-only, etc).

Johannes Schindelin (1):
mingw: only test index entries for backslashes, not tree entries

 read-cache.c               | 5 +++++
 t/t7415-submodule-names.sh | 7 ++++---
 tree-walk.c                | 6 ------
 3 files changed, 9 insertions(+), 9 deletions(-)

base-commit: 12029dc57db23baef008e77db1909367599210ee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-682%2Fdscho%2Fmingw-only-error-on-backslash-in-index-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-682/dscho/mingw-only-error-on-backslash-in-index-v2
Pull-Request: https://github.com/git/git/pull/682

Range-diff vs v1:

1: 4a120fd0b3 ! 1: d6da8315d3 mingw: only test index entries for backslashes, not tree entries
@@ -8,14 +8,31 @@

error: filename in tree entry contains backslash: '\'

- While the clone still succeeds, a similar error prevents the equivalent
- git fetch operation, which is inconsistent.
+ The idea is to prevent Git from even trying to write files with
+ backslashes in their file names: while these characters are valid in
+ file names on other platforms, on Windows it is interpreted as directory
+ separator (which would obviously lead to ambiguities, e.g. when there is
+ a file a\b and there is also a file a/b).

- Arguably, this is the wrong layer for that error, anyway: As long as the
- user never checks out the files whose names contain backslashes, there
- should not be any problem in the first place.
+ Arguably, this is the wrong layer for that error: As long as the user
+ never checks out the files whose names contain backslashes, there should
+ not be any problem in the first place.

- So let's instead prevent such files to be added to the index.
+ So let's loosen the requirements: we now leave tree entries with
+ backslashes in their file names alone, but we do require any entries
+ that are added to the Git index to contain no backslashes on Windows.
+
+ Note: just as before, the check is guarded by core.protectNTFS (to
+ allow overriding the check by toggling that config setting), and it
+ is _only_ performed on Windows, as the backslash is not a directory
+ separator elsewhere, even when writing to NTFS-formatted volumes.
+
+ An alternative approach would be to try to prevent creating files with
+ backslashes in their file names. However, that comes with its own set of
+ problems. For example, git config -f C:\ProgramData\Git\config ... is
+ a very valid way to specify a custom config location, and we obviously
+ do _not_ want to prevent that. Therefore, the approach chosen in this
+ patch would appear to be better.

This addresses https://github.com/git-for-windows/git/issues/2435

gitgitgadget

From: Johannes Schindelin <[email protected]>

During a clone of a repository that contained a file with a backslash in its name in the past, as of v2.24.1(2), Git for Windows prints errors like this:

error: filename in tree entry contains backslash: '\'

The idea is to prevent Git from even trying to write files with backslashes in their file names: while these characters are valid in file names on other platforms, on Windows it is interpreted as directory separator (which would obviously lead to ambiguities, e.g. when there is a file a\b and there is also a file a/b).

Arguably, this is the wrong layer for that error: As long as the user never checks out the files whose names contain backslashes, there should not be any problem in the first place.

So let's loosen the requirements: we now leave tree entries with backslashes in their file names alone, but we do require any entries that are added to the Git index to contain no backslashes on Windows.

Note: just as before, the check is guarded by core.protectNTFS (to allow overriding the check by toggling that config setting), and it is _only_ performed on Windows, as the backslash is not a directory separator elsewhere, even when writing to NTFS-formatted volumes.

An alternative approach would be to try to prevent creating files with backslashes in their file names. However, that comes with its own set of problems. For example, git config -f C:\ProgramData\Git\config ... is a very valid way to specify a custom config location, and we obviously do _not_ want to prevent that. Therefore, the approach chosen in this patch would appear to be better.

This addresses https://github.com/git-for-windows/git/issues/2435

---
 read-cache.c               | 5 +++++
 t/t7415-submodule-names.sh | 7 ++++---
 tree-walk.c                | 6 ------
 3 files changed, 9 insertions(+), 9 deletions(-)

index ad0b48c84d..737916ebd9 100644
@@ -1278,6 +1278,11 @@ 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);
 

index 905a557585..7ae0dc8ff4 100755
@@ -207,6 +207,9 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
 			git hash-object -w --stdin)" &&
 		rev="$(git rev-parse --verify HEAD)" &&
 		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 &&
 		git -c core.protectNTFS=false update-index --add \
 			--cacheinfo 100644,$modules,.gitmodules \
 			--cacheinfo 160000,$rev,c \

@@ -214,9 +217,7 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
 			--cacheinfo 100644,$hash,d./a/x \
 			--cacheinfo 100644,$hash,d./a/..git &&
 		test_tick &&
-		git -c core.protectNTFS=false commit -m "module" &&
-		test_must_fail git show HEAD: 2>err &&
-		test_i18ngrep backslash err
+		git -c core.protectNTFS=false commit -m "module"
 	) &&
 	test_must_fail git -c core.protectNTFS=false \
 		clone --recurse-submodules squatting squatting-clone 2>err &&

index b3d162051f..d5a8e096a6 100644
@@ -43,12 +43,6 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
 		strbuf_addstr(err, _("empty filename in tree entry"));
 		return -1;
 	}
-#ifdef GIT_WINDOWS_NATIVE
-	if (protect_ntfs && strchr(path, '\\')) {
-		strbuf_addf(err, _("filename in tree entry contains backslash: '%s'"), path);
-		return -1;
-	}
-#endif
 	len = strlen(path) + 1;
 
 	/* Initialize the descriptor entry */

gitgitgadget

Hi Junio,

My intention was actually to integrate this patch into Git for Windows v2.25.0-rc1 already, that's why I sent it out for review even this late in the -rc phase.

I sent out a new iteration of the patch *just* before the new year arrived over here ;-)

The same to you!
Dscho

Hi Jonathan,

To answer this question, I added this paragraph to the commit message:

The idea is to prevent Git from even trying to write files with backslashes in their file names: while these characters are valid in file names on other platforms, on Windows it is interpreted as directory separator (which would obviously lead to ambiguities, e.g. when there is a file a\b and there is also a file a/b).

As mentioned above, the idea is to prevent Git from attempting to create files with illegal file name characters.

As mentioned in my reply to Junio, I don't think that it would be wise to even try to warn about backslashes in the file names. There are _so_ many Git users out there, I am convinced that at least some of them have valid use cases for file names with backslashes in them.

Ciao,
Dscho

[...]

Thanks for the quick answers. It helps.

I think allowing people to clone
https://github.com/zephyrproject-rtos/civetweb but not to check out the problematic historic revision is a reasonable choice, especially since it's still possible to get the data from there using

	git checkout <rev> -- . ':!bad-paths'

[...]

Every once in a while someone (e.g., in #git) has wanted "git checkout --skip-index <rev> -- <paths>", and that would be the natural way to implement such a thing. But no one has done it yet. :)

We'll just have to keep a watchful eye as people make new contributions.

Sincerely,
Jonathan

Hi Jonathan,

Right.

As to my patch, in the meantime I wondered whether I should simply extend verify_path() to take a flags parameter where I can specify whether this is intended for checkout, and handle the backslashes _there_. That would cover e.g. uses of make_transient_cache_entry(), too.

The only reason why I would want to introduce flags is that git apply has this --unsafe-paths option where verify_paths() is called on the file names in the diff, and I was unsure whether there are legitimate use cases with diffs containing DOS-style paths (I suspect that there are legitimate scenarios where this is desirable).

What do you think of that?

Ciao,
Dscho