Jeff, We have a fix like this in the next set of transaction updates. https://code-review.googlesource.com/#/c/1012/13/fast-import.c However, if your concerns are the integrity of the servers and not taking any chances you might not want to wait for my patches to graduate. ronnie sahlberg On Fri, Aug 22, 2014 at 10:32 PM, Jeff King <peff@xxxxxxxx> wrote: > When creating a new annotated tag, we sprintf the refname > into a static-sized buffer. If we have an absurdly long > tagname, like: > > git init repo && > cd repo && > git commit --allow-empty -m foo && > git tag -m message mytag && > git fast-export mytag | > perl -lpe '/^tag/ and s/mytag/"a" x 8192/e' | > git fast-import <input > > we'll overflow the buffer. We can fix it by using a strbuf. > > Signed-off-by: Jeff King <peff@xxxxxxxx> > --- > I'm not sure how easily exploitable this is. The buffer is on the stack, > and we definitely demolish the return address. But we never actually > return from the function, since lock_ref_sha1 will fail in such a case > and die (it cannot succeed because the name is longer than PATH_MAX, > which we check when concatenating it with $GIT_DIR). > > Still, there is no limit to the size of buffer you can feed it, so it's > entirely possible you can overwrite something else and cause some > mischief. So I wouldn't call it trivially exploitable, but I would not > bet my servers that it is not (and of course it is easy to trigger if > you can convince somebody to run fast-import a stream, so any remote > helpers produce a potentially vulnerable situation). > > fast-import.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/fast-import.c b/fast-import.c > index f25a4ae..a1479e9 100644 > --- a/fast-import.c > +++ b/fast-import.c > @@ -1734,14 +1734,16 @@ static void dump_tags(void) > static const char *msg = "fast-import"; > struct tag *t; > struct ref_lock *lock; > - char ref_name[PATH_MAX]; > + struct strbuf ref_name = STRBUF_INIT; > > for (t = first_tag; t; t = t->next_tag) { > - sprintf(ref_name, "tags/%s", t->name); > - lock = lock_ref_sha1(ref_name, NULL); > + strbuf_reset(&ref_name); > + strbuf_addf(&ref_name, "tags/%s", t->name); > + lock = lock_ref_sha1(ref_name.buf, NULL); > if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0) > - failure |= error("Unable to update %s", ref_name); > + failure |= error("Unable to update %s", ref_name.buf); > } > + strbuf_release(&ref_name); > } > > static void dump_marks_helper(FILE *f, > -- > 2.1.0.346.ga0367b9 > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html