Re: [PATCH 1/1] Allow cloning from repositories owned by another user

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Mar 31, 2025, 13:14 UTC
Message-ID: <Z+qVJPQ17urm06QN@szeder.dev>
In-Reply-To: <20241115005404.3747302-2-sandals@crustytoothpaste.net>

On Fri, Nov 15, 2024 at 12:54:04AM +0000, brian m. carlson wrote:

Show 35 quoted lines

> 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.

Show 17 quoted lines

> diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
> index d9a320abd2..286099390c 100755
> --- a/t/t5605-clone-local.sh
> +++ b/t/t5605-clone-local.sh
> @@ -154,6 +154,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
> +
> +'

This test succeeds without checking everything it is supposed to:

  + git clone --upload-pack=GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack --no-local a nonlocal-otheruser
  + repo_is_hardlinked nonlocal-otheruser
  + find nonlocal-otheruser/objects -type f -links 1
  find: 'nonlocal-otheruser/objects': No such file or directory
  + git -C nonlocal-otheruser rev-parse --show-toplevel
  /home/szeder/src/git/t/trash directory.t5605-clone-local/nonlocal-otheruser
  + test_grep detected dubious ownership err
  error: 'grep detected dubious ownership err' didn't find a match in:
  Cloning into 'nonlocal-otheruser'...
  warning: remote HEAD refers to nonexistent ref, unable to checkout
  ok 21 - cloning a local path with --no-local from a different user succeeds

The 'repo_is_hardlinked' helper function expects the path to the .git directory as parameter, but in this case it gets the top-level path of the repository instead, which results in 'find' rightfully complaining about the non-existing 'objects' directory. Thanks to the &&-chain in 'repo_is_hardlinked' this error then causes the helper function to return with non-zero error code, just what this test case expected.

All other tests using this helper function create bare clones, and they all pass the right path as parameter.

The trivial fix would be to either use a bare clone in this test as well, or to pass the right path to 'repo_is_hardlinked', i.e. the path to the '.git' directory.

The right fix, in my opinion, is to teach 'repo_is_hardlinked' a negation parameter, so tests expecting the repo to be not hardlinked could invoke it as 'repo_is_hardlinked ! <path>'. An error from 'find' within the helper function should then always result in an error of the helper function, i.e. both with and without that '!' parameter.

Furthermore, this test should use 'test_grep ! ...' instead of '! test_grep ...', so we don't get that misleading error message in the test's output.

And there is a stray empty line at the end of the test as well.