{"thread":{"id":"59724","subject":"[PATCH] merge-tree: load default git config","startedAt":"2023-05-10T19:07:43Z","lastAt":"2023-05-11T22:08:08Z","messageCount":16,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Felipe Contreras","Taylor Blau","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"476981","messageId":"pull.1530.git.1683745654800.gitgitgadget@gmail.com","threadId":"59724","inReplyTo":null,"subject":"[PATCH] merge-tree: load default git config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-10T19:07:34Z","receivedAt":"2023-05-10T19:07:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nThe 'git merge-tree' command handles creating root trees for merges\nwithout using the worktree. This is a critical operation in many Git\nhosts, as they typically store bare repositories.\n\nThis builtin does not load the default Git config, which can have\nseveral important ramifications.\n\nIn particular, one config that is loaded by default is\ncore.useReplaceRefs. This is typically disabled in Git hosts due to\nthe ability to spoof commits in strange ways.\n\nSince this config is not loaded specifically during merge-tree, users\nwere previously able to use refs/replace/ references to make pull\nrequests that looked valid but introduced malicious content. The\nresulting merge commit would have the correct commit history, but the\nmalicious content would exist in the root tree of the merge.\n\nThe fix is simple: load the default Git config in cmd_merge_tree().\nThis may also fix other behaviors that are effected by reading default\nconfig. The only possible downside is a little extra computation time\nspent reading config. The config parsing is placed after basic argument\nparsing so it does not slow down usage errors.\n\nHelped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    merge-tree: load default git config\n    \n    This patch was reviewed on the Git security list, but the impact seemed\n    limited to Git forges using merge-ort to create merge commits. The\n    forges represented on the list have deployed versions of this patch and\n    thus are no longer vulnerable.\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1530%2Fderrickstolee%2Fstolee%2Frefs-replace-upstream-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1530/derrickstolee/stolee/refs-replace-upstream-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1530\n\n builtin/merge-tree.c  |  3 +++\n t/t4300-merge-tree.sh | 18 ++++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex aa8040c2a6a..b8f8a8b5d9f 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -17,6 +17,7 @@\n #include \"merge-blobs.h\"\n #include \"quote.h\"\n #include \"tree.h\"\n+#include \"config.h\"\n \n static int line_termination = '\\n';\n \n@@ -628,6 +629,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n \tif (argc != expected_remaining_argc)\n \t\tusage_with_options(merge_tree_usage, mt_options);\n \n+\tgit_config(git_default_config, NULL);\n+\n \t/* Do the relevant type of merge */\n \tif (o.mode == MODE_REAL)\n \t\treturn real_merge(&o, merge_base, argv[0], argv[1], prefix);\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex c52c8a21fae..57c4f26e461 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -334,4 +334,22 @@ test_expect_success 'turn tree to file' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'merge-tree respects core.useReplaceRefs=false' '\n+\ttest_commit merge-to &&\n+\ttest_commit valid base &&\n+\tgit reset --hard HEAD^ &&\n+\ttest_commit malicious base &&\n+\n+\ttest_when_finished \"git replace -d $(git rev-parse valid^0)\" &&\n+\tgit replace valid^0 malicious^0 &&\n+\n+\ttree=$(git -c core.useReplaceRefs=true merge-tree --write-tree merge-to valid) &&\n+\tmerged=$(git cat-file -p $tree:base) &&\n+\ttest malicious = $merged &&\n+\n+\ttree=$(git -c core.useReplaceRefs=false merge-tree --write-tree merge-to valid) &&\n+\tmerged=$(git cat-file -p $tree:base) &&\n+\ttest valid = $merged\n+'\n+\n test_done\n\nbase-commit: 5597cfdf47db94825213fefe78c4485e6a5702d8\n-- \ngitgitgadget\n"},{"id":"476984","messageId":"xmqqsfc43si0.fsf@gitster.g","threadId":"59724","inReplyTo":"pull.1530.git.1683745654800.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-10T19:18:15Z","receivedAt":"2023-05-10T19:18:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     This patch was reviewed on the Git security list, but the impact seemed\n>     limited to Git forges using merge-ort to create merge commits. The\n>     forges represented on the list have deployed versions of this patch and\n>     thus are no longer vulnerable.\n\nLet's queue directly on 'next' (unlike 'master', where we want to\nmerge only commits that had exposure in 'next' for a week or so,\nthere is no formal requirement for topics to enter 'next' before\nspending any time in 'seen') and fast-track to 'master', as I've\nseen it already reviewed adequately over there.\n\nThanks.\n"},{"id":"476986","messageId":"cc058647-554b-3b40-b8b9-52de0b7bd9e4@github.com","threadId":"59724","inReplyTo":"xmqqsfc43si0.fsf@gitster.g","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-05-10T19:26:34Z","receivedAt":"2023-05-10T19:26:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/10/2023 3:18 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>>     This patch was reviewed on the Git security list, but the impact seemed\n>>     limited to Git forges using merge-ort to create merge commits. The\n>>     forges represented on the list have deployed versions of this patch and\n>>     thus are no longer vulnerable.\n> \n> Let's queue directly on 'next' (unlike 'master', where we want to\n> merge only commits that had exposure in 'next' for a week or so,\n> there is no formal requirement for topics to enter 'next' before\n> spending any time in 'seen') and fast-track to 'master', as I've\n> seen it already reviewed adequately over there.\n\nThanks for picking it up so quickly. I did not mean to imply that\nwe should merge to master without giving the public list time to\ndigest the patch. It is a rather simple one, though.\n\nThanks,\n-Stolee\n"},{"id":"476990","messageId":"645bfed357efc_3819294e1@chronos.notmuch","threadId":"59724","inReplyTo":"pull.1530.git.1683745654800.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-10T20:30:11Z","receivedAt":"2023-05-10T20:30:16Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <derrickstolee@github.com>\n> \n> The 'git merge-tree' command handles creating root trees for merges\n> without using the worktree. This is a critical operation in many Git\n> hosts, as they typically store bare repositories.\n> \n> This builtin does not load the default Git config, which can have\n> several important ramifications.\n\nFor the record, I had already sent a better version of this patch almost 2\nyears ago [1], not just for `git merge-tree`, but other commands as well.\n\nThe obvious fix was completely ignored by the maintainer.\n\nThe reason why it should be git_xmerge_config and not git_default_config, is\nthat merge.conflictstyle would not be parsed if you call git_default_config.\n\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index aa8040c2a6a..b8f8a8b5d9f 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -17,6 +17,7 @@\n>  #include \"merge-blobs.h\"\n>  #include \"quote.h\"\n>  #include \"tree.h\"\n> +#include \"config.h\"\n>  \n>  static int line_termination = '\\n';\n>  \n> @@ -628,6 +629,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n>  \tif (argc != expected_remaining_argc)\n>  \t\tusage_with_options(merge_tree_usage, mt_options);\n>  \n> +\tgit_config(git_default_config, NULL);\n\nIt should be git_xmerge_config.\n\n> +\n>  \t/* Do the relevant type of merge */\n>  \tif (o.mode == MODE_REAL)\n>  \t\treturn real_merge(&o, merge_base, argv[0], argv[1], prefix);\n\n[1] https://lore.kernel.org/git/20210622002714.1720891-3-felipe.contreras@gmail.com/\n\n-- \nFelipe Contreras\n"},{"id":"476999","messageId":"xmqq5y8z3jif.fsf@gitster.g","threadId":"59724","inReplyTo":"pull.1530.git.1683745654800.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-10T22:32:24Z","receivedAt":"2023-05-10T22:32:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> The fix is simple: load the default Git config in cmd_merge_tree().\n> This may also fix other behaviors that are effected by reading default\n> config. The only possible downside is a little extra computation time\n> spent reading config. The config parsing is placed after basic argument\n> parsing so it does not slow down usage errors.\n\nPresumably merge-tree wants to serve a low-level machinery that\ngives reliable reproducible result, we may want to keep the\nconfiguration variables we read as narrow as practical.  The\ndefault_config() callback may still be wider than desirable from\nthat point of view, but I guess that is the most reasonable choice?\n\nThanks.\n\n\n"},{"id":"477008","messageId":"ZFwm9ZZN57opWbra@nand.local","threadId":"59724","inReplyTo":"xmqqsfc43si0.fsf@gitster.g","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-10T23:21:25Z","receivedAt":"2023-05-10T23:21:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 10, 2023 at 12:18:15PM -0700, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> >     This patch was reviewed on the Git security list, but the impact seemed\n> >     limited to Git forges using merge-ort to create merge commits. The\n> >     forges represented on the list have deployed versions of this patch and\n> >     thus are no longer vulnerable.\n>\n> Let's queue directly on 'next' (unlike 'master', where we want to\n> merge only commits that had exposure in 'next' for a week or so,\n> there is no formal requirement for topics to enter 'next' before\n> spending any time in 'seen') and fast-track to 'master', as I've\n> seen it already reviewed adequately over there.\n\nAgreed. I also participated in the earlier rounds of review and the\nresulting patch looks obviously correct to me. I would be happy to see\nit merged.\n\nI added Elijah to the CC list, since he is likely to be interested in\nthis topic and may have thoughts to share.\n\nThanks,\nTaylor\n"},{"id":"477010","messageId":"645c28576290d_7b63e2942d@chronos.notmuch","threadId":"59724","inReplyTo":"xmqq5y8z3jif.fsf@gitster.g","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-10T23:27:19Z","receivedAt":"2023-05-10T23:27:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> > The fix is simple: load the default Git config in cmd_merge_tree().\n> > This may also fix other behaviors that are effected by reading default\n> > config. The only possible downside is a little extra computation time\n> > spent reading config. The config parsing is placed after basic argument\n> > parsing so it does not slow down usage errors.\n> \n> Presumably merge-tree wants to serve a low-level machinery that\n> gives reliable reproducible result, we may want to keep the\n> configuration variables we read as narrow as practical.  The\n> default_config() callback may still be wider than desirable from\n> that point of view, but I guess that is the most reasonable choice?\n\nIf you want to *intentionally* ignore merge.conflictStyle in `git merge-tree`\nthen the documentation has to be updated as this is clearly not true:\n\n  The performed merge will use the same feature as the \"real\"\n  linkgit:git-merge[1], including:\n\n    * three way content merges of individual files\n    * rename detection\n    * proper directory/file conflict handling\n    * recursive ancestor consolidation (i.e. when there is more than one\n      merge base, creating a virtual merge base by merging the merge bases)\n    * etc.\n\nIf you want to ignore merge.conflictStyle, then `git merge-tree` would *not* be\nusing the same features as the real `git merge`, in particular proper conflict\nhalding (the conflict markers will be different).\n\n-- \nFelipe Contreras\n"},{"id":"477024","messageId":"CABPp-BECZgACeEUqG3pajJpHAaY=-orNwwOUEX5qqzAKVRMFdQ@mail.gmail.com","threadId":"59724","inReplyTo":"pull.1530.git.1683745654800.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-05-11T06:34:52Z","receivedAt":"2023-05-11T06:35:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, May 10, 2023 at 12:33 PM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> The 'git merge-tree' command handles creating root trees for merges\n> without using the worktree. This is a critical operation in many Git\n> hosts, as they typically store bare repositories.\n>\n> This builtin does not load the default Git config, which can have\n> several important ramifications.\n>\n> In particular, one config that is loaded by default is\n> core.useReplaceRefs. This is typically disabled in Git hosts due to\n> the ability to spoof commits in strange ways.\n>\n> Since this config is not loaded specifically during merge-tree, users\n> were previously able to use refs/replace/ references to make pull\n> requests that looked valid but introduced malicious content. The\n> resulting merge commit would have the correct commit history, but the\n> malicious content would exist in the root tree of the merge.\n\nOuch!  So sorry for creating this problem.\n\n> The fix is simple: load the default Git config in cmd_merge_tree().\n> This may also fix other behaviors that are effected by reading default\n> config. The only possible downside is a little extra computation time\n> spent reading config. The config parsing is placed after basic argument\n> parsing so it does not slow down usage errors.\n>\n> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> ---\n>     merge-tree: load default git config\n>\n>     This patch was reviewed on the Git security list, but the impact seemed\n>     limited to Git forges using merge-ort to create merge commits. The\n>     forges represented on the list have deployed versions of this patch and\n>     thus are no longer vulnerable.\n>\n>     Thanks, -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1530%2Fderrickstolee%2Fstolee%2Frefs-replace-upstream-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1530/derrickstolee/stolee/refs-replace-upstream-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1530\n>\n>  builtin/merge-tree.c  |  3 +++\n>  t/t4300-merge-tree.sh | 18 ++++++++++++++++++\n>  2 files changed, 21 insertions(+)\n>\n> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> index aa8040c2a6a..b8f8a8b5d9f 100644\n> --- a/builtin/merge-tree.c\n> +++ b/builtin/merge-tree.c\n> @@ -17,6 +17,7 @@\n>  #include \"merge-blobs.h\"\n>  #include \"quote.h\"\n>  #include \"tree.h\"\n> +#include \"config.h\"\n>\n>  static int line_termination = '\\n';\n>\n> @@ -628,6 +629,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n>         if (argc != expected_remaining_argc)\n>                 usage_with_options(merge_tree_usage, mt_options);\n>\n> +       git_config(git_default_config, NULL);\n> +\n\nAlways nice when it's a simple fix.  :-)\n\nI am curious though...\n\ninit_merge_options() in merge-recursive.c (which is also used by\nmerge-ort) calls merge_recursive_config().  merge_recursive_config()\ndoes a bunch of config parsing, regardless of whatever config parsing\nis done beforehand by the caller of init_merge_options().  This makes\nme wonder if the config which handles replace refs should be included\nin merge_recursive_config() as well.  Doing so would have the added\nbenefit of making sure all the builtins calling the merge logic behave\nsimilarly.  And if we copy/move the replace-refs-handling config\nlogic, does that replace the fix in this patch, or just supplement it?\n\nTo be honest, I've mostly ignored the config side of things while\nworking on the merge machinery, so I didn't even know (or at least\nremember) the above details until I went digging just now.  I don't\nknow if the way init_merge_options()/merge_recursive_config() is how\nwe should do things, or just vestiges of how it's evolved from 15\nyears ago.\n\n>         /* Do the relevant type of merge */\n>         if (o.mode == MODE_REAL)\n>                 return real_merge(&o, merge_base, argv[0], argv[1], prefix);\n> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> index c52c8a21fae..57c4f26e461 100755\n> --- a/t/t4300-merge-tree.sh\n> +++ b/t/t4300-merge-tree.sh\n> @@ -334,4 +334,22 @@ test_expect_success 'turn tree to file' '\n>         test_cmp expect actual\n>  '\n>\n> +test_expect_success 'merge-tree respects core.useReplaceRefs=false' '\n> +       test_commit merge-to &&\n> +       test_commit valid base &&\n> +       git reset --hard HEAD^ &&\n> +       test_commit malicious base &&\n> +\n> +       test_when_finished \"git replace -d $(git rev-parse valid^0)\" &&\n> +       git replace valid^0 malicious^0 &&\n> +\n> +       tree=$(git -c core.useReplaceRefs=true merge-tree --write-tree merge-to valid) &&\n> +       merged=$(git cat-file -p $tree:base) &&\n> +       test malicious = $merged &&\n> +\n> +       tree=$(git -c core.useReplaceRefs=false merge-tree --write-tree merge-to valid) &&\n> +       merged=$(git cat-file -p $tree:base) &&\n> +       test valid = $merged\n> +'\n> +\n>  test_done\n\nThanks for adding the test case too, as always.\n\n> base-commit: 5597cfdf47db94825213fefe78c4485e6a5702d8\n> --\n> gitgitgadget\n\nLooks good.  I am curious for other's thoughts on whether it may make\nsense to add parsing of core.useReplaceRefs within\nmerge_recursive_config().\n"},{"id":"477025","messageId":"CABPp-BFvXnBWmuWb+ttLAoTUxL8Sw0HpyXcB7+tWEPX1xpz+cA@mail.gmail.com","threadId":"59724","inReplyTo":"ZFwm9ZZN57opWbra@nand.local","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-05-11T06:39:10Z","receivedAt":"2023-05-11T06:39:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, May 10, 2023 at 4:21 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Wed, May 10, 2023 at 12:18:15PM -0700, Junio C Hamano wrote:\n> > \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> >\n> > >     This patch was reviewed on the Git security list, but the impact seemed\n> > >     limited to Git forges using merge-ort to create merge commits. The\n> > >     forges represented on the list have deployed versions of this patch and\n> > >     thus are no longer vulnerable.\n> >\n> > Let's queue directly on 'next' (unlike 'master', where we want to\n> > merge only commits that had exposure in 'next' for a week or so,\n> > there is no formal requirement for topics to enter 'next' before\n> > spending any time in 'seen') and fast-track to 'master', as I've\n> > seen it already reviewed adequately over there.\n>\n> Agreed. I also participated in the earlier rounds of review and the\n> resulting patch looks obviously correct to me. I would be happy to see\n> it merged.\n>\n> I added Elijah to the CC list, since he is likely to be interested in\n> this topic and may have thoughts to share.\n\nThanks.  I took a look and left some comments (it looks like the merge\nmachinery already parses _most_ relevant merge-related config\nunconditionally, each time we set up a merge), but I had more\nquestions than answers.  :-)\n"},{"id":"477028","messageId":"645c9d245c155_16db0a29494@chronos.notmuch","threadId":"59724","inReplyTo":"CABPp-BECZgACeEUqG3pajJpHAaY=-orNwwOUEX5qqzAKVRMFdQ@mail.gmail.com","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-11T07:45:40Z","receivedAt":"2023-05-11T07:45:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Wed, May 10, 2023 at 12:33 PM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > From: Derrick Stolee <derrickstolee@github.com>\n> >\n> > The 'git merge-tree' command handles creating root trees for merges\n> > without using the worktree. This is a critical operation in many Git\n> > hosts, as they typically store bare repositories.\n> >\n> > This builtin does not load the default Git config, which can have\n> > several important ramifications.\n> >\n> > In particular, one config that is loaded by default is\n> > core.useReplaceRefs. This is typically disabled in Git hosts due to\n> > the ability to spoof commits in strange ways.\n> >\n> > Since this config is not loaded specifically during merge-tree, users\n> > were previously able to use refs/replace/ references to make pull\n> > requests that looked valid but introduced malicious content. The\n> > resulting merge commit would have the correct commit history, but the\n> > malicious content would exist in the root tree of the merge.\n> \n> Ouch!  So sorry for creating this problem.\n> \n> > The fix is simple: load the default Git config in cmd_merge_tree().\n> > This may also fix other behaviors that are effected by reading default\n> > config. The only possible downside is a little extra computation time\n> > spent reading config. The config parsing is placed after basic argument\n> > parsing so it does not slow down usage errors.\n> >\n> > Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> > ---\n> >     merge-tree: load default git config\n> >\n> >     This patch was reviewed on the Git security list, but the impact seemed\n> >     limited to Git forges using merge-ort to create merge commits. The\n> >     forges represented on the list have deployed versions of this patch and\n> >     thus are no longer vulnerable.\n> >\n> >     Thanks, -Stolee\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1530%2Fderrickstolee%2Fstolee%2Frefs-replace-upstream-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1530/derrickstolee/stolee/refs-replace-upstream-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/1530\n> >\n> >  builtin/merge-tree.c  |  3 +++\n> >  t/t4300-merge-tree.sh | 18 ++++++++++++++++++\n> >  2 files changed, 21 insertions(+)\n> >\n> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\n> > index aa8040c2a6a..b8f8a8b5d9f 100644\n> > --- a/builtin/merge-tree.c\n> > +++ b/builtin/merge-tree.c\n> > @@ -17,6 +17,7 @@\n> >  #include \"merge-blobs.h\"\n> >  #include \"quote.h\"\n> >  #include \"tree.h\"\n> > +#include \"config.h\"\n> >\n> >  static int line_termination = '\\n';\n> >\n> > @@ -628,6 +629,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n> >         if (argc != expected_remaining_argc)\n> >                 usage_with_options(merge_tree_usage, mt_options);\n> >\n> > +       git_config(git_default_config, NULL);\n> > +\n> \n> Always nice when it's a simple fix.  :-)\n> \n> I am curious though...\n> \n> init_merge_options() in merge-recursive.c (which is also used by\n> merge-ort) calls merge_recursive_config().  merge_recursive_config()\n> does a bunch of config parsing, regardless of whatever config parsing\n> is done beforehand by the caller of init_merge_options().  This makes\n> me wonder if the config which handles replace refs should be included\n> in merge_recursive_config() as well.  Doing so would have the added\n> benefit of making sure all the builtins calling the merge logic behave\n> similarly.  And if we copy/move the replace-refs-handling config\n> logic, does that replace the fix in this patch, or just supplement it?\n> \n> To be honest, I've mostly ignored the config side of things while\n> working on the merge machinery, so I didn't even know (or at least\n> remember) the above details until I went digging just now.  I don't\n> know if the way init_merge_options()/merge_recursive_config() is how\n> we should do things, or just vestiges of how it's evolved from 15\n> years ago.\n\nIt's obvious this is not the way we should do things as configurations are\noverriden when they shouldn't be.\n\nSomething like this [1] is obviously needed.\n\n[1] https://lore.kernel.org/git/20210609192842.696646-5-felipe.contreras@gmail.com/\n\n-- \nFelipe Contreras"},{"id":"477036","messageId":"2ce5dd62-3b76-c14f-35e2-5cb2ccda42a1@github.com","threadId":"59724","inReplyTo":"CABPp-BECZgACeEUqG3pajJpHAaY=-orNwwOUEX5qqzAKVRMFdQ@mail.gmail.com","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-05-11T15:00:15Z","receivedAt":"2023-05-11T15:00:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/11/2023 2:34 AM, Elijah Newren wrote:\n> On Wed, May 10, 2023 at 12:33 PM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Derrick Stolee <derrickstolee@github.com>\n>>\n>> The 'git merge-tree' command handles creating root trees for merges\n>> without using the worktree. This is a critical operation in many Git\n>> hosts, as they typically store bare repositories.\n>>\n>> This builtin does not load the default Git config, which can have\n>> several important ramifications.\n>>\n>> In particular, one config that is loaded by default is\n>> core.useReplaceRefs. This is typically disabled in Git hosts due to\n>> the ability to spoof commits in strange ways.\n\n>> +       git_config(git_default_config, NULL);\n>> +\n> \n> Always nice when it's a simple fix.  :-)\n> \n> I am curious though...\n> \n> init_merge_options() in merge-recursive.c (which is also used by\n> merge-ort) calls merge_recursive_config().  merge_recursive_config()\n> does a bunch of config parsing, regardless of whatever config parsing\n> is done beforehand by the caller of init_merge_options().  This makes\n> me wonder if the config which handles replace refs should be included\n> in merge_recursive_config() as well.  Doing so would have the added\n> benefit of making sure all the builtins calling the merge logic behave\n> similarly.  And if we copy/move the replace-refs-handling config\n> logic, does that replace the fix in this patch, or just supplement it?\n> \n> To be honest, I've mostly ignored the config side of things while\n> working on the merge machinery, so I didn't even know (or at least\n> remember) the above details until I went digging just now.  I don't\n> know if the way init_merge_options()/merge_recursive_config() is how\n> we should do things, or just vestiges of how it's evolved from 15\n> years ago.\n...\n> Looks good.  I am curious for other's thoughts on whether it may make\n> sense to add parsing of core.useReplaceRefs within\n> merge_recursive_config().\n\nIn terms of a \"real\" fix to this kind of problem, I'm thinking that\nwe actually need to be sure we've parsed things like core.useReplaceRefs\nwhen loading the object database for the first time.\n\nHere, I'm suggesting the simplest fix before we can go about a more\nrigorous change to prevent this from happening again.\n\nThe custom ahead-behind builtin that we have in our fork once also had\nthis same problem, and the fix was exactly like this. The impact was\nless severe (mostly, things slowed down because the commit-graph was\ndisabled, but also the numbers could be different from expected).\n\nThanks,\n-Stolee\n"},{"id":"477037","messageId":"015dcb79-9630-e188-65cf-23b005184db1@github.com","threadId":"59724","inReplyTo":"645bfed357efc_3819294e1@chronos.notmuch","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-05-11T15:09:29Z","receivedAt":"2023-05-11T15:09:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/10/2023 4:30 PM, Felipe Contreras wrote:\n> Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee <derrickstolee@github.com>\n>>\n>> The 'git merge-tree' command handles creating root trees for merges\n>> without using the worktree. This is a critical operation in many Git\n>> hosts, as they typically store bare repositories.\n>>\n>> This builtin does not load the default Git config, which can have\n>> several important ramifications.\n> \n> For the record, I had already sent a better version of this patch almost 2\n> years ago [1], not just for `git merge-tree`, but other commands as well.\n> \n> The obvious fix was completely ignored by the maintainer.\n> \n> The reason why it should be git_xmerge_config and not git_default_config, is\n> that merge.conflictstyle would not be parsed if you call git_default_config.\n\nAs mentioned by Elijah in a different thread, the merge machinery loads\nthe merge config as needed. I confirmed by creating this test, which I\nmay submit as an independent patch:\n\ntest_expect_success 'merge-tree respects merge.conflictstyle' '\n\ttest_commit conflict-base &&\n\tfor branch in left right\n\tdo\n\t\tgit checkout -b $branch conflict-base &&\n\t\techo $branch >>conflict-base.t &&\n\t\tgit add conflict-base.t &&\n\t\tgit commit -m $branch || return 1\n\tdone &&\n\n\ttest_must_fail git merge-tree left right >out1 &&\n\ttest_must_fail git -c merge.conflictstyle=diff3 merge-tree left right >out2 &&\n\n\ttree1=$(head -n 1 out1) &&\n\ttree2=$(head -n 1 out2) &&\n\n\tgit cat-file -p $tree1:conflict-base.t >conflict1 &&\n\tgit cat-file -p $tree2:conflict-base.t >conflict2 &&\n\t! test_cmp conflict1 conflict2 &&\n\t! grep \"||||||\" conflict1 &&\n\tgrep \"||||||\" conflict2\n'\n\nThus we do not need to use git_xmerge_config at this point in the\nprocess.\n\nThanks,\n-Stolee\n"},{"id":"477046","messageId":"645d1fb69e9e8_26011a294a5@chronos.notmuch","threadId":"59724","inReplyTo":"015dcb79-9630-e188-65cf-23b005184db1@github.com","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-11T17:02:46Z","receivedAt":"2023-05-11T17:03:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Derrick Stolee wrote:\n> On 5/10/2023 4:30 PM, Felipe Contreras wrote:\n> > Derrick Stolee via GitGitGadget wrote:\n> >> From: Derrick Stolee <derrickstolee@github.com>\n> >>\n> >> The 'git merge-tree' command handles creating root trees for merges\n> >> without using the worktree. This is a critical operation in many Git\n> >> hosts, as they typically store bare repositories.\n> >>\n> >> This builtin does not load the default Git config, which can have\n> >> several important ramifications.\n> > \n> > For the record, I had already sent a better version of this patch almost 2\n> > years ago [1], not just for `git merge-tree`, but other commands as well.\n> > \n> > The obvious fix was completely ignored by the maintainer.\n> > \n> > The reason why it should be git_xmerge_config and not git_default_config, is\n> > that merge.conflictstyle would not be parsed if you call git_default_config.\n> \n> As mentioned by Elijah in a different thread, the merge machinery loads\n> the merge config as needed. I confirmed by creating this test, which I\n> may submit as an independent patch:\n> \n> test_expect_success 'merge-tree respects merge.conflictstyle' '\n> \ttest_commit conflict-base &&\n> \tfor branch in left right\n> \tdo\n> \t\tgit checkout -b $branch conflict-base &&\n> \t\techo $branch >>conflict-base.t &&\n> \t\tgit add conflict-base.t &&\n> \t\tgit commit -m $branch || return 1\n> \tdone &&\n> \n> \ttest_must_fail git merge-tree left right >out1 &&\n> \ttest_must_fail git -c merge.conflictstyle=diff3 merge-tree left right >out2 &&\n> \n> \ttree1=$(head -n 1 out1) &&\n> \ttree2=$(head -n 1 out2) &&\n> \n> \tgit cat-file -p $tree1:conflict-base.t >conflict1 &&\n> \tgit cat-file -p $tree2:conflict-base.t >conflict2 &&\n> \t! test_cmp conflict1 conflict2 &&\n> \t! grep \"||||||\" conflict1 &&\n> \tgrep \"||||||\" conflict2\n> '\n\nThis test is doing a real merge, not a trivial merge.\n\nTry doing a trivial merge instead--which is what most of our testing framework\nchecks--and your test fails:\n \n@@ -14,17 +14,12 @@ test_expect_success 'merge-tree respects merge.conflictstyle' '\n                 git commit -m $branch || return 1\n         done &&\n \n-        test_must_fail git merge-tree left right >out1 &&\n-        test_must_fail git -c merge.conflictstyle=diff3 merge-tree left right >out2 &&\n+        test_expect_code 0 git merge-tree conflict-base left right >out1 &&\n+        test_expect_code 0 git -c merge.conflictstyle=diff3 merge-tree conflict-base left right >out2 &&\n \n-        tree1=$(head -n 1 out1) &&\n-        tree2=$(head -n 1 out2) &&\n-\n-        git cat-file -p $tree1:conflict-base.t >conflict1 &&\n-        git cat-file -p $tree2:conflict-base.t >conflict2 &&\n-        ! test_cmp conflict1 conflict2 &&\n-        ! grep \"||||||\" conflict1 &&\n-        grep \"||||||\" conflict2\n+        ! test_cmp out1 out2 &&\n+        ! grep \"||||||\" out1 &&\n+        grep \"||||||\" out2\n '\n\n-- \nFelipe Contreras\n"},{"id":"477047","messageId":"645d22b8c6346_26011a29478@chronos.notmuch","threadId":"59724","inReplyTo":"xmqq5y8z3jif.fsf@gitster.g","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-11T17:15:36Z","receivedAt":"2023-05-11T17:15:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> > The fix is simple: load the default Git config in cmd_merge_tree().\n> > This may also fix other behaviors that are effected by reading default\n> > config. The only possible downside is a little extra computation time\n> > spent reading config. The config parsing is placed after basic argument\n> > parsing so it does not slow down usage errors.\n> \n> Presumably merge-tree wants to serve a low-level machinery that\n> gives reliable reproducible result, we may want to keep the\n> configuration variables we read as narrow as practical.  The\n> default_config() callback may still be wider than desirable from\n> that point of view, but I guess that is the most reasonable choice?\n\nIf you want `git merge-tree` to not call git_xmerge_config, then why did you\nmerge 1f0c3a29da (merge-tree: implement real merges, 2022-06-18) which\nintroduces a call to init_merge_options, which calls merge_recursive_config,\nwhich calls git_xmerge_config?\n\nAnd BTW, the way merge_recursive_config is implemented is completely different\nto how the rest of the config infraestructure works.\n\n-- \nFelipe Contreras\n"},{"id":"477099","messageId":"20230511215608.1297686-1-felipe.contreras@gmail.com","threadId":"59724","inReplyTo":"pull.1530.git.1683745654800.gitgitgadget@gmail.com","subject":"[PATCH] merge-tree: load config correctly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-11T21:56:08Z","receivedAt":"2023-05-11T21:56:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"When real merges were implemented in 1f0c3a29da (merge-tree: implement\nreal merges, 2022-06-18), init_merge_options was called after doing\nget_merge_parent, which means core.useReplaceRefs was not parsed at the\nmoment the commit objects were parsed, and thus essentially this option\nwas ignored.\n\nThis configuration is typically disabled in git hosts due to the ability\nto spoof commits in strange ways. Users are able to use refs/replace/\nreferences to make pull requests that look valid but introduce malicious\ncontent. The resulting merge has the correct commit history, but the\nmalicious content exists in the root tree of the merge.\n\nTo fix this let's simply load the configuration before any call to\nget_merge_parent().\n\nCc: Elijah Newren <newren@gmail.com>\nTests-by: Derrick Stolee <derrickstolee@github.com>\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/merge-tree.c  |  4 ++--\n t/t4300-merge-tree.sh | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex aa8040c2a6..b405cf448f 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -424,6 +424,8 @@ static int real_merge(struct merge_tree_options *o,\n \tstruct merge_result result = { 0 };\n \tint show_messages = o->show_messages;\n \n+\tinit_merge_options(&opt, the_repository);\n+\n \tparent1 = get_merge_parent(branch1);\n \tif (!parent1)\n \t\thelp_unknown_ref(branch1, \"merge-tree\",\n@@ -434,8 +436,6 @@ static int real_merge(struct merge_tree_options *o,\n \t\thelp_unknown_ref(branch2, \"merge-tree\",\n \t\t\t\t _(\"not something we can merge\"));\n \n-\tinit_merge_options(&opt, the_repository);\n-\n \topt.show_rename_progress = 0;\n \n \topt.branch1 = branch1;\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex c52c8a21fa..57c4f26e46 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -334,4 +334,22 @@ test_expect_success 'turn tree to file' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'merge-tree respects core.useReplaceRefs=false' '\n+\ttest_commit merge-to &&\n+\ttest_commit valid base &&\n+\tgit reset --hard HEAD^ &&\n+\ttest_commit malicious base &&\n+\n+\ttest_when_finished \"git replace -d $(git rev-parse valid^0)\" &&\n+\tgit replace valid^0 malicious^0 &&\n+\n+\ttree=$(git -c core.useReplaceRefs=true merge-tree --write-tree merge-to valid) &&\n+\tmerged=$(git cat-file -p $tree:base) &&\n+\ttest malicious = $merged &&\n+\n+\ttree=$(git -c core.useReplaceRefs=false merge-tree --write-tree merge-to valid) &&\n+\tmerged=$(git cat-file -p $tree:base) &&\n+\ttest valid = $merged\n+'\n+\n test_done\n-- \n2.40.0+fc1\n\n"},{"id":"477100","messageId":"645d672447ebb_13d3fe294f@chronos.notmuch","threadId":"59724","inReplyTo":"015dcb79-9630-e188-65cf-23b005184db1@github.com","subject":"Re: [PATCH] merge-tree: load default git config","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-11T22:07:32Z","receivedAt":"2023-05-11T22:08:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Derrick Stolee wrote:\n> On 5/10/2023 4:30 PM, Felipe Contreras wrote:\n> > Derrick Stolee via GitGitGadget wrote:\n> >> From: Derrick Stolee <derrickstolee@github.com>\n> >>\n> >> The 'git merge-tree' command handles creating root trees for merges\n> >> without using the worktree. This is a critical operation in many Git\n> >> hosts, as they typically store bare repositories.\n> >>\n> >> This builtin does not load the default Git config, which can have\n> >> several important ramifications.\n> > \n> > For the record, I had already sent a better version of this patch almost 2\n> > years ago [1], not just for `git merge-tree`, but other commands as well.\n> > \n> > The obvious fix was completely ignored by the maintainer.\n> > \n> > The reason why it should be git_xmerge_config and not git_default_config, is\n> > that merge.conflictstyle would not be parsed if you call git_default_config.\n> \n> As mentioned by Elijah in a different thread, the merge machinery loads\n> the merge config as needed. I confirmed by creating this test, which I\n> may submit as an independent patch:\n\nI wrote my patches before Elijah wrote the real merge implementation, and in\nhis function he does `init_merge_options()`, which eventually calls\n`git_config(git_xmerge_config, NULL)`.\n\nBut if `git_config()` is already called, you shouldn't need to add yet another\n`git_config()` call.\n\nThe problem is that he added `init_merge_options()` *after* the\n`get_merge_parent()` calls, that's why the configuration is ignored.\n\nIf we move `init_merge_options()` to the right place, the problem is fixed:\n\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -424,6 +424,8 @@ static int real_merge(struct merge_tree_options *o,\n \tstruct merge_result result = { 0 };\n \tint show_messages = o->show_messages;\n \n+\tinit_merge_options(&opt, the_repository);\n+\n \tparent1 = get_merge_parent(branch1);\n \tif (!parent1)\n \t\thelp_unknown_ref(branch1, \"merge-tree\",\n@@ -434,8 +436,6 @@ static int real_merge(struct merge_tree_options *o,\n \t\thelp_unknown_ref(branch2, \"merge-tree\",\n \t\t\t\t _(\"not something we can merge\"));\n \n-\tinit_merge_options(&opt, the_repository);\n-\n \topt.show_rename_progress = 0;\n \n \topt.branch1 = branch1;\n\nI ran your test case, and it passes.\n\nI sent a patch for that here:\n\nhttps://lore.kernel.org/git/20230511215608.1297686-1-felipe.contreras@gmail.com/\n\nThis is a more proper fix because a) it doesn't add any new line of code, b) it\ndoesn't add a new include, and c) it doesn't call `git_config()` twice.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}