This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 820733 - avoid stack-smash when processing unusual commit [fedora-all]
avoid stack-smash when processing unusual commit [fedora-all]
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: cgit (Show other bugs)
rawhide
Unspecified All
unspecified Severity unspecified
: ---
: ---
Assigned To: Todd Zullinger
Fedora Extras Quality Assurance
: Security, SecurityTracking
Depends On:
Blocks: CVE-2012-4465
  Show dependency treegraph
 
Reported: 2012-05-10 15:56 EDT by Jim Meyering
Modified: 2016-04-26 11:15 EDT (History)
2 users (show)

See Also:
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:


Attachments (Terms of Use)

  None (edit)
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.

Note You need to log in before you can comment on or make changes to this bug.