GNU bug report logs - #19563
grep -F: fix a heap buffer (read) overrun

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 19563 in the body.
You can then email your comments to 19563 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: bug-grep <at> gnu.org
Cc: Yuliy Pisetsky <ypisetsky <at> fb.com>, Nima Aghdaii <naghdaii <at> fb.com>
Subject: grep -F: fix a heap buffer (read) overrun
Date: Sat, 10 Jan 2015 15:43:02 -0800

[Message part 1 (text/plain, inline)]

Colleagues Nima Aghdaii and Yuliy Pisetsky found and fixed a heap
buffer overrun in grep's kwset.c.  The underlying bug, already hard to
trigger, would most often result in a "mere" heap UMR (uninitialized
memory read), but Yuliy constructed inputs (see the new test) that
cause a buffer overrun.

I'm attaching two other related patches:
  - grep: avoid false-positive UMR
  - tests: add support for ASAN memory poisoning

I expect to push these by Monday.

[0001-grep-avoid-false-positive-UMR.patch (application/octet-stream, attachment)]

[0002-grep-F-fix-a-heap-buffer-read-overrun.patch (application/octet-stream, attachment)]

[0003-tests-add-support-for-ASAN-memory-poisoning.patch (application/octet-stream, attachment)]

Message #8 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>, 19563 <at> debbugs.gnu.org
Cc: Yuliy Pisetsky <ypisetsky <at> fb.com>, Nima Aghdaii <naghdaii <at> fb.com>
Subject: Re: bug#19563: grep -F: fix a heap buffer (read) overrun
Date: Sat, 10 Jan 2015 16:02:46 -0800

Jim Meyering wrote:
> +#if defined __clang__
> +# if __has_feature(address_sanitizer)
> +#  define HAVE_ASAN 1
> +# endif
> +#elif defined __GNUC__ \
> +  && (((__GNUC__ == 4) && (__GNUC_MINOR__ >= 8)) || (__GNUC__ >= 5)) \
> +  && __SANITIZE_ADDRESS__
> +# define HAVE_ASAN 1
> +#endif

How about the following instead?

#ifndef __has_feature
# define __has_feature(a) false
#endif

#if defined __SANITIZE_ADDRESS__ || __has_feature (address_sanitizer)
# define HAVE_ASAN 1
#else
# define HAVE_ASAN 0
#endif

