Zeroize - make sure it's used were needed, and capture practices

[gww-parity]

This issues is to capture following two aspects:

• for current codebase: to check if it's used were use is appropriate
• for future codebase: make some Action Item that will allow us to increase probability that Zeroize will be used in future in simmilar usecases

[cheme]

@gww-parity I finally rewrite my notes about secret memory.
Disclaimer, I mainly look at substrate code base (did not use parity-crypto from parity-common but it is just a detail).
Bullet points are possible changes.

Keystore

Substrate key usage looks very well designed, care was taken to isolate signing into a keystore abstraction: see https://github.com/paritytech/substrate/blob/c51455b9ce6901fb1624bcfee22724f7a6f69c6e/client/keystore/src/lib.rs#L329 (BareCryptoStore trait).
So secret are encrypted as a file and require a passphrase, and all should happen on stack (still paritytech/substrate-bip39#10 will be needed).
Only the secret phrase is on heap and we are using SecretString for it so it is zeroize.

• shall we try to protect more sign_with (eg stack overwrite similar to clear_on_drop technic)?
  I am not too kind on it since it can easilly be a maintenance burden even if on paper it seems ok.

The keystore also store 'ephemeral' secret in heap memory this time.

• They could potentially be encrypted with the password but at the time the passphrase is a keyderivation seed, so we probably don't want to change to much thing here.
• They could simply be managed in SecretString to be zeroize and manage as the main passphrase.

On this point, if we wanted to change something, I would probably try to put those in SecretString (zeroize them as for the passphrase).
But the main question is do we want to protect 'ephemeral' secrets, I am not sure (so I would only consider trying to zeroize if it ends up being impactless).

Still some function inevitably have to return secrets on stack, those function are currently public. One could consider trying to make them private, but given the big codebase and the multiple use case
I would not expect it to be that straightforward (it is needed by many tests and rust/cargo can not be that good at making test function accessible across crates).

• I think something could be try to make sure that user do not use them (I put comment on my branch paritytech/substrate@master...cheme:read_crypto where those function are defined).

host functions

In primitives/io/src/lib.rs the runtime host functions from crypto interface uses these keystore (with ephemeral key).
Those function are linked properly to the keystore
Note that there may be to be a small design issue:

• the host function that generate a key allows you to provide no seed and then use a random one from current keystore secret, this do not make sense to me as it is non determinist.

One could probably propose to switch the optionall seed parameter to a non optional one.
I guess it is done this way for other use cases like offchain processing, so maybe just some comments should be added to avoid users coalling it on chain.

But this does not relate directly with our problematic.

Also note that in case someone is using a secret key in runtime memory (except for offchain), this is onchain processing and not a secret.
But for offchain, the host sign function is using the keystore sign_with function so it is fine.

I really start thinking that those host function are more designed for offchain (at least generate one). CC\ @tomusdrw ?

full_crypto feature

From the doc:

# This feature enables all crypto primitives for `no_std` builds like microcontrollers
# or Intel SGX.
# For the regular wasm runtime builds this should not be used.

Key are easilly exposed when using this feature, but I think from reading the doc that we should keep it out of scope (for sgx it is inherently fine).
Maybe we should add a short comment as in paritytech/substrate@master...cheme:read_crypto .

Sum-up

To sum-up the only thing that I would prioritize at this point is paritytech/substrate-bip39#10, the others are more open thinkings.
Note that some additional info on ephemeral key, host functio, and full_crypto are welcome.
Other devs thought are welcome.

[gww-parity]

\close with paritytech/substrate#6955 merged