Description of problem: cgit segfaults Version-Release number of selected component (if applicable): 0.9.0.2-3.fc17 How reproducible: every time I posted the following to the upstream mailing list in April, but it appears not to have reached the list. ================================================= This started when I noticed some cgit segfaults on savannah.gnu.org. Finding the offending URL/commit and then constructing a stand-alone reproducer were far more time-consuming than writing the actual patch. The problem arises with a commit like this, in which the user name part of the "Author" field is empty: $ git log -1 commit 6f3f41d73393278f3ede68a2cb1e7a2a23fa3421 Author: <T> Date: Mon Apr 23 22:29:16 2012 +0200 Here's what happens: (this is due to buf=malloc(0); strncpy (buf, head, -1); where "head" may point to plenty of attacker-specified non-NUL bytes, so we can overwrite a zero-length heap buffer with arbitrary data) ==3227== Invalid write of size 1 ==3227== at 0x4A09361: strncpy (mc_replace_strmem.c:463) ==3227== by 0x408977: substr (parsing.c:61) ==3227== by 0x4089EF: parse_user (parsing.c:73) ==3227== by 0x408D10: cgit_parse_commit (parsing.c:153) ==3227== by 0x40A540: cgit_mk_refinfo (shared.c:171) ==3227== by 0x40A581: cgit_refs_cb (shared.c:181) ==3227== by 0x43DEB3: do_for_each_ref (refs.c:690) ==3227== by 0x41075E: cgit_print_branches (ui-refs.c:191) ==3227== by 0x416EF2: cgit_print_summary (ui-summary.c:56) ==3227== by 0x40780A: summary_fn (cmd.c:120) ==3227== by 0x40667A: process_request (cgit.c:544) ==3227== by 0x404078: cache_process (cache.c:322) ==3227== Address 0x4c718d0 is 0 bytes after a block of size 0 alloc'd ==3227== at 0x4A0884D: malloc (vg_replace_malloc.c:263) ==3227== by 0x455C85: xmalloc (wrapper.c:35) ==3227== by 0x40894C: substr (parsing.c:60) ==3227== by 0x4089EF: parse_user (parsing.c:73) ==3227== by 0x408D10: cgit_parse_commit (parsing.c:153) ==3227== by 0x40A540: cgit_mk_refinfo (shared.c:171) ==3227== by 0x40A581: cgit_refs_cb (shared.c:181) ==3227== by 0x43DEB3: do_for_each_ref (refs.c:690) ==3227== by 0x41075E: cgit_print_branches (ui-refs.c:191) ==3227== by 0x416EF2: cgit_print_summary (ui-summary.c:56) ==3227== by 0x40780A: summary_fn (cmd.c:120) ==3227== by 0x40667A: process_request (cgit.c:544) ==3227== ==3227== Invalid write of size 1 ==3227== at 0x4A09400: strncpy (mc_replace_strmem.c:463) ==3227== by 0x408977: substr (parsing.c:61) ==3227== by 0x4089EF: parse_user (parsing.c:73) ==3227== by 0x408D10: cgit_parse_commit (parsing.c:153) ==3227== by 0x40A540: cgit_mk_refinfo (shared.c:171) ==3227== by 0x40A581: cgit_refs_cb (shared.c:181) ==3227== by 0x43DEB3: do_for_each_ref (refs.c:690) ==3227== by 0x41075E: cgit_print_branches (ui-refs.c:191) ==3227== by 0x416EF2: cgit_print_summary (ui-summary.c:56) ==3227== by 0x40780A: summary_fn (cmd.c:120) ==3227== by 0x40667A: process_request (cgit.c:544) ==3227== by 0x404078: cache_process (cache.c:322) ==3227== Address 0x4c7192b is not stack'd, malloc'd or (recently) free'd ==3227== ==3227== Invalid write of size 1 ==3227== at 0x4A0940E: strncpy (mc_replace_strmem.c:463) ==3227== by 0x408977: substr (parsing.c:61) ==3227== by 0x4089EF: parse_user (parsing.c:73) ==3227== by 0x408D10: cgit_parse_commit (parsing.c:153) ==3227== by 0x40A540: cgit_mk_refinfo (shared.c:171) ==3227== by 0x40A581: cgit_refs_cb (shared.c:181) ==3227== by 0x43DEB3: do_for_each_ref (refs.c:690) ==3227== by 0x41075E: cgit_print_branches (ui-refs.c:191) ==3227== by 0x416EF2: cgit_print_summary (ui-summary.c:56) ==3227== by 0x40780A: summary_fn (cmd.c:120) ==3227== by 0x40667A: process_request (cgit.c:544) ==3227== by 0x404078: cache_process (cache.c:322) ==3227== Address 0x4c7192d is not stack'd, malloc'd or (recently) free'd ==3227== ==3227== ==3227== Process terminating with default action of signal 11 (SIGSEGV) ==3227== Access not within mapped region at address 0x502F000 ==3227== at 0x4A09400: strncpy (mc_replace_strmem.c:463) ==3227== by 0x408977: substr (parsing.c:61) ==3227== by 0x4089EF: parse_user (parsing.c:73) ==3227== by 0x408D10: cgit_parse_commit (parsing.c:153) ==3227== by 0x40A540: cgit_mk_refinfo (shared.c:171) ==3227== by 0x40A581: cgit_refs_cb (shared.c:181) ==3227== by 0x43DEB3: do_for_each_ref (refs.c:690) ==3227== by 0x41075E: cgit_print_branches (ui-refs.c:191) ==3227== by 0x416EF2: cgit_print_summary (ui-summary.c:56) ==3227== by 0x40780A: summary_fn (cmd.c:120) ==3227== by 0x40667A: process_request (cgit.c:544) ==3227== by 0x404078: cache_process (cache.c:322) This happens when tail - head == -1 here: (parsing.c) char *substr(const char *head, const char *tail) { char *buf; buf = xmalloc(tail - head + 1); strncpy(buf, head, tail - head); buf[tail - head] = '\0'; return buf; } char *parse_user(char *t, char **name, char **email, unsigned long *date) { char *p = t; int mode = 1; while (p && *p) { if (mode == 1 && *p == '<') { *name = substr(t, p - 1); t = p; mode++; } else if (mode == 1 && *p == '\n') { Here's a fix: From 8c389f87d43a3c8fbf3763e87193032a65a49952 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering> Date: Mon, 23 Apr 2012 22:06:35 +0200 Subject: [PATCH] do not write outside heap buffer * parsing.c (substr): Handle tail < head. --- parsing.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/parsing.c b/parsing.c index 602e3de..1b2a551 100644 --- a/parsing.c +++ b/parsing.c @@ -56,6 +56,8 @@ char *substr(const char *head, const char *tail) { char *buf; + if (tail < head) + return xstrdup(""); buf = xmalloc(tail - head + 1); strncpy(buf, head, tail - head); buf[tail - head] = '\0'; -- 1.7.10.228.g81f95 And here's the reproducer: It was tricky to reproduce, because git prohibits use of an empty "name" in a commit ID. To construct the offending commit, I had to resort to using "git hash-object". git init -q foo && ( cd foo && echo a > j && git add . && git ci -q --author='au <T>' -m. . && h=$(git cat-file commit HEAD|sed 's/au //' \ |git hash-object -t commit -w --stdin) && git co -q -b test $h && git br -q -D master && git br -q -m test master) git clone -q --bare foo foo.git cat <<EOF > in repo.url=foo.git repo.path=foo.git EOF CGIT_CONFIG=in QUERY_STRING=url=foo.git valgrind ./cgit The valgrind output is what you see above. AFAICS, this is not exploitable thanks (ironically) to the use of strncpy. Since that -1 translates to SIZE_MAX and this is strncpy, not only does it copy whatever is in "head" (up to first NUL), but it also writes SIZE_MAX - strlen(head) NUL bytes into the destination buffer, and that latter is guaranteed to evoke a segfault. Since cgit is single-threaded, AFAICS, there is no way that the buffer clobbering can be turned into an exploit.
FYI, posted upstream, http://hjemli.net/pipermail/cgit/2012-July/000652.html but afaics, still not on any branch in the git repository.
This needs to be corrected in all versions of Fedora and EPEL (not just rawhide). Please see bug #862035 for the security bug (CVE assignment pending). I'm going to turn this into a tracking bug for all versions of Fedora.
cgit-0.9.1-1.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/cgit-0.9.1-1.fc18
cgit-0.9.1-1.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/cgit-0.9.1-1.fc17
cgit-0.9.1-1.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/cgit-0.9.1-1.fc16
cgit-0.9.1-1.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/cgit-0.9.1-1.el6
cgit-0.9.1-1.el5 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/cgit-0.9.1-1.el5
Package cgit-0.9.1-1.el6: * should fix your issue, * was pushed to the Fedora EPEL 6 testing repository, * should be available at your local mirror within two days. Update it with: # su -c 'yum update --enablerepo=epel-testing cgit-0.9.1-1.el6' as soon as you are able to. Please go to the following url: https://admin.fedoraproject.org/updates/FEDORA-EPEL-2012-13477/cgit-0.9.1-1.el6 then log in and leave karma (feedback).
cgit-0.9.1-2.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/cgit-0.9.1-2.fc18
cgit-0.9.1-2.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/cgit-0.9.1-2.fc17
cgit-0.9.1-2.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/cgit-0.9.1-2.el6
cgit-0.9.1-2.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/cgit-0.9.1-2.fc16
cgit-0.9.1-2.fc18 has been pushed to the Fedora 18 stable repository. If problems still persist, please make note of it in this bug report.
cgit-0.9.1-2.fc17 has been pushed to the Fedora 17 stable repository. If problems still persist, please make note of it in this bug report.
cgit-0.9.1-2.fc16 has been pushed to the Fedora 16 stable repository. If problems still persist, please make note of it in this bug report.
cgit-0.9.1-1.el5 has been pushed to the Fedora EPEL 5 stable repository. If problems still persist, please make note of it in this bug report.
cgit-0.9.1-2.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.