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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 27, 2014, 17:59 UTC
Message-ID: <xmqq61jm2pz7.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqa98y2q8k.fsf@gitster.dls.corp.google.com>

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

Show 16 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> 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"?

Actually, the textual symrefs have been invented to replace symbolic links used for .git/HEAD on symlink-incapable filesystems, and we do even not let the filesystem follow symlinks. The rule we have there are like so:

		/* Follow "normalized" - ie "refs/.." symlinks by hand */
		if (S_ISLNK(st.st_mode)) {
			len = readlink(path, buffer, sizeof(buffer)-1);
			if (len < 0) {
				if (errno == ENOENT || errno == EINVAL)
					/* inconsistent with lstat; retry */
					goto stat_ref;
				else
					return NULL;
			}
			buffer[len] = 0;
			if (starts_with(buffer, "refs/") &&
					!check_refname_format(buffer, 0)) {
				strcpy(refname_buffer, buffer);
				refname = refname_buffer;
				if (flag)
					*flag |= REF_ISSYMREF;
				continue;
			}
		}

So we should do exactly the same check, I would think, no?

In a typical clone, the ".git/refs/remotes/origin/HEAD" textual symref stores "ref: refs/remotes/origin/master" and it is neither "ref: master" nor "ref: ./master", so it should be sensible to insist on "must start with 'refs/' and its format valid."