Bug 820733

Summary: avoid stack-smash when processing unusual commit [fedora-all]
Product: [Fedora] Fedora Reporter: Jim Meyering <meyering>
Component: cgitAssignee: Todd Zullinger <tmz>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: jim, tmz
Target Milestone: ---Keywords: Security, SecurityTracking
Target Release: ---   
Hardware: Unspecified   
OS: All   
Whiteboard:
Fixed In Version: Doc Type: Release Note
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-11-23 02:18:26 EST Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 862035    

Description Jim Meyering 2012-05-10 15:56:34 EDT
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@h.or>
    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@redhat.com>
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@h.or>' -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.
Comment 1 Jim Meyering 2012-07-25 10:14:38 EDT
FYI, posted upstream,

  http://hjemli.net/pipermail/cgit/2012-July/000652.html

but afaics, still not on any branch in the git repository.
Comment 2 Vincent Danen 2012-10-01 12:56:02 EDT
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.
Comment 3 Fedora Update System 2012-11-15 12:31:33 EST
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
Comment 4 Fedora Update System 2012-11-15 12:41:51 EST
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
Comment 5 Fedora Update System 2012-11-15 12:54:35 EST
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
Comment 6 Fedora Update System 2012-11-15 13:11:51 EST
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
Comment 7 Fedora Update System 2012-11-15 13:26:24 EST
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
Comment 8 Fedora Update System 2012-11-15 14:32:59 EST
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).
Comment 9 Fedora Update System 2012-11-17 13:13:20 EST
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
Comment 10 Fedora Update System 2012-11-17 13:26:07 EST
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
Comment 11 Fedora Update System 2012-11-17 13:38:33 EST
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
Comment 12 Fedora Update System 2012-11-17 13:39:25 EST
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
Comment 13 Fedora Update System 2012-11-23 02:18:28 EST
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.
Comment 14 Fedora Update System 2012-11-28 06:28:38 EST
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.
Comment 15 Fedora Update System 2012-11-28 06:46:37 EST
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.
Comment 16 Fedora Update System 2012-11-30 19:58:06 EST
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.
Comment 17 Fedora Update System 2012-12-02 14:36:07 EST
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.