@matheusbsbs / matheus-bagliano / commits / 150e6b0aed

builtin/clone: stop resolving symlinks when copying files

When a user performs a local clone without `--no-local`, then we end up copying the source repository into the target repository directly. To optimize this even further, we try to hardlink files into place instead of copying data over, which helps both disk usage and speed. There is an important edge case in this context though, namely when we try to hardlink symlinks from the source repository into the target repository. Depending on both platform and filesystem the resulting behaviour here can be different: - On macOS and NetBSD, calling link(3P) with a symlink target creates a hardlink to the file pointed to by the symlink. - On Linux, calling link(3P) instead creates a hardlink to the symlink itself. To unify this behaviour, 36596fd2df (clone: better handle symlinked files at .git/objects/, 2019-07-10) introduced logic to resolve symlinks before we try to link(3P) files. Consequently, the new behaviour was to always create a hard link to the target of the symlink on all platforms. Eventually though, we figured out that following symlinks like this can cause havoc when performing a local clone of a malicious repository, which resulted in CVE-2022-39253. This issue was fixed via 6f054f9fb3 (builtin/clone.c: disallow `--local` clones with symlinks, 2022-07-28), by refusing symlinks in the source repository. But even though we now shouldn't ever link symlinks anymore, the code that resolves symlinks still exists. In the best case the code does not end up doing anything because there are no symlinks anymore. In the worst case though this can be abused by an adversary that rewrites the source file after it has been checked not to be a symlink such that it actually is a symlink when we call link(3P). Thus, it is still possible to recreate CVE-2022-39253 due to this time-of-check-time-of-use bug. Remove the call to `realpath()`. This doesn't yet address the actual vulnerability, which will be handled in a subsequent commit. Reported-by: Apple Product Security <product-security@apple.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Patrick Steinhardt committed Apr 15, 2024 at 13:30 UTC 150e6b0aedf57d224c3c49038c306477fa159886

1 file changed +1 -5

builtin/clone.c

+1 -5

@@ -320,7 +320,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,

320 int src_len, dest_len;

321 struct dir_iterator *iter;

322 int iter_status;

323 - struct strbuf realpath = STRBUF_INIT;

323

324 mkdir_if_missing(dest->buf, 0777);

325

@@ -358,8 +357,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,

357 if (unlink(dest->buf) && errno != ENOENT)

358 die_errno(_("failed to unlink '%s'"), dest->buf);

359 if (!option_no_hardlinks) {

361 - strbuf_realpath(&realpath, src->buf, 1);

362 - if (!link(realpath.buf, dest->buf))

360 + if (!link(src->buf, dest->buf))

361 continue;

362 if (option_local > 0)

363 die_errno(_("failed to create link '%s'"), dest->buf);

@@ -373,8 +371,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,

371 strbuf_setlen(src, src_len);

372 die(_("failed to iterate over '%s'"), src->buf);

373 }

376 -

377 - strbuf_release(&realpath);

374 }

375

376 static void clone_local(const char *src_repo, const char *dest_repo)