Elijah Newren wrote: > 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. It's obvious this is not the way we should do things as configurations are overriden when they shouldn't be. Something like this [1] is obviously needed. [1] https://lore.kernel.org/git/20210609192842.696646-5-felipe.contreras@xxxxxxxxx/ -- Felipe Contreras
