{"thread":{"id":"55143","subject":"Limited local file inclusion with .mailmap symlinks and git-archive","startedAt":"2021-02-13T17:50:27Z","lastAt":"2021-02-26T06:36:31Z","messageCount":25,"participants":["Blake Burkhart","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"416843","messageId":"CAP3OtXj15f9XV=Rzz2oBXQ1TQH3WWKJGBbeaWrmp6Ha4ZTn9nA@mail.gmail.com","threadId":"55143","inReplyTo":null,"subject":"Limited local file inclusion with .mailmap symlinks and git-archive","fromName":"Blake Burkhart","fromEmail":"bburky@bburky.com","sentAt":"2021-02-13T17:49:32Z","receivedAt":"2021-02-13T17:50:27Z","isPatch":false,"sender":{"key":"bburky@bburky.com","avatar":"https://avatars.githubusercontent.com/u/140526?v=4"},"body":"Git's mailmap implementation first tries using the blob from the\nrepository, but also supports using a local (possibly uncommitted)\n.mailmap file. When reading from the local file, git will follow\nsymlinks. If a symlink is committed to a repository named .mailmap,\ngit will parse the file on the other side of the symlink if the\nrepository is cloned locally.\n\nGit log supports an %aN placeholder which prints the result of the\nmailmap, if it is possible for this value to be sent to an attacker\nthis could become a local file inclusion concern. With git-archive it\nis possible to use $Format:%aN$ to include this value in an exported\narchive.\n\nRunning git on bare repos or using git archive --remote=... is\nunaffected because a local file is never used, only the in-repo blob.\n\nGit's mailmap parser is very forgiving, it reads in each line, skips\nlines starting with #, then considers whatever it finds between < and\n> as the email address. It is even possible to use binary files as a\n.mailmap. As a demonstration I used a symlink to /proc/self/exe (which\nitself is a symlink to /usr/bin/git). The string [--exec-path[= was\nextracted from the binary as the author name.\n\ngit init mailmap\ncd mailmap\nln -s /proc/self/exe .mailmap\necho \"test export-subst\" > .gitattributes\necho '$Format:%aN$' > test\ngit add .mailmap .gitattributes test\ngit commit -m \"test\" --author=\"foo \"\ncd ..\n\n# Pretend you're cloning from the internet...\ngit clone mailmap mailmap-clone\ncd mailmap-clone\ngit archive --format=tar HEAD\n# Output contains [--exec-path[=\n\nThese are unaffected:\n\ncd ..\ngit --git-dir=mailmap/.git archive --format=tar HEAD\ngit archive --remote=git://localhost/ --format=tar HEAD\n\nI reported this issue to the private security list first and discussed\nthis issue with Peff. This is similar to existing concerns with\n.gitmodules, .gitattributes and .gitignore. Git already disallows\nchecking out a .gitmodules file from a repository, and I understand\nthere are in progress patches to add similar protection for\n.gitattributes and .gitignore. Please ensure the .mailmap file gets\nsimilar symlink protection.\n\nExploitability is limited because the targeted file must contain a\nstring formatted like known to the attacker, or attacker\ncontrolled. Also, most automated build systems that checkout code are\nsandboxed and prepared to run arbitrary code already (it is\ntechnically possible to read potentially sensitive variables from\n/proc/self/environ with this, but is very limited because it contains\nnull bytes).\n\n--\nBlake Burkhart\n"},{"id":"416989","messageId":"YCsBA002yv8XpppM@coredump.intra.peff.net","threadId":"55143","inReplyTo":"CAP3OtXj15f9XV=Rzz2oBXQ1TQH3WWKJGBbeaWrmp6Ha4ZTn9nA@mail.gmail.com","subject":"Re: Limited local file inclusion with .mailmap symlinks and git-archive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-15T23:17:23Z","receivedAt":"2021-02-15T23:18:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 13, 2021 at 11:49:32AM -0600, Blake Burkhart wrote:\n\n> I reported this issue to the private security list first and discussed\n> this issue with Peff. This is similar to existing concerns with\n> .gitmodules, .gitattributes and .gitignore. Git already disallows\n> checking out a .gitmodules file from a repository, and I understand\n> there are in progress patches to add similar protection for\n> .gitattributes and .gitignore. Please ensure the .mailmap file gets\n> similar symlink protection.\n\nThanks again for bringing this up.\n\nHere are some patches that I think will help. They're meant to be\napplied on the stalled jk/symlinked-dotgitx-files topic, which Junio has\nbeen carrying in \"seen\" for a few months now.\n\nThe sticking point there was that we were concerned that the fsck checks\nfor .gitattributes/.gitignore would catch historical commits in real\nprojects, making them annoying to work with. So the first patch here\nloosens those checks to warnings. I think this is safe enough, as the\nreal protection is in preventing checkouts in the index code paths (the\nfsck checks are really just about protecting other clients using older\nversions, but the severity of these attacks is so low that the tradeoff\ndoesn't make as much sense).\n\nObviously this could be squashed into the earlier patches, but I think\ndocumenting the change of direction with a separate commit makes sense.\n\nAnd then the second patch adds similar .mailmap support (also as a\nwarning, since I think it is largely in the same boat, and it makes\nsense to be consistent).\n\n [1/2]: fsck: make symlinked .gitignore and .gitattributes a warning\n [2/2]: disallow symlinked .mailmap files\n\n cache.h | 1 +\n fsck.c | 10 ++++++++--\n path.c | 5 +++++\n read-cache.c | 6 ++++--\n t/helper/test-path-utils.c | 5 +++++\n t/t0060-path-utils.sh | 10 ++++++++++\n t/t7450-bad-dotgitx-files.sh | 26 +++++++++++++++++++-------\n utf8.c | 5 +++++\n utf8.h | 1 +\n 9 files changed, 58 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"416990","messageId":"YCsBRUQkrAm8l2gz@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCsBA002yv8XpppM@coredump.intra.peff.net","subject":"[PATCH 1/2] fsck: make symlinked .gitignore and .gitattributes a warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-15T23:18:29Z","receivedAt":"2021-02-15T23:19:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We recently added fsck checks to complain about symlinked .gitignore and\n.gitattributes files, which are no longer allowed to be checked out.\nThis is partially to inform fsck users of the problem, but also to\nprotect older clients from receiving them (by blocking push and fetch\nvia transfer.fsckObjects).\n\nWhile there are some minor security implications to having these files\nbe symlinks, this is out-weighed by the inconvenience of blocking\nhistorical commits in some projects that might include them.\n\nLet's loosen the fsck check to a warning. It will continue to be\nreported by both git-fsck and transfer.fsckObjects, but will not impact\nthe exit code or the acceptance of objects. Note that internally in\nfsck.c this is called \"INFO\", but the word \"warning\" will appear in\nuser-visible output.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 4 ++--\n t/t7450-bad-dotgitx-files.sh | 26 ++++++++++++++++++--------\n 2 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex d0a201348d..c75c7d7dc7 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -67,8 +67,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(GITMODULES_URL, ERROR) \\\n \tFUNC(GITMODULES_PATH, ERROR) \\\n \tFUNC(GITMODULES_UPDATE, ERROR) \\\n-\tFUNC(GITIGNORE_SYMLINK, ERROR) \\\n-\tFUNC(GITATTRIBUTES_SYMLINK, ERROR) \\\n \t/* warnings */ \\\n \tFUNC(BAD_FILEMODE, WARN) \\\n \tFUNC(EMPTY_NAME, WARN) \\\n@@ -81,6 +79,8 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(NUL_IN_COMMIT, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(GITMODULES_PARSE, INFO) \\\n+\tFUNC(GITIGNORE_SYMLINK, INFO) \\\n+\tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO) \\\n \t/* ignored (elevated when requested) */ \\\ndiff --git a/t/t7450-bad-dotgitx-files.sh b/t/t7450-bad-dotgitx-files.sh\nindex 326b34e167..4b1edb150e 100755\n--- a/t/t7450-bad-dotgitx-files.sh\n+++ b/t/t7450-bad-dotgitx-files.sh\n@@ -140,6 +140,16 @@ test_expect_success 'index-pack --strict works for non-repo pack' '\n '\n \n check_forbidden_symlink () {\n+\tfsck_must_fail=test_must_fail\n+\tfsck_prefix=error\n+\tcase \"$1\" in\n+\t--fsck-warning)\n+\t\tfsck_must_fail=\n+\t\tfsck_prefix=warning\n+\t\tshift\n+\t\t;;\n+\tesac\n+\n \tname=$1\n \ttype=$2\n \tpath=$3\n@@ -172,8 +182,8 @@ check_forbidden_symlink () {\n \n \t\t\t# Check not only that we fail, but that it is due to the\n \t\t\t# symlink detector\n-\t\t\ttest_must_fail git fsck 2>output &&\n-\t\t\ttest_i18ngrep \"tree $tree: ${name}Symlink\" output\n+\t\t\t$fsck_must_fail git fsck 2>output &&\n+\t\t\ttest_i18ngrep \"$fsck_prefix.*tree $tree: ${name}Symlink\" output\n \t\t)\n \t'\n \n@@ -193,13 +203,13 @@ check_forbidden_symlink gitmodules vanilla .gitmodules\n check_forbidden_symlink gitmodules ntfs \".gitmodules .\"\n check_forbidden_symlink gitmodules hfs \".${u200c}gitmodules\"\n \n-check_forbidden_symlink gitattributes vanilla .gitattributes\n-check_forbidden_symlink gitattributes ntfs \".gitattributes .\"\n-check_forbidden_symlink gitattributes hfs \".${u200c}gitattributes\"\n+check_forbidden_symlink --fsck-warning gitattributes vanilla .gitattributes\n+check_forbidden_symlink --fsck-warning gitattributes ntfs \".gitattributes .\"\n+check_forbidden_symlink --fsck-warning gitattributes hfs \".${u200c}gitattributes\"\n \n-check_forbidden_symlink gitignore vanilla .gitignore\n-check_forbidden_symlink gitignore ntfs \".gitignore .\"\n-check_forbidden_symlink gitignore hfs \".${u200c}gitignore\"\n+check_forbidden_symlink --fsck-warning gitignore vanilla .gitignore\n+check_forbidden_symlink --fsck-warning gitignore ntfs \".gitignore .\"\n+check_forbidden_symlink --fsck-warning gitignore hfs \".${u200c}gitignore\"\n \n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n-- \n2.30.1.986.gd86016a168\n\n"},{"id":"416991","messageId":"YCsBc6jap4qqtbcE@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCsBA002yv8XpppM@coredump.intra.peff.net","subject":"[PATCH 2/2] disallow symlinked .mailmap files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-15T23:19:15Z","receivedAt":"2021-02-15T23:20:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Symlinked .mailmap files do not work as users might expect and open up a\nsecurity concern:\n\n - when we read the files from a blob (e.g., in a bare repository as\n HEAD:.mailmap), we would not respect the symlink (and in fact would\n treat the symlink target as a mailmap entry, which is confusing at\n best).\n\n - if the symlink points outside of the working tree, this can be used\n to exfiltrate data from a service that can be convinced to check out\n an untrusted repo and run \"git shortlog\" or similar\n\nLet's give .mailmap the same treatment we gave to .gitignore and\n.gitattributes: it will be disallowed for checkout, and produce an fsck\nwarning (which can be upgraded in severity using config).\n\nNote that we're relying on our ntfs/hfs path-matching infrastructure\nhere. That was originally designed for .git*, but it should work in this\ninstance, too (the important thing is that our \"needle\" path is still\nplain ASCII, so we don't need to know the full range of case folds).\n\nThe NTFS short-name hash was generated using the perl script included in\ne7cb0b4455 (is_ntfs_dotgit: match other .git files, 2018-05-11). Though\nnote that I needed to use \"%04x\" when formatting the final hex, as the\nhash value for this file happens to begin with a 0.\n\nReported-by: Blake Burkhart \nSigned-off-by: Jeff King \n---\n cache.h | 1 +\n fsck.c | 6 ++++++\n path.c | 5 +++++\n read-cache.c | 6 ++++--\n t/helper/test-path-utils.c | 5 +++++\n t/t0060-path-utils.sh | 10 ++++++++++\n t/t7450-bad-dotgitx-files.sh | 4 ++++\n utf8.c | 5 +++++\n utf8.h | 1 +\n 9 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex d928149614..a29084e87c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1254,6 +1254,7 @@ int is_ntfs_dotgit(const char *name);\n int is_ntfs_dotgitmodules(const char *name);\n int is_ntfs_dotgitignore(const char *name);\n int is_ntfs_dotgitattributes(const char *name);\n+int is_ntfs_dotmailmap(const char *name);\n \n /*\n * Returns true iff \"str\" could be confused as a command-line option when\ndiff --git a/fsck.c b/fsck.c\nindex c75c7d7dc7..860af9cb5b 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -81,6 +81,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(GITMODULES_PARSE, INFO) \\\n \tFUNC(GITIGNORE_SYMLINK, INFO) \\\n \tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\n+\tFUNC(MAILMAP_SYMLINK, INFO) \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO) \\\n \t/* ignored (elevated when requested) */ \\\n@@ -703,6 +704,11 @@ static int fsck_tree(const struct object_id *tree_oid,\n \t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n \t\t\t\t\t\t FSCK_MSG_GITATTRIBUTES_SYMLINK,\n \t\t\t\t\t\t \".gitattributes is a symlink\");\n+\t\t\tif (is_hfs_dotmailmap(name) ||\n+\t\t\t is_ntfs_dotmailmap(name))\n+\t\t\t\tretval += report(options, tree_oid, OBJ_TREE,\n+\t\t\t\t\t\t FSCK_MSG_MAILMAP_SYMLINK,\n+\t\t\t\t\t\t \".mailmap is a symlink\");\n \t\t}\n \n \t\tif ((backslash = strchr(name, '\\\\'))) {\ndiff --git a/path.c b/path.c\nindex 7b385e5eb2..2c496d6161 100644\n--- a/path.c\n+++ b/path.c\n@@ -1493,6 +1493,11 @@ int is_ntfs_dotgitattributes(const char *name)\n \treturn is_ntfs_dot_str(name, \"gitattributes\", \"gi7d29\");\n }\n \n+int is_ntfs_dotmailmap(const char *name)\n+{\n+\treturn is_ntfs_dot_str(name, \"mailmap\", \"maba30\");\n+}\n+\n int looks_like_command_line_option(const char *str)\n {\n \treturn str && str[0] == '-';\ndiff --git a/read-cache.c b/read-cache.c\nindex dd17ebd236..322eddb874 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -984,7 +984,8 @@ int verify_path(const char *path, unsigned mode)\n \t\t\t\tif (S_ISLNK(mode)) {\n \t\t\t\t\tif (is_hfs_dotgitmodules(path) ||\n \t\t\t\t\t is_hfs_dotgitignore(path) ||\n-\t\t\t\t\t is_hfs_dotgitattributes(path))\n+\t\t\t\t\t is_hfs_dotgitattributes(path) ||\n+\t\t\t\t\t is_hfs_dotmailmap(path))\n \t\t\t\t\t\treturn 0;\n \t\t\t\t}\n \t\t\t}\n@@ -998,7 +999,8 @@ int verify_path(const char *path, unsigned mode)\n \t\t\t\tif (S_ISLNK(mode)) {\n \t\t\t\t\tif (is_ntfs_dotgitmodules(path) ||\n \t\t\t\t\t is_ntfs_dotgitignore(path) ||\n-\t\t\t\t\t is_ntfs_dotgitattributes(path))\n+\t\t\t\t\t is_ntfs_dotgitattributes(path) ||\n+\t\t\t\t\t is_ntfs_dotmailmap(path))\n \t\t\t\t\t\treturn 0;\n \t\t\t\t}\n \t\t\t}\ndiff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c\nindex 732372b97f..98044d53fa 100644\n--- a/t/helper/test-path-utils.c\n+++ b/t/helper/test-path-utils.c\n@@ -409,6 +409,11 @@ int cmd__path_utils(int argc, const char **argv)\n \t\t\t\t is_hfs_dotgitattributes,\n \t\t\t\t is_ntfs_dotgitattributes);\n \t}\n+\tif (argc > 2 && !strcmp(argv[1], \"is_dotmailmap\")) {\n+\t\treturn check_dotgitx(\"mailmap\", argv + 2,\n+\t\t\t\t is_hfs_dotmailmap,\n+\t\t\t\t is_ntfs_dotmailmap);\n+\t}\n \n \tif (argc > 2 && !strcmp(argv[1], \"file-size\")) {\n \t\tint res = 0, i;\ndiff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh\nindex fab077b6f7..de4960783f 100755\n--- a/t/t0060-path-utils.sh\n+++ b/t/t0060-path-utils.sh\n@@ -488,6 +488,16 @@ test_expect_success 'match .gitignore' '\n \t\tGI250A~1\n '\n \n+test_expect_success 'match .mailmap' '\n+\ttest-tool path-utils is_dotmailmap \\\n+\t\t.mailmap \\\n+\t\t.mail${u200c}map \\\n+\t\t.Mailmap \\\n+\t\t.mailmaP \\\n+\t\tMAILMA~1 \\\n+\t\tMABA30~1\n+'\n+\n test_expect_success MINGW 'is_valid_path() on Windows' '\n \ttest-tool path-utils is_valid_path \\\n \t\twin32 \\\ndiff --git a/t/t7450-bad-dotgitx-files.sh b/t/t7450-bad-dotgitx-files.sh\nindex 4b1edb150e..c12f0373fd 100755\n--- a/t/t7450-bad-dotgitx-files.sh\n+++ b/t/t7450-bad-dotgitx-files.sh\n@@ -211,6 +211,10 @@ check_forbidden_symlink --fsck-warning gitignore vanilla .gitignore\n check_forbidden_symlink --fsck-warning gitignore ntfs \".gitignore .\"\n check_forbidden_symlink --fsck-warning gitignore hfs \".${u200c}gitignore\"\n \n+check_forbidden_symlink --fsck-warning mailmap vanilla .mailmap\n+check_forbidden_symlink --fsck-warning mailmap ntfs \".mailmap .\"\n+check_forbidden_symlink --fsck-warning mailmap hfs \".${u200c}mailmap\"\n+\n test_expect_success 'fsck detects non-blob .gitmodules' '\n \tgit init non-blob &&\n \t(\ndiff --git a/utf8.c b/utf8.c\nindex 5b39361ada..de4ce5c0e6 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -777,6 +777,11 @@ int is_hfs_dotgitattributes(const char *path)\n \treturn is_hfs_dot_str(path, \"gitattributes\");\n }\n \n+int is_hfs_dotmailmap(const char *path)\n+{\n+\treturn is_hfs_dot_str(path, \"mailmap\");\n+}\n+\n const char utf8_bom[] = \"\\357\\273\\277\";\n \n int skip_utf8_bom(char **text, size_t len)\ndiff --git a/utf8.h b/utf8.h\nindex fcd5167baf..9a16c8679d 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -61,6 +61,7 @@ int is_hfs_dotgit(const char *path);\n int is_hfs_dotgitmodules(const char *path);\n int is_hfs_dotgitignore(const char *path);\n int is_hfs_dotgitattributes(const char *path);\n+int is_hfs_dotmailmap(const char *path);\n \n typedef enum {\n \tALIGN_LEFT,\n-- \n2.30.1.986.gd86016a168\n"},{"id":"416993","messageId":"87y2foaltl.fsf@evledraar.gmail.com","threadId":"55143","inReplyTo":"YCsBRUQkrAm8l2gz@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] fsck: make symlinked .gitignore and .gitattributes a warning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-16T00:38:30Z","receivedAt":"2021-02-16T00:39:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 16 2021, Jeff King wrote:\n\n> While there are some minor security implications to having these files\n> be symlinks, this is out-weighed by the inconvenience of blocking\n> historical commits in some projects that might include them.\n\nDigging up the relevant thread that's the projects noted at\nhttps://lore.kernel.org/git/20201027033518.GH2645313@google.com/ ?\n\nI cloned the openmrn.git repository noted there, and checkout dies with:\n\n error: invalid path 'applications/clinic_app/targets/linux.x86/.gitignore'\n fatal: Could not reset index file to revision 'HEAD'.\n\nI'm running a recent-ish snapshot of next at d98b1dd5eaa7, so with your\nverify_path() change in current \"seen\".\n\nSo this series changes nothing about the checkout, just the fsck check?\n\nI see there's your\nhttps://lore.kernel.org/git/20201027075853.GH3005508@coredump.intra.peff.net/#t\nto improve the !!symlink() codepath in apply.c\n\nStill, it seems like a rather jarring gap in implementation to just warn\nabout this in fsck for the benefit of e.g. server operations, but then\nhard die on the current client.\n\nThere seems to be no way around that hard die, and both repos in that\nreport are ones that are just symlinking .gitignore to a\n../somedir/.gitignore deep in their own tree.\n\nSo aren't we both making the fsck check too loose and the client too\nstrict? Would anyone care if this was an error on fsck if we did the \"is\noutside repo?\" check?\n"},{"id":"416994","messageId":"YCsc0OePtrotjeg5@coredump.intra.peff.net","threadId":"55143","inReplyTo":"87y2foaltl.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] fsck: make symlinked .gitignore and .gitattributes a warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T01:16:00Z","receivedAt":"2021-02-16T01:16:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 01:38:30AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Feb 16 2021, Jeff King wrote:\n> \n> > While there are some minor security implications to having these files\n> > be symlinks, this is out-weighed by the inconvenience of blocking\n> > historical commits in some projects that might include them.\n> \n> Digging up the relevant thread that's the projects noted at\n> https://lore.kernel.org/git/20201027033518.GH2645313@google.com/ ?\n> \n> I cloned the openmrn.git repository noted there, and checkout dies with:\n> \n> error: invalid path 'applications/clinic_app/targets/linux.x86/.gitignore'\n> fatal: Could not reset index file to revision 'HEAD'.\n> \n> I'm running a recent-ish snapshot of next at d98b1dd5eaa7, so with your\n> verify_path() change in current \"seen\".\n> \n> So this series changes nothing about the checkout, just the fsck check?\n\nRight.\n\n> I see there's your\n> https://lore.kernel.org/git/20201027075853.GH3005508@coredump.intra.peff.net/#t\n> to improve the !!symlink() codepath in apply.c\n\nThat's a somewhat orthogonal approach, that tries to look at out-of-tree\nsymlinks (rather than banning all symlinks for these files).\n\nI think it's worth banning them all, though; they don't actually work\nas you'd expect (i.e., whenever we read them in-repo the symlinks do\nnothing).\n\n> Still, it seems like a rather jarring gap in implementation to just warn\n> about this in fsck for the benefit of e.g. server operations, but then\n> hard die on the current client.\n\nIt's not for the benefits of servers. It's because the solution for them\nis to stop symlinking, which fixes clone/checkout of new commits. But\nthey'll still have those old trees hanging around in their history. If\neverybody rejects them, then it becomes difficult to push/fetch at all.\n\nThat said, they'd probably want to checkout those old commits, too. So\nwe probably do need a config override, even if it's a broad one (\"trust\nme, this repo is OK, just allow symlinks for these special files\").\n\n> There seems to be no way around that hard die, and both repos in that\n> report are ones that are just symlinking .gitignore to a\n> ../somedir/.gitignore deep in their own tree.\n> \n> So aren't we both making the fsck check too loose and the client too\n> strict? Would anyone care if this was an error on fsck if we did the \"is\n> outside repo?\" check?\n\nAn outside-the-repo check would probably be less invasive, but:\n\n - it still allows broken setups\n\n - it's significantly more complex. I know that the implementation I\n showed errs on the side of complaining in at least some cases\n (because it doesn't know if intermediate paths are themselves\n symlinks). But I'd worry there are also cases where it covers too\n little, nullifying the protection.\n\n-Peff\n"},{"id":"416998","messageId":"xmqqtuqcsrkt.fsf@gitster.c.googlers.com","threadId":"55143","inReplyTo":"YCsc0OePtrotjeg5@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] fsck: make symlinked .gitignore and .gitattributes a warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-16T01:56:50Z","receivedAt":"2021-02-16T01:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That said, they'd probably want to checkout those old commits, too. So\n> we probably do need a config override, even if it's a broad one (\"trust\n> me, this repo is OK, just allow symlinks for these special files\").\n\nIs this about the check that is overly strict for some existing\nprojects that kept the jk/symlinked-dotgitx-files topic in the\n'seen' so far?\n\nOn the fsck end, we know we can demote the error level per\nrepository, but I wonder if we should make checkout/clone honor the\nsame setting?\n\nI think GITMODULES_SYMLINK has been there for quite some time at\n\"error\" level and we do want to discourage it to be a symbolic link,\nso I am not quite sure what the demoting of these two achieves. Why\naren't we having a similar issue on .gitmodules that is a symbolic\nlink?\n\nThanks.\n\n"},{"id":"417043","messageId":"YCu/FoLl8p15mwio@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCsc0OePtrotjeg5@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] fsck: make symlinked .gitignore and .gitattributes a warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T12:48:22Z","receivedAt":"2021-02-16T12:49:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2021 at 08:16:00PM -0500, Jeff King wrote:\n\n> That said, they'd probably want to checkout those old commits, too. So\n> we probably do need a config override, even if it's a broad one (\"trust\n> me, this repo is OK, just allow symlinks for these special files\").\n\nAnother option here is setting core.symlinks to false. That works more\nbroadly than just the one symlink, though.\n\nIt might be possible to apply that same setting (perhaps automatically,\neven) to just these .gitattributes, etc, metafiles. That may get tricky,\nthough, as we'd need to do it not just on checkout, but any time we're\nconsidering the file (because we wouldn't want \"git add\" to re-add it as\na non-symlink, nor git-diff to report it, etc).\n\n> > So aren't we both making the fsck check too loose and the client too\n> > strict? Would anyone care if this was an error on fsck if we did the \"is\n> > outside repo?\" check?\n> \n> An outside-the-repo check would probably be less invasive, but:\n> \n> - it still allows broken setups\n> \n> - it's significantly more complex. I know that the implementation I\n> showed errs on the side of complaining in at least some cases\n> (because it doesn't know if intermediate paths are themselves\n> symlinks). But I'd worry there are also cases where it covers too\n> little, nullifying the protection.\n\nAdding to the \"complexity\" point: it's also impossible to implement via\nfsck, where we do not have the full path of the tree entry. We could\nlive without the fsck support if need be, though.\n\nI am beginning to wonder if just opening them all with O_NOFOLLOW (and a\nhacky 2-syscall fallback for portability) might be less ugly than all of\nthis.\n\n-Peff\n"},{"id":"417045","messageId":"YCvAmkO8ZQnlp4m9@coredump.intra.peff.net","threadId":"55143","inReplyTo":"xmqqtuqcsrkt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] fsck: make symlinked .gitignore and .gitattributes a warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T12:54:50Z","receivedAt":"2021-02-16T12:55:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2021 at 05:56:50PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > That said, they'd probably want to checkout those old commits, too. So\n> > we probably do need a config override, even if it's a broad one (\"trust\n> > me, this repo is OK, just allow symlinks for these special files\").\n> \n> Is this about the check that is overly strict for some existing\n> projects that kept the jk/symlinked-dotgitx-files topic in the\n> 'seen' so far?\n\nYes.\n\n> On the fsck end, we know we can demote the error level per\n> repository, but I wonder if we should make checkout/clone honor the\n> same setting?\n\nWhat would the default be? If it's permissive, then it feels like we are\nnot really solving much, as anybody who wanted to be careful can already\ninspect the tree contents. This is about avoiding surprises in the\ndefault config.\n\nIf it's to forbid by default, then yes, I think the \"trust me this repo\nis OK\" I gave above would be a viable path forward.\n\n> I think GITMODULES_SYMLINK has been there for quite some time at\n> \"error\" level and we do want to discourage it to be a symbolic link,\n> so I am not quite sure what the demoting of these two achieves. Why\n> aren't we having a similar issue on .gitmodules that is a symbolic\n> link?\n\nI think it's just less common to have symlinked .gitmodules. To be\nclear, I think symlinked .gitignore is also pretty uncommon. Back when\nwe discussed this originally in 2018 I scanned most of GitHub and came\nup with only a handful of repositories that did so.\n\n-Peff\n"},{"id":"417055","messageId":"YCvaJg8o882IqNnx@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCu/FoLl8p15mwio@coredump.intra.peff.net","subject":"[PATCH 0/6] open in-tree files with O_NOFOLLOW","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:43:50Z","receivedAt":"2021-02-16T14:44:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 07:48:23AM -0500, Jeff King wrote:\n\n> I am beginning to wonder if just opening them all with O_NOFOLLOW (and a\n> hacky 2-syscall fallback for portability) might be less ugly than all of\n> this.\n\nSo here's what that series might look like. It would replace all of this\nverify_path() stuff entirely (and fsck, though we might want to add\ndetection to fsck just as an informational thing). It gives similar\nprotections, and would similarly force people using an in-tree symlink\nto stop doing that. But it makes it much less of a pain to do so,\nbecause they can still check out, etc; the symlinks just won't be\nfollowed.\n\nI think we could even use the same technique to roll back the\nrestrictions on .gitmodules being a symlink. That one makes me a bit\nmore nervous, just because we also write it. I _think_ that might be\nsafe, because we only do so using a temp file and rename(), which should\nreplace the symlink.\n\n [1/6]: add open_nofollow() helper\n [2/6]: attr: convert \"macro_ok\" into a flags field\n [3/6]: exclude: add flags parameter to add_patterns()\n [4/6]: attr: do not respect symlinks for in-tree .gitattributes\n [5/6]: exclude: do not respect symlinks for in-tree .gitignore\n [6/6]: mailmap: do not respect symlinks for in-tree .mailmap\n\n attr.c | 60 +++++++++++++++++++++++++--------------\n builtin/sparse-checkout.c | 8 +++---\n dir.c | 21 ++++++++++----\n dir.h | 3 +-\n git-compat-util.h | 7 +++++\n mailmap.c | 22 ++++++++++----\n t/t0003-attributes.sh | 36 +++++++++++++++++++++--\n t/t0008-ignores.sh | 34 ++++++++++++++++++++++\n t/t4203-mailmap.sh | 31 ++++++++++++++++++++\n wrapper.c | 16 +++++++++++\n 10 files changed, 197 insertions(+), 41 deletions(-)\n\n-Peff\n"},{"id":"417056","messageId":"YCvaRpQs99TBaCrQ@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvaJg8o882IqNnx@coredump.intra.peff.net","subject":"[PATCH 1/6] add open_nofollow() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:44:22Z","receivedAt":"2021-02-16T14:45:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some callers of open() would like to use O_NOFOLLOW, but it is not\navailable on all platforms. Let's abstract this into a helper function\nso we can provide system-specific implementations.\n\nSome light web-searching reveals that we might be able to get something\nsimilar on Windows using FILE_FLAG_OPEN_REPARSE_POINT. I didn't dig into\nthis further.\n\nFor other systems without O_NOFOLLOW or any equivalent, we have two\noptions for fallback:\n\n - we can just open anyway, following symlinks; this may have security\n implications (e.g., following untrusted in-tree symlinks)\n\n - we can determine whether the path is a symlink with lstat().\n\n This is slower (two syscalls instead of one), but that may be\n acceptable for infrequent uses like looking up .gitattributes files\n (especially because we can get away with a single syscall for the\n common case of ENOENT).\n\n It's also racy, but should be sufficient for our needs (we are\n worried about in-tree symlinks that we ourselves would have\n previously created). We could make it non-racy at the cost of making\n it even slower, by doing an fstat() on the opened descriptor and\n comparing the dev/ino fields to the original lstat().\n\nThis patch implements the lstat() option in its slightly-faster racy\nform.\n\nSigned-off-by: Jeff King \n---\n git-compat-util.h | 7 +++++++\n wrapper.c | 16 ++++++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 838246289c..fd99eaeb6d 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1231,6 +1231,13 @@ int access_or_die(const char *path, int mode, unsigned flag);\n /* Warn on an inaccessible file if errno indicates this is an error */\n int warn_on_fopen_errors(const char *path);\n \n+/*\n+ * Open with O_NOFOLLOW, or equivalent. Note that the fallback equivalent\n+ * may be racy. Do not use this as protection against an attacker who can\n+ * simultaneously create paths.\n+ */\n+int open_nofollow(const char *path, int flags);\n+\n #if !defined(USE_PARENS_AROUND_GETTEXT_N) && defined(__GNUC__)\n #define USE_PARENS_AROUND_GETTEXT_N 1\n #endif\ndiff --git a/wrapper.c b/wrapper.c\nindex bcda41e374..563ad590df 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -678,3 +678,19 @@ int is_empty_or_missing_file(const char *filename)\n \n \treturn !st.st_size;\n }\n+\n+int open_nofollow(const char *path, int flags)\n+{\n+#ifdef O_NOFOLLOW\n+\treturn open(path, flags | O_NOFOLLOW);\n+#else\n+\tstruct stat st;\n+\tif (lstat(path, &st) < 0)\n+\t\treturn -1;\n+\tif (S_ISLNK(st.st_mode)) {\n+\t\terrno = ELOOP;\n+\t\treturn -1;\n+\t}\n+\treturn open(path, flags);\n+#endif\n+}\n-- \n2.30.1.986.gd86016a168\n\n"},{"id":"417057","messageId":"YCvaSeHIEF3yNkVI@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvaJg8o882IqNnx@coredump.intra.peff.net","subject":"[PATCH 2/6] attr: convert \"macro_ok\" into a flags field","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:44:25Z","receivedAt":"2021-02-16T14:45:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The attribute code can have a rather deep callstack, through\nwhich we have to pass the \"macro_ok\" flag. In anticipation\nof adding other flags, let's convert this to a generic\nbit-field.\n\nSigned-off-by: Jeff King \n---\n attr.c | 43 ++++++++++++++++++++++++-------------------\n 1 file changed, 24 insertions(+), 19 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 4ef85d668b..7b0cab085f 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -278,6 +278,9 @@ struct match_attr {\n \n static const char blank[] = \" \\t\\r\\n\";\n \n+/* Flags usable in read_attr() and parse_attr_line() family of functions. */\n+#define READ_ATTR_MACRO_OK (1<<0)\n+\n /*\n * Parse a whitespace-delimited attribute state (i.e., \"attr\",\n * \"-attr\", \"!attr\", or \"attr=value\") from the string starting at src.\n@@ -331,7 +334,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n }\n \n static struct match_attr *parse_attr_line(const char *line, const char *src,\n-\t\t\t\t\t int lineno, int macro_ok)\n+\t\t\t\t\t int lineno, unsigned flags)\n {\n \tint namelen;\n \tint num_attr, i;\n@@ -355,7 +358,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \n \tif (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&\n \t starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {\n-\t\tif (!macro_ok) {\n+\t\tif (!(flags & READ_ATTR_MACRO_OK)) {\n \t\t\tfprintf_ln(stderr, _(\"%s not allowed: %s:%d\"),\n \t\t\t\t name, src, lineno);\n \t\t\tgoto fail_return;\n@@ -653,11 +656,11 @@ static void handle_attr_line(struct attr_stack *res,\n \t\t\t const char *line,\n \t\t\t const char *src,\n \t\t\t int lineno,\n-\t\t\t int macro_ok)\n+\t\t\t unsigned flags)\n {\n \tstruct match_attr *a;\n \n-\ta = parse_attr_line(line, src, lineno, macro_ok);\n+\ta = parse_attr_line(line, src, lineno, flags);\n \tif (!a)\n \t\treturn;\n \tALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);\n@@ -672,7 +675,8 @@ static struct attr_stack *read_attr_from_array(const char **list)\n \n \tres = xcalloc(1, sizeof(*res));\n \twhile ((line = *(list++)) != NULL)\n-\t\thandle_attr_line(res, line, \"[builtin]\", ++lineno, 1);\n+\t\thandle_attr_line(res, line, \"[builtin]\", ++lineno,\n+\t\t\t\t READ_ATTR_MACRO_OK);\n \treturn res;\n }\n \n@@ -698,7 +702,7 @@ void git_attr_set_direction(enum git_attr_direction new_direction)\n \tdirection = new_direction;\n }\n \n-static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n+static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n {\n \tFILE *fp = fopen_or_warn(path, \"r\");\n \tstruct attr_stack *res;\n@@ -712,15 +716,15 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n \t\tchar *bufp = buf;\n \t\tif (!lineno)\n \t\t\tskip_utf8_bom(&bufp, strlen(bufp));\n-\t\thandle_attr_line(res, bufp, path, ++lineno, macro_ok);\n+\t\thandle_attr_line(res, bufp, path, ++lineno, flags);\n \t}\n \tfclose(fp);\n \treturn res;\n }\n \n static struct attr_stack *read_attr_from_index(const struct index_state *istate,\n \t\t\t\t\t const char *path,\n-\t\t\t\t\t int macro_ok)\n+\t\t\t\t\t unsigned flags)\n {\n \tstruct attr_stack *res;\n \tchar *buf, *sp;\n@@ -741,35 +745,35 @@ static struct attr_stack *read_attr_from_index(const struct index_state *istate,\n \t\tep = strchrnul(sp, '\\n');\n \t\tmore = (*ep == '\\n');\n \t\t*ep = '\\0';\n-\t\thandle_attr_line(res, sp, path, ++lineno, macro_ok);\n+\t\thandle_attr_line(res, sp, path, ++lineno, flags);\n \t\tsp = ep + more;\n \t}\n \tfree(buf);\n \treturn res;\n }\n \n static struct attr_stack *read_attr(const struct index_state *istate,\n-\t\t\t\t const char *path, int macro_ok)\n+\t\t\t\t const char *path, unsigned flags)\n {\n \tstruct attr_stack *res = NULL;\n \n \tif (direction == GIT_ATTR_INDEX) {\n-\t\tres = read_attr_from_index(istate, path, macro_ok);\n+\t\tres = read_attr_from_index(istate, path, flags);\n \t} else if (!is_bare_repository()) {\n \t\tif (direction == GIT_ATTR_CHECKOUT) {\n-\t\t\tres = read_attr_from_index(istate, path, macro_ok);\n+\t\t\tres = read_attr_from_index(istate, path, flags);\n \t\t\tif (!res)\n-\t\t\t\tres = read_attr_from_file(path, macro_ok);\n+\t\t\t\tres = read_attr_from_file(path, flags);\n \t\t} else if (direction == GIT_ATTR_CHECKIN) {\n-\t\t\tres = read_attr_from_file(path, macro_ok);\n+\t\t\tres = read_attr_from_file(path, flags);\n \t\t\tif (!res)\n \t\t\t\t/*\n \t\t\t\t * There is no checked out .gitattributes file\n \t\t\t\t * there, but we might have it in the index.\n \t\t\t\t * We allow operation in a sparsely checked out\n \t\t\t\t * work tree, so read from it.\n \t\t\t\t */\n-\t\t\t\tres = read_attr_from_index(istate, path, macro_ok);\n+\t\t\t\tres = read_attr_from_index(istate, path, flags);\n \t\t}\n \t}\n \n@@ -844,6 +848,7 @@ static void bootstrap_attr_stack(const struct index_state *istate,\n \t\t\t\t struct attr_stack **stack)\n {\n \tstruct attr_stack *e;\n+\tunsigned flags = READ_ATTR_MACRO_OK;\n \n \tif (*stack)\n \t\treturn;\n@@ -854,23 +859,23 @@ static void bootstrap_attr_stack(const struct index_state *istate,\n \n \t/* system-wide frame */\n \tif (git_attr_system()) {\n-\t\te = read_attr_from_file(git_etc_gitattributes(), 1);\n+\t\te = read_attr_from_file(git_etc_gitattributes(), flags);\n \t\tpush_stack(stack, e, NULL, 0);\n \t}\n \n \t/* home directory */\n \tif (get_home_gitattributes()) {\n-\t\te = read_attr_from_file(get_home_gitattributes(), 1);\n+\t\te = read_attr_from_file(get_home_gitattributes(), flags);\n \t\tpush_stack(stack, e, NULL, 0);\n \t}\n \n \t/* root directory */\n-\te = read_attr(istate, GITATTRIBUTES_FILE, 1);\n+\te = read_attr(istate, GITATTRIBUTES_FILE, flags);\n \tpush_stack(stack, e, xstrdup(\"\"), 0);\n \n \t/* info frame */\n \tif (startup_info->have_repository)\n-\t\te = read_attr_from_file(git_path_info_attributes(), 1);\n+\t\te = read_attr_from_file(git_path_info_attributes(), flags);\n \telse\n \t\te = NULL;\n \tif (!e)\n-- \n2.30.1.986.gd86016a168\n\n"},{"id":"417058","messageId":"YCvaUHnf6eFll++h@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvaJg8o882IqNnx@coredump.intra.peff.net","subject":"[PATCH 4/6] attr: do not respect symlinks for in-tree .gitattributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:44:32Z","receivedAt":"2021-02-16T14:45:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The attributes system may sometimes read in-tree files from the\nfilesystem, and sometimes from the index. In the latter case, we do not\nresolve symbolic links (and are not likely to ever start doing so).\nLet's open filesystem links with O_NOFOLLOW so that the two cases behave\nconsistently.\n\nAs a bonus, this means that git will not follow such symlinks to read\nand parse out-of-tree paths. In some cases this could have security\nimplications, as a malicious repository can cause Git to open and read\narbitrary files. It could already feed arbitrary content to the parser,\nbut in certain setups it might be able to exfiltrate data from those\npaths (e.g., if an automated service operating on the malicious repo\nreveals its stderr to an attacker).\n\nNote that O_NOFOLLOW only prevents following links for the path itself,\nnot intermediate directories in the path. At first glance, it seems\nlike\n\n ln -s /some/path in-repo\n\nmight still look at \"in-repo/.gitattributes\", following the symlink to\n\"/some/path/.gitattributes\". However, if \"in-repo\" is a symbolic link,\nthen we know that it has no git paths below it, and will never look at\nits .gitattributes file.\n\nWe will continue to support out-of-tree symbolic links (e.g., in\n$GIT_DIR/info/attributes); this just affects in-tree links. When a\nsymbolic link is encountered, the contents are ignored and a warning is\nprinted. POSIX specifies ELOOP in this case, so the user would generally\nsee something like:\n\n warning: unable to access '.gitattributes': Too many levels of symbolic links\n\nSigned-off-by: Jeff King \n---\n attr.c | 19 +++++++++++++++----\n t/t0003-attributes.sh | 36 +++++++++++++++++++++++++++++++++---\n 2 files changed, 48 insertions(+), 7 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7b0cab085f..a28177915b 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -280,6 +280,7 @@ static const char blank[] = \" \\t\\r\\n\";\n \n /* Flags usable in read_attr() and parse_attr_line() family of functions. */\n #define READ_ATTR_MACRO_OK (1<<0)\n+#define READ_ATTR_NOFOLLOW (1<<1)\n \n /*\n * Parse a whitespace-delimited attribute state (i.e., \"attr\",\n@@ -704,13 +705,23 @@ void git_attr_set_direction(enum git_attr_direction new_direction)\n \n static struct attr_stack *read_attr_from_file(const char *path, unsigned flags)\n {\n-\tFILE *fp = fopen_or_warn(path, \"r\");\n+\tint fd;\n+\tFILE *fp;\n \tstruct attr_stack *res;\n \tchar buf[2048];\n \tint lineno = 0;\n \n-\tif (!fp)\n+\tif (flags & READ_ATTR_NOFOLLOW)\n+\t\tfd = open_nofollow(path, O_RDONLY);\n+\telse\n+\t\tfd = open(path, O_RDONLY);\n+\n+\tif (fd < 0) {\n+\t\twarn_on_fopen_errors(path);\n \t\treturn NULL;\n+\t}\n+\tfp = xfdopen(fd, \"r\");\n+\n \tres = xcalloc(1, sizeof(*res));\n \twhile (fgets(buf, sizeof(buf), fp)) {\n \t\tchar *bufp = buf;\n@@ -870,7 +881,7 @@ static void bootstrap_attr_stack(const struct index_state *istate,\n \t}\n \n \t/* root directory */\n-\te = read_attr(istate, GITATTRIBUTES_FILE, flags);\n+\te = read_attr(istate, GITATTRIBUTES_FILE, flags | READ_ATTR_NOFOLLOW);\n \tpush_stack(stack, e, xstrdup(\"\"), 0);\n \n \t/* info frame */\n@@ -961,7 +972,7 @@ static void prepare_attr_stack(const struct index_state *istate,\n \t\tstrbuf_add(&pathbuf, path + pathbuf.len, (len - pathbuf.len));\n \t\tstrbuf_addf(&pathbuf, \"/%s\", GITATTRIBUTES_FILE);\n \n-\t\tnext = read_attr(istate, pathbuf.buf, 0);\n+\t\tnext = read_attr(istate, pathbuf.buf, READ_ATTR_NOFOLLOW);\n \n \t\t/* reset the pathbuf to not include \"/.gitattributes\" */\n \t\tstrbuf_setlen(&pathbuf, len);\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex b660593c20..1e4c672b84 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -4,12 +4,16 @@ test_description=gitattributes\n \n . ./test-lib.sh\n \n-attr_check () {\n+attr_check_basic () {\n \tpath=\"$1\" expect=\"$2\" git_opts=\"$3\" &&\n \n \tgit $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \techo \"$path: test: $expect\" >expect &&\n-\ttest_cmp expect actual &&\n+\ttest_cmp expect actual\n+}\n+\n+attr_check () {\n+\tattr_check_basic \"$@\" &&\n \ttest_must_be_empty err\n }\n \n@@ -331,12 +335,38 @@ test_expect_success 'binary macro expanded by -a' '\n \ttest_cmp expect actual\n '\n \n-\n test_expect_success 'query binary macro directly' '\n \techo \"file binary\" >.gitattributes &&\n \techo file: binary: set >expect &&\n \tgit check-attr binary file >actual &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success SYMLINKS 'set up symlink tests' '\n+\techo \"* test\" >attr &&\n+\trm -f .gitattributes\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in core.attributesFile' '\n+\ttest_when_finished \"rm symlink\" &&\n+\tln -s attr symlink &&\n+\ttest_config core.attributesFile \"$(pwd)/symlink\" &&\n+\tattr_check file set\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in info/attributes' '\n+\ttest_when_finished \"rm .git/info/attributes\" &&\n+\tln -s ../../attr .git/info/attributes &&\n+\tattr_check file set\n+'\n+\n+test_expect_success SYMLINKS 'symlinks not respected in-tree' '\n+\ttest_when_finished \"rm -rf .gitattributes subdir\" &&\n+\tln -s attr .gitattributes &&\n+\tmkdir subdir &&\n+\tln -s ../attr subdir/.gitattributes &&\n+\tattr_check_basic subdir/file unspecified &&\n+\ttest_i18ngrep \"unable to access.*gitattributes\" err\n+'\n+\n test_done\n-- \n2.30.1.986.gd86016a168\n\n"},{"id":"417059","messageId":"YCvaUukNiliIXHgq@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvaJg8o882IqNnx@coredump.intra.peff.net","subject":"[PATCH 5/6] exclude: do not respect symlinks for in-tree .gitignore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:44:34Z","receivedAt":"2021-02-16T14:45:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As with .gitattributes, we would like to make sure that .gitignore files\nare handled consistently whether read from the index or from the\nfilesystem. Likewise, we would like to avoid reading out-of-tree files\npointed to by the symlinks, which could have security implications in\ncertain setups.\n\nWe can cover both by using open_nofollow() when opening the in-tree\nfiles. We'll continue to follow links for core.excludesFile, as well as\n$GIT_DIR/info/exclude.\n\nSigned-off-by: Jeff King \n---\n dir.c | 12 ++++++++++--\n t/t0008-ignores.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex f7fb1db718..3692a28186 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1035,6 +1035,9 @@ static int add_patterns_from_buffer(char *buf, size_t size,\n \t\t\t\t const char *base, int baselen,\n \t\t\t\t struct pattern_list *pl);\n \n+/* Flags for add_patterns() */\n+#define PATTERN_NOFOLLOW (1<<0)\n+\n /*\n * Given a file with name \"fname\", read it (either from disk, or from\n * an index if 'istate' is non-null), parse it and store the\n@@ -1054,7 +1057,11 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \tsize_t size = 0;\n \tchar *buf;\n \n-\tfd = open(fname, O_RDONLY);\n+\tif (flags & PATTERN_NOFOLLOW)\n+\t\tfd = open_nofollow(fname, O_RDONLY);\n+\telse\n+\t\tfd = open(fname, O_RDONLY);\n+\n \tif (fd < 0 || fstat(fd, &st) < 0) {\n \t\tif (fd < 0)\n \t\t\twarn_on_fopen_errors(fname);\n@@ -1558,7 +1565,8 @@ static void prep_exclude(struct dir_struct *dir,\n \t\t\tstrbuf_addbuf(&sb, &dir->basebuf);\n \t\t\tstrbuf_addstr(&sb, dir->exclude_per_dir);\n \t\t\tpl->src = strbuf_detach(&sb, NULL);\n-\t\t\tadd_patterns(pl->src, pl->src, stk->baselen, pl, istate, 0,\n+\t\t\tadd_patterns(pl->src, pl->src, stk->baselen, pl, istate,\n+\t\t\t\t PATTERN_NOFOLLOW,\n \t\t\t\t untracked ? &oid_stat : NULL);\n \t\t}\n \t\t/*\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 370a389e5c..854cfda11f 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -865,4 +865,38 @@ test_expect_success 'info/exclude trumps core.excludesfile' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SYMLINKS 'set up ignore file for symlink tests' '\n+\techo \"*\" >ignore &&\n+\trm -f .gitignore .git/info/exclude\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in core.excludesFile' '\n+\ttest_when_finished \"rm symlink\" &&\n+\tln -s ignore symlink &&\n+\ttest_config core.excludesFile \"$(pwd)/symlink\" &&\n+\techo file >expect &&\n+\tgit check-ignore file >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in info/exclude' '\n+\ttest_when_finished \"rm .git/info/exclude\" &&\n+\tln -s ../../ignore .git/info/exclude &&\n+\techo file >expect &&\n+\tgit check-ignore file >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err\n+'\n+\n+test_expect_success SYMLINKS 'symlinks not respected in-tree' '\n+\ttest_when_finished \"rm .gitignore\" &&\n+\tln -s ignore .gitignore &&\n+\tmkdir subdir &&\n+\tln -s ignore subdir/.gitignore &&\n+\ttest_must_fail git check-ignore subdir/file >actual 2>err &&\n+\ttest_must_be_empty actual &&\n+\ttest_i18ngrep \"unable to access.*gitignore\" err\n+'\n+\n test_done\n-- \n2.30.1.986.gd86016a168\n\n"},{"id":"417060","messageId":"YCvaTJjma1Mi29pz@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvaJg8o882IqNnx@coredump.intra.peff.net","subject":"[PATCH 3/6] exclude: add flags parameter to add_patterns()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:44:28Z","receivedAt":"2021-02-16T14:45:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are a number of callers of add_patterns() and its sibling\nfunctions. Let's give them a \"flags\" parameter for adding new options\nwithout having to touch each caller. We'll use this in a future patch to\nadd O_NOFOLLOW support. But for now each caller just passes 0.\n\nSigned-off-by: Jeff King \n---\n builtin/sparse-checkout.c | 8 ++++----\n dir.c | 13 +++++++------\n dir.h | 3 ++-\n 3 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 2306a9ad98..d7da50ada5 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -64,7 +64,7 @@ static int sparse_checkout_list(int argc, const char **argv)\n \tpl.use_cone_patterns = core_sparse_checkout_cone;\n \n \tsparse_filename = get_sparse_checkout_filename();\n-\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, &pl, NULL);\n+\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, &pl, NULL, 0);\n \tfree(sparse_filename);\n \n \tif (res < 0) {\n@@ -321,7 +321,7 @@ static int sparse_checkout_init(int argc, const char **argv)\n \tmemset(&pl, 0, sizeof(pl));\n \n \tsparse_filename = get_sparse_checkout_filename();\n-\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, &pl, NULL);\n+\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, &pl, NULL, 0);\n \n \t/* If we already have a sparse-checkout file, use it. */\n \tif (res >= 0) {\n@@ -483,7 +483,7 @@ static void add_patterns_cone_mode(int argc, const char **argv,\n \texisting.use_cone_patterns = core_sparse_checkout_cone;\n \n \tif (add_patterns_from_file_to_list(sparse_filename, \"\", 0,\n-\t\t\t\t\t &existing, NULL))\n+\t\t\t\t\t &existing, NULL, 0))\n \t\tdie(_(\"unable to load existing sparse-checkout patterns\"));\n \tfree(sparse_filename);\n \n@@ -507,7 +507,7 @@ static void add_patterns_literal(int argc, const char **argv,\n {\n \tchar *sparse_filename = get_sparse_checkout_filename();\n \tif (add_patterns_from_file_to_list(sparse_filename, \"\", 0,\n-\t\t\t\t\t pl, NULL))\n+\t\t\t\t\t pl, NULL, 0))\n \t\tdie(_(\"unable to load existing sparse-checkout patterns\"));\n \tfree(sparse_filename);\n \tadd_patterns_from_input(pl, argc, argv);\ndiff --git a/dir.c b/dir.c\nindex d153a63bbd..f7fb1db718 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1046,7 +1046,7 @@ static int add_patterns_from_buffer(char *buf, size_t size,\n */\n static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t\tstruct pattern_list *pl, struct index_state *istate,\n-\t\t\tstruct oid_stat *oid_stat)\n+\t\t\tunsigned flags, struct oid_stat *oid_stat)\n {\n \tstruct stat st;\n \tint r;\n@@ -1143,9 +1143,10 @@ static int add_patterns_from_buffer(char *buf, size_t size,\n \n int add_patterns_from_file_to_list(const char *fname, const char *base,\n \t\t\t\t int baselen, struct pattern_list *pl,\n-\t\t\t\t struct index_state *istate)\n+\t\t\t\t struct index_state *istate,\n+\t\t\t\t unsigned flags)\n {\n-\treturn add_patterns(fname, base, baselen, pl, istate, NULL);\n+\treturn add_patterns(fname, base, baselen, pl, istate, flags, NULL);\n }\n \n int add_patterns_from_blob_to_list(\n@@ -1194,7 +1195,7 @@ static void add_patterns_from_file_1(struct dir_struct *dir, const char *fname,\n \tif (!dir->untracked)\n \t\tdir->unmanaged_exclude_files++;\n \tpl = add_pattern_list(dir, EXC_FILE, fname);\n-\tif (add_patterns(fname, \"\", 0, pl, NULL, oid_stat) < 0)\n+\tif (add_patterns(fname, \"\", 0, pl, NULL, 0, oid_stat) < 0)\n \t\tdie(_(\"cannot use %s as an exclude file\"), fname);\n }\n \n@@ -1557,7 +1558,7 @@ static void prep_exclude(struct dir_struct *dir,\n \t\t\tstrbuf_addbuf(&sb, &dir->basebuf);\n \t\t\tstrbuf_addstr(&sb, dir->exclude_per_dir);\n \t\t\tpl->src = strbuf_detach(&sb, NULL);\n-\t\t\tadd_patterns(pl->src, pl->src, stk->baselen, pl, istate,\n+\t\t\tadd_patterns(pl->src, pl->src, stk->baselen, pl, istate, 0,\n \t\t\t\t untracked ? &oid_stat : NULL);\n \t\t}\n \t\t/*\n@@ -3009,7 +3010,7 @@ int get_sparse_checkout_patterns(struct pattern_list *pl)\n \tchar *sparse_filename = get_sparse_checkout_filename();\n \n \tpl->use_cone_patterns = core_sparse_checkout_cone;\n-\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, pl, NULL);\n+\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, pl, NULL, 0);\n \n \tfree(sparse_filename);\n \treturn res;\ndiff --git a/dir.h b/dir.h\nindex facfae4740..04d886cfce 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -420,7 +420,8 @@ int hashmap_contains_parent(struct hashmap *map,\n struct pattern_list *add_pattern_list(struct dir_struct *dir,\n \t\t\t\t int group_type, const char *src);\n int add_patterns_from_file_to_list(const char *fname, const char *base, int baselen,\n-\t\t\t\t struct pattern_list *pl, struct index_state *istate);\n+\t\t\t\t struct pattern_list *pl, struct index_state *istate,\n+\t\t\t\t unsigned flags);\n void add_patterns_from_file(struct dir_struct *, const char *fname);\n int add_patterns_from_blob_to_list(struct object_id *oid,\n \t\t\t\t const char *base, int baselen,\n-- \n2.30.1.986.gd86016a168\n\n"},{"id":"417061","messageId":"YCvaVZEUuIwAFWxe@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvaJg8o882IqNnx@coredump.intra.peff.net","subject":"[PATCH 6/6] mailmap: do not respect symlinks for in-tree .mailmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:44:37Z","receivedAt":"2021-02-16T14:45:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As with .gitattributes and .gitignore, we would like to make sure that\n.mailmap files are handled consistently whether read from the a blob (as\nis the default behavior in a bare repo) or from the filesystem.\nLikewise, we would like to avoid reading out-of-tree files pointed to by\na symlink, which could have security implications in certain setups.\n\nWe can cover both by using open_nofollow() when opening the in-tree\nfiles. We'll continue to follow links for mailmap.file, as well as when\nreading .mailmap from the current directory when outside of a repository\nentirely.\n\nSigned-off-by: Jeff King \n---\n mailmap.c | 22 +++++++++++++++++-----\n t/t4203-mailmap.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 5 deletions(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex eb77c6e77c..7ac966107e 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -157,20 +157,30 @@ static void read_mailmap_line(struct string_list *map, char *buffer)\n \t\tadd_mapping(map, name1, email1, name2, email2);\n }\n \n-static int read_mailmap_file(struct string_list *map, const char *filename)\n+/* Flags for read_mailmap_file() */\n+#define MAILMAP_NOFOLLOW (1<<0)\n+\n+static int read_mailmap_file(struct string_list *map, const char *filename,\n+\t\t\t unsigned flags)\n {\n \tchar buffer[1024];\n \tFILE *f;\n+\tint fd;\n \n \tif (!filename)\n \t\treturn 0;\n \n-\tf = fopen(filename, \"r\");\n-\tif (!f) {\n+\tif (flags & MAILMAP_NOFOLLOW)\n+\t\tfd = open_nofollow(filename, O_RDONLY);\n+\telse\n+\t\tfd = open(filename, O_RDONLY);\n+\n+\tif (fd < 0) {\n \t\tif (errno == ENOENT)\n \t\t\treturn 0;\n \t\treturn error_errno(\"unable to open mailmap at %s\", filename);\n \t}\n+\tf = xfdopen(fd, \"r\");\n \n \twhile (fgets(buffer, sizeof(buffer), f) != NULL)\n \t\tread_mailmap_line(map, buffer);\n@@ -225,10 +235,12 @@ int read_mailmap(struct string_list *map)\n \tif (!git_mailmap_blob && is_bare_repository())\n \t\tgit_mailmap_blob = \"HEAD:.mailmap\";\n \n-\terr |= read_mailmap_file(map, \".mailmap\");\n+\terr |= read_mailmap_file(map, \".mailmap\",\n+\t\t\t\t startup_info->have_repository ?\n+\t\t\t\t MAILMAP_NOFOLLOW : 0);\n \tif (startup_info->have_repository)\n \t\terr |= read_mailmap_blob(map, git_mailmap_blob);\n-\terr |= read_mailmap_file(map, git_mailmap_file);\n+\terr |= read_mailmap_file(map, git_mailmap_file, 0);\n \treturn err;\n }\n \ndiff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh\nindex 621f9962d5..96a4e6132f 100755\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@ -889,4 +889,35 @@ test_expect_success 'empty syntax: setup' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success SYMLINKS 'set up symlink tests' '\n+\tgit commit --allow-empty -m foo --author=\"Orig \" &&\n+\techo \"New \" >map &&\n+\trm -f .mailmap\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in mailmap.file' '\n+\ttest_when_finished \"rm symlink\" &&\n+\tln -s map symlink &&\n+\tgit -c mailmap.file=\"$(pwd)/symlink\" log -1 --format=%aE >actual &&\n+\techo \"new@example.com\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success SYMLINKS 'symlinks respected in non-repo shortlog' '\n+\tgit log -1 >input &&\n+\ttest_when_finished \"nongit rm .mailmap\" &&\n+\tnongit ln -sf \"$TRASH_DIRECTORY/map\" .mailmap &&\n+\tnongit git shortlog -s actual &&\n+\techo \" 1\tNew\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success SYMLINKS 'symlinks not respected in-tree' '\n+\ttest_when_finished \"rm .mailmap\" &&\n+\tln -s map .mailmap &&\n+\tgit log -1 --format=%aE >actual &&\n+\techo \"orig@example.com\" >expect&&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.30.1.986.gd86016a168\n"},{"id":"417062","messageId":"YCvcoPnDAEWNcv+h@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvaRpQs99TBaCrQ@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] add open_nofollow() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:54:24Z","receivedAt":"2021-02-16T14:55:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 09:44:23AM -0500, Jeff King wrote:\n\n> - we can determine whether the path is a symlink with lstat().\n> \n> This is slower (two syscalls instead of one), but that may be\n> acceptable for infrequent uses like looking up .gitattributes files\n> (especially because we can get away with a single syscall for the\n> common case of ENOENT).\n> \n> It's also racy, but should be sufficient for our needs (we are\n> worried about in-tree symlinks that we ourselves would have\n> previously created). We could make it non-racy at the cost of making\n> it even slower, by doing an fstat() on the opened descriptor and\n> comparing the dev/ino fields to the original lstat().\n> \n> This patch implements the lstat() option in its slightly-faster racy\n> form.\n\nI manually compared the performance of the O_NOFOLLOW and fallback paths\nby running:\n\n git ls-files | git check-attr --stdin -a\n\non linux.git, which would try to look at quite a few in-tree attribute\nfiles. The slowdown didn't seem measurable (in fact, the fallback seems\nabout 1% faster even over many trials, but I think it's just noise).\nBoth take ~50ms.\n\nWhich makes sense, because there's only one .gitattributes file, so most\nof the lookups are hitting ENOENT on the lstat() and not even calling\nopen(). If I do:\n\n find * -type d | sed 's,$,/.gitattributes,' | xargs touch\n\nthen the O_NOFOLLOW case takes ~54ms (which makes sense; we're opening a\nbunch of extra empty attribute files), and the fallback case goes to\n~56ms. So the difference becomes measurable, but I wonder if it's worth\neven caring about. That's 2ms on a pathological case. I'd even be\ntempted to implement the non-racy version with the extra fstat(). I\ndon't think we need it, but just as a least-surprise thing.\n\nThis is all on Linux, of course. Perhaps other systems with slower\nsyscalls may be more impacted.\n\n-Peff\n"},{"id":"417063","messageId":"YCvdYTSOK5bLpo5h@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvaVZEUuIwAFWxe@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] mailmap: do not respect symlinks for in-tree .mailmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T14:57:37Z","receivedAt":"2021-02-16T14:58:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 09:44:37AM -0500, Jeff King wrote:\n\n> @@ -225,10 +235,12 @@ int read_mailmap(struct string_list *map)\n> \tif (!git_mailmap_blob && is_bare_repository())\n> \t\tgit_mailmap_blob = \"HEAD:.mailmap\";\n> \n> -\terr |= read_mailmap_file(map, \".mailmap\");\n> +\terr |= read_mailmap_file(map, \".mailmap\",\n> +\t\t\t\t startup_info->have_repository ?\n> +\t\t\t\t MAILMAP_NOFOLLOW : 0);\n\nThis conflicts with my jk/mailmap-only-at-root topic currently in\n\"next\", of course. Ditto for the new tests.\n\nThe resolution is pretty straight-forward, though:\n\ndiff --cc mailmap.c\nindex 9bb9cf8b30,7ac966107e..0000000000\n--- a/mailmap.c\n+++ b/mailmap.c\n@@@ -225,11 -235,12 +235,13 @@@ int read_mailmap(struct string_list *ma\n \tif (!git_mailmap_blob && is_bare_repository())\n \t\tgit_mailmap_blob = \"HEAD:.mailmap\";\n \n -\terr |= read_mailmap_file(map, \".mailmap\",\n -\t\t\t\t startup_info->have_repository ?\n -\t\t\t\t MAILMAP_NOFOLLOW : 0);\n +\tif (!startup_info->have_repository || !is_bare_repository())\n- \t\terr |= read_mailmap_file(map, \".mailmap\");\n++\t\terr |= read_mailmap_file(map, \".mailmap\",\n++\t\t\t\t\t startup_info->have_repository ?\n++\t\t\t\t\t MAILMAP_NOFOLLOW : 0);\n \tif (startup_info->have_repository)\n \t\terr |= read_mailmap_blob(map, git_mailmap_blob);\n- \terr |= read_mailmap_file(map, git_mailmap_file);\n+ \terr |= read_mailmap_file(map, git_mailmap_file, 0);\n \treturn err;\n }\n \ndiff --cc t/t4203-mailmap.sh\nindex 93caf9a46d,96a4e6132f..0000000000\n--- a/t/t4203-mailmap.sh\n+++ b/t/t4203-mailmap.sh\n@@@ -889,47 -889,35 +889,78 @@@ test_expect_success 'empty syntax: setu\n \ttest_cmp expect actual\n '\n \n +test_expect_success 'set up mailmap location tests' '\n +\tgit init --bare loc-bare &&\n +\tgit --git-dir=loc-bare --work-tree=. commit \\\n +\t\t--allow-empty -m foo --author=\"Orig \" &&\n +\techo \"New \" >loc-bare/.mailmap\n +'\n +\n +test_expect_success 'bare repo with --work-tree finds mailmap at top-level' '\n +\tgit -C loc-bare --work-tree=. log -1 --format=%aE >actual &&\n +\techo new@example.com >expect &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'bare repo does not look in current directory' '\n +\tgit -C loc-bare log -1 --format=%aE >actual &&\n +\techo orig@example.com >expect &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'non-git shortlog respects mailmap in current dir' '\n +\tgit --git-dir=loc-bare log -1 >input &&\n +\tnongit cp \"$TRASH_DIRECTORY/loc-bare/.mailmap\" . &&\n +\tnongit git shortlog -s actual &&\n +\techo \" 1\tNew\" >expect &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'shortlog on stdin respects mailmap from repo' '\n +\tcp loc-bare/.mailmap . &&\n +\tgit shortlog -s actual &&\n +\techo \" 1\tNew\" >expect &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'find top-level mailmap from subdir' '\n +\tgit clone loc-bare loc-wt &&\n +\tcp loc-bare/.mailmap loc-wt &&\n +\tmkdir loc-wt/subdir &&\n +\tgit -C loc-wt/subdir log -1 --format=%aE >actual &&\n +\techo new@example.com >expect &&\n +\ttest_cmp expect actual\n +'\n +\n+ test_expect_success SYMLINKS 'set up symlink tests' '\n+ \tgit commit --allow-empty -m foo --author=\"Orig \" &&\n+ \techo \"New \" >map &&\n+ \trm -f .mailmap\n+ '\n+ \n+ test_expect_success SYMLINKS 'symlinks respected in mailmap.file' '\n+ \ttest_when_finished \"rm symlink\" &&\n+ \tln -s map symlink &&\n+ \tgit -c mailmap.file=\"$(pwd)/symlink\" log -1 --format=%aE >actual &&\n+ \techo \"new@example.com\" >expect &&\n+ \ttest_cmp expect actual\n+ '\n+ \n+ test_expect_success SYMLINKS 'symlinks respected in non-repo shortlog' '\n+ \tgit log -1 >input &&\n+ \ttest_when_finished \"nongit rm .mailmap\" &&\n+ \tnongit ln -sf \"$TRASH_DIRECTORY/map\" .mailmap &&\n+ \tnongit git shortlog -s actual &&\n+ \techo \" 1\tNew\" >expect &&\n+ \ttest_cmp expect actual\n+ '\n+ \n+ test_expect_success SYMLINKS 'symlinks not respected in-tree' '\n+ \ttest_when_finished \"rm .mailmap\" &&\n+ \tln -s map .mailmap &&\n+ \tgit log -1 --format=%aE >actual &&\n+ \techo \"orig@example.com\" >expect&&\n+ \ttest_cmp expect actual\n+ '\n+ \n test_done\n"},{"id":"417064","messageId":"YCvoQBhmCeeR9Cup@nand.local","threadId":"55143","inReplyTo":"YCvcoPnDAEWNcv+h@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] add open_nofollow() helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-16T15:44:00Z","receivedAt":"2021-02-16T15:45:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 16, 2021 at 09:54:24AM -0500, Jeff King wrote:\n> This is all on Linux, of course. Perhaps other systems with slower\n> syscalls may be more impacted.\n\nI timed it on macOS, which (as you know) I don't use for daily\ndevelopment, but it's a useful testbed from time to time.\n\nOn your branch, 'git check-attr -a' took 193.4ms with O_NOFOLLOW, and\n245.3ms without. After touching every .gitattributes file, those numbers\nshot up to 340.9ms and 346.6ms, respectively.\n\n(All numbers on linux.git, of course).\n\nThere isn't an apples-to-apples comparison between my numbers and yours\n(since my laptop is much slower than yours), but the relative numbers\nare quite clear that only doing a single syscall is worth it in the\nnon-pathological case.\n\nThanks,\nTaylor\n"},{"id":"417067","messageId":"YCvsj2Teym2K06wa@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvoQBhmCeeR9Cup@nand.local","subject":"Re: [PATCH 1/6] add open_nofollow() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T16:02:23Z","receivedAt":"2021-02-16T16:03:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 10:44:00AM -0500, Taylor Blau wrote:\n\n> On Tue, Feb 16, 2021 at 09:54:24AM -0500, Jeff King wrote:\n> > This is all on Linux, of course. Perhaps other systems with slower\n> > syscalls may be more impacted.\n> \n> I timed it on macOS, which (as you know) I don't use for daily\n> development, but it's a useful testbed from time to time.\n> \n> On your branch, 'git check-attr -a' took 193.4ms with O_NOFOLLOW, and\n> 245.3ms without. After touching every .gitattributes file, those numbers\n> shot up to 340.9ms and 346.6ms, respectively.\n> \n> (All numbers on linux.git, of course).\n\nThat seems...really weird. Your \"after touching every file\" case is\nslightly slower, which is totally expected. But why would the _before_\ncase be so different? It should have made exactly one extra lstat()\ncall, and replaced a bunch of open(NOFOLLOW) calls with lstat() (and if\nthose were so expensive, we'd have presumably seen it in the \"touching\nevery file\" case which is running _both_ syscalls).\n\nCan you double-check your initial timings?\n\n> There isn't an apples-to-apples comparison between my numbers and yours\n> (since my laptop is much slower than yours), but the relative numbers\n> are quite clear that only doing a single syscall is worth it in the\n> non-pathological case.\n\nI guess in a sense it doesn't matter that much how macos performs. If it\nhas O_NOFOLLOW, then it's a no-brainer to use it. The bigger question\nis: how many platforms don't have it, and what are _their_ syscalls like\n(and are they niche enough that we can accept a small slowdown)?\n\nThe obvious one we'd care the most about is Windows, but I still hold\nout hope that there's some equivalent mechanism there we can use.\n\n-Peff\n"},{"id":"417069","messageId":"YCvty0WvJycB7WXi@nand.local","threadId":"55143","inReplyTo":"YCvsj2Teym2K06wa@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] add open_nofollow() helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-16T16:07:39Z","receivedAt":"2021-02-16T16:08:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 16, 2021 at 11:02:23AM -0500, Jeff King wrote:\n> Can you double-check your initial timings?\n\nAha, I forgot to update the input to the second check-attr tests after\nputting .gitattributes files everywhere.\n\nRerunning with O_NOFOLLOW, the initial timings look something like\n128.8ms and 183.7ms before/after.\n\nThanks,\nTaylor\n"},{"id":"417070","messageId":"YCvuy80k/P8qQ1h7@nand.local","threadId":"55143","inReplyTo":"YCvty0WvJycB7WXi@nand.local","subject":"Re: [PATCH 1/6] add open_nofollow() helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-16T16:11:55Z","receivedAt":"2021-02-16T16:13:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 16, 2021 at 11:07:39AM -0500, Taylor Blau wrote:\n> On Tue, Feb 16, 2021 at 11:02:23AM -0500, Jeff King wrote:\n> > Can you double-check your initial timings?\n>\n> Aha, I forgot to update the input to the second check-attr tests after\n> putting .gitattributes files everywhere.\n>\n> Rerunning with O_NOFOLLOW, the initial timings look something like\n> 128.8ms and 183.7ms before/after.\n\nI should add that \"before\" refers to a clean checkout, and \"after\"\nrefers to the state after running 'find * -type d | ... | xargs touch'.\nBoth of those numbers are with the O_NOFOLLOW branch.\n\nIf I repeat the test after applying:\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 563ad590df..90f603e749 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -681,7 +681,7 @@ int is_empty_or_missing_file(const char *filename)\n\n int open_nofollow(const char *path, int flags)\n {\n-#ifdef O_NOFOLLOW\n+#if 0\n return open(path, flags | O_NOFOLLOW);\n #else\n struct stat st;\n\nThen those numbers go from 155.9ms to 197.2ms.\n\nThanks,\nTaylor\n"},{"id":"417071","messageId":"YCvwjBm3Jpmi+8Dv@coredump.intra.peff.net","threadId":"55143","inReplyTo":"YCvuy80k/P8qQ1h7@nand.local","subject":"Re: [PATCH 1/6] add open_nofollow() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-16T16:19:24Z","receivedAt":"2021-02-16T16:20:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2021 at 11:11:55AM -0500, Taylor Blau wrote:\n\n> On Tue, Feb 16, 2021 at 11:07:39AM -0500, Taylor Blau wrote:\n> > On Tue, Feb 16, 2021 at 11:02:23AM -0500, Jeff King wrote:\n> > > Can you double-check your initial timings?\n> >\n> > Aha, I forgot to update the input to the second check-attr tests after\n> > putting .gitattributes files everywhere.\n> >\n> > Rerunning with O_NOFOLLOW, the initial timings look something like\n> > 128.8ms and 183.7ms before/after.\n> \n> I should add that \"before\" refers to a clean checkout, and \"after\"\n> refers to the state after running 'find * -type d | ... | xargs touch'.\n> Both of those numbers are with the O_NOFOLLOW branch.\n> \n> If I repeat the test after applying:\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index 563ad590df..90f603e749 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -681,7 +681,7 @@ int is_empty_or_missing_file(const char *filename)\n> \n> int open_nofollow(const char *path, int flags)\n> {\n> -#ifdef O_NOFOLLOW\n> +#if 0\n> return open(path, flags | O_NOFOLLOW);\n> #else\n> struct stat st;\n> \n> Then those numbers go from 155.9ms to 197.2ms.\n\nOK, thanks. So to lay out all cases:\n\n one file | many files\n +--------------------\nO_NOFOLLOW | 128.8ms | 183.7ms\nlstat+open | 155.9ms | 197.2ms\n\nThe jump for both from \"one\" to \"many\" files for both rows is expected;\nthere's more work to do. The jump from 183 to 197 in the \"many\" column\nis what I was wanting to measure (how expensive is the fallback code),\nand is around the order of magnitude I'd expect (and IMHO probably\nacceptable).\n\nBut the jump in the \"one file\" column between the two code paths is\nreally confusing. Those are making the same number of syscalls. It's\npossible that lstat() is way more expensive than open(), but in that\ncase I'd expect to see a similarly large jump in the \"many\" column.\nWeird.\n\n-Peff\n"},{"id":"417837","messageId":"xmqqh7m0uezk.fsf@gitster.g","threadId":"55143","inReplyTo":"YCvaJg8o882IqNnx@coredump.intra.peff.net","subject":"Re: [PATCH 0/6] open in-tree files with O_NOFOLLOW","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-25T19:25:19Z","receivedAt":"2021-02-25T19:28:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 16, 2021 at 07:48:23AM -0500, Jeff King wrote:\n>\n>> I am beginning to wonder if just opening them all with O_NOFOLLOW (and a\n>> hacky 2-syscall fallback for portability) might be less ugly than all of\n>> this.\n>\n> So here's what that series might look like. It would replace all of this\n> verify_path() stuff entirely (and fsck, though we might want to add\n> detection to fsck just as an informational thing). It gives similar\n> protections, and would similarly force people using an in-tree symlink\n> to stop doing that. But it makes it much less of a pain to do so,\n> because they can still check out, etc; the symlinks just won't be\n> followed.\n>\n> I think we could even use the same technique to roll back the\n> restrictions on .gitmodules being a symlink. That one makes me a bit\n> more nervous, just because we also write it. I _think_ that might be\n> safe, because we only do so using a temp file and rename(), which should\n> replace the symlink.\n>\n> [1/6]: add open_nofollow() helper\n> [2/6]: attr: convert \"macro_ok\" into a flags field\n> [3/6]: exclude: add flags parameter to add_patterns()\n> [4/6]: attr: do not respect symlinks for in-tree .gitattributes\n> [5/6]: exclude: do not respect symlinks for in-tree .gitignore\n> [6/6]: mailmap: do not respect symlinks for in-tree .mailmap\n>\n> attr.c | 60 +++++++++++++++++++++++++--------------\n> builtin/sparse-checkout.c | 8 +++---\n> dir.c | 21 ++++++++++----\n> dir.h | 3 +-\n> git-compat-util.h | 7 +++++\n> mailmap.c | 22 ++++++++++----\n> t/t0003-attributes.sh | 36 +++++++++++++++++++++--\n> t/t0008-ignores.sh | 34 ++++++++++++++++++++++\n> t/t4203-mailmap.sh | 31 ++++++++++++++++++++\n> wrapper.c | 16 +++++++++++\n> 10 files changed, 197 insertions(+), 41 deletions(-)\n\nSo, I've read these changes and they all looked quite reasonable.\n\nWhere do we want to go from here?\n\nMerge it down and forget about the changes in verify_path() and fsck\nin the jk/symlinked-dotgitx-files topic? Do we want to also cover\nthe .gitmodules file with the same mechansim?\n\nThanks.\n"},{"id":"417874","messageId":"YDiWs6yyv3U9YvC2@coredump.intra.peff.net","threadId":"55143","inReplyTo":"xmqqh7m0uezk.fsf@gitster.g","subject":"Re: [PATCH 0/6] open in-tree files with O_NOFOLLOW","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-02-26T06:35:31Z","receivedAt":"2021-02-26T06:36:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 25, 2021 at 11:25:19AM -0800, Junio C Hamano wrote:\n\n> > [1/6]: add open_nofollow() helper\n> > [2/6]: attr: convert \"macro_ok\" into a flags field\n> > [3/6]: exclude: add flags parameter to add_patterns()\n> > [4/6]: attr: do not respect symlinks for in-tree .gitattributes\n> > [5/6]: exclude: do not respect symlinks for in-tree .gitignore\n> > [6/6]: mailmap: do not respect symlinks for in-tree .mailmap\n> [...]\n> \n> So, I've read these changes and they all looked quite reasonable.\n>\n> Where do we want to go from here?\n> \n> Merge it down and forget about the changes in verify_path() and fsck\n> in the jk/symlinked-dotgitx-files topic? Do we want to also cover\n> the .gitmodules file with the same mechansim?\n\nThanks, I'm glad somebody looked at it. :)\n\nHaving pondered it, this really seems like a less risky approach than\nforbidding symlinks for those paths. It is not impacting what is allowed\nin Git, so nobody's repo will break. And we do not even have to worry\nthat our name-matching code is correct, since we are asking the OS to do\nthe right thing.\n\nThe biggest risk to me is that there is some hiccup with Windows: they\ndon't have any NOFOLLOW equivalent, and the fallback lstat() is somehow\nslower than a real open(). But that seems unlikely (I could well believe\nthat lstat+open is slower for them, but that only matters if you\nactually have a huge number of gitattributes files to open, in which\ncase you're probably spending your time reading and parsing them\nanyway).\n\nSo I'd be happy to proceed with this and throw out\njk/symlinked-dotgitx-files. We can salvage the fsck checks from there,\nleaving them as warnings, but it's not urgent (they are just\ninformational as \"btw, this symlink won't work like you think it will\").\nSo I'd probably do that as a separate series.\n\nWe could also cover .gitmodules, but I'm inclined not to. It's work and\nrisk to convert it to this form, for little gain. Nobody seems to have\nbeen bothered by the symlink restriction. I guess it would take some\nis_ntfs_gitmodules() checks out of the verify_path() code, which could\nhave some small performance benefit. But we'd definitely still need to\nidentify .gitmodules files in fsck, because we have to check over their\nactual contents.\n\nSo I'd likewise be content to leave that to another series (or never if\nnobody sees an upside to it).\n\n-Peff\n"}]}