Allow cloning from repositories owned by another user

Historically, Git has allowed users to clone from an untrusted
repository, and we have documented that this is safe to do so:

    `upload-pack` tries to avoid any dangerous configuration options or
    hooks from the repository it's serving, making it safe to clone an
    untrusted directory and run commands on the resulting clone.

However, this was broken by f4aa8c8bb1 ("fetch/clone: detect dubious
ownership of local repositories", 2024-04-10) in an attempt to make
things more secure.  That change resulted in a variety of problems when
cloning locally and over SSH, but it did not change the stated security
boundary.  Because the security boundary has not changed, it is safe to
adjust part of the code that patch introduced.

To do that and restore the previous functionality, adjust enter_repo to
take two flags instead of one.

The two bits are

 - ENTER_REPO_STRICT: callers that require exact paths (as opposed
   to allowing known suffixes like ".git", ".git/.git" to be
   omitted) can set this bit.  Corresponds to the "strict" parameter
   that the flags word replaces.

 - ENTER_REPO_ANY_OWNER_OK: callers that are willing to run without
   ownership check can set this bit.

The former is --strict-paths option of "git daemon".  The latter is
set only by upload-pack, which honors the claimed security boundary.

Note that local clones across ownership boundaries require --no-local so
that upload-pack is used.  Document this fact in the manual page and
provide an example.

This patch was based on one written by Junio C Hamano.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6e43eb9c205371548655c5abae5e59bb963c959a..5347cc52554395074553ae0ba78dfec8b35cb4a5 100644 (file)
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -63,6 +63,9 @@ symbolic link, the clone will fail. This is a security measure to
 prevent the unintentional copying of files by dereferencing the symbolic
 links.
 +
+This option does not work with repositories owned by other users for security
+reasons, and `--no-local` must be specified for the clone to succeed.
++
 *NOTE*: this operation can race with concurrent modification to the
 source repository, similar to running `cp -r src dst` while modifying
 `src`.
@@ -381,6 +384,12 @@ $ cd my-linux
 $ git clone --bare -l /home/proj/.git /pub/scm/proj.git
 ------------
 
+* Clone a local repository from a different user:
++
+------------
+$ git clone --no-local /home/otheruser/proj.git /pub/scm/proj.git
+------------
+
 CONFIGURATION
 -------------
 

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 272cddaafdd633f120acf3c6f8a87bf0a3fede38..72af0094e4149e9d1dbd0da17d688ec6c94f60c9 100644 (file)
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -34,6 +34,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
                            N_("interrupt transfer after <n> seconds of inactivity")),
                OPT_END()
        };
+       unsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;
 
        packet_trace_identity("upload-pack");
        disable_replace_refs();
@@ -49,7 +50,9 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 
        dir = argv[0];
 
-       if (!enter_repo(dir, strict))
+       if (strict)
+               enter_repo_flags |= ENTER_REPO_STRICT;
+       if (!enter_repo(dir, enter_repo_flags))
                die("'%s' does not appear to be a git repository", dir);
 
        switch (determine_protocol_version_server()) {

diff --git a/daemon.c b/daemon.c
index 17d331b2f38465e295c019e534eff371ef3e6e49..fb3713552157a73ee19bdd79a8d71e2096f60ab4 100644 (file)
--- a/daemon.c
+++ b/daemon.c
@@ -149,6 +149,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
        size_t rlen;
        const char *path;
        const char *dir;
+       unsigned enter_repo_flags;
 
        dir = directory;
 
@@ -239,14 +240,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
                dir = rpath;
        }
 
