git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: gitster@pobox.com, me@ttaylorr.com, christian.couder@gmail.com,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: [PATCH] merge-tree: load default git config
Date: Wed, 10 May 2023 19:07:34 +0000	[thread overview]
Message-ID: <pull.1530.git.1683745654800.gitgitgadget@gmail.com> (raw)

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.

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);
+
 	/* 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

base-commit: 5597cfdf47db94825213fefe78c4485e6a5702d8
-- 
gitgitgadget

             reply	other threads:[~2023-05-10 19:07 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-10 19:07 Derrick Stolee via GitGitGadget [this message]
2023-05-10 19:18 ` [PATCH] merge-tree: load default git config Junio C Hamano
2023-05-10 19:26   ` Derrick Stolee
2023-05-10 23:21   ` Taylor Blau
2023-05-11  6:39     ` Elijah Newren
2023-05-10 20:30 ` Felipe Contreras
2023-05-11 15:09   ` Derrick Stolee
2023-05-11 17:02     ` Felipe Contreras
2023-05-11 22:07     ` Felipe Contreras
2023-05-10 22:32 ` Junio C Hamano
2023-05-10 23:27   ` Felipe Contreras
2023-05-11 17:15   ` Felipe Contreras
2023-05-11  6:34 ` Elijah Newren
2023-05-11  7:45   ` Felipe Contreras
2023-05-11 15:00   ` Derrick Stolee
2023-05-11 21:56 ` [PATCH] merge-tree: load config correctly Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.1530.git.1683745654800.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).