On Wed, May 10, 2023 at 12:33 PM Derrick Stolee via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > From: Derrick Stolee <derrickstolee@xxxxxxxxxx> > > The 'git merge-tree' command handles creating root trees for merges > without using the worktree. This is a critical operation in many Git > hosts, as they typically store bare repositories. > > This builtin does not load the default Git config, which can have > several important ramifications. > > In particular, one config that is loaded by default is > core.useReplaceRefs. This is typically disabled in Git hosts due to > the ability to spoof commits in strange ways. > > Since this config is not loaded specifically during merge-tree, users > were previously able to use refs/replace/ references to make pull > requests that looked valid but introduced malicious content. The > resulting merge commit would have the correct commit history, but the > malicious content would exist in the root tree of the merge. Ouch! So sorry for creating this problem. > The fix is simple: load the default Git config in cmd_merge_tree(). > This may also fix other behaviors that are effected by reading default > config. The only possible downside is a little extra computation time > spent reading config. The config parsing is placed after basic argument > parsing so it does not slow down usage errors. > > Helped-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > Signed-off-by: Derrick Stolee <derrickstolee@xxxxxxxxxx> > --- > merge-tree: load default git config > > This patch was reviewed on the Git security list, but the impact seemed > limited to Git forges using merge-ort to create merge commits. The > forges represented on the list have deployed versions of this patch and > thus are no longer vulnerable. > > Thanks, -Stolee > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1530%2Fderrickstolee%2Fstolee%2Frefs-replace-upstream-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1530/derrickstolee/stolee/refs-replace-upstream-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1530 > > builtin/merge-tree.c | 3 +++ > t/t4300-merge-tree.sh | 18 ++++++++++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c > index aa8040c2a6a..b8f8a8b5d9f 100644 > --- a/builtin/merge-tree.c > +++ b/builtin/merge-tree.c > @@ -17,6 +17,7 @@ > #include "merge-blobs.h" > #include "quote.h" > #include "tree.h" > +#include "config.h" > > static int line_termination = '\n'; > > @@ -628,6 +629,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix) > if (argc != expected_remaining_argc) > usage_with_options(merge_tree_usage, mt_options); > > + git_config(git_default_config, NULL); > + Always nice when it's a simple fix. :-) I am curious though... init_merge_options() in merge-recursive.c (which is also used by merge-ort) calls merge_recursive_config(). merge_recursive_config() does a bunch of config parsing, regardless of whatever config parsing is done beforehand by the caller of init_merge_options(). This makes me wonder if the config which handles replace refs should be included in merge_recursive_config() as well. Doing so would have the added benefit of making sure all the builtins calling the merge logic behave similarly. And if we copy/move the replace-refs-handling config logic, does that replace the fix in this patch, or just supplement it? To be honest, I've mostly ignored the config side of things while working on the merge machinery, so I didn't even know (or at least remember) the above details until I went digging just now. I don't know if the way init_merge_options()/merge_recursive_config() is how we should do things, or just vestiges of how it's evolved from 15 years ago. > /* Do the relevant type of merge */ > if (o.mode == MODE_REAL) > return real_merge(&o, merge_base, argv[0], argv[1], prefix); > diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh > index c52c8a21fae..57c4f26e461 100755 > --- a/t/t4300-merge-tree.sh > +++ b/t/t4300-merge-tree.sh > @@ -334,4 +334,22 @@ test_expect_success 'turn tree to file' ' > test_cmp expect actual > ' > > +test_expect_success 'merge-tree respects core.useReplaceRefs=false' ' > + test_commit merge-to && > + test_commit valid base && > + git reset --hard HEAD^ && > + test_commit malicious base && > + > + test_when_finished "git replace -d $(git rev-parse valid^0)" && > + git replace valid^0 malicious^0 && > + > + tree=$(git -c core.useReplaceRefs=true merge-tree --write-tree merge-to valid) && > + merged=$(git cat-file -p $tree:base) && > + test malicious = $merged && > + > + tree=$(git -c core.useReplaceRefs=false merge-tree --write-tree merge-to valid) && > + merged=$(git cat-file -p $tree:base) && > + test valid = $merged > +' > + > test_done Thanks for adding the test case too, as always. > base-commit: 5597cfdf47db94825213fefe78c4485e6a5702d8 > -- > gitgitgadget Looks good. I am curious for other's thoughts on whether it may make sense to add parsing of core.useReplaceRefs within merge_recursive_config().
