From: Philip Oakley <philipoakley@iee.email>
To: Jeff King <peff@peff.net>, git@vger.kernel.org
Cc: Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore
Date: Wed, 21 Oct 2020 00:19:25 +0100 [thread overview]
Message-ID: <6c0a0036-e217-a334-2a74-dd59a4592c1f@iee.email> (raw)
In-Reply-To: <20201005121609.GA2907272@coredump.intra.peff.net>
A rather late comment,
On 05/10/2020 13:16, Jeff King wrote:
> 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
>>
>> 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 &&
> + (
next prev parent reply other threads:[~2020-10-20 23:19 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-05 7:17 [PATCH 0/7] forbidding symlinked .gitattributes and .gitignore 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 ` [PATCH v2 0/8] " Jeff King
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 [this message]
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=6c0a0036-e217-a334-2a74-dd59a4592c1f@iee.email \
--to=philipoakley@iee.email \
--cc=git@vger.kernel.org \
--cc=jrnieder@gmail.com \
--cc=peff@peff.net \
/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 public 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).