Bug 820733 - avoid stack-smash when processing unusual commit [fedora-all]
Summary: avoid stack-smash when processing unusual commit [fedora-all]
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: cgit
Version: rawhide
Hardware: Unspecified
OS: All
unspecified
unspecified
Target Milestone: ---
Assignee: Todd Zullinger
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: CVE-2012-4465
TreeView+ depends on / blocked
 
Reported: 2012-05-10 19:56 UTC by Jim Meyering
Modified: 2016-04-26 15:15 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2012-11-23 07:18:26 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Jim Meyering 2012-05-10 19:56:34 UTC
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.

Comment 1 Jim Meyering 2012-07-25 14:14:38 UTC
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 16:56:02 UTC
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 17:31:33 UTC
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 17:41:51 UTC
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 17:54:35 UTC
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 18:11:51 UTC
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 18:26:24 UTC
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 19:32:59 UTC
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 18:13:20 UTC
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 18:26:07 UTC
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 18:38:33 UTC
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 18:39:25 UTC
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 07:18:28 UTC
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 11:28:38 UTC
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 11:46:37 UTC
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-12-01 00:58:06 UTC
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 19:36:07 UTC
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.