Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 30 Apr 2014 09:44:26 -0700
From: Kees Cook <keescook@...omium.org>
To: Dan Carpenter <dan.carpenter@...cle.com>
Cc: LKML <linux-kernel@...r.kernel.org>, linux-acpi@...r.kernel.org, 
	devel@...ica.org, 
	"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, Dave Jones <davej@...hat.com>, 
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [patch] lib: check for strcpy() overflows to fixed length buffers

On Wed, Apr 30, 2014 at 9:19 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> On Wed, Apr 30, 2014 at 08:33:21AM -0700, Kees Cook wrote:
>> On Wed, Apr 30, 2014 at 8:08 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
>> > +#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
>> > +#define strcpy(dest, src) do {                                         \
>> > +       int len = __compiletime_size(dest);                             \
>> > +       if (len > 1 && strlen(src) >= len)                              \
>> > +               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \
>>
>> This should probably BUG. An overflowing strcpy shouldn't be allowed
>> to continue.
>
> I was worried about false positives.

Sure, good to be initially cautious but I think if this goes in, it should BUG.

> Speaking of false positives the STRICT checks on copy_from_user() have
> been disabled for a year now because of a three year old GCC bug.  I
> wonder if the GCC people realize the security impact it has.  See
> commit 2fb0815c9ee6 ('gcc4: disable __compiletime_object_size for GCC
> 4.6+') and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

Yeah, lots of badness here. I'll see if I can find someone to look at
solutions for this.

>
>> > +config DEBUG_STRICT_SLOW_STRCPY_CHECKS
>> > +       bool "Strict checks for strcpy() overflows"
>> > +       depends on DEBUG_KERNEL
>> > +       help
>> > +         Enabling this option adds some extra sanity checks when strcpy() is
>> > +         called().  This will slow down the kernel a bit.
>>
>> Isn't this an entirely compile-time check? I would expect it to be
>> entirely optimized by the compiler. In fact, could this be turned into
>> a build failure instead?
>
> No.  The problem is when we don't know the size of the src string.  Also
> if GCC is able to find the compile time size of both the src and
> dest string then Smatch and other static checkers are able to as well so
> I'm not very concerned about that case because we already catch them.

Ah, right, the source. But that shouldn't make it "slow". How about
naming this DEBUG_STRICT_STRCPY_SIZE_CHECKS or something? I can't
imagine the performance from adding a single compare to be bad. You
can even branch-hint it with "if (unlikely(...."

-Kees

-- 
Kees Cook
Chrome OS Security

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.