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

dscho · 2019-12-17T12:30:37Z

It was reported over in #2435 that the protections against file names containing backslashes (added as part of v2.24.1(2)) might be overzealous and we should instead only prevent them from being written to disk.

This commit does something slightly different, still: it prevents them from being added to the Git index (which is the staging area through which all remote commits go before they are written to disk).

dscho · 2019-12-17T12:31:40Z

At this point, I am not completely convinced that this PR is 100% correct: I might very well miss a vector by which Git for Windows (with this patch) could be tricked into trying to write files with backslashes in their name.

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 git-for-windows#2435 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

mbolivar · 2019-12-17T22:10:22Z

I might very well miss a vector by which Git for Windows (with this patch) could be tricked into trying to write files with backslashes in their name.

I am certainly not an expert on git's internals, but wonder whether protecting the index is critical either way you feel about our use case in #2435 .

What would happen in response to something like this if the index is not protected?

user clones repository which has backslashes in some tree's path in its history using an old version of git
user upgrades to git version 2.24.1(2) without re-cloning this repository
user attempts to check out a commit with backslashes in its paths

Without this PR, could step 3 result in bad behavior? More generally, do you have a pointer to information on the threat modeling that was done on this vulnerability? I found an older article mentioning config.protectNTFS here:

https://blog.developer.atlassian.com/securing-your-git-server/

The concern there appears to be an attack on the .git directory on case insensitive file systems. It looks like the relevant protections went into read-cache.c as well, e.g. in verify_path(). So assuming those checks are implemented correctly, this PR moving code from tree-walk.c to read-cache.c also seems sane.

Is there a reason why is_valid_win32_path() or verify_path() is not being changed here?

Sorry if I'm talking nonsense, like I said, I'm not an expert here.

PhilipOakley · 2019-12-18T21:23:59Z

@dscho The patch looks good to me.

I haven't fully thought through the distinction between checking the index, and checking the worktree paths.

carlescufi · 2019-12-19T12:18:40Z

@dscho Not an expert either, but looking at the CVE description I don't clearly see how there is a potential with arbitrary file overwriting until git actually reaches the filesystem itself, which means that your patch at first glance seems to be secure.

carlescufi · 2019-12-23T18:48:15Z

@dscho how would you like to proceed with this? is there someone else we can pull in for code review?

dscho · 2019-12-25T11:52:19Z

@carlescufi I just opened git#682 and will submit it as soon as the the PR build passes (actually, once I am online after it passed 😄).

dscho · 2020-01-03T18:36:35Z

The concern there appears to be an attack on the .git directory on case insensitive file systems. It looks like the relevant protections went into read-cache.c as well, e.g. in verify_path(). So assuming those checks are implemented correctly, this PR moving code from tree-walk.c to read-cache.c also seems sane.

The .git file/directory indeed needs to be protected. In this instance, an attack was demonstrated that used the ambiguity between submodule/.git+ and .submodule\.git+ to fool Git into writing a file into the directory submodule that would "squat" on the NTFS short name git~1, essentially causing the submodule to get a .git file that does not have the short name git~1, and therefore side-stepping our careful protections. This, in turn, would allow a file git~2 in the submodule to overwrite the contents of the .git file, allowing for really bad things like remote code execution during a recursive clone.

Is there a reason why is_valid_win32_path() or verify_path() is not being changed here?

Yes. is_valid_win32_path() needs to find paths like C:\Program Files\Git to be valid. There are two backlashes in that valid path.

As to verify_path(): It is used in git apply, too. If you have a diff that refers to the paths with the backslash as directory separator, I would think that it should still work.

mbolivar · 2020-01-03T18:49:21Z

The .git file/directory indeed needs to be protected.

Thanks for the explanation!

I'm not sure I saw an answer to my question, though, which still feels relevant in light of that explanation:

Without this PR, could step 3 result in bad behavior?

Did I miss something?

Yes. is_valid_win32_path() needs to find paths like C:\Program Files\Git to be valid. There are two backlashes in that valid path.

OK, thanks.

As to verify_path(): It is used in git apply, too. If you have a diff that refers to the paths with the backslash as directory separator, I would think that it should still work.

OK, that's reasonable, thanks.

dscho · 2020-01-03T18:50:25Z

I'm not sure I saw an answer to my question, though, which still feels relevant in light of that explanation:

Without this PR, could step 3 result in bad behavior?

Sorry, I forgot to answer: yes, it would.

dscho · 2020-01-03T19:41:32Z

I've decided to merge this for now, even if I want to refine it over the course of the -rc versions leading up to v2.25.0.

…slash-in-index Disallow writing, but not fetching commits with file names containing backslashes

