Re: [PATCH v2] upload-pack: allow shallow fetching from a read-only repository
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 4, 2014, 18:10 UTC
- Message-ID
- <xmqqr46hltrl.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1393936205-15953-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 16 quoted lines
> 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.
Show 53 quoted lines
> 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?
Show 28 quoted lines
> + 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;
> }