Re: [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 27, 2014, 17:53 UTC
Message-ID: <xmqqa98y2q8k.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1403866877-15733-1-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 8 quoted lines

> When reading a symbolic ref via resolve_gitlink_ref_recursive(), check
> that the reference name that is pointed at is formatted correctly,
> using the same check as resolve_ref_unsafe() uses for non-gitlink
> references.  This prevents bogosity like
>
>     ref: ../../other/file
>
> from causing problems.

I do agree that a textual symref "ref: ../../x/y" that is stored in ".git/HEAD" or in ".git/refs/L" will step outside ".git/" and it is problematic. But if ".git/refs/heads/a/b/LINK" has "ref: ../../x" in it, shouldn't we interpret it as referring to the ref at "refs/heads/x"?

Show 19 quoted lines

> Given that symbolic references cannot be transferred via the Git
> protocol, I do not expect this bug to be exploitable.
>
>  refs.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index dc45774..7da8e7d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1273,6 +1273,9 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
>  	while (isspace(*p))
>  		p++;
>  
> +	if (check_refname_format(p, REFNAME_ALLOW_ONELEVEL))
> +		return -1;
> +
>  	return resolve_gitlink_ref_recursive(refs, p, sha1, recursion+1);
>  }