Re: [PATCH v2] upload-pack: allow shallow fetching from a read-only repository

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Before cdab485 (upload-pack: delegate rev walking in shallow fetch to
> pack-objects - 2013-08-16) upload-pack does not write to the source
> repository. cdab485 starts to write $GIT_DIR/shallow_XXXXXX if it's a
> shallow fetch, so the source repo must be writable.
>
> git:// servers do not need write access to repos and usually don't,
> which mean cdab485 breaks shallow clone over git://
>
> Fall back to $TMPDIR if $GIT_DIR/shallow_XXXXXX cannot be created in
> this case. Note that in other cases that write $GIT_DIR/shallow_XXXXXX
> and eventually rename it to $GIT_DIR/shallow, there is no fallback to
> $TMPDIR.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Rebased on top of jk/shallow-update-fix

Hmph.

I notice that the original code, with or without this change, allows
upload-pack spawned by daemon to attempt to write into GIT_DIR.
As upload-pack is supposed to be a read-only operation, this is
quite bad.

Perhaps we should give server operators an option to run their
daemon -> upload-pack chain to always write to a throw-away
directory of their choice, without ever attempting to write to
GIT_DIR it serves?

How well is the access to the temporary shallow file controlled in
your code (sorry, but I do not recall carefully reading your patch
that added the mechanism with security issues in mind, so now I am
asking)?  When it is redirected to TMPDIR (let's forget GIT_DIR for
now---if an attacker can write into there, the repository is already
lost), can an attacker race with us to cause us to overwrite we do
not expect to?

Even if it turns out that this patch is secure enough as-is, we
definitely need to make sure that server operators, who want to keep
their upload-pack truly a read-only operation, know that it is
necessary to (1) keep the system user they run git-daemon under
incapable of writing into GIT_DIR, and (2) make sure TMPDIR points
at somewhere only git-daemon user and nobody else can write into,
somewhere in the documentation.

> diff --git a/fetch-pack.c b/fetch-pack.c
> index ae8550e..b71d186 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>  					NULL);
>  	else if (si->nr_ours || si->nr_theirs)
> -		alternate_shallow_file = setup_temporary_shallow(si->shallow);
> +		alternate_shallow_file = setup_temporary_shallow(si->shallow, 0);
>  	else
>  		alternate_shallow_file = NULL;
>  	if (get_pack(args, fd, pack_lockfile))
> diff --git a/shallow.c b/shallow.c
> index c7602ce..ad28af6 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -224,7 +224,8 @@ static void remove_temporary_shallow_on_signal(int signo)
>  	raise(signo);
>  }
>  
> -const char *setup_temporary_shallow(const struct sha1_array *extra)
> +const char *setup_temporary_shallow(const struct sha1_array *extra,
> +				    int read_only)
>  {
>  	static int installed_handler;
>  	struct strbuf sb = STRBUF_INIT;
> @@ -235,7 +236,15 @@ const char *setup_temporary_shallow(const struct sha1_array *extra)
>  
>  	if (write_shallow_commits(&sb, 0, extra)) {
>  		strbuf_addstr(&temporary_shallow, git_path("shallow_XXXXXX"));
> -		fd = xmkstemp(temporary_shallow.buf);
> +		fd = mkstemp(temporary_shallow.buf);
> +		if (read_only && fd < 0) {
> +			strbuf_grow(&temporary_shallow, PATH_MAX);
> +			fd = git_mkstemp(temporary_shallow.buf, PATH_MAX,
> +					 "shallow_XXXXXX");
> +		}
> +		if (fd < 0)
> +			die_errno("Unable to create temporary file '%s'",
> +				  temporary_shallow.buf);
>  
>  		if (!installed_handler) {
>  			atexit(remove_temporary_shallow);
> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index b0fa738..171db88 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -173,6 +173,19 @@ EOF
>  	)
>  '
>  
> +test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '

s/POSIXPERM/&,SANITY/, perhaps?

Thinking of it again, perhaps POSIXPERM should imply SANITY is required?

> +	cp -R .git read-only.git &&
> +	find read-only.git -print | xargs chmod -w &&
> +	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
> +	git clone --no-local --depth=2 read-only.git from-read-only &&
> +	git --git-dir=from-read-only/.git log --format=%s >actual &&
> +	cat >expect <<EOF &&
> +add-1-back
> +4
> +EOF
> +	test_cmp expect actual
> +'
> +
>  if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
>  	say 'skipping remaining tests, git built without http support'
>  	test_done
> diff --git a/upload-pack.c b/upload-pack.c
> index a3c52f6..b538f32 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -84,7 +84,7 @@ static void create_pack_file(void)
>  	const char *shallow_file = NULL;
>  
>  	if (shallow_nr) {
> -		shallow_file = setup_temporary_shallow(NULL);
> +		shallow_file = setup_temporary_shallow(NULL, 1);
>  		argv[arg++] = "--shallow-file";
>  		argv[arg++] = shallow_file;
>  	}