Git clone reads safe.directory differently?

Git clone reads safe.directory differently?

[W. Michael Petullo]

Glen Choo's commit 6061601d modified Git so that safe.directory could
be set on the command line using "-c". It seems most sub-commands work
this way, but not "clone". Here is an example:

====================
$ whoami
mike
$ ls -ld /tmp/y | cut -d " " -f 3
root
$ /home/mike/Scratch/git/git -c safe.directory="*" -C /tmp/y log
commit 4be57d4489010047fd3ef39a633264f634a432b5 (HEAD -> master)
Author: [...]
Date:   Sat Jul 27 11:06:19 2024 -0500

    Initial commit
$ /home/mike/Scratch/git/git -c safe.directory="*" clone /tmp/y /tmp/z
Cloning into '/tmp/z'...
warning: templates not found in /home/mike/share/git-core/templates
fatal: detected dubious ownership in repository at '/tmp/y/.git'
To add an exception for this directory, call:

	git config --global --add safe.directory /tmp/y/.git
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
====================

Glen's commit 6061601d seems to indicate there is no security reason
for this:

	"As a result, `safe.directory` now respects '-c', so update the
	tests and docs accordingly. It used to ignore '-c' due to how it
	was implemented, not because of security or correctness concerns."

I added some printf statements to ensure_valid_ownership() in setup.c,
and I found that they executed in the case of running "git log" but not
"git clone".

A little more research found that "git clone" bailed after the call to
transport_get_remote_refs() many lines before the call to clone_local()
-> copy_or_link_directory() -> die_upon_dubious_ownership().
I suspect something along that earlier call path is still using
setup_git_directory_gently(), rather than git_protected_config(), to
check for dubious ownership.

[System Info]
git version:
git version 2.46.0.rc2.dirty
cpu: x86_64
built from commit: ad57f148c6b5f8735b62238dda8f571c582e0e54
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
libcurl: 8.6.0
OpenSSL: OpenSSL 3.2.1 30 Jan 2024
zlib: 1.3.1.zlib-ng
uname: Linux 6.9.8-200.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Jul  5 16:20:11 UTC 2024 x86_64
compiler info: gnuc: 14.1
libc info: glibc: 2.39
$SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks]

-- 
Mike

:wq

Re: Git clone reads safe.directory differently?

[Jeff King]

On Sat, Jul 27, 2024 at 11:14:40AM -0500, W. Michael Petullo wrote:

> Glen Choo's commit 6061601d modified Git so that safe.directory could
> be set on the command line using "-c". It seems most sub-commands work
> this way, but not "clone". Here is an example:

This is because upload-pack, the server half of a fetch/clone, is run as
a separate process which does not accept the client-side "-c" options.
See this email for more details and a workaround:

  https://lore.kernel.org/git/20240529102307.GF1098944@coredump.intra.peff.net/

-Peff

Re: Git clone reads safe.directory differently?

[W. Michael Petullo]

>> Glen Choo's commit 6061601d modified Git so that safe.directory could
>> be set on the command line using "-c". It seems most sub-commands work
>> this way, but not "clone". Here is an example:
> 
> This is because upload-pack, the server half of a fetch/clone, is run as
> a separate process which does not accept the client-side "-c" options.
> See this email for more details and a workaround:
> 
>   https://lore.kernel.org/git/20240529102307.GF1098944@coredump.intra.peff.net/

Thank you, Jeff! This works and makes sense, although I can see how users
would be confused by the edge case.

For completeness, I investigated how to do the same over SSH. Imagine a
repository user-owned by Bob, but group-owned with r/w/x permissions by
a group containing Alice. It seems the same trick fails because git-shell
rejects the custom upload-pack command:

	git -c safe.directory="*" clone -u 'git -c safe.directory="*" upload-pack' alice@git.example.com:/shared/repository
	Cloning into 'git'...
	fatal: unrecognized command 'git -c safe.directory="*" upload-pack '/shared/repository''
	fatal: Could not read from remote repository.

	Please make sure you have the correct access rights
	and the repository exists.

I was able to overcome this by creating /home/alice/git-shell-commands/upload-pack-safe,
placing the following there

	#!/bin/sh

	git -c safe.directory="$1" upload-pack $1

and running:

	git -c safe.directory="*" clone -u upload-pack-safe alice@git.example.com:/shared/repository

