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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Sun, Nov 9, 2014 at 5:33 PM, Stephan Mueller <[email protected]> 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.
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 <[email protected]> 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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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