* avoid stack-smash when processing unusual commit
[not found] <8762cp9rq0.fsf@rho.meyering.net>
@ 2012-07-03 12:10 ` jim
2012-09-29 19:35 ` jim
0 siblings, 1 reply; 3+ messages in thread
From: jim @ 2012-07-03 12:10 UTC (permalink / raw)
Resending, now that the list is functioning again.
FTR, I also sent to hjemli at gmail.com (no reply) and finally
resorted to filing a BZ: http://bugzilla.redhat.com/820733
Jim Meyering wrote:
> 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 at 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 at 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 at 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.
^ permalink raw reply [flat|nested] 3+ messages in thread