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.

Quoted 4 lines · Derrick Stolee via GitGitGadget

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.

Let's queue directly on 'next' (unlike 'master', where we want to merge only commits that had exposure in 'next' for a week or so, there is no formal requirement for topics to enter 'next' before spending any time in 'seen') and fast-track to 'master', as I've seen it already reviewed adequately over there.

Quoted 4 lines · Derrick Stolee via GitGitGadget

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.

Let's queue directly on 'next' (unlike 'master', where we want to merge only commits that had exposure in 'next' for a week or so, there is no formal requirement for topics to enter 'next' before spending any time in 'seen') and fast-track to 'master', as I've seen it already reviewed adequately over there.

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · Derrick Stolee via GitGitGadget

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.

Quoted 5 lines

Let's queue directly on 'next' (unlike 'master', where we want to merge only commits that had exposure in 'next' for a week or so, there is no formal requirement for topics to enter 'next' before spending any time in 'seen') and fast-track to 'master', as I've seen it already reviewed adequately over there.

Agreed. I also participated in the earlier rounds of review and the resulting patch looks obviously correct to me. I would be happy to see it merged.

I added Elijah to the CC list, since he is likely to be interested in this topic and may have thoughts to share.

From: Derrick Stolee <[email protected]>

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.

builtin/merge-tree.c +1 -0

index aa8040c2a6a..b8f8a8b5d9f 100644
@@ -17,6 +17,7 @@
 #include "merge-blobs.h"
 #include "quote.h"
 #include "tree.h"
+#include "config.h"
 
 static int line_termination = '\n';
 

Quoted 8 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

For the record, I had already sent a better version of this patch almost 2 years ago [1], not just for git merge-tree, but other commands as well.

The obvious fix was completely ignored by the maintainer.

The reason why it should be git_xmerge_config and not git_default_config, is that merge.conflictstyle would not be parsed if you call git_default_config.

Quoted 10 lines · Felipe Contreras

Quoted 8 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

Quoted 7 lines

For the record, I had already sent a better version of this patch almost 2 years ago [1], not just for git merge-tree, but other commands as well.

The obvious fix was completely ignored by the maintainer.

The reason why it should be git_xmerge_config and not git_default_config, is that merge.conflictstyle would not be parsed if you call git_default_config.

As mentioned by Elijah in a different thread, the merge machinery loads the merge config as needed. I confirmed by creating this test, which I may submit as an independent patch:

test_expect_success 'merge-tree respects merge.conflictstyle' '

	test_commit conflict-base &&
	for branch in left right
	do
		git checkout -b $branch conflict-base &&
		echo $branch >>conflict-base.t &&
		git add conflict-base.t &&
		git commit -m $branch || return 1
	done &&

	test_must_fail git merge-tree left right >out1 &&
	test_must_fail git -c merge.conflictstyle=diff3 merge-tree left right >out2 &&

	tree1=$(head -n 1 out1) &&
	tree2=$(head -n 1 out2) &&

	git cat-file -p $tree1:conflict-base.t >conflict1 &&
	git cat-file -p $tree2:conflict-base.t >conflict2 &&
	! test_cmp conflict1 conflict2 &&
	! grep "||||||" conflict1 &&
	grep "||||||" conflict2

'

Quoted 13 lines · Felipe Contreras

Quoted 8 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

For the record, I had already sent a better version of this patch almost 2 years ago [1], not just for git merge-tree, but other commands as well.

Quoted 4 lines

The obvious fix was completely ignored by the maintainer.

The reason why it should be git_xmerge_config and not git_default_config, is that merge.conflictstyle would not be parsed if you call git_default_config.

As mentioned by Elijah in a different thread, the merge machinery loads the merge config as needed. I confirmed by creating this test, which I may submit as an independent patch:

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.

Quoted 5 lines · Derrick Stolee via GitGitGadget

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.

Presumably merge-tree wants to serve a low-level machinery that gives reliable reproducible result, we may want to keep the configuration variables we read as narrow as practical. The default_config() callback may still be wider than desirable from that point of view, but I guess that is the most reasonable choice?

Quoted 5 lines · Derrick Stolee via GitGitGadget

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.

Presumably merge-tree wants to serve a low-level machinery that gives reliable reproducible result, we may want to keep the configuration variables we read as narrow as practical. The default_config() callback may still be wider than desirable from that point of view, but I guess that is the most reasonable choice?

From: Derrick Stolee <[email protected]>

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 <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
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(+)

builtin/merge-tree.c +1 -0

index aa8040c2a6a..b8f8a8b5d9f 100644
@@ -17,6 +17,7 @@
 #include "merge-blobs.h"
 #include "quote.h"
 #include "tree.h"
+#include "config.h"
 
 static int line_termination = '\n';
 

builtin/merge-tree.c +0 -0

        /* Do the relevant type of merge */
        if (o.mode == MODE_REAL)
                return real_merge(&o, merge_base, argv[0], argv[1], prefix);

t/t4300-merge-tree.sh +9 -0

index c52c8a21fae..57c4f26e461 100755
@@ -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 &&
+

base-commit: 5597cfdf47db94825213fefe78c4485e6a5702d8 --
gitgitgadget

Quoted 14 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

Quoted 5 lines

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.

Quoted 39 lines

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 <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
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(+)

builtin/merge-tree.c +1 -0

index aa8040c2a6a..b8f8a8b5d9f 100644
@@ -17,6 +17,7 @@
 #include "merge-blobs.h"
 #include "quote.h"
 #include "tree.h"
+#include "config.h"
 
 static int line_termination = '\n';
 

builtin/merge-tree.c int cmd_merge_tree(int argc, const char **argv, const char *prefix) · line 633

Quoted 5 lines

builtin/merge-tree.c +2 -0

@@ -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.

Quoted 14 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

builtin/merge-tree.c int cmd_merge_tree(int argc, const char **argv, const char *prefix) · line 633

Quoted 2 lines

builtin/merge-tree.c +2 -0

+       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.

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().