submodule: require the submodule path to contain directories only
Submodules are stored in subdirectories of their superproject. When these subdirectories have been replaced with symlinks by a malicious actor, all kinds of mayhem can be caused. This _should_ not be possible, but many CVEs in the past showed that _when_ possible, it allows attackers to slip in code that gets executed during, say, a `git clone --recursive` operation. Let's add some defense-in-depth to disallow submodule paths to have anything except directories in them. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Johannes Schindelin committed
Mar 26, 2024 at 14:37 UTC
e8d0608944486019ea0e1ed2ed29776811a565c2
4 files changed
+113
-5
builtin/submodule--helper.c
+31
-1
@@ -294,6 +294,9 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
294
struct child_process cp = CHILD_PROCESS_INIT;
295
char *displaypath;
296
297
+ if (validate_submodule_path(path) < 0)
298
+ exit(128);
299
+
300
displaypath = get_submodule_displaypath(path, info->prefix);
301
302
sub = submodule_from_path(the_repository, null_oid(), path);
@@ -620,6 +623,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
623
.free_removed_argv_elements = 1,
624
};
625
626
+ if (validate_submodule_path(path) < 0)
627
+ exit(128);
628
+
629
if (!submodule_from_path(the_repository, null_oid(), path))
630
die(_("no submodule mapping found in .gitmodules for path '%s'"),
631
path);
@@ -1220,6 +1226,9 @@ static void sync_submodule(const char *path, const char *prefix,
1226
if (!is_submodule_active(the_repository, path))
1227
return;
1228
1229
+ if (validate_submodule_path(path) < 0)
1230
+ exit(128);
1231
+
1232
sub = submodule_from_path(the_repository, null_oid(), path);
1233
1234
if (sub && sub->url) {
@@ -1360,6 +1369,9 @@ static void deinit_submodule(const char *path, const char *prefix,
1369
struct strbuf sb_config = STRBUF_INIT;
1370
char *sub_git_dir = xstrfmt("%s/.git", path);
1371
1372
+ if (validate_submodule_path(path) < 0)
1373
+ exit(128);
1374
+
1375
sub = submodule_from_path(the_repository, null_oid(), path);
1376
1377
if (!sub || !sub->name)
@@ -1674,6 +1686,9 @@ static int clone_submodule(const struct module_clone_data *clone_data,
1686
const char *clone_data_path = clone_data->path;
1687
char *to_free = NULL;
1688
1689
+ if (validate_submodule_path(clone_data_path) < 0)
1690
+ exit(128);
1691
+
1692
if (!is_absolute_path(clone_data->path))
1693
clone_data_path = to_free = xstrfmt("%s/%s", get_git_work_tree(),
1694
clone_data->path);
@@ -2542,6 +2557,9 @@ static int update_submodule(struct update_data *update_data)
2557
{
2558
int ret;
2559
2560
+ if (validate_submodule_path(update_data->sm_path) < 0)
2561
+ return -1;
2562
+
2563
ret = determine_submodule_update_strategy(the_repository,
2564
update_data->just_cloned,
2565
update_data->sm_path,
@@ -2649,12 +2667,21 @@ static int update_submodules(struct update_data *update_data)
2667
2668
for (i = 0; i < suc.update_clone_nr; i++) {
2669
struct update_clone_data ucd = suc.update_clone[i];
2652
- int code;
2670
+ int code = 128;
2671
2672
oidcpy(&update_data->oid, &ucd.oid);
2673
update_data->just_cloned = ucd.just_cloned;
2674
update_data->sm_path = ucd.sub->path;
2675
2676
+ /*
2677
+ * Verify that the submodule path does not contain any
2678
+ * symlinks; if it does, it might have been tampered with.
2679
+ * TODO: allow exempting it via
2680
+ * `safe.submodule.path` or something
2681
+ */
2682
+ if (validate_submodule_path(update_data->sm_path) < 0)
2683
+ goto fail;
2684
+
2685
code = ensure_core_worktree(update_data->sm_path);
2686
if (code)
2687
goto fail;
@@ -3361,6 +3388,9 @@ static int module_add(int argc, const char **argv, const char *prefix)
3388
normalize_path_copy(add_data.sm_path, add_data.sm_path);
3389
strip_dir_trailing_slashes(add_data.sm_path);
3390
3391
+ if (validate_submodule_path(add_data.sm_path) < 0)
3392
+ exit(128);
3393
+
3394
die_on_index_match(add_data.sm_path, force);
3395
die_on_repo_without_commits(add_data.sm_path);
3396
submodule.c
+72
@@ -1005,6 +1005,9 @@ static int submodule_has_commits(struct repository *r,
1005
.super_oid = super_oid
1006
};
1007
1008
+ if (validate_submodule_path(path) < 0)
1009
+ exit(128);
1010
+
1011
oid_array_for_each_unique(commits, check_has_commit, &has_commit);
1012
1013
if (has_commit.result) {
@@ -1127,6 +1130,9 @@ static int push_submodule(const char *path,
1130
const struct string_list *push_options,
1131
int dry_run)
1132
{
1133
+ if (validate_submodule_path(path) < 0)
1134
+ exit(128);
1135
+
1136
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
1137
struct child_process cp = CHILD_PROCESS_INIT;
1138
strvec_push(&cp.args, "push");
@@ -1176,6 +1182,9 @@ static void submodule_push_check(const char *path, const char *head,
1182
struct child_process cp = CHILD_PROCESS_INIT;
1183
int i;
1184
1185
+ if (validate_submodule_path(path) < 0)
1186
+ exit(128);
1187
+
1188
strvec_push(&cp.args, "submodule--helper");
1189
strvec_push(&cp.args, "push-check");
1190
strvec_push(&cp.args, head);
@@ -1507,6 +1516,9 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
1516
struct fetch_task *task = xmalloc(sizeof(*task));
1517
memset(task, 0, sizeof(*task));
1518
1519
+ if (validate_submodule_path(path) < 0)
1520
+ exit(128);
1521
+
1522
task->sub = submodule_from_path(spf->r, treeish_name, path);
1523
1524
if (!task->sub) {
@@ -1879,6 +1891,9 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
1891
const char *git_dir;
1892
int ignore_cp_exit_code = 0;
1893
1894
+ if (validate_submodule_path(path) < 0)
1895
+ exit(128);
1896
+
1897
strbuf_addf(&buf, "%s/.git", path);
1898
git_dir = read_gitfile(buf.buf);
1899
if (!git_dir)
@@ -1955,6 +1970,9 @@ int submodule_uses_gitfile(const char *path)
1970
struct strbuf buf = STRBUF_INIT;
1971
const char *git_dir;
1972
1973
+ if (validate_submodule_path(path) < 0)
1974
+ exit(128);
1975
+
1976
strbuf_addf(&buf, "%s/.git", path);
1977
git_dir = read_gitfile(buf.buf);
1978
if (!git_dir) {
@@ -1994,6 +2012,9 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
2012
struct strbuf buf = STRBUF_INIT;
2013
int ret = 0;
2014
2015
+ if (validate_submodule_path(path) < 0)
2016
+ exit(128);
2017
+
2018
if (!file_exists(path) || is_empty_dir(path))
2019
return 0;
2020
@@ -2044,6 +2065,9 @@ void submodule_unset_core_worktree(const struct submodule *sub)
2065
{
2066
struct strbuf config_path = STRBUF_INIT;
2067
2068
+ if (validate_submodule_path(sub->path) < 0)
2069
+ exit(128);
2070
+
2071
submodule_name_to_gitdir(&config_path, the_repository, sub->name);
2072
strbuf_addstr(&config_path, "/config");
2073
@@ -2066,6 +2090,9 @@ static int submodule_has_dirty_index(const struct submodule *sub)
2090
{
2091
struct child_process cp = CHILD_PROCESS_INIT;
2092
2093
+ if (validate_submodule_path(sub->path) < 0)
2094
+ exit(128);
2095
+
2096
prepare_submodule_repo_env(&cp.env);
2097
2098
cp.git_cmd = 1;
@@ -2083,6 +2110,10 @@ static int submodule_has_dirty_index(const struct submodule *sub)
2110
static void submodule_reset_index(const char *path)
2111
{
2112
struct child_process cp = CHILD_PROCESS_INIT;
2113
+
2114
+ if (validate_submodule_path(path) < 0)
2115
+ exit(128);
2116
+
2117
prepare_submodule_repo_env(&cp.env);
2118
2119
cp.git_cmd = 1;
@@ -2287,6 +2318,34 @@ int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
2318
return 0;
2319
}
2320
2321
+int validate_submodule_path(const char *path)
2322
+{
2323
+ char *p = xstrdup(path);
2324
+ struct stat st;
2325
+ int i, ret = 0;
2326
+ char sep;
2327
+
2328
+ for (i = 0; !ret && p[i]; i++) {
2329
+ if (!is_dir_sep(p[i]))
2330
+ continue;
2331
+
2332
+ sep = p[i];
2333
+ p[i] = '\0';
2334
+ /* allow missing components, but no symlinks */
2335
+ ret = lstat(p, &st) || !S_ISLNK(st.st_mode) ? 0 : -1;
2336
+ p[i] = sep;
2337
+ if (ret)
2338
+ error(_("expected '%.*s' in submodule path '%s' not to "
2339
+ "be a symbolic link"), i, p, p);
2340
+ }
2341
+ if (!lstat(p, &st) && S_ISLNK(st.st_mode))
2342
+ ret = error(_("expected submodule path '%s' not to be a "
2343
+ "symbolic link"), p);
2344
+ free(p);
2345
+ return ret;
2346
+}
2347
+
2348
+
2349
/*
2350
* Embeds a single submodules git directory into the superprojects git dir,
2351
* non recursively.
@@ -2297,6 +2356,9 @@ static void relocate_single_git_dir_into_superproject(const char *path)
2356
struct strbuf new_gitdir = STRBUF_INIT;
2357
const struct submodule *sub;
2358
2359
+ if (validate_submodule_path(path) < 0)
2360
+ exit(128);
2361
+
2362
if (submodule_uses_worktrees(path))
2363
die(_("relocate_gitdir for submodule '%s' with "
2364
"more than one worktree not supported"), path);
@@ -2337,6 +2399,9 @@ static void absorb_git_dir_into_superproject_recurse(const char *path)
2399
2400
struct child_process cp = CHILD_PROCESS_INIT;
2401
2402
+ if (validate_submodule_path(path) < 0)
2403
+ exit(128);
2404
+
2405
cp.dir = path;
2406
cp.git_cmd = 1;
2407
cp.no_stdin = 1;
@@ -2359,6 +2424,10 @@ void absorb_git_dir_into_superproject(const char *path)
2424
int err_code;
2425
const char *sub_git_dir;
2426
struct strbuf gitdir = STRBUF_INIT;
2427
+
2428
+ if (validate_submodule_path(path) < 0)
2429
+ exit(128);
2430
+
2431
strbuf_addf(&gitdir, "%s/.git", path);
2432
sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
2433
@@ -2501,6 +2570,9 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
2570
const char *git_dir;
2571
int ret = 0;
2572
2573
+ if (validate_submodule_path(submodule) < 0)
2574
+ exit(128);
2575
+
2576
strbuf_reset(buf);
2577
strbuf_addstr(buf, submodule);
2578
strbuf_complete(buf, '/');
submodule.h
+5
@@ -148,6 +148,11 @@ void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,
148
*/
149
int validate_submodule_git_dir(char *git_dir, const char *submodule_name);
150
151
+/*
152
+ * Make sure that the given submodule path does not follow symlinks.
153
+ */
154
+int validate_submodule_path(const char *path);
155
+
156
#define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)
157
#define SUBMODULE_MOVE_HEAD_FORCE (1<<1)
158
int submodule_move_head(const char *path,
t/t7423-submodule-symlinks.sh
+5
-4
@@ -14,15 +14,16 @@ test_expect_success 'prepare' '
14
git commit -m submodule
15
'
16
17
-test_expect_failure SYMLINKS 'git submodule update must not create submodule behind symlink' '
17
+test_expect_success SYMLINKS 'git submodule update must not create submodule behind symlink' '
18
rm -rf a b &&
19
mkdir b &&
20
ln -s b a &&
21
+ test_path_is_missing b/sm &&
22
test_must_fail git submodule update &&
23
test_path_is_missing b/sm
24
'
25
25
-test_expect_failure SYMLINKS,CASE_INSENSITIVE_FS 'git submodule update must not create submodule behind symlink on case insensitive fs' '
26
+test_expect_success SYMLINKS,CASE_INSENSITIVE_FS 'git submodule update must not create submodule behind symlink on case insensitive fs' '
27
rm -rf a b &&
28
mkdir b &&
29
ln -s b A &&
@@ -46,7 +47,7 @@ test_expect_success SYMLINKS 'git restore --recurse-submodules must not be confu
47
test_path_is_missing a/target/submodule_file
48
'
49
49
-test_expect_failure SYMLINKS 'git restore --recurse-submodules must not migrate git dir of symlinked repo' '
50
+test_expect_success SYMLINKS 'git restore --recurse-submodules must not migrate git dir of symlinked repo' '
51
prepare_symlink_to_repo &&
52
rm -rf .git/modules &&
53
test_must_fail git restore --recurse-submodules a/sm &&
@@ -55,7 +56,7 @@ test_expect_failure SYMLINKS 'git restore --recurse-submodules must not migrate
56
test_path_is_missing a/target/submodule_file
57
'
58
58
-test_expect_failure SYMLINKS 'git checkout -f --recurse-submodules must not migrate git dir of symlinked repo when removing submodule' '
59
+test_expect_success SYMLINKS 'git checkout -f --recurse-submodules must not migrate git dir of symlinked repo when removing submodule' '
60
prepare_symlink_to_repo &&
61
rm -rf .git/modules &&
62
test_must_fail git checkout -f --recurse-submodules initial &&