On Tue, Nov 10 2020, Demi M. Obenour wrote:
> 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;
