crypto: zeroization of sensitive data in af_alg

crypto: zeroization of sensitive data in af_alg

[Stephan Mueller]

Hi Herbert,

while working on the AF_ALG interface, I saw no active zeroizations of memory 
that may hold sensitive data that is maintained outside the kernel crypto API 
cipher handles. I think the following memory segments fall under that 
category:

	* message digest

	* IV

	* plaintext / ciphertext handed in by consumer

	* ciphertext / plaintext that is send back to the consumer

May I ask whether such zeroizations are present? At least I did not find it. 
If we conclude that there is a need for adding such zeroizations, I checked 
the code for the appropriate locations:

I think I found the location for the first one: hash_sock_destruct that should 
be enhanced with a memset(0) of ctx->result. I have a patch ready which is 
tested and works.

For the IV, I think I found the spot as well: skcipher_sock_destruct. This 
function should be enhanced with a memset(0) of ctx->iv. Again, I have a patch 
ready which is tested and works.

However, I am failing to find the right spot to add a zeroization for the 
latter one, i.e. the code that handles the pages send in by the user or the 
pages that are returned by the crypto API. Initially I thought 
skcipher_pull_sgl is a good spot for the symmetric ciphers as it evicts the 
used pages out of the scope of the kernel crypto API. I added a 
clear_page(sg_page(sg+1)) as well as memset(sg_page(sg+1), 0, plen) right 
before the put_page call. All that I got in return was a BUG() from the memory 
management layer.

Then I tried the same in af_alg_free_sg() as this function is used by 
algif_hash.c -- with the same result.

That makes me wonder: where should such a zeroization call be added?

Thanks

-- 
Ciao
Stephan

Re: crypto: zeroization of sensitive data in af_alg

[Herbert Xu]

On Sun, Nov 09, 2014 at 11:33:52PM +0100, Stephan Mueller wrote:
> 
> while working on the AF_ALG interface, I saw no active zeroizations of memory 
> that may hold sensitive data that is maintained outside the kernel crypto API 
> cipher handles. I think the following memory segments fall under that 
> category:

Are you talking about temporary data that we generate as part of
the processing? If so they should be zeroed by the entity that
generates them.
 
> However, I am failing to find the right spot to add a zeroization for the 
> latter one, i.e. the code that handles the pages send in by the user or the 
> pages that are returned by the crypto API. Initially I thought 
> skcipher_pull_sgl is a good spot for the symmetric ciphers as it evicts the 
> used pages out of the scope of the kernel crypto API. I added a 
> clear_page(sg_page(sg+1)) as well as memset(sg_page(sg+1), 0, plen) right 
> before the put_page call. All that I got in return was a BUG() from the memory 
> management layer.

I don't think I understand what exactly you're trying to zero.
Can you give an example?

Thanks,
-- 
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: crypto: zeroization of sensitive data in af_alg

[Stephan Mueller]

Am Montag, 10. November 2014, 22:05:18 schrieb Herbert Xu:

Hi Herbert,

> On Sun, Nov 09, 2014 at 11:33:52PM +0100, Stephan Mueller wrote:
> > while working on the AF_ALG interface, I saw no active zeroizations of
> > memory that may hold sensitive data that is maintained outside the kernel
> > crypto API cipher handles. I think the following memory segments fall
> > under that
> > category:
> Are you talking about temporary data that we generate as part of
> the processing? If so they should be zeroed by the entity that
> generates them.

I currently see that the IV buffer (owned by skcipher) and the message digest 
buffer (owned by hash) are not memset(0) before freeing them. I agree that 
both are not really sensitive data. But wouldn't it be prudent to memset(0) 
them nonetheless in the skcipher_sock_destruct and hash_sock_destruct 
functions, respectively? 
> 
> > However, I am failing to find the right spot to add a zeroization for the
> > latter one, i.e. the code that handles the pages send in by the user or
> > the
> > pages that are returned by the crypto API. Initially I thought
> > skcipher_pull_sgl is a good spot for the symmetric ciphers as it evicts
> > the
> > used pages out of the scope of the kernel crypto API. I added a
> > clear_page(sg_page(sg+1)) as well as memset(sg_page(sg+1), 0, plen) right
> > before the put_page call. All that I got in return was a BUG() from the
> > memory management layer.
> 
> I don't think I understand what exactly you're trying to zero.
> Can you give an example?