This seems to be another interface edge case. Is my solution reasonable,
or is there something else that would be more consistent?

Related: Would anyone be interested in working on an academic paper
about safe.directory? It seems like people who write Git forge-type
software or other collaborative systems based on Git would benefit from
a rigorous description of the conditions and operations under which the
use of safe.directory is indeed safe. I am not sure I have this worked
out in my own mind yet. Something like "Setuid Demystified" from USENIX
2002 might help.

-- 
Mike

:wq

Re: Git clone reads safe.directory differently?

[Jeff King]

On Sun, Jul 28, 2024 at 10:27:00AM -0500, W. Michael Petullo wrote:

> For completeness, I investigated how to do the same over SSH. Imagine a
> repository user-owned by Bob, but group-owned with r/w/x permissions by
> a group containing Alice. It seems the same trick fails because git-shell
> rejects the custom upload-pack command:
> 
> 	git -c safe.directory="*" clone -u 'git -c safe.directory="*" upload-pack' alice@git.example.com:/shared/repository
> 	Cloning into 'git'...
> 	fatal: unrecognized command 'git -c safe.directory="*" upload-pack '/shared/repository''
> 	fatal: Could not read from remote repository.
> 
> 	Please make sure you have the correct access rights
> 	and the repository exists.

It should work over ssh in the general case, but yes, if you're using
git-shell, the whole idea there is to restrict what the client can
specify. And so there won't be any workaround from the client side (if
there were, it would be a bug!).

> I was able to overcome this by creating /home/alice/git-shell-commands/upload-pack-safe,
> placing the following there
> 
> 	#!/bin/sh
> 
> 	git -c safe.directory="$1" upload-pack $1
> 
> and running:
> 
> 	git -c safe.directory="*" clone -u upload-pack-safe alice@git.example.com:/shared/repository
> 
> This seems to be another interface edge case. Is my solution reasonable,
> or is there something else that would be more consistent?

So yeah, that would work. But it might be simpler still to just put
"[safe]directory = *" into /home/alice/.gitconfig. Then the client side
does not have to do anything differently (or you could even put in a
more limited relaxation of the rules, like "/shared/repository/*" or
whatever).

-Peff

Re: Git clone reads safe.directory differently?

[W. Michael Petullo]

>> I was able to overcome this by creating /home/alice/git-shell-commands/upload-pack-safe,
>> placing the following there
>> 
>> 	#!/bin/sh
>> 
>> 	git -c safe.directory="$1" upload-pack $1
>> 
>> and running:
>> 
>> 	git -c safe.directory="*" clone -u upload-pack-safe alice@git.example.com:/shared/repository
>> 
>> This seems to be another interface edge case. Is my solution reasonable,
>> or is there something else that would be more consistent?
 
> So yeah, that would work. But it might be simpler still to just put
> "[safe]directory = *" into /home/alice/.gitconfig. Then the client side
> does not have to do anything differently (or you could even put in a
> more limited relaxation of the rules, like "/shared/repository/*" or
> whatever).

A little feedback to regarding why I chose to avoid .gitconfig along
with some consequences:

I have a system built around Git in which I use the permissions approaches
I have described in this email thread. The downside of .gitconfig is
that its effect is global. I fear I might lose track of the fact that I
have turned off a security protection in a global way, possibly years
down the road as I maintain my system.  Instead, I went about this by
patching particular uses of Git to use "-c" and so on. I only needed to
do this at two or three places my code executes Git out of dozens.

This makes my intention clear in the code (that executes Git) and lessens
the likelihood of me (a year or so later?) forgetting I am responsible
for ensuring the circumstances leave my use of safe.directory safe.

I suppose the inconsistency surrounding "-c" that we have discussed sort
of works against my approach. Indeed, I wonder if in an ideal world we
should remove the dangerous features that have good intentions
(hardlinks and so on) and instead require users to opt in to them.
I say ideal world because this ignores backward/forward compatibility
issues.

Perhaps a compromise would be to tie safe.directory to the type
of source directory given to clone. Would a remote URL be enough to
turn off the safe.directory checks on a clone, similar to the effect of
a remote URL on --local/--no-local?

The current behavior seems to mean:

	(1) -c safe.directory works for some sub-commands, but not clone,

	(2) clone requires your additional workaround, and 

	(3) your workaround does not work with SSH/git-shell for the
	obvious reason that git-shell needs to limit the "commands" it
	will run; this requires another level of intervention.