This is what Emacs uses (its symbol is ADDRESS_SANITIZER instead of HAVE_ASAN, 
for what that's worth).

> +  ASAN_POISON_MEMORY_REGION (buflim + sizeof(uword),
> +                             bufalloc - (buflim - buffer) - sizeof(uword));
>

The two 'sizeof's need spaces afterwards.

> +#ifdef HAVE_ASAN
> +# define ASAN_POISON_MEMORY_REGION(addr, size) \
> +  __asan_poison_memory_region ((addr), (size))
> +# define ASAN_UNPOISON_MEMORY_REGION(addr, size) \
> +  __asan_unpoison_memory_region ((addr), (size))
> +#else
> +# define ASAN_POISON_MEMORY_REGION(addr, size) \
> +  (ignore_value (addr), ignore_value (size))
> +# define ASAN_UNPOISON_MEMORY_REGION(addr, size) \
> +  (ignore_value (addr), ignore_value (size))
> +#endif

I don't see the value of having macros here.  How about the following instead?

#ifndef HAVE_ASAN
static void
__asan_unpoison_memory_region (void const volatile *addr, size_t size)
{
}

static void
__asan_unpoison_memory_region (void const volatile *addr, size_t size)
{
}
#endif

And then have the callers invoke '__asan_poison_memory_region' instead of 
'ASAN_POISON_MEMORY_REGION'.  This way, there should be no need to pull in the 
ignore-value machinery, it's two less macros to worry about, and there's better 
type checking when address sanitization is not in use.

Message #11 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 19563 <at> debbugs.gnu.org, Yuliy Pisetsky <ypisetsky <at> fb.com>,
 Nima Aghdaii <naghdaii <at> fb.com>
Subject: Re: bug#19563: grep -F: fix a heap buffer (read) overrun
Date: Sat, 10 Jan 2015 16:42:28 -0800

[Message part 1 (text/plain, inline)]

On Sat, Jan 10, 2015 at 4:02 PM, Paul Eggert <eggert <at> cs.ucla.edu> wrote:
> Jim Meyering wrote:
>>
>> +#if defined __clang__
>> +# if __has_feature(address_sanitizer)
>> +#  define HAVE_ASAN 1
>> +# endif
>> +#elif defined __GNUC__ \
>> +  && (((__GNUC__ == 4) && (__GNUC_MINOR__ >= 8)) || (__GNUC__ >= 5)) \
>> +  && __SANITIZE_ADDRESS__
>> +# define HAVE_ASAN 1
>> +#endif
>
>
> How about the following instead?
>
> #ifndef __has_feature
> # define __has_feature(a) false
> #endif
>
> #if defined __SANITIZE_ADDRESS__ || __has_feature (address_sanitizer)
> # define HAVE_ASAN 1
> #else
> # define HAVE_ASAN 0
> #endif
>
> This is what Emacs uses (its symbol is ADDRESS_SANITIZER instead of
> HAVE_ASAN, for what that's worth).
>
>> +  ASAN_POISON_MEMORY_REGION (buflim + sizeof(uword),
>> +                             bufalloc - (buflim - buffer) -
>> sizeof(uword));
>>
>
> The two 'sizeof's need spaces afterwards.

Fixed.

> I don't see the value of having macros here.  How about the following
> instead?
>
> #ifndef HAVE_ASAN
> static void
> __asan_unpoison_memory_region (void const volatile *addr, size_t size)
> {
> }
>
> static void
> __asan_unpoison_memory_region (void const volatile *addr, size_t size)
> {
> }
> #endif

I agree.  Adjusted via s/unpoison/poison/ in the first.
In addition, I have added _GL_UNUSED so as not to run afoul of grep's
use of -Werror=unused-function. Also, I have adapted not to
declare when using the static wrappers.

and to use #if, not #ifndef, since now HAVE_ASAN is always defined.

> And then have the callers invoke '__asan_poison_memory_region' instead of
> 'ASAN_POISON_MEMORY_REGION'.  This way, there should be no need to pull in
> the ignore-value machinery, it's two less macros to worry about, and there's
> better type checking when address sanitization is not in use.

Thanks for all the good suggestions.  Here's an updated version:

[0001-tests-add-support-for-ASAN-memory-poisoning.patch (application/octet-stream, attachment)]

Message #14 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: Jim Meyering <jim <at> meyering.net>
Cc: 19563 <at> debbugs.gnu.org, Yuliy Pisetsky <ypisetsky <at> fb.com>,
 Nima Aghdaii <naghdaii <at> fb.com>
Subject: Re: bug#19563: grep -F: fix a heap buffer (read) overrun
Date: Mon, 12 Jan 2015 09:31:09 +0900

[Message part 1 (text/plain, inline)]

On Sat, 10 Jan 2015 15:43:02 -0800
Jim Meyering <jim <at> meyering.net> wrote:

> Colleagues Nima Aghdaii and Yuliy Pisetsky found and fixed a heap
> buffer overrun in grep's kwset.c.  The underlying bug, already hard to
> trigger, would most often result in a "mere" heap UMR (uninitialized
> memory read), but Yuliy constructed inputs (see the new test) that
> cause a buffer overrun.
> 
> I'm attaching two other related patches:
>   - grep: avoid false-positive UMR
>   - tests: add support for ASAN memory poisoning
> 
> I expect to push these by Monday.

How about the attachments instead for the second patch?

[0002-grep-F-fix-a-heap-buffer-read-overrun.patch (text/plain, attachment)]

Message #17 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 19563 <at> debbugs.gnu.org, Yuliy Pisetsky <ypisetsky <at> fb.com>,
 Nima Aghdaii <naghdaii <at> fb.com>
Subject: Re: bug#19563: grep -F: fix a heap buffer (read) overrun
Date: Sun, 11 Jan 2015 17:49:22 -0800

On Sun, Jan 11, 2015 at 4:31 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
...
> How about the attachments instead for the second patch?

Thank you for the suggestion.

However, I do not see a problem with Yuliy's fix, so have pushed it,
along with the other two commits.

Comparing your change to Yuliy's, I have a slight preference
for his, since it adds work only to the rarely-used code path on
which this bug was introduced, and keeps the handling of
"out of bounds TP" closer to the code that makes TP too large.

If you can provide justification for this proposed change,
would you please do so in the commit log of a rebased patch?

Message #20 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: Jim Meyering <jim <at> meyering.net>
Cc: 19563 <at> debbugs.gnu.org, Yuliy Pisetsky <ypisetsky <at> fb.com>,
 Nima Aghdaii <naghdaii <at> fb.com>
Subject: Re: bug#19563: grep -F: fix a heap buffer (read) overrun
Date: Mon, 12 Jan 2015 11:31:44 +0900

On Sun, 11 Jan 2015 17:49:22 -0800
Jim Meyering <jim <at> meyering.net> wrote:

> On Sun, Jan 11, 2015 at 4:31 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
> ...
> > How about the attachments instead for the second patch?
> 
> Thank you for the suggestion.
> 
> However, I do not see a problem with Yuliy's fix, so have pushed it,
> along with the other two commits.
> 
> Comparing your change to Yuliy's, I have a slight preference
> for his, since it adds work only to the rarely-used code path on
> which this bug was introduced, and keeps the handling of
> "out of bounds TP" closer to the code that makes TP too large.
> 
> If you can provide justification for this proposed change,
> would you please do so in the commit log of a rebased patch?

I understood.  However, if fill d == 0 before reach in memchr(), even if
fill ep <= tp, bm_delta2_search() can be called, and it is not buggy.
So It is difficult for me to understand that we must exit the loop if
tp <= ep  at the point, although I understand that his fix is correct.

Message #23 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: 19563 <at> debbugs.gnu.org
Subject: CVE number and trivial NSC follow-up patch
Date: Sun, 1 Feb 2015 08:39:08 -0800

[Message part 1 (text/plain, inline)]

I obtained a CVE number for this flaw and added a reference to it in NEWS.
Also fixed a now-unnecessary "goto" in related code.

[0001-maint-convert-goto-to-continue-and-remove-now-spurio.patch (application/octet-stream, attachment)]

[0002-maint-reference-CVE-2015-1345-from-NEWS.patch (application/octet-stream, attachment)]

Message #26 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Santiago Ruano Rincón <santiago <at> riseup.net>
Cc: 19563 <at> debbugs.gnu.org, Michael Gilbert <mgilbert <at> debian.org>,
 anibal <at> debian.org
Subject: Re: <DKIM> bug#19563: CVE number and trivial NSC follow-up patch
Date: Mon, 9 Feb 2015 08:40:13 -0800

On Mon, Feb 9, 2015 at 2:08 AM, Santiago Ruano Rincón
<santiago <at> riseup.net> wrote:
> El 01/02/15 a las 08:39, Jim Meyering escribió:
>> I obtained a CVE number for this flaw and added a reference to it in NEWS.
>> Also fixed a now-unnecessary "goto" in related code.
>
> Hi,
>
> I'm running kwset-abuse test, but I don't get any difference with or
> without the fix for this CVE (in kwset.c). Do you think there is an
> issue with the test? Maybe something related to my platform?
>
> Cheers,
>
> Santiago
>
> PS. kwset-abuse.log attached

Thanks for checking. I've just confirmed that backing out that fix and
running kwset-abuse does trigger a segfault on a rawhide x86-64
system, but not on a debian unstable (also x86-64) system. The
trouble is that the test case is sensitive to the implementation
details of the allocator and system details like page size. The test
case was designed to trigger the segfault, given a particular
observed behavior. If you can tune the test to trigger a failure
on your system, I'd be happy to accept a patch that adds
another case for that.

Message #29 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Santiago Ruano Rincón <santiago <at> riseup.net>
To: Jim Meyering <jim <at> meyering.net>
Cc: 19563 <at> debbugs.gnu.org, Michael Gilbert <mgilbert <at> debian.org>,
 anibal <at> debian.org
Subject: Re: <DKIM> bug#19563: CVE number and trivial NSC follow-up patch
Date: Mon, 9 Feb 2015 11:08:57 +0100

[Message part 1 (text/plain, inline)]

El 01/02/15 a las 08:39, Jim Meyering escribió:
> I obtained a CVE number for this flaw and added a reference to it in NEWS.
> Also fixed a now-unnecessary "goto" in related code.

Hi,

I'm running kwset-abuse test, but I don't get any difference with or
without the fix for this CVE (in kwset.c). Do you think there is an
issue with the test? Maybe something related to my platform?

Cheers,

Santiago

PS. kwset-abuse.log attached

[kwset-abuse.log (text/plain, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #32 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: 19563 <at> debbugs.gnu.org
Subject: [PATCH] maint: use ASAN-poisoning more carefully
Date: Wed, 11 Feb 2015 20:50:28 -0800

[Message part 1 (text/plain, inline)]

I noticed some false-positive use-of-poisoned-memory reports
when testing with ASAN enabled.  This avoids them:

[0001-maint-use-ASAN-poisoning-more-carefully.patch (application/octet-stream, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.