From cf2e095cbd208ea72d8e0ca5a9aff531e1dc2537cba341f9b24bab4bb20853b6 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Wed, 10 Oct 2012 16:22:11 +0000 Subject: [PATCH] Accepting request 137759 from home:tiwai:branches:devel:tools:scm - Fix VUL-0: specially-crafted commits can trigger a heap-based buffer overflow (CVE-2012-4465, bnc#783012) OBS-URL: https://build.opensuse.org/request/show/137759 OBS-URL: https://build.opensuse.org/package/show/devel:tools:scm/cgit?expand=0&rev=13 --- cgit-CVE-2012-4465-fix.diff | 171 ++++++++++++++++++++++++++++++++++++ cgit.changes | 6 ++ cgit.spec | 6 +- 3 files changed, 181 insertions(+), 2 deletions(-) create mode 100644 cgit-CVE-2012-4465-fix.diff diff --git a/cgit-CVE-2012-4465-fix.diff b/cgit-CVE-2012-4465-fix.diff new file mode 100644 index 0000000..035efdb --- /dev/null +++ b/cgit-CVE-2012-4465-fix.diff @@ -0,0 +1,171 @@ +From 7757d1b046ecb67b830151d20715c658867df1ec Mon Sep 17 00:00:00 2001 +From: Jim Meyering +Date: Mon, 23 Apr 2012 20:06:35 +0000 +Subject: do not write outside heap buffer + +* parsing.c (substr): Handle tail < head. + +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: + 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) + + Invalid write of size 1 + at 0x4A09361: strncpy (mc_replace_strmem.c:463) + by 0x408977: substr (parsing.c:61) + by 0x4089EF: parse_user (parsing.c:73) + by 0x408D10: cgit_parse_commit (parsing.c:153) + by 0x40A540: cgit_mk_refinfo (shared.c:171) + by 0x40A581: cgit_refs_cb (shared.c:181) + by 0x43DEB3: do_for_each_ref (refs.c:690) + by 0x41075E: cgit_print_branches (ui-refs.c:191) + by 0x416EF2: cgit_print_summary (ui-summary.c:56) + by 0x40780A: summary_fn (cmd.c:120) + by 0x40667A: process_request (cgit.c:544) + by 0x404078: cache_process (cache.c:322) + Address 0x4c718d0 is 0 bytes after a block of size 0 alloc'd + at 0x4A0884D: malloc (vg_replace_malloc.c:263) + by 0x455C85: xmalloc (wrapper.c:35) + by 0x40894C: substr (parsing.c:60) + by 0x4089EF: parse_user (parsing.c:73) + by 0x408D10: cgit_parse_commit (parsing.c:153) + by 0x40A540: cgit_mk_refinfo (shared.c:171) + by 0x40A581: cgit_refs_cb (shared.c:181) + by 0x43DEB3: do_for_each_ref (refs.c:690) + by 0x41075E: cgit_print_branches (ui-refs.c:191) + by 0x416EF2: cgit_print_summary (ui-summary.c:56) + by 0x40780A: summary_fn (cmd.c:120) + by 0x40667A: process_request (cgit.c:544) + + Invalid write of size 1 + at 0x4A09400: strncpy (mc_replace_strmem.c:463) + by 0x408977: substr (parsing.c:61) + by 0x4089EF: parse_user (parsing.c:73) + by 0x408D10: cgit_parse_commit (parsing.c:153) + by 0x40A540: cgit_mk_refinfo (shared.c:171) + by 0x40A581: cgit_refs_cb (shared.c:181) + by 0x43DEB3: do_for_each_ref (refs.c:690) + by 0x41075E: cgit_print_branches (ui-refs.c:191) + by 0x416EF2: cgit_print_summary (ui-summary.c:56) + by 0x40780A: summary_fn (cmd.c:120) + by 0x40667A: process_request (cgit.c:544) + by 0x404078: cache_process (cache.c:322) + Address 0x4c7192b is not stack'd, malloc'd or (recently) free'd + + Invalid write of size 1 + at 0x4A0940E: strncpy (mc_replace_strmem.c:463) + by 0x408977: substr (parsing.c:61) + by 0x4089EF: parse_user (parsing.c:73) + by 0x408D10: cgit_parse_commit (parsing.c:153) + by 0x40A540: cgit_mk_refinfo (shared.c:171) + by 0x40A581: cgit_refs_cb (shared.c:181) + by 0x43DEB3: do_for_each_ref (refs.c:690) + by 0x41075E: cgit_print_branches (ui-refs.c:191) + by 0x416EF2: cgit_print_summary (ui-summary.c:56) + by 0x40780A: summary_fn (cmd.c:120) + by 0x40667A: process_request (cgit.c:544) + by 0x404078: cache_process (cache.c:322) + Address 0x4c7192d is not stack'd, malloc'd or (recently) free'd + + Process terminating with default action of signal 11 (SIGSEGV) + Access not within mapped region at address 0x502F000 + at 0x4A09400: strncpy (mc_replace_strmem.c:463) + by 0x408977: substr (parsing.c:61) + by 0x4089EF: parse_user (parsing.c:73) + by 0x408D10: cgit_parse_commit (parsing.c:153) + by 0x40A540: cgit_mk_refinfo (shared.c:171) + by 0x40A581: cgit_refs_cb (shared.c:181) + by 0x43DEB3: do_for_each_ref (refs.c:690) + by 0x41075E: cgit_print_branches (ui-refs.c:191) + by 0x416EF2: cgit_print_summary (ui-summary.c:56) + by 0x40780A: summary_fn (cmd.c:120) + by 0x40667A: process_request (cgit.c:544) + 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') { + +The fix is to handle the case of (tail < head) before calling xmalloc, +thus avoiding passing an invalid value to xmalloc. + +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 ' -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 < 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. +--- +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'; +-- +cgit v0.9.0.3-79-g88f8 diff --git a/cgit.changes b/cgit.changes index f902fe6..e21bf3b 100644 --- a/cgit.changes +++ b/cgit.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Oct 10 15:22:03 CEST 2012 - tiwai@suse.de + +- Fix VUL-0: specially-crafted commits can trigger a heap-based + buffer overflow (CVE-2012-4465, bnc#783012) + ------------------------------------------------------------------- Mon Feb 13 10:44:54 UTC 2012 - coolo@suse.com diff --git a/cgit.spec b/cgit.spec index e9685ad..b0fdbeb 100644 --- a/cgit.spec +++ b/cgit.spec @@ -33,8 +33,9 @@ Source2: cgitrc Patch: cgit-optflags.diff Patch1: cgit-git-1.7.6_build_fix.patch Patch2: cgit-CVE-2011-2711-fix.diff -Patch3: cgit-fix-print-tree.diff -Patch4: cgit-fix-more-read_tree_recursive-invocations.diff +Patch3: cgit-fix-print-tree.diff +Patch4: cgit-fix-more-read_tree_recursive-invocations.diff +Patch5: cgit-CVE-2012-4465-fix.diff # Requirements for cgit BuildRequires: gnu-crypto libopenssl-devel libzip-devel # Requirements for cgitrc man page generation @@ -57,6 +58,7 @@ Authors: %patch2 -p1 %patch3 %patch4 +%patch5 -p1 rm -rf git mv git-%{git_version} git