Re: [PATCH] merge-tree: load default git config

[Felipe Contreras <felipe.contreras@gmail.com>]

Elijah Newren wrote:
> On Wed, May 10, 2023 at 12:33 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Derrick Stolee <derrickstolee@github.com>
> >
> > 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@gmx.de>
> > Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> > ---
> >     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@gmail.com/

-- 
Felipe Contreras