linux-kernel - Re: [PATCH v2] keys/encrypted: Fix two crypto-on-the-stack bugs

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: <20161214081024.GA22765@zzz>
Date:   Wed, 14 Dec 2016 00:10:24 -0800
From:   Eric Biggers <ebiggers3@...il.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Andy Lutomirski <luto@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        David Howells <dhowells@...hat.com>, keyrings@...r.kernel.org,
        linux-crypto@...r.kernel.org, Stephan Mueller <smueller@...onox.de>
Subject: Re: [PATCH v2] keys/encrypted: Fix two crypto-on-the-stack bugs

On Wed, Dec 14, 2016 at 01:04:04PM +0800, Herbert Xu wrote:
> On Tue, Dec 13, 2016 at 06:53:03PM -0800, Andy Lutomirski wrote:
> > On Tue, Dec 13, 2016 at 6:48 PM, Andy Lutomirski <luto@...nel.org> wrote:
> > > The driver put a constant buffer of all zeros on the stack and
> > > pointed a scatterlist entry at it in two places.  This doesn't work
> > > with virtual stacks.  Use ZERO_PAGE instead.
> > 
> > Wait a second...
> > 
> > > -       sg_set_buf(&sg_out[1], pad, sizeof pad);
> > > +       sg_set_buf(&sg_out[1], empty_zero_page, 16);
> > 
> > My fix here is obviously bogus (I meant to use ZERO_PAGE(0)), but what
> > exactly is the code trying to do?  The old code makes no sense.  It's
> > setting the *output* buffer to zeroed padding.
> 
> It's decrypting so I presume it just needs to the extra space for
> the padding and the result will be thrown away.
> 

It looks like the data is zero-padded to a 16-byte AES block boundary before
being encrypted with CBC, so the encrypted data may be longer than the original
data.  (I don't know why it doesn't use CTS mode instead, which would make the
lengths the same.)

Since the crypto API can do in-place operations, when *encrypting* I suggest
copying the decrypted data to the output buffer, padding it with 0's, and
encrypting it in-place.  This eliminates the need for the stack buffer.

But when *decrypting* there will be up to 15 extra throwaway bytes of output
produced by the decryption.  There must be space made for these, and since it's
output it can't be empty_zero_page.  I guess either check whether space can be
made for it in-place, or allocate a temporary buffer...

Eric