From: Julian Prein <julian@xxxxxxxxx>
Due to security concerns the commit e8d060894448 (submodule: require the
submodule path to contain directories only, 2024-03-26) introduced some
checks and hard `exit(128)` calls for the case that a submodule path
contains symbolic links. This change was motivated by CVE-2024-32002,
which was discovered shortly before. Consequently, git currently aborts
actions of which some could be continued by not processing the faulty
module.
As an example, one of these actions that may be aborted is showing a
diff with diff.submodule set to `log` or `diff`: Let's assume a
repository where a submodule was moved and its old path replaced with a
symlink pointing to the new location. A git-show on an older commit from
before the move that also touches the module will now have only partial
output. If the submodule is the first file to list, there will be no
diff at all after the commit message. Other examples for actions that
are aborted after already being started are fetching and pushing with
their respective recurseSubmodules turned on.
Handle these cases more gracefully by returning an error value instead
of dying, if possible. This was done only in functions that already
supported returning an error.
With this change the mentioned git-show will display the full diff and
show a "(commits not present)" for the submodule - the same is done
currently when a submodule is simply moved. The fetch and push will
complete their action while skipping the faulty module and printing an
error message.
Signed-off-by: Julian Prein <julian@xxxxxxxxx>
---
submodule: gracefully handle links in module paths
Hello,
I'm submitting this patch primarily to fix the issue described with
diff.submodule. I maintain a repository in which this kind of move
occurred, and I have diff.submodule set to log. With current git I can't
properly display some older commits. Changing submodule_to_gitdir would
be sufficient to address this, but I noticed more places doing the same
thing (exit(128) even though an error value could be returned), so I
changed these as well.
Since this is security-relevant, and I'm not very familiar with the
codebase, I probably lack the foresight to foresee all consequences of
this change. For this reason, I'm submitting this as an RFC and without
any tests for now. If you deem this patch reasonable, I'd be happy to
write tests and add them to a future version.
Thanks,
Julian
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2041%2Fdruckdev%2Fpr2041-graceful-submodule-links-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2041/druckdev/pr2041-graceful-submodule-links-v1
Pull-Request: https://github.com/git/git/pull/2041
submodule.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/submodule.c b/submodule.c
index fff3c75570..843ec48339 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1127,7 +1127,7 @@ static int push_submodule(const char *path,
int dry_run)
{
if (validate_submodule_path(path) < 0)
- exit(128);
+ return 0;
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1514,10 +1514,19 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
{
struct fetch_task *task;
- CALLOC_ARRAY(task, 1);
+ if (validate_submodule_path(path) < 0) {
+ // Add submodule path to the list of submodules with errors.
+ // validate_submodule_path already printed an error message, but
+ // it would be confusing if the submodule wouldn't be listed
+ // together with other erroneous modules at the end.
+ //
+ // NEEDSWORK: name instead of path would be nice
+ spf->result = 1;
+ strbuf_addf(&spf->submodules_with_errors, "\t%s\n", path);
+ return NULL;
+ }
- if (validate_submodule_path(path) < 0)
- exit(128);
+ CALLOC_ARRAY(task, 1);
task->sub = submodule_from_path(spf->r, treeish_name, path);
@@ -1999,8 +2008,12 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
struct strbuf buf = STRBUF_INIT;
int ret = 0;
- if (validate_submodule_path(path) < 0)
- exit(128);
+ if (validate_submodule_path(path) < 0) {
+ if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
+ die(NULL);
+ ret = -1;
+ goto out;
+ }
if (!file_exists(path) || is_empty_dir(path))
return 0;
@@ -2555,7 +2568,7 @@ int submodule_to_gitdir(struct repository *repo,
int ret = 0;
if (validate_submodule_path(submodule) < 0)
- exit(128);
+ return -1;
strbuf_reset(buf);
strbuf_addstr(buf, submodule);
base-commit: f814da676ae46aac5be0a98b99373a76dee6cedb
--
gitgitgadget
