linux-kernel - Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

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: <e1a3afb4-dd45-ac0a-cc07-031757ef3463@gmail.com>
Date:   Fri, 14 Jul 2017 23:26:22 +0300
From:   Andrey Rybainin <ryabinin.a.a@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>
Cc:     Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Daniel Micay <danielmicay@...il.com>,
        Dave Jones <davej@...emonkey.org.uk>,
        Anna Schumaker <schumaker.anna@...il.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "J. Bruce Fields" <bfields@...ldses.org>,
        Alexander Potapenko <glider@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>, kasan-dev@...glegroups.com
Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13



On 07/14/2017 10:58 PM, Linus Torvalds wrote:
> On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin
> <aryabinin@...tuozzo.com> wrote:
>>
>>> yet when I look at the generated code for __ip_map_lookup, I see
>>>
>>>        movl    $32, %edx       #,
>>>        movq    %r13, %rsi      # class,
>>>        leaq    48(%rax), %rdi  #, tmp126
>>>        call    strscpy #
>>>
>>> what's the bug here? Look at that third argume8nt - %rdx. It is
>>> initialized to 32.
>>
>> It's not a compiler bug, it's a bug in our strcpy().
>> Whoever wrote this strcpy() into strscpy() code apparently didn't read carefully
>> enough gcc manual about __builtin_object_size().
>>
>> Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html :
>>
>>         __builtin_object_size(ptr, type) returns a constant number of bytes from 'ptr' to the end of the object 'ptr'
>>         pointer points to. "type" is an integer constant from 0 to 3. If the least significant bit is clear, objects
>>         are whole variables, if it is set, a closest surrounding subobject is considered the object a pointer points to.
>>         The second bit determines if maximum or minimum of remaining bytes is computed.
>>
>> We have type = 0 in strcpy(), so the least significant bit is clear. So the 'ptr' is considered as a pointer to the whole
>> variable i.e. pointer to struct ip_map ip;
>> And the number of bytes from 'ip.m_class' to the end of the ip object is exactly 32.
>>
>> I suppose that changing the type to 1 should fix this bug.
> 
> Oh, that absolutely needs to be done.
> 
> Because that "strcpy() -> strscpy()" conversion really depends on that
> size being the right size (well, in this case minimal safe size) for
> the actual accesses, exactly because "strscpy()" is perfectly willing
> to write *past* the end of the destination string within that given
> size limit (ie it reads and writes in the same 8-byte chunks).
> 
> So if you have a small target string that is contained in a big
> object, then the "hardened" strcpy() code can actually end up
> overwriting things past the end of the strring, even if the string
> itself were to have fit in the buffer.
> 
> I note that every single use in string.h is buggy, and it worries me
> that __compiletime_object_size() does this too. The only user of that
> seems to be check_copy_size(), and now I'm a bit worried what that bug
> may have hidden.
> 
> I find "hardening" code that adds bugs to be particularly bad and
> ugly, the same way that I absolutely *hate* debugging code that turns
> out to make debugging impossible (we had that with the "better" stack
> tracing code that caused kernel panics to kill the machine entirely
> rather than show the backtrace, and I'm still bitter about it a decade
> after the fact).
> 

A have some more news to make you even more "happier" :)
strcpy() choose to copy 32-bytes instead of smaller 5-bytes because it has one more bug :)

GCC couldn't determine size of class (which is 5-byte string):
	strcpy(ip.m_class, class);

So, p_size = 32 and q_size  = -1, this "if (p_size == (size_t)-1 && q_size == (size_t)-1)" is false
(because of bogus '&&', obviously we should have '||' here)

and since (32 < (size_t)-1) 
	if (strscpy(p, q, p_size < q_size ? p_size : q_size) < 0)

we end up with 32-bytes strscpy().

Enjoy :)


> There is something actively *evil* about it. Daniel, Kees, please jump on this.
> 
> Andrey, thanks for noticing this thing,
> 
>                           Linus
>