-       path = enter_repo(dir, strict_paths);
+       enter_repo_flags = strict_paths ? ENTER_REPO_STRICT : 0;
+       path = enter_repo(dir, enter_repo_flags);
        if (!path && base_path && base_path_relaxed) {
                /*
                 * if we fail and base_path_relaxed is enabled, try without
                 * prefixing the base path
                 */
                dir = directory;
-               path = enter_repo(dir, strict_paths);
+               path = enter_repo(dir, enter_repo_flags);
        }
 
        if (!path) {

diff --git a/path.c b/path.c
index 1d3e67936bfe742152f366827567055f0cd81231..11177b46f4d7e9fbaca49681ed083bb4fdb78e19 100644 (file)
--- a/path.c
+++ b/path.c
@@ -794,7 +794,7 @@ return_null:
  * links.  User relative paths are also returned as they are given,
  * except DWIM suffixing.
  */
-const char *enter_repo(const char *path, int strict)
+const char *enter_repo(const char *path, unsigned flags)
 {
        static struct strbuf validated_path = STRBUF_INIT;
        static struct strbuf used_path = STRBUF_INIT;
@@ -802,7 +802,7 @@ const char *enter_repo(const char *path, int strict)
        if (!path)
                return NULL;
 
-       if (!strict) {
+       if (!(flags & ENTER_REPO_STRICT)) {
                static const char *suffix[] = {
                        "/.git", "", ".git/.git", ".git", NULL,
                };
@@ -846,7 +846,8 @@ const char *enter_repo(const char *path, int strict)
                if (!suffix[i])
                        return NULL;
                gitfile = read_gitfile(used_path.buf);
-               die_upon_dubious_ownership(gitfile, NULL, used_path.buf);
+               if (!(flags & ENTER_REPO_ANY_OWNER_OK))
+                       die_upon_dubious_ownership(gitfile, NULL, used_path.buf);
                if (gitfile) {
                        strbuf_reset(&used_path);
                        strbuf_addstr(&used_path, gitfile);
@@ -857,7 +858,8 @@ const char *enter_repo(const char *path, int strict)
        }
        else {
                const char *gitfile = read_gitfile(path);
-               die_upon_dubious_ownership(gitfile, NULL, path);
+               if (!(flags & ENTER_REPO_ANY_OWNER_OK))
+                       die_upon_dubious_ownership(gitfile, NULL, path);
                if (gitfile)
                        path = gitfile;
                if (chdir(path))

diff --git a/path.h b/path.h
index b3233c51fa0f1acdf87368db4a7d9c3f0955c964..39673094f71e5f84c089eca7aae1b4f4b506d158 100644 (file)
--- a/path.h
+++ b/path.h
@@ -184,7 +184,22 @@ int validate_headref(const char *ref);
 int adjust_shared_perm(const char *path);
 
 char *interpolate_path(const char *path, int real_home);
-const char *enter_repo(const char *path, int strict);
+
+/* The bits are as follows:
+ *
+ * - ENTER_REPO_STRICT: callers that require exact paths (as opposed
+ *   to allowing known suffixes like ".git", ".git/.git" to be
+ *   omitted) can set this bit.
+ *
+ * - ENTER_REPO_ANY_OWNER_OK: callers that are willing to run without
+ *   ownership check can set this bit.
+ */
+enum {
+       ENTER_REPO_STRICT = (1<<0),
+       ENTER_REPO_ANY_OWNER_OK = (1<<1),
+};
+
+const char *enter_repo(const char *path, unsigned flags);
 const char *remove_leading_path(const char *in, const char *prefix);
 const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);
 int normalize_path_copy_len(char *dst, const char *src, int *prefix_len);

diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh
index c98d5018695a137b57cc9634312f220cff897cce..196fc617843cb94125903410adb4b847f8c3d48a 100755 (executable)
--- a/t/t0411-clone-from-partial.sh
+++ b/t/t0411-clone-from-partial.sh
@@ -28,7 +28,6 @@ test_expect_success 'local clone must not fetch from promisor remote and execute
        test_must_fail git clone \
                --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
                evil clone1 2>err &&
-       test_grep "detected dubious ownership" err &&
        test_grep ! "fake-upload-pack running" err &&
        test_path_is_missing script-executed
 '
@@ -38,7 +37,6 @@ test_expect_success 'clone from file://... must not fetch from promisor remote a
        test_must_fail git clone \
                --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
                "file://$(pwd)/evil" clone2 2>err &&
-       test_grep "detected dubious ownership" err &&
        test_grep ! "fake-upload-pack running" err &&
        test_path_is_missing script-executed
 '
@@ -48,7 +46,6 @@ test_expect_success 'fetch from file://... must not fetch from promisor remote a
        test_must_fail git fetch \
                --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
                "file://$(pwd)/evil" 2>err &&
-       test_grep "detected dubious ownership" err &&
        test_grep ! "fake-upload-pack running" err &&
        test_path_is_missing script-executed
 '

diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index a3055869bc7bc182e2e4af736b06785ee28bed85..31f6249ac9b76e3f630ff7f0c1301fb35a0eeee6 100755 (executable)
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -153,6 +153,16 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' '
        ! repo_is_hardlinked force-nonlocal
 '
 
+test_expect_success 'cloning a local path with --no-local from a different user succeeds' '
+       git clone --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \
+               --no-local a nonlocal-otheruser 2>err &&
+       ! repo_is_hardlinked nonlocal-otheruser &&
+       # Verify that this is a git repository.
+       git -C nonlocal-otheruser rev-parse --show-toplevel &&
+       ! test_grep "detected dubious ownership" err
+
+'
+
 test_expect_success 'cloning locally respects "-u" for fetching refs' '
        test_must_fail git clone --bare -u false a should_not_work.git
 '