Repositories with old commits containing backslashes in file names [can now be fetched/cloned again](git-for-windows/git#2437) (but Git will still refuse to check out files with backslashes in their file names). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

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

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

…slash-in-index Disallow writing, but not fetching commits with file names containing backslashes

dscho · 2020-01-09T11:34:00Z

As to verify_path(): It is used in git apply, too. If you have a diff that refers to the paths with the backslash as directory separator, I would think that it should still work.

OK, that's reasonable, thanks.

I have had second thoughts and went back to investigating the code paths in question. Unfortunately, I could not convince myself that the current approach is robust enough, even if it does allow for a much better error message than verify_path() does. So I went back to the drawing board, analyzed a bit more of the code and changed it to do what you suggested in d6e3142

@mbolivar thank you so much for your thoughts. I really think that it helped prevent a broken Git for Windows v2.25.0.

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

mbolivar · 2020-01-09T17:54:33Z

So I went back to the drawing board, analyzed a bit more of the code and changed it to do what you suggested in d6e3142

@mbolivar thank you so much for your thoughts. I really think that it helped prevent a broken Git for Windows v2.25.0.

Thanks for saying so! I'm glad they were useful. Please let me know if you'd like me to re-test the next rc to make sure #2435 is still gone :).

dscho · 2020-01-09T20:38:40Z

Please let me know if you'd like me to re-test the next rc to make sure #2435 is still gone :).

@mbolivar that would be very valuable: https://github.com/git-for-windows/git/releases/tag/v2.25.0-rc2.windows.1

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

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

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

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

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 <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>

mbolivar · 2020-01-10T22:10:20Z

Johannes Schindelin <notifications@github.com> writes:

> Please let me know if you'd like me to re-test the next rc to make sure #2435 is still gone :). @mbolivar that would be very valuable: https://github.com/git-for-windows/git/releases/tag/v2.25.0-rc2.windows.1

All good for me. This one also allows me to fetch the repository but not checkout the affected version. I found the "error: invalid path 'test/\/a.txt'" message reasonably understandable as well. Thanks again for all your work on this and Git for Windows!

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

dscho mentioned this pull request Dec 17, 2019

Git 2.24 breaks existing repositories: filename in tree entry contains backslash #2435

Closed

1 task

dscho force-pushed the only-error-on-backslash-in-index branch from 5d64889 to d192fa9 Compare December 17, 2019 14:51

weekly-digest bot mentioned this pull request Dec 22, 2019

Weekly Digest (15 December, 2019 - 22 December, 2019) #2441

Closed

dscho changed the title ~~Disallow writing, but not fetch commits with file names containing backslashes~~ Disallow writing, but not fetching commits with file names containing backslashes Dec 26, 2019

weekly-digest bot mentioned this pull request Dec 29, 2019

Weekly Digest (22 December, 2019 - 29 December, 2019) #2447

Closed

dscho merged commit 8fc9b86 into git-for-windows:master Jan 3, 2020

dscho deleted the only-error-on-backslash-in-index branch January 3, 2020 19:41

dscho added a commit to dscho/git that referenced this pull request Jan 3, 2020

Merge pull request git-for-windows#2437 from dscho/only-error-on-back…

e38345a

…slash-in-index Disallow writing, but not fetching commits with file names containing backslashes

dscho added this to the Next release milestone Jan 3, 2020

mbolivar mentioned this pull request Jan 3, 2020

civetweb triggers an error on Windows with Git 2.24 zephyrproject-rtos/zephyr#21426

Closed

dscho added a commit that referenced this pull request Jan 4, 2020

Merge pull request #2437 from dscho/only-error-on-backslash-in-index

0b2aa34

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

dscho added a commit to dscho/git that referenced this pull request Jan 9, 2020

Merge pull request git-for-windows#2437 from dscho/only-error-on-back…

f6c393d

…slash-in-index Disallow writing, but not fetching commits with file names containing backslashes

git-for-windows-ci pushed a commit that referenced this pull request Jan 9, 2020

Merge pull request #2437 from dscho/only-error-on-backslash-in-index

20c0ab8

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

dscho added a commit that referenced this pull request Jan 9, 2020

Merge pull request #2437 from dscho/only-error-on-backslash-in-index

93797e4

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

bturner mentioned this pull request Jun 10, 2020

Merge error (Bitbucket) when using Git 2.27 - works with Git 2.24 #2682

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dscho commented Dec 17, 2019

dscho commented Dec 17, 2019

mbolivar commented Dec 17, 2019 •

edited

Loading

PhilipOakley commented Dec 18, 2019

carlescufi commented Dec 19, 2019

carlescufi commented Dec 23, 2019

dscho commented Dec 25, 2019

dscho commented Jan 3, 2020

mbolivar commented Jan 3, 2020

dscho commented Jan 3, 2020

dscho commented Jan 3, 2020

dscho commented Jan 9, 2020

mbolivar commented Jan 9, 2020

dscho commented Jan 9, 2020

mbolivar commented Jan 10, 2020 via email

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

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

Conversation

dscho commented Dec 17, 2019

dscho commented Dec 17, 2019

mbolivar commented Dec 17, 2019 • edited Loading

PhilipOakley commented Dec 18, 2019

carlescufi commented Dec 19, 2019

carlescufi commented Dec 23, 2019

dscho commented Dec 25, 2019

dscho commented Jan 3, 2020

mbolivar commented Jan 3, 2020

dscho commented Jan 3, 2020

dscho commented Jan 3, 2020

dscho commented Jan 9, 2020

mbolivar commented Jan 9, 2020

dscho commented Jan 9, 2020

mbolivar commented Jan 10, 2020 via email

mbolivar commented Dec 17, 2019 •

edited

Loading