Apologies, my bad as I did not check get_user_pages_fast well enough. I see 
now that we operate on the pages in user space directly without copy_from_user 
that would imply a kernel-internal copy. Please disregard my comment.
> 
> Thanks,


-- 
Ciao
Stephan

Re: crypto: zeroization of sensitive data in af_alg

[Herbert Xu]

On Tue, Nov 11, 2014 at 03:06:32AM +0100, Stephan Mueller wrote:
> Am Montag, 10. November 2014, 22:05:18 schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > On Sun, Nov 09, 2014 at 11:33:52PM +0100, Stephan Mueller wrote:
> > > while working on the AF_ALG interface, I saw no active zeroizations of
> > > memory that may hold sensitive data that is maintained outside the kernel
> > > crypto API cipher handles. I think the following memory segments fall
> > > under that
> > > category:
> > Are you talking about temporary data that we generate as part of
> > the processing? If so they should be zeroed by the entity that
> > generates them.
> 
> I currently see that the IV buffer (owned by skcipher) and the message digest 
> buffer (owned by hash) are not memset(0) before freeing them. I agree that 
> both are not really sensitive data. But wouldn't it be prudent to memset(0) 
> them nonetheless in the skcipher_sock_destruct and hash_sock_destruct 
> functions, respectively? 

Yes please submit your patches.
 
> Apologies, my bad as I did not check get_user_pages_fast well enough. I see 
> now that we operate on the pages in user space directly without copy_from_user 
> that would imply a kernel-internal copy. Please disregard my comment.

OK.

Thanks,
-- 
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: crypto: zeroization of sensitive data in af_alg

[Sandy Harris]

On Sun, Nov 9, 2014 at 5:33 PM, Stephan Mueller <smueller@chronox.de> wrote:

> while working on the AF_ALG interface, I saw no active zeroizations of memory
> that may hold sensitive data that is maintained outside the kernel crypto API
> cipher handles. ...

> I think I found the location for the first one: hash_sock_destruct that should
> be enhanced with a memset(0) of ctx->result.

See also a thread titled "memset() in crypto code?" on the linux
crypto list. The claim is that gcc can optimise memset() away so you
need a different function to guarantee the intended results. There's a
patch to the random driver that uses a new function
memzero_explicit(), and one of the newer C standards has a different
function name for the purpose.

Re: crypto: zeroization of sensitive data in af_alg

[Stephan Mueller]

Am Montag, 10. November 2014, 21:55:43 schrieb Sandy Harris:

Hi Sandy, Herbert,

> On Sun, Nov 9, 2014 at 5:33 PM, Stephan Mueller <smueller@chronox.de> wrote:
> > while working on the AF_ALG interface, I saw no active zeroizations of
> > memory that may hold sensitive data that is maintained outside the kernel
> > crypto API cipher handles. ...
> > 
> > I think I found the location for the first one: hash_sock_destruct that
> > should be enhanced with a memset(0) of ctx->result.
> 
> See also a thread titled "memset() in crypto code?" on the linux
> crypto list. The claim is that gcc can optimise memset() away so you
> need a different function to guarantee the intended results. There's a
> patch to the random driver that uses a new function
> memzero_explicit(), and one of the newer C standards has a different
> function name for the purpose.

That is a good idea.

Herbert: I can prepare a patch that uses memzero_explicit. However, your 
current tree does not yet implement that function as it was added to Linus' 
tree after you pulled from it.

Shall I now still use memset(0) or prepare a patch that does not yet compile 
by using memzero_explicit?

-- 
Ciao
Stephan

Re: crypto: zeroization of sensitive data in af_alg

[Herbert Xu]

On Tue, Nov 11, 2014 at 05:16:54AM +0100, Stephan Mueller wrote:
> 
> Shall I now still use memset(0) or prepare a patch that does not yet compile 
> by using memzero_explicit?

Just send the patch with the memzer_explicit and I'll make sure
that I pull the requisite changes in before I apply your patch.

Thanks,
-- 
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: crypto: zeroization of sensitive data in af_alg

[Daniel Borkmann]

On 11/11/2014 05:16 AM, Stephan Mueller wrote:
...
> That is a good idea.
>
> Herbert: I can prepare a patch that uses memzero_explicit. However, your
> current tree does not yet implement that function as it was added to Linus'
> tree after you pulled from it.

Yep, Ted took it [1] on top of the random driver fix as discussed.

   [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7185ad2672a7d50bc384de0e38d90b75d99f3d82