* [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs @ 2014-06-27 11:01 Michael Haggerty 2014-06-27 17:53 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: Michael Haggerty @ 2014-06-27 11:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jens Lehmann, git, Michael Haggerty 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. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> --- 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); } -- 2.0.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs 2014-06-27 11:01 [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs Michael Haggerty @ 2014-06-27 17:53 ` Junio C Hamano 2014-06-27 17:59 ` Junio C Hamano 0 siblings, 1 reply; 4+ messages in thread From: Junio C Hamano @ 2014-06-27 17:53 UTC (permalink / raw) To: Michael Haggerty; +Cc: Jens Lehmann, git 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"? > 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); > } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs 2014-06-27 17:53 ` Junio C Hamano @ 2014-06-27 17:59 ` Junio C Hamano 2014-06-28 5:34 ` Michael Haggerty 0 siblings, 1 reply; 4+ messages in thread From: Junio C Hamano @ 2014-06-27 17:59 UTC (permalink / raw) To: Michael Haggerty; +Cc: Jens Lehmann, git Junio C Hamano <gitster@pobox.com> writes: > 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." ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs 2014-06-27 17:59 ` Junio C Hamano @ 2014-06-28 5:34 ` Michael Haggerty 0 siblings, 0 replies; 4+ messages in thread From: Michael Haggerty @ 2014-06-28 5:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jens Lehmann, git On 06/27/2014 07:59 PM, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> 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"? I've never seen that usage, nor seen it advocated. Symrefs are not propagated via the Git protocol, so even if somebody were doing this privately, it could hardly be a project-wide practice. I can't think of a practical use for this feature. And it would be mildly annoying to implement. So my inclination is to forbid it. > 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? I think you overlooked that if the (starts_with() && !check_refname_format()) check fails, execution falls through, ending up here: /* * Anything else, just open it and try to use it as * a ref */ fd = open(path, O_RDONLY); if (fd < 0) { if (errno == ENOENT) /* inconsistent with lstat; retry */ goto stat_ref; else return NULL; } len = read_in_full(fd, buffer, sizeof(buffer)-1); close(fd); [...etc...] This has been the behavior since time immemorial [1]. In fact, another bug (which I probably introduced) is that in the case of a symlink that points at a non-existent file, this code goes into an infinite loop due to the "if (errno == ENOENT) goto stat_ref" in the code that I quoted. My mistake was forgetting that lstat() is statting the link whereas open() follows the link, so the success of the former does not imply that the latter should not ENOENT. I suggest we fix both problems by making the code behave the way you *thought* it behaves: symlinks are never followed via the filesystem, but if the symlink contents have the form of a legitimate refname that starts with "refs/", then we follow it the same way as we would follow a textual-style symref. > 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." Yes, we don't even have a notation for "relative refnames" because we would have no way to distinguish them from "absolute refnames" except maybe via some artifice like a "./" prefix. Michael [1] Where by "time immemorial" I mean "since before I ever touched refs.c". -- Michael Haggerty mhagger@alum.mit.edu ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-06-28 5:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-27 11:01 [PATCH] resolve_gitlink_ref_recursive(): verify format of symbolic refs Michael Haggerty 2014-06-27 17:53 ` Junio C Hamano 2014-06-27 17:59 ` Junio C Hamano 2014-06-28 5:34 ` Michael Haggerty
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.