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

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

[Andy Lutomirski]

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.

Cc: stable@vger.kernel.org # 4.9 only
Reported-by: Eric Biggers <ebiggers3@gmail.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 security/keys/encrypted-keys/encrypted.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 17a06105ccb6..e963160ed26a 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -481,7 +481,6 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
 	unsigned int encrypted_datalen;
 	u8 iv[AES_BLOCK_SIZE];
 	unsigned int padlen;
-	char pad[16];
 	int ret;
 
 	encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
@@ -493,11 +492,10 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
 		goto out;
 	dump_decrypted_data(epayload);
 
-	memset(pad, 0, sizeof pad);
 	sg_init_table(sg_in, 2);
 	sg_set_buf(&sg_in[0], epayload->decrypted_data,
 		   epayload->decrypted_datalen);
-	sg_set_buf(&sg_in[1], pad, padlen);
+	sg_set_page(&sg_in[1], ZERO_PAGE(0), padlen, 0);
 
 	sg_init_table(sg_out, 1);
 	sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen);
@@ -584,7 +582,6 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
 	struct skcipher_request *req;
 	unsigned int encrypted_datalen;
 	u8 iv[AES_BLOCK_SIZE];
-	char pad[16];
 	int ret;
 
 	encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
@@ -594,13 +591,12 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
 		goto out;
 	dump_encrypted_data(epayload, encrypted_datalen);
 
-	memset(pad, 0, sizeof pad);
 	sg_init_table(sg_in, 1);
 	sg_init_table(sg_out, 2);
 	sg_set_buf(sg_in, epayload->encrypted_data, encrypted_datalen);
 	sg_set_buf(&sg_out[0], epayload->decrypted_data,
 		   epayload->decrypted_datalen);
-	sg_set_buf(&sg_out[1], pad, sizeof pad);
+	sg_set_buf(&sg_out[1], empty_zero_page, 16);
 
 	memcpy(iv, epayload->iv, sizeof(iv));
 	skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv);
-- 
2.9.3

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

[Andy Lutomirski]

On Tue, Dec 13, 2016 at 6:48 PM, Andy Lutomirski <luto@kernel.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.

--Andy

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

[Herbert Xu]

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@kernel.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.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Eric Biggers]

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@kernel.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

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

[David Howells]

Andy Lutomirski <luto@amacapital.net> wrote:

> > -       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.

Padding goes into the encrypt function and is going to come out of the decrypt
function.  Possibly derived_key_decrypt() should be checking that the padding
that comes out is actually a bunch of zeros.  Maybe we don't actually need to
get the padding out, but I'm not sure whether the crypto layer will
malfunction if we don't give it a buffer for the padding.

David

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

[Andy Lutomirski]

On Wed, Dec 14, 2016 at 12:37 AM, David Howells <dhowells@redhat.com> wrote:
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> > -       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.
>
> Padding goes into the encrypt function and is going to come out of the decrypt
> function.  Possibly derived_key_decrypt() should be checking that the padding
> that comes out is actually a bunch of zeros.  Maybe we don't actually need to
> get the padding out, but I'm not sure whether the crypto layer will
> malfunction if we don't give it a buffer for the padding.

It was the memset that threw me for a loop.

David, are these encrypted keys ever exported anywhere?  If not, could
the code use a mode that doesn't need padding?

--Andy

>
> David



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[David Howells]

Andy Lutomirski <luto@amacapital.net> wrote:

> David, are these encrypted keys ever exported anywhere?  If not, could
> the code use a mode that doesn't need padding?

ecryptfs uses them, I think.

David