Another thing I came across was in the git-clone man page:

	"When given, and the repository to clone from is accessed via ssh,
	this specifies a non-default path for the command run on the other end."

Your "-u 'git -c safe.directory=X upload-pack'" workaround uses
upload-pack in the absence of SSH, so I wonder if "and the repository
to clone from is accessed via ssh" is inaccurate.

-- 
Mike

:wq

Re: Git clone reads safe.directory differently?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

On 2024-07-30 at 11:37:47, W. Michael Petullo wrote:
> Perhaps a compromise would be to tie safe.directory to the type
> of source directory given to clone. Would a remote URL be enough to
> turn off the safe.directory checks on a clone, similar to the effect of
> a remote URL on --local/--no-local?

I think if we're using --no-local (that is, if we're using upload-pack
instead of creating symlinks), then we should not complain about the
repository ownership.  It's supposed to always be safe to clone or fetch
from an untrusted repository, and we shouldn't complain about that.

Both of these commands should work correctly and do not, and that's a
bug (assuming tr1 is owned by a different user):

  git clone --no-local --no-hardlinks $PWD/tr1 tr2
  git clone --no-local --no-hardlinks ssh://localhost$PWD/tr1 tr2

git-upload-pack should not complain about safe.directory at all. If it's
not secure to clone or fetch from an untrusted repository with
git-upload-pack, then we have a bigger security problem that needs to be
addressed.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Git clone reads safe.directory differently?

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I think if we're using --no-local (that is, if we're using upload-pack
> instead of creating symlinks), then we should not complain about the
> repository ownership.  It's supposed to always be safe to clone or fetch
> from an untrusted repository, and we shouldn't complain about that.

The safety is promised by "git fetch" when you fetch from some other
machine because the only thing you will be seeing from that
untrusted source is a bytestream that is the packfile, plus the tips
of their histories---nothing runs as yourself in this exchange other
than what you control, i.e. "git fetch", locally defined hooks and
filters defined by your configuration..  They cannot affect your
configuration file and hooks that may name extra programs that may
run as you while fetching or cloning.

When you are using "--no-local" on the same machine, I do not think
there is any guarantee that "upload-pack" side is safe.  And that is
where the safe.directory thing needs to kick in.

Stepping into an untrusted repository and running git operations
opens up the user the Git process runs as to attacks by the
untrusted repository, i.e. you may invoke hooks on the upload-pack
side, defined in the source repository that is controlled by others,
and that is where the safe.directory thing kicks in.  You need to
declare that you trust that source repository.

Re: Git clone reads safe.directory differently?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> When you are using "--no-local" on the same machine, I do not think
> there is any guarantee that "upload-pack" side is safe.  And that is
> where the safe.directory thing needs to kick in.

Ah, I take it back.  packObjectsHook won't run based on what the
untrusted source repository configures, so at least we have been
aware of the fact that upload-pack must be more careful than other
stuff.

Re: Git clone reads safe.directory differently?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]

On 2024-07-30 at 22:49:37, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > I think if we're using --no-local (that is, if we're using upload-pack
> > instead of creating symlinks), then we should not complain about the
> > repository ownership.  It's supposed to always be safe to clone or fetch
> > from an untrusted repository, and we shouldn't complain about that.
> 
> The safety is promised by "git fetch" when you fetch from some other
> machine because the only thing you will be seeing from that
> untrusted source is a bytestream that is the packfile, plus the tips
> of their histories---nothing runs as yourself in this exchange other
> than what you control, i.e. "git fetch", locally defined hooks and
> filters defined by your configuration..  They cannot affect your
> configuration file and hooks that may name extra programs that may
> run as you while fetching or cloning.
> 
> When you are using "--no-local" on the same machine, I do not think
> there is any guarantee that "upload-pack" side is safe.  And that is
> where the safe.directory thing needs to kick in.
> 
> Stepping into an untrusted repository and running git operations
> opens up the user the Git process runs as to attacks by the
> untrusted repository, i.e. you may invoke hooks on the upload-pack
> side, defined in the source repository that is controlled by others,
> and that is where the safe.directory thing kicks in.  You need to
> declare that you trust that source repository.

I don't believe git-upload-pack invokes hooks.  At least, I don't see
any documented to do so, and I'm not aware of any from my experience
hosting repositories.  Certainly git-receive-pack does so, which I can
understand as a problem, but upload-pack should not.

And the manual page does say this:

  Most Git commands should not be run in an untrusted `.git` directory
  (see the section `SECURITY` in git(1)). `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.

The git(1) manual page also says this:

  If you have an untrusted `.git` directory, you should first clone it
  with `git clone --no-local` to obtain a clean copy. Git does restrict
  the set of options and hooks that will be run by `upload-pack`, which
  handles the server side of a clone or fetch, but beware that the
  surface area for attack against `upload-pack` is large, so this does
  carry some risk. The safest thing is to serve the repository as an
  unprivileged user (either via git-daemon(1), ssh, or using
  other tools to change user ids). See the discussion in the `SECURITY`
  section of git-upload-pack(1).

I think that has been traditionally the policy that we've had here, and
given that we document that it is safe to do so, I think it should be
fine.  This documentation apparently came from Peff, who I think should
be reasonably well informed on such matters.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Git clone reads safe.directory differently?

[Jeff King]

On Tue, Jul 30, 2024 at 11:05:24PM +0000, brian m. carlson wrote:

> The git(1) manual page also says this:
> 
>   If you have an untrusted `.git` directory, you should first clone it
>   with `git clone --no-local` to obtain a clean copy. Git does restrict
>   the set of options and hooks that will be run by `upload-pack`, which
>   handles the server side of a clone or fetch, but beware that the
>   surface area for attack against `upload-pack` is large, so this does
>   carry some risk. The safest thing is to serve the repository as an
>   unprivileged user (either via git-daemon(1), ssh, or using
>   other tools to change user ids). See the discussion in the `SECURITY`
>   section of git-upload-pack(1).
> 
> I think that has been traditionally the policy that we've had here, and
> given that we document that it is safe to do so, I think it should be
> fine.  This documentation apparently came from Peff, who I think should
> be reasonably well informed on such matters.

Right, that was added in v2.45, but I think was just explaining the
long-standing approach.

My understanding of the recent expansion of the ownership checks was
that they were a defense-in-depth. It _should_ be safe to run
upload-pack against an untrusted repository, but it would be easy for us
to accidentally violate that.

But I admit I did not carefully follow all of the discussion around
crossing filesystem boundaries (e.g., symbolic and hard links pointing
outside docker containers that are bind-mounted, or something like
that). I did not find those cases all that compelling from a security
perspective, but again, I didn't look into them that closely.

It could be that "clone" should try to avoid a "--local" clone from a
repo with different ownership, if the local hardlink path is more
dangerous. But that distinction is not something upload-pack even knows
about, so the code would have to go into clone. And then upload-pack
could be free to drop the ownership check. Certainly a lot of people
have complained about it (I had actually thought we reverted it in
v2.45.2, but that was just the extra hooks defense-in-depth; so again, I
may be getting confused about the extra value of the enter_repo()
ownership check that came at the same time).

-Peff

Re: Git clone reads safe.directory differently?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It could be that "clone" should try to avoid a "--local" clone from a
> repo with different ownership, if the local hardlink path is more
> dangerous. But that distinction is not something upload-pack even knows
> about, so the code would have to go into clone.

Sounds good.

> And then upload-pack
> could be free to drop the ownership check. Certainly a lot of people
> have complained about it (I had actually thought we reverted it in
> v2.45.2, but that was just the extra hooks defense-in-depth; so again, I
> may be getting confused about the extra value of the enter_repo()
> ownership check that came at the same time).

As enter_repo() is about the protocol driver thing and not about
normal users working inside a repository, calls to it appear only in
receive-pack, upload-pack, upload-archive, http-backend, and daemon.

Among them, upload-pack is the only thing we promise that is safe to
work even in a hostile repository?  If we push into a repository
over the local transport, we would trigger post-receive hook as
ourselves, which we would probably not want.  The same story goes
for daemon, http-backend, and upload-archive.

So we probably need to add another axis to the "strict" parameter
enter_repo() takes to selectively disable the ownership checks only
for upload-pack, or something like that.

We may want to restrict "tar.<format>.command" only to protected
configuration and then we may be able to loosen the ownership check
for the upload-archive command.

Thanks.

Re: Git clone reads safe.directory differently?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So we probably need to add another axis to the "strict" parameter
> enter_repo() takes to selectively disable the ownership checks only
> for upload-pack, or something like that.

So, here is a rough sketch for the above.  Interested parties may
build on top of it, perhaps by adding separate knobs to loosen or
tighten the second parameter given to enter_repo() at different
callsites, by writing tests to make sure they work as intended, and
by documenting the security story around it none of which I do here
;-).

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

 - 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, but you may want to add configuration knobs
in protected configuration files to loosen it per callsites.

 builtin/upload-pack.c |  5 ++++-
 daemon.c              |  6 ++++--
 path.c                | 10 ++++++----
 path.h                |  7 ++++++-
 4 files changed, 20 insertions(+), 8 deletions(-)

diff --git c/builtin/upload-pack.c w/builtin/upload-pack.c
index 46d93278d9..fe50ce3eed 100644
--- c/builtin/upload-pack.c
+++ w/builtin/upload-pack.c
@@ -36,6 +36,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();
@@ -51,7 +52,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 c/daemon.c w/daemon.c
index 17d331b2f3..fb37135521 100644
--- c/daemon.c
+++ w/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 c/path.c w/path.c
index 19f7684f38..df5aefdb8f 100644
--- c/path.c
+++ w/path.c
@@ -727,7 +727,7 @@ char *interpolate_path(const char *path, int real_home)
  * 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;
@@ -735,7 +735,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,
 		};
@@ -779,7 +779,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);
@@ -790,7 +791,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 c/path.h w/path.h
index a6f0b70692..a0021a1425 100644
--- c/path.h
+++ w/path.h
@@ -187,7 +187,12 @@ int calc_shared_perm(int mode);
 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);
+
+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);

Re: Git clone reads safe.directory differently?

[Jeff King]

On Wed, Jul 31, 2024 at 03:08:55PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > So we probably need to add another axis to the "strict" parameter
> > enter_repo() takes to selectively disable the ownership checks only
> > for upload-pack, or something like that.
> 
> So, here is a rough sketch for the above.  Interested parties may
> build on top of it, perhaps by adding separate knobs to loosen or
> tighten the second parameter given to enter_repo() at different
> callsites, by writing tests to make sure they work as intended, and
> by documenting the security story around it none of which I do here
> ;-).

This looks roughly like I'd expect, but...

> diff --git c/builtin/upload-pack.c w/builtin/upload-pack.c
> index 46d93278d9..fe50ce3eed 100644
> --- c/builtin/upload-pack.c
> +++ w/builtin/upload-pack.c
> @@ -36,6 +36,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();
> @@ -51,7 +52,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()) {

...this is doing that loosening for upload-pack by default. I'm not sure
if that is OK or not. My mental model has remained "it is OK to run
upload-pack on an untrusted repository", but it would make sense to get
input from folks who looked at this in the past, like Dscho, and/or to
reassess the threat model from scratch.

In particular I did not follow all of the potential issues with linked
local files. Are we good now after other fixes (in which case this patch
is OK)? Are we good only for non-local clones (so this patch is OK only
combined with a fix for clone to check ownership for --local mode)? Or
are there still problems if an attacker controls the repo paths, in
which case upload-pack should remain conservative?

-Peff

Re: Git clone reads safe.directory differently?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... if that is OK or not. My mental model has remained "it is OK to run
> upload-pack on an untrusted repository", but it would make sense to get
> input from folks who looked at this in the past, like Dscho, and/or to
> reassess the threat model from scratch.
>
> In particular I did not follow all of the potential issues with linked
> local files. Are we good now after other fixes (in which case this patch
> is OK)? Are we good only for non-local clones (so this patch is OK only
> combined with a fix for clone to check ownership for --local mode)? Or
> are there still problems if an attacker controls the repo paths, in
> which case upload-pack should remain conservative?

Good questions.

Re: Git clone reads safe.directory differently?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

On 2024-08-01 at 06:14:17, Jeff King wrote:
> ...this is doing that loosening for upload-pack by default. I'm not sure
> if that is OK or not. My mental model has remained "it is OK to run
> upload-pack on an untrusted repository", but it would make sense to get
> input from folks who looked at this in the past, like Dscho, and/or to
> reassess the threat model from scratch.
> 
> In particular I did not follow all of the potential issues with linked
> local files. Are we good now after other fixes (in which case this patch
> is OK)? Are we good only for non-local clones (so this patch is OK only
> combined with a fix for clone to check ownership for --local mode)? Or
> are there still problems if an attacker controls the repo paths, in
> which case upload-pack should remain conservative?

I think we already have such a patch.  In clone, clone_local either has
option_shared (in which case we simply refer to the other repository via
an alternates file and don't touch it in any way), or it calls
copy_or_link_directory, which in turn calls die_upon_dubious_ownership.

The other case, where is_local is not set (and thus clone_local is not
called), calls transport_fetch_refs, which either calls
fetch_refs_via_pack or fetch_refs_via_bundle, both of which I assume
actually make a git-upload-pack call.

One related topic that is potentially interesting as well is whether
`git bundle create` also offers the same security guarantees as `git
upload-pack` in that it can be safely run on an untrusted repository.
Either way, we may want to document that.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: Git clone reads safe.directory differently?

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> The other case, where is_local is not set (and thus clone_local is not
> called), calls transport_fetch_refs, which either calls
> fetch_refs_via_pack or fetch_refs_via_bundle, both of which I assume
> actually make a git-upload-pack call.

OK.

> One related topic that is potentially interesting as well is whether
> `git bundle create` also offers the same security guarantees as `git
> upload-pack` in that it can be safely run on an untrusted repository.
> Either way, we may want to document that.

True.  I think "bundle create" in that regard can be viewed as a
thin wrapper around pack-objects and there is no customization
possibilities (smudge/clean filters, hooks, etc.) that malicious
repositories can take advantage of.

But what worries me more is the fact that any such evaluation can
only be about the current state.  A careless change to say
pack-objects [*] that allows innocent looking customzation to take
place _could_ turn out to be triggerable by the repository when
upload-pack is run, and the "innocent looking" customization may be
more generic than necessary and can be used creatively to cause
damage.  "Don't allow any customizations to 'rev-list' because its
internal is shared with 'pack-objects' that in turn is run from
'upload-pack'" would not be an answer.

It is unclear to me how to make sure such an evaluation done once in
the past will stay valid.  That is something we need to come up with
a viable approach and document, too.


[Footnote]

 * ... or rev-list or any pieces of machinery that are recursively
   relied on by a command that ought to be kept safe.

Re: Git clone reads safe.directory differently?

[Jeff King]

On Thu, Aug 01, 2024 at 09:26:07PM +0000, brian m. carlson wrote:

> On 2024-08-01 at 06:14:17, Jeff King wrote:
> > ...this is doing that loosening for upload-pack by default. I'm not sure
> > if that is OK or not. My mental model has remained "it is OK to run
> > upload-pack on an untrusted repository", but it would make sense to get
> > input from folks who looked at this in the past, like Dscho, and/or to
> > reassess the threat model from scratch.
> > 
> > In particular I did not follow all of the potential issues with linked
> > local files. Are we good now after other fixes (in which case this patch
> > is OK)? Are we good only for non-local clones (so this patch is OK only
> > combined with a fix for clone to check ownership for --local mode)? Or
> > are there still problems if an attacker controls the repo paths, in
> > which case upload-pack should remain conservative?
> 
> I think we already have such a patch.  In clone, clone_local either has
> option_shared (in which case we simply refer to the other repository via
> an alternates file and don't touch it in any way), or it calls
> copy_or_link_directory, which in turn calls die_upon_dubious_ownership.

Ah, thanks, I didn't realize that. Looks like it comes from 1204e1a824
(builtin/clone: refuse local clones of unsafe repositories, 2024-04-15).
I'm not sure if this is redundant currently; we still run
git-upload-pack even in the --local case, to get the list of refs. So
presumably it would fail on the same ownership check before even getting
to the local copy code.

But regardless, it would not be redundant if we loosen upload-pack. And
it means that my first two questions have the same answer.

The third one (does upload-pack have race problems with an attacker who
owns the repo?) I'm still not sure of.

> One related topic that is potentially interesting as well is whether
> `git bundle create` also offers the same security guarantees as `git
> upload-pack` in that it can be safely run on an untrusted repository.
> Either way, we may want to document that.

I suspect it could be made to give similar guarantees, but I don't think
anybody has ever made a conscious effort. At the very least this isn't
great:

  $ git config pager.bundle 'echo yikes'
  $ git bundle create foo.bundle --all
  yikes

upload-pack doesn't trigger this because the pager setup is tied to
the builtin RUN_SETUP option (arguably it should be made more explicit,
though). I think pack-objects actually exhibits the same problem, but
the pager is only triggered if isatty(1), and that would never be the
case when upload-pack runs it.

-Peff

Re: Git clone reads safe.directory differently?

[W. Michael Petullo]

Thank you---from a somewhat sophisticated Git user, but not a Git
developer---for all of the discussion that followed my initial inquiry.
I thought I would follow up with some comments that follow from my reading
of the responses and the SECURITY sections of various man pages.

> [From Jeff:] So there's an open question on the degree to which
> running upload-pack is actually dangerous. It's not _supposed_ to be,
> but the ownership check is a defense-in-depth approach to safety.

The discussion here is partially an attempt to better understand some
of the internal workings and guarantees to determine whether or not the
protections applied in some cases are spurious, right?

> [From Brian:] Both of these commands should work correctly and do not,
> and that's a bug (assuming tr1 is owned by a different user):
>
>   git clone --no-local --no-hardlinks $PWD/tr1 tr2
>   git clone --no-local --no-hardlinks ssh://localhost$PWD/tr1 tr2

The remarks above closely resemble the use cases that prompted my
initial inquiry. It would help a great deal if these worked, and also if
"--no-local --no-hardlinks" remains the default for the SSH variant.
Is it fair to conclude that these not working represent a bug?

> [From Brian:] The git(1) manual page also says this:
>   [...] but beware that the surface area for attack against
>   `upload-pack` is large

Does the use of attack surface here mean the general idea of "the volume
of lines of code in upload-pack is large and thus upload-pack likely
contains bugs that might represent vulnerabilities", or is there
something specific about upload-pack that should raise concern?

I do have a related question on the SSH/push side. Let us imagine a host
that has user accounts bearing git-shell as their shell. Thus these users
can only interact with the host (and the repositories therein) using Git.
Further imagine a trusted agent on the host creates these repositories and
any hooks therein. Can a special account, with permission to access all
of the repositories, safely clone and push other users' repositories? Or,
could a user introduce a dangerous hook or configuration option to their
remote repository using only Git by way of git-shell?

Thank you again, especially Jeff, who provided me with some practical
workarounds early in this thread.

-- 
Mike

:wq

Re: Git clone reads safe.directory differently?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I suspect it could be made to give similar guarantees, but I don't think
> anybody has ever made a conscious effort.

I also recall that we thought upload-pack was safe because it does
nothing more than what it was asked to do, until we realized that in
a lazy clone it would slurp what it is missing from its promisors,
at which point it does more than just "serve things from here".

What worries me more is the fact that any such evaluation can only
be about the current state.  A careless change to say pack-objects
that allows innocent looking customization to take place _could_
turn out to be triggerable by the repository when upload-pack is
run, and the "innocent looking" customization may be more generic
than necessary and can be used creatively to cause damage.  "Don't
allow any customizations to 'rev-list' because its internal is
shared with 'pack-objects' that in turn is run from 'upload-pack'"
would not be an answer.

It is unclear to me how to make sure such an evaluation, that was
done once in the past, will stay valid.  That is something we need
to come up with a viable approach and document, too.

Thanks.

Re: Git clone reads safe.directory differently?

[Jeff King]

On Wed, Jul 31, 2024 at 09:23:49AM -0700, Junio C Hamano wrote:

> > And then upload-pack
> > could be free to drop the ownership check. Certainly a lot of people
> > have complained about it (I had actually thought we reverted it in
> > v2.45.2, but that was just the extra hooks defense-in-depth; so again, I
> > may be getting confused about the extra value of the enter_repo()
> > ownership check that came at the same time).
> 
> As enter_repo() is about the protocol driver thing and not about
> normal users working inside a repository, calls to it appear only in
> receive-pack, upload-pack, upload-archive, http-backend, and daemon.
> 
> Among them, upload-pack is the only thing we promise that is safe to
> work even in a hostile repository?  If we push into a repository
> over the local transport, we would trigger post-receive hook as
> ourselves, which we would probably not want.  The same story goes
> for daemon, http-backend, and upload-archive.

Yes, upload-pack is the only safe one.

> So we probably need to add another axis to the "strict" parameter
> enter_repo() takes to selectively disable the ownership checks only
> for upload-pack, or something like that.

Agreed.

> We may want to restrict "tar.<format>.command" only to protected
> configuration and then we may be able to loosen the ownership check
> for the upload-archive command.

Yes, though I don't now how valuable that is in practice (versus the
regression for folks who have a custom tar.*.command in their local-repo
config).

-Peff

Re: Git clone reads safe.directory differently?

[Jeff King]

On Tue, Jul 30, 2024 at 06:37:47AM -0500, W. Michael Petullo wrote:

> A little feedback to regarding why I chose to avoid .gitconfig along
> with some consequences:
> 
> I have a system built around Git in which I use the permissions approaches
> I have described in this email thread. The downside of .gitconfig is
> that its effect is global. I fear I might lose track of the fact that I
> have turned off a security protection in a global way, possibly years
> down the road as I maintain my system.  Instead, I went about this by
> patching particular uses of Git to use "-c" and so on. I only needed to
> do this at two or three places my code executes Git out of dozens.
> 
> This makes my intention clear in the code (that executes Git) and lessens
> the likelihood of me (a year or so later?) forgetting I am responsible
> for ensuring the circumstances leave my use of safe.directory safe.

Makes sense, though I think it's somewhat specific to your setup / use
case. If you had arbitrary clients connecting, telling them to use "-u"
probably wouldn't scale.

> I suppose the inconsistency surrounding "-c" that we have discussed sort
> of works against my approach. Indeed, I wonder if in an ideal world we
> should remove the dangerous features that have good intentions
> (hardlinks and so on) and instead require users to opt in to them.
> I say ideal world because this ignores backward/forward compatibility
> issues.

So there's an open question on the degree to which running upload-pack
is actually dangerous. It's not _supposed_ to be, but the ownership
check is a defense-in-depth approach to safety.

> Perhaps a compromise would be to tie safe.directory to the type
> of source directory given to clone. Would a remote URL be enough to
> turn off the safe.directory checks on a clone, similar to the effect of
> a remote URL on --local/--no-local?

It already is tied to the URL, in the sense that "clone" is not doing
anything at all. It is entirely the server upload-pack that has started
doing the ownership check (as of v2.45). For a local clone, the "server"
is just another process owned by your user on the same machine, and
that's where most people are running into it.

But for all other remotes, the ownership issue is one for the server to
deal with. So server operators serving over http or via git-daemon would
need to make sure their daemon processes match the on-disk repository
ownership (or use safe.directory to get around it). And there really is
_potential_ danger there. The server-side daemon process is running
upload-pack against a repository owned by some other user. If you're
serving untrusted repositories that way (say, a system git-daemon with
repositories owned by users), there could be a mismatch there (but
again, this is defense-in-depth and upload-pack is supposed to be safe
here).

Cloning over ssh is sort of a middle ground. It still is the server's
problem, and the "user" in this case is the user on the server side, not
the user on the local machine running "git clone"). But often that user
is conceptually the same entity (but doesn't have to be; as you can
imagine, GitHub's ssh endpoint does not have one Unix user per
conceptual user).

> The current behavior seems to mean:
> 
> 	(1) -c safe.directory works for some sub-commands, but not clone,

Also fetch, push, ls-remote, etc. Anything dealing with a "remote"
repository which spawn extra processes that clear the environment of
"-c" config.

> 	(2) clone requires your additional workaround, and 
> 
> 	(3) your workaround does not work with SSH/git-shell for the
> 	obvious reason that git-shell needs to limit the "commands" it
> 	will run; this requires another level of intervention.

Right. The workaround does work, though, if you're not using git-shell.

> Another thing I came across was in the git-clone man page:
> 
> 	"When given, and the repository to clone from is accessed via ssh,
> 	this specifies a non-default path for the command run on the other end."
> 
> Your "-u 'git -c safe.directory=X upload-pack'" workaround uses
> upload-pack in the absence of SSH, so I wonder if "and the repository
> to clone from is accessed via ssh" is inaccurate.

Yes, I think that documentation is overly specific. The "-u" command is
used whenever we can ask the other side to run an arbitrary command. So
local clones, as well as ssh. Not git-daemon or http, where we just tell
the other side the type of operation to perform.

-Peff