* [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.