linux-kernel - Re: [patch] lib: check for strcpy() overflows to fixed length buffers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5j+G778YMcbiW2cdeZ++0MDcTiNVOnm5E5W86vfKKDvUjg@mail.gmail.com>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/