Re: check_refname_format allows refs with components that begin with -, even though `git tag` does not

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 10, 2020, 21:33 UTC
Message-ID: <87imachp6x.fsf@evledraar.gmail.com>
In-Reply-To: <c926193b-a328-7562-6d4b-1ab2765c8cca@invisiblethingslab.com>

On Tue, Nov 10 2020, Demi M. Obenour wrote:

Show 18 quoted lines

> If I try to create a Git tag with a name beginning with `-`,
> Git complains.  However, Git does not check that a repository does
> not have tags containing `-`.  This almost led to a vulnerability
> in the QubesOS `verify-git-tag` script.  Fortunately, this was not
> exploitable, as neither `git tag -v`, `git verify-tag --raw`, nor
> `git describe` have options that are useful to an attacker.
>
> Since this could cause vulnerabilities in other programs, I initially
> reported it as an embargoed security bug, but was told to post it
> publicly.
>
> The best idea I had for a fix is to print names beginning with `-`
> using the fully-qualified form, such as "refs/tags/-a".  Also, `--`
> is used as a delimiter in many commands, and can’t be escaped,
> so disallowing it might be a good idea.
>
> In the long run, I hope to see leading dashes banned entirely, but
> backwards compatibility might prevent that.

[Re-posted from the very brief discussion on the security list]

I'm inclined to say that we might as well remove the porcelain limitation of these "-" tags and branches. In reality we support them, fsck doesn't even warn about them, and git-check-ref-format(1) doesn't document them as forbidden.

From reading 63486240108 ("disallow branch names that start with a hyphen", 2010-09-14) and 4f0accd638b ("tag: disallow '-' as tag name", 2011-05-10) I don't see why we'd need to forbid these. A WIP patch of removing the checks from strbuf_check_branch_ref() and strbuf_check_tag_ref() fails just one test.

The reason we forbade these seems to have been mainly to avoid the UI confusion of "git checkout -" or "git checkout -foo" and "git checkout -- -foo" not working. We can just make the "-" a special-case, and since then we've had "git switch -- -foo" which works, and you can do "git checkout refs/heads/-foo".

I'm probably missing some porcelain bits but with my trivial patch "git [opts] [tag|branch] -- -foo" works.

So I think we should just allow these, since at best it just leads to a false sense of security.

FWIW this is the very basic WIP patch I was playing with. Fails just one test that explicitly tests for a branch named "-naster" with check-ref-format, should probably still disallow refs named just "-" (or maybe not...).

diff --git a/builtin/tag.c b/builtin/tag.c
index ecf011776dc..86088838bb4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -369,9 +369,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 
 static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 {
-	if (name[0] == '-')
-		return -1;
-
 	strbuf_reset(sb);
 	strbuf_addf(sb, "refs/tags/%s", name);
 
diff --git a/sha1-name.c b/sha1-name.c
index 0b23b86ceb4..9a01138336c 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1592,7 +1592,7 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 	 */
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
 
-	if (*name == '-' ||
+	if (!strcmp(name, "-") ||
 	    !strcmp(sb->buf, "refs/heads/HEAD"))
 		return -1;