git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore

From
Philip Oakley <philipoakley@iee.email>
Date
Oct 20, 2020, 23:19 UTC
Message-ID
<6c0a0036-e217-a334-2a74-dd59a4592c1f@iee.email>
In-Reply-To
<20201005121609.GA2907272@coredump.intra.peff.net>
A rather late comment,
On 05/10/2020 13:16, Jeff King wrote:
Show 7 quoted lines
> On Mon, Oct 05, 2020 at 03:17:51AM -0400, Jeff King wrote:
>
>> About 2 years ago as part of a security release we made it illegal to
>> have a symlinked .gitmodules file (refusing it both in the index and via
>> fsck). At the time we discussed (on the security list) outlawing
>> symlinks for other .git files in the same way, but we decided not to do
>> so as part of the security release, as it wasn't strictly necessary.

Is this something that should be recorded in the documentation, either as a simple (sensible) limitation, or explicitly as a security related safety measure?

I didn't see any changes to the .txt docs in the change list below.
Philip
Show 280 quoted lines
>>
>> We publicly revisited the topic in:
>>
>>   https://lore.kernel.org/git/20190114230902.GG162110@google.com/
>>
>> but there were a few fixes needed, and it got forgotten. So here it is
>> again, with those fixes:
>> [...]
> And here's a v2 incorporating feedback from Jonathan. There are no
> substantial changes in the code. Most of the fixes are cosmetic, but the
> tests are beefed up a bit, as well:
>
>  - we now test that ntfs and hfs names are matched via fsck and
>    verify_path() for all file types. The bulk of this is in a new patch
>    5, and the final patches are adjusted to use the new helper.
>
>  - we confirm that read-tree doesn't write the forbidden entry into the
>    index (in addition to seeing that it complains)
>
>  - the test script name is now "bad-dotgitx" instead of the vague
>    "bad-meta-files"
>
>  - whitespace, typo-fixes, clarity, etc; the range diff is below
>
>   [1/8]: fsck_tree(): fix shadowed variable
>   [2/8]: fsck_tree(): wrap some long lines
>   [3/8]: t7415: rename to expand scope
>   [4/8]: t7450: test verify_path() handling of gitmodules
>   [5/8]: t7450: test .gitmodules symlink matching against obscured names
>   [6/8]: t0060: test obscured .gitattributes and .gitignore matching
>   [7/8]: verify_path(): disallow symlinks in .gitattributes and .gitignore
>   [8/8]: fsck: complain when .gitattributes or .gitignore is a symlink
>
>  fsck.c                                        | 79 +++++++++++----
>  read-cache.c                                  | 12 ++-
>  t/helper/test-path-utils.c                    | 41 +++++---
>  t/t0060-path-utils.sh                         | 20 ++++
>  ...le-names.sh => t7450-bad-dotgitx-files.sh} | 99 +++++++++++++------
>  5 files changed, 187 insertions(+), 64 deletions(-)
>  rename t/{t7415-submodule-names.sh => t7450-bad-dotgitx-files.sh} (73%)
>
> 1:  d4c4b98188 ! 1:  78689a44ba fsck_tree(): fix shadowed variable
>     @@ Commit message
>      
>          Let's rename both variables in the function to avoid confusion. This
>          makes the diff a little noisy (e.g., all of the report() calls outside
>     -    the loop wee already correct but need touched), but makes sure we catch
>     -    all cases and will avoid similar confusion in the future.
>     +    the loop were already correct but need to be touched), but makes sure we
>     +    catch all cases and will avoid similar confusion in the future.
>     +
>     +    Note that our test change removes the comment about translation. It was
>     +    arguably confusing since 674ba34038 (fsck: mark strings for translation,
>     +    2018-11-10); we wouldn't translate gitmodulesSymlink, but it did get
>     +    removed by GETTEXT_POISON because that feature eats embedded
>     +    %s characters. But certainly after this patch, when we look for the
>     +    "tree %s: %s" format, we could get foiled by translation.
>      
>          Signed-off-by: Jeff King <peff@peff.net>
>      
>     @@ t/t7415-submodule-names.sh: test_expect_success 'fsck detects symlinked .gitmodu
>      +		tree=$(git mktree <bad-tree) &&
>       
>       		# Check not only that we fail, but that it is due to the
>     - 		# symlink detector; this grep string comes from the config
>     - 		# variable name and will not be translated.
>     +-		# symlink detector; this grep string comes from the config
>     +-		# variable name and will not be translated.
>     ++		# symlink detector
>       		test_must_fail git fsck 2>output &&
>      -		test_i18ngrep gitmodulesSymlink output
>      +		test_i18ngrep "tree $tree: gitmodulesSymlink" output
> 2:  29d0d3af44 = 2:  b1f7ec465c fsck_tree(): wrap some long lines
> 3:  8679f0b2f2 ! 3:  d26ef683fd t7415: rename to expand scope
>     @@ Commit message
>      
>          Signed-off-by: Jeff King <peff@peff.net>
>      
>     - ## t/t7415-submodule-names.sh => t/t7450-bad-meta-files.sh ##
>     + ## t/t7415-submodule-names.sh => t/t7450-bad-dotgitx-files.sh ##
>      @@
>       #!/bin/sh
>       
> 4:  84e58f7f46 ! 4:  493a4c79a3 t7450: test verify_path() handling of gitmodules
>     @@ Commit message
>      
>          Signed-off-by: Jeff King <peff@peff.net>
>      
>     - ## t/t7450-bad-meta-files.sh ##
>     -@@ t/t7450-bad-meta-files.sh: test_expect_success 'index-pack --strict works for non-repo pack' '
>     + ## t/t7450-bad-dotgitx-files.sh ##
>     +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'index-pack --strict works for non-repo pack' '
>       	grep gitmodulesName output
>       '
>       
>      -test_expect_success 'fsck detects symlinked .gitmodules file' '
>     -+test_expect_success 'create repo with symlinked .gitmodules file' '
>     ++test_expect_success 'set up repo with symlinked .gitmodules file' '
>       	git init symlink &&
>       	(
>       		cd symlink &&
>     -@@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '
>     +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '
>       		{
>       			printf "100644 blob $content\t$tricky\n" &&
>       			printf "120000 blob $target\t.gitmodules\n"
>     @@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodul
>      +		cd symlink &&
>       
>       		# Check not only that we fail, but that it is due to the
>     - 		# symlink detector; this grep string comes from the config
>     -@@ t/t7450-bad-meta-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '
>     + 		# symlink detector
>     +@@ t/t7450-bad-dotgitx-files.sh: test_expect_success 'fsck detects symlinked .gitmodules file' '
>       	)
>       '
>       
>     -+test_expect_success 'refuse to load symlinked .gitmodule into index' '
>     ++test_expect_success 'refuse to load symlinked .gitmodules into index' '
>      +	test_must_fail git -C symlink read-tree $tree 2>err &&
>     -+	test_i18ngrep "invalid path.*gitmodules" err
>     ++	test_i18ngrep "invalid path.*gitmodules" err &&
>     ++	git -C symlink ls-files >out &&
>     ++	test_must_be_empty out
>      +'
>      +
>       test_expect_success 'fsck detects non-blob .gitmodules' '
> -:  ---------- > 5:  db5e78ff5b t7450: test .gitmodules symlink matching against obscured names
> 5:  e141e49a5b ! 6:  b5962a75a4 t0060: test obscured .gitattributes and .gitignore matching
>     @@ t/helper/test-path-utils.c: static struct test_data dirname_data[] = {
>      +		if (!strcmp("--not", *argv))
>      +			expect = !expect;
>      +		else if (expect != (is_hfs(*argv) || is_ntfs(*argv)))
>     -+			 res = error("'%s' is %s.%s", *argv,
>     -+				     expect ? "not " : "", x);
>     ++			res = error("'%s' is %s.git%s", *argv,
>     ++				    expect ? "not " : "", x);
>      +		else
>     -+			fprintf(stderr, "ok: '%s' is %s.%s\n",
>     ++			fprintf(stderr, "ok: '%s' is %s.git%s\n",
>      +				*argv, expect ? "" : "not ", x);
>      +	}
>      +	return !!res;
>     @@ t/helper/test-path-utils.c: int cmd__path_utils(int argc, const char **argv)
>      -				fprintf(stderr, "ok: '%s' is %s.gitmodules\n",
>      -					argv[i], expect ? "" : "not ");
>      -		return !!res;
>     -+		return check_dotgitx("gitmodules", argv + 2,
>     ++		return check_dotgitx("modules", argv + 2,
>      +				     is_hfs_dotgitmodules,
>      +				     is_ntfs_dotgitmodules);
>      +	}
>      +	if (argc > 2 && !strcmp(argv[1], "is_dotgitignore")) {
>     -+		return check_dotgitx("gitignore", argv + 2,
>     ++		return check_dotgitx("ignore", argv + 2,
>      +				     is_hfs_dotgitignore,
>      +				     is_ntfs_dotgitignore);
>      +	}
>      +	if (argc > 2 && !strcmp(argv[1], "is_dotgitattributes")) {
>     -+		return check_dotgitx("gitattributes", argv + 2,
>     ++		return check_dotgitx("attributes", argv + 2,
>      +				     is_hfs_dotgitattributes,
>      +				     is_ntfs_dotgitattributes);
>       	}
> 6:  d214bbd8ec ! 7:  e4ec698a5b verify_path(): disallow symlinks in .gitattributes and .gitignore
>     @@ Commit message
>          0fc333ba20 (is_hfs_dotgit: match other .git files, 2018-05-02), which
>          were done as part of the series touching .gitmodules.
>      
>     +    No tests yet, as we'll add them in a subsequent patch once we have fsck
>     +    support, too.
>     +
>          Signed-off-by: Jeff King <peff@peff.net>
>      
>       ## read-cache.c ##
>     @@ read-cache.c: int verify_path(const char *path, unsigned mode)
>       						return 0;
>       				}
>       			}
>     -
>     - ## t/t7450-bad-meta-files.sh ##
>     -@@ t/t7450-bad-meta-files.sh: test_expect_success 'git dirs of sibling submodules must not be nested' '
>     - 	test_i18ngrep "is inside git dir" err
>     - '
>     - 
>     -+test_expect_success 'create repo with symlinked .gitattributes file' '
>     -+	git init symlink-attr &&
>     -+	target=$(echo target | git -C symlink-attr hash-object -w --stdin) &&
>     -+	tree=$(
>     -+		printf "120000 blob $target\t.gitattributes\n" |
>     -+		git -C symlink-attr mktree
>     -+	)
>     -+'
>     -+
>     -+test_expect_success 'refuse to load symlinked .gitattributes into index' '
>     -+	test_must_fail git -C symlink-attr read-tree $tree 2>err &&
>     -+	test_i18ngrep "invalid path.*gitattributes" err
>     -+'
>     -+
>     -+test_expect_success 'create repo with symlinked .gitignore file' '
>     -+	git init symlink-ignore &&
>     -+	target=$(echo target | git -C symlink-ignore hash-object -w --stdin) &&
>     -+	tree=$(
>     -+		printf "120000 blob $target\t.gitignore\n" |
>     -+		git -C symlink-ignore mktree
>     -+	)
>     -+'
>     -+
>     -+test_expect_success 'refuse to load symlinked .gitignore into index' '
>     -+	test_must_fail git -C symlink-ignore read-tree $tree 2>err &&
>     -+	test_i18ngrep "invalid path.*gitignore" err
>     -+'
>     -+
>     -+
>     - test_done
> 7:  49423d03b5 ! 8:  58c9ce0f3c fsck: complain when .gitattributes or .gitignore is a symlink
>     @@ Commit message
>          check to fsck, which matches how we handle .gitmodules symlinks, via
>          b7b1fca175 (fsck: complain when .gitmodules is a symlink, 2018-05-04).
>      
>     -    Note that we won't add these to the existing gitmodules block. Its logic
>     -    is a bit more complicated, as we also check the content of non-symlink
>     -    instances we find. But for these new files, there is no content check;
>     -    we're just looking at the name and mode of the tree entry (and we can
>     -    avoid even the complicated name checks in the common case that the mode
>     -    doesn't indicate a symlink).
>     +    Note that we won't add these to the existing gitmodules block. The logic
>     +    for gitmodules is a bit more complicated, as we also check the content
>     +    of non-symlink instances we find. But for these new files, there is no
>     +    content check; we're just looking at the name and mode of the tree entry
>     +    (and we can avoid even the complicated name checks in the common case
>     +    that the mode doesn't indicate a symlink).
>     +
>     +    We can reuse the test helper function we defined for .gitmodules,
>     +    though (and this covers the verify_path() change from the previous
>     +    commit, as well).
>      
>          Signed-off-by: Jeff King <peff@peff.net>
>      
>     @@ fsck.c: static int fsck_tree(const struct object_id *tree_oid,
>       			while (backslash) {
>       				backslash++;
>      
>     - ## t/t7450-bad-meta-files.sh ##
>     -@@ t/t7450-bad-meta-files.sh: test_expect_success 'refuse to load symlinked .gitattributes into index' '
>     - 	test_i18ngrep "invalid path.*gitattributes" err
>     - '
>     + ## t/t7450-bad-dotgitx-files.sh ##
>     +@@ t/t7450-bad-dotgitx-files.sh: check_forbidden_symlink gitmodules vanilla .gitmodules
>     + check_forbidden_symlink gitmodules ntfs ".gitmodules ."
>     + check_forbidden_symlink gitmodules hfs ".${u200c}gitmodules"
>       
>     -+test_expect_success 'fsck detects symlinked .gitattributes file' '
>     -+	test_must_fail git -C symlink-attr fsck 2>err &&
>     -+	test_i18ngrep "tree $tree: gitattributesSymlink" err
>     -+'
>     ++check_forbidden_symlink gitattributes vanilla .gitattributes
>     ++check_forbidden_symlink gitattributes ntfs ".gitattributes ."
>     ++check_forbidden_symlink gitattributes hfs ".${u200c}gitattributes"
>      +
>     - test_expect_success 'create repo with symlinked .gitignore file' '
>     - 	git init symlink-ignore &&
>     - 	target=$(echo target | git -C symlink-ignore hash-object -w --stdin) &&
>     -@@ t/t7450-bad-meta-files.sh: test_expect_success 'refuse to load symlinked .gitignore into index' '
>     - 	test_i18ngrep "invalid path.*gitignore" err
>     - '
>     - 
>     -+test_expect_success 'fsck detects symlinked .gitignore file' '
>     -+	test_must_fail git -C symlink-ignore fsck 2>err &&
>     -+	test_i18ngrep "tree $tree: gitignoreSymlink" err
>     -+'
>     - 
>     - test_done
>     ++check_forbidden_symlink gitignore vanilla .gitignore
>     ++check_forbidden_symlink gitignore ntfs ".gitignore ."
>     ++check_forbidden_symlink gitignore hfs ".${u200c}gitignore"
>     ++
>     + test_expect_success 'fsck detects non-blob .gitmodules' '
>     + 	git init non-blob &&
>     + 	(
Previous: Junio C HamanoNext: Jeff King
Message 46 of 55 in “forbidding symlinked .gitattributes and .gitignore”
  1. 0/7 forbidding symlinked .gitattributes and .gitignoreJeff King, Oct 5, 2020
  2. 1/7 fsck_tree(): fix shadowed variableJeff King, Oct 5, 2020
  3. Jonathan NiederOct 5, 2020
  4. Jeff KingOct 5, 2020
  5. Jonathan NiederOct 5, 2020
  6. 2/7 fsck_tree(): wrap some long linesJeff King, Oct 5, 2020
  7. Jonathan NiederOct 5, 2020
  8. 3/7 t7415: rename to expand scopeJeff King, Oct 5, 2020
  9. Jonathan NiederOct 5, 2020
  10. Jeff KingOct 5, 2020
  11. Jonathan NiederOct 5, 2020
  12. Jeff KingOct 5, 2020
  13. 4/7 t7450: test verify_path() handling of gitmodulesJeff King, Oct 5, 2020
  14. Jonathan NiederOct 5, 2020
  15. Jeff KingOct 5, 2020
  16. Jonathan NiederOct 5, 2020
  17. 5/7 t0060: test obscured .gitattributes and .gitignore matchingJeff King, Oct 5, 2020
  18. Jonathan NiederOct 5, 2020
  19. Jeff KingOct 5, 2020
  20. Johannes SchindelinOct 5, 2020
  21. Jeff KingOct 6, 2020
  22. 6/7 verify_path(): disallow symlinks in .gitattributes and .gitignoreJeff King, Oct 5, 2020
  23. Jonathan NiederOct 5, 2020
  24. Jeff KingOct 5, 2020
  25. 7/7 fsck: complain when .gitattributes or .gitignore is a symlinkJeff King, Oct 5, 2020
  26. Jonathan NiederOct 5, 2020
  27. Jeff KingOct 5, 2020
  28. Jonathan NiederOct 5, 2020
  29. Jeff KingOct 5, 2020
  30. 0/8 forbidding symlinked .gitattributes and .gitignoreJeff King, Oct 5, 2020
  31. 1/8 fsck_tree(): fix shadowed variableJeff King, Oct 5, 2020
  32. 2/8 fsck_tree(): wrap some long linesJeff King, Oct 5, 2020
  33. 3/8 t7415: rename to expand scopeJeff King, Oct 5, 2020
  34. 4/8 t7450: test verify_path() handling of gitmodulesJeff King, Oct 5, 2020
  35. 5/8 t7450: test .gitmodules symlink matching against obscured namesJeff King, Oct 5, 2020
  36. 7/8 verify_path(): disallow symlinks in .gitattributes and .gitignoreJeff King, Oct 5, 2020
  37. Jonathan NiederOct 27, 2020
  38. Jeff KingOct 27, 2020
  39. Junio C HamanoOct 27, 2020
  40. Jeff KingOct 28, 2020
  41. Jonathan NiederOct 27, 2020
  42. Junio C HamanoOct 28, 2020
  43. 6/8 t0060: test obscured .gitattributes and .gitignore matchingJeff King, Oct 5, 2020
  44. 8/8 fsck: complain when .gitattributes or .gitignore is a symlinkJeff King, Oct 5, 2020
  45. Junio C HamanoOct 6, 2020
  46. Philip OakleyOct 20, 2020
  47. documentation symlink restrictions for .git* filesJeff King, Oct 23, 2020
  48. Jeff KingOct 23, 2020
  49. Philip OakleyOct 26, 2020
  50. Jeff KingOct 26, 2020
  51. Junio C HamanoOct 26, 2020
  52. Jeff KingOct 27, 2020
  53. Junio C HamanoOct 27, 2020
  54. Philip OakleyOct 27, 2020
  55. Junio C HamanoOct 28, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.