1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
|
From cf8f6ce02a13f4d1979a53241afbee15a293fce9 Mon Sep 17 00:00:00 2001
From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 24 Jan 2023 19:43:48 -0500
Subject: [PATCH 2/3] clone: delay picking a transport until after get_repo_path()
In the previous commit, t5619 demonstrates an issue where two calls to
`get_repo_path()` could trick Git into using its local clone mechanism
in conjunction with a non-local transport.
That sequence is:
- the starting state is that the local path https:/example.com/foo is a
symlink that points to ../../../.git/modules/foo. So it's dangling.
- get_repo_path() sees that no such path exists (because it's
dangling), and thus we do not canonicalize it into an absolute path
- because we're using --separate-git-dir, we create .git/modules/foo.
Now our symlink is no longer dangling!
- we pass the url to transport_get(), which sees it as an https URL.
- we call get_repo_path() again, on the url. This second call was
introduced by f38aa83 (use local cloning if insteadOf makes a
local URL, 2014-07-17). The idea is that we want to pull the url
fresh from the remote.c API, because it will apply any aliases.
And of course now it sees that there is a local file, which is a
mismatch with the transport we already selected.
The issue in the above sequence is calling `transport_get()` before
deciding whether or not the repository is indeed local, and not passing
in an absolute path if it is local.
This is reminiscent of a similar bug report in [1], where it was
suggested to perform the `insteadOf` lookup earlier. Taking that
approach may not be as straightforward, since the intent is to store the
original URL in the config, but to actually fetch from the insteadOf
one, so conflating the two early on is a non-starter.
Note: we pass the path returned by `get_repo_path(remote->url[0])`,
which should be the same as `repo_name` (aside from any `insteadOf`
rewrites).
We *could* pass `absolute_pathdup()` of the same argument, which
86521ac (Bring local clone's origin URL in line with that of a remote
clone, 2008-09-01) indicates may differ depending on the presence of
".git/" for a non-bare repo. That matters for forming relative submodule
paths, but doesn't matter for the second call, since we're just feeding
it to the transport code, which is fine either way.
[1]: https://lore.kernel.org/git/CAMoD=Bi41mB3QRn3JdZL-FGHs4w3C2jGpnJB-CqSndO7FMtfzA@mail.gmail.com/
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Upstream-Status: Backport
[https://github.com/git/git/commit/cf8f6ce02a13f4d1979a53241afbee15a293fce9]
CVE: CVE-2023-22490
Signed-off-by: Vijay Anusuri <vanusuri@mvista.com>
---
builtin/clone.c | 8 ++++----
t/t5619-clone-local-ambiguous-transport.sh | 15 +++++++++++----
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 53e04b1..b57e703 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1112,10 +1112,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
branch_top.buf);
refspec_append(&remote->fetch, default_refspec.buf);
- transport = transport_get(remote, remote->url[0]);
- transport_set_verbosity(transport, option_verbosity, option_progress);
- transport->family = family;
-
path = get_repo_path(remote->url[0], &is_bundle);
is_local = option_local != 0 && path && !is_bundle;
if (is_local) {
@@ -1135,6 +1131,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
}
if (option_local > 0 && !is_local)
warning(_("--local is ignored"));
+
+ transport = transport_get(remote, path ? path : remote->url[0]);
+ transport_set_verbosity(transport, option_verbosity, option_progress);
+ transport->family = family;
transport->cloning = 1;
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
diff --git a/t/t5619-clone-local-ambiguous-transport.sh b/t/t5619-clone-local-ambiguous-transport.sh
index 7ebd31a..cce62bf 100644
--- a/t/t5619-clone-local-ambiguous-transport.sh
+++ b/t/t5619-clone-local-ambiguous-transport.sh
@@ -53,11 +53,18 @@ test_expect_success 'setup' '
git -C "$REPO" update-server-info
'
-test_expect_failure 'ambiguous transport does not lead to arbitrary file-inclusion' '
+test_expect_success 'ambiguous transport does not lead to arbitrary file-inclusion' '
git clone malicious clone &&
- git -C clone submodule update --init &&
-
- test_path_is_missing clone/.git/modules/sub/objects/secret
+ test_must_fail git -C clone submodule update --init 2>err &&
+
+ test_path_is_missing clone/.git/modules/sub/objects/secret &&
+ # We would actually expect "transport .file. not allowed" here,
+ # but due to quirks of the URL detection in Git, we mis-parse
+ # the absolute path as a bogus URL and die before that step.
+ #
+ # This works for now, and if we ever fix the URL detection, it
+ # is OK to change this to detect the transport error.
+ grep "protocol .* is not supported" err
'
test_done
--
2.25.1
|
