git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Jonathan Nieder <jrnieder@gmail.com>
Subject: [PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore
Date: Mon, 5 Oct 2020 08:16:09 -0400	[thread overview]
Message-ID: <20201005121609.GA2907272@coredump.intra.peff.net> (raw)
In-Reply-To: <20201005071751.GA2290770@coredump.intra.peff.net>

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.
> 
> 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 &&
    + 	(

  parent reply	other threads:[~2020-10-05 12:16 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-05  7:17 [PATCH 0/7] " Jeff King
2020-10-05  7:19 ` [PATCH 1/7] fsck_tree(): fix shadowed variable Jeff King
2020-10-05  7:44   ` Jonathan Nieder
2020-10-05  8:20     ` Jeff King
2020-10-05  8:29       ` Jonathan Nieder
2020-10-05  7:19 ` [PATCH 2/7] fsck_tree(): wrap some long lines Jeff King
2020-10-05  7:46   ` Jonathan Nieder
2020-10-05  7:19 ` [PATCH 3/7] t7415: rename to expand scope Jeff King
2020-10-05  7:50   ` Jonathan Nieder
2020-10-05  8:24     ` Jeff King
2020-10-05  8:34       ` Jonathan Nieder
2020-10-05  8:49         ` Jeff King
2020-10-05  7:20 ` [PATCH 4/7] t7450: test verify_path() handling of gitmodules Jeff King
2020-10-05  7:53   ` Jonathan Nieder
2020-10-05  8:30     ` Jeff King
2020-10-05  8:38       ` Jonathan Nieder
2020-10-05  7:21 ` [PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching Jeff King
2020-10-05  8:03   ` Jonathan Nieder
2020-10-05  8:40     ` Jeff King
2020-10-05 21:20       ` Johannes Schindelin
2020-10-06 14:01         ` Jeff King
2020-10-05  7:24 ` [PATCH 6/7] verify_path(): disallow symlinks in .gitattributes and .gitignore Jeff King
2020-10-05  8:09   ` Jonathan Nieder
2020-10-05 12:07     ` Jeff King
2020-10-05  7:25 ` [PATCH 7/7] fsck: complain when .gitattributes or .gitignore is a symlink Jeff King
2020-10-05  8:12   ` Jonathan Nieder
2020-10-05  8:53     ` Jeff King
2020-10-05  7:32 ` [PATCH 0/7] forbidding symlinked .gitattributes and .gitignore Jonathan Nieder
2020-10-05  8:58   ` Jeff King
2020-10-05 12:16 ` Jeff King [this message]
2020-10-05 12:16   ` [PATCH v2 1/8] fsck_tree(): fix shadowed variable Jeff King
2020-10-05 12:16   ` [PATCH v2 2/8] fsck_tree(): wrap some long lines Jeff King
2020-10-05 12:16   ` [PATCH v2 3/8] t7415: rename to expand scope Jeff King
2020-10-05 12:16   ` [PATCH v2 4/8] t7450: test verify_path() handling of gitmodules Jeff King
2020-10-05 12:16   ` [PATCH v2 5/8] t7450: test .gitmodules symlink matching against obscured names Jeff King
2020-10-05 12:16   ` [PATCH v2 6/8] t0060: test obscured .gitattributes and .gitignore matching Jeff King
2020-10-05 12:16   ` [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore Jeff King
2020-10-27  3:35     ` Jonathan Nieder
2020-10-27  7:58       ` Jeff King
2020-10-27 22:00         ` Junio C Hamano
2020-10-28  9:41           ` Jeff King
2020-10-27 23:43         ` Jonathan Nieder
2020-10-28 19:18           ` Junio C Hamano
2020-10-05 12:16   ` [PATCH v2 8/8] fsck: complain when .gitattributes or .gitignore is a symlink Jeff King
2020-10-06 20:41   ` [PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore Junio C Hamano
2020-10-20 23:19   ` Philip Oakley
2020-10-23  8:17     ` [PATCH] documentation symlink restrictions for .git* files Jeff King
2020-10-23  8:27       ` Jeff King
2020-10-26 22:18       ` Philip Oakley
2020-10-26 22:53         ` Jeff King
2020-10-26 23:32           ` Junio C Hamano
2020-10-27  7:26             ` Jeff King
2020-10-27 18:45               ` Junio C Hamano
2020-10-27 21:00                 ` Philip Oakley
2020-10-28 19:14                   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201005121609.GA2907272@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).