parity-crypto: use mlock for Secret

[ordian]

Fixes #411.

As mentioned in our internal issue tracker, it's not clear to me how it could integrated into secrecy for two reasons:

• secrecy::SecretBox<S> is generic over S and in order to call mlock, we would need something like a slice. This could be potentially "solved" by introducing a trait that abstracts over a slice (e.g. AsRef<[u8]>), but that would limit SecretBox only to these types.
• it's not clear how an error on mlock should be propagated. In this PR we simply ignore it as this security measure is optional and the syscall may fail due to multiple reasons, e.g. on windows there is a small amount of maximal pages a process can lock by default. Also it's not guaranteed that a memory page won't be written to disk, e.g. in case of hibernation or memory starvation.

This PR suggests a simple approach to not introduce an abstraction for it (at least for now) and ignore the errors on construction.
If you have ideas how it could be improved though, please share!

This is a non-breaking change.

[cheme]

secrecy::SecretBox<S> is generic over S and in order to call mlock, we would need something like a slice. This could be potentially "solved" by introducing a trait that abstracts over a slice (e.g. AsRef<[u8]>), but that would limit SecretBox only to these types.

In substrate we are only using secrecy::SecretString which is statically defined, there also exist secrecy::SecretVec, both could be enhanced with the mlock guard I guess.
On the other hand adding the guard in substrate keystore struct https://github.com/paritytech/substrate/blob/f488598287b48c79cba9bc7c0da748d3d852862a/client/keystore/src/lib.rs#L98 directly is certainly doable too.

[cheme]

LGTM

[ordian]

I would have log error, but warn is fine too.

error is usually printed before termination, I chose a warning because it's not a hard error, but something to be aware of

We use 4.8 in other parity common crate, but crates are rather unrelated, so I guess it is fine.

yeah, that's fine, cargo will select the latest compatible version in Cargo.lock.

I filed iqlusioninc/crates#480 and now reading https://eklitzke.org/mlock-and-mlockall :)

[dvdplm]

I have done some reading and talking to @ordian in DMs and at the end of it all I'm rather skeptical about mlock. I don't think it buys us much and seems more like ceremony (as in "sounds like a good idea") than an actual security improvement. Sure, we can take the "it can't hurt"-approach and go ahead and merge this, but I would prefer we didn't.

• There is no guarantee that future maintainers and users fully understand what mlock is and what it is not; it's not a commonly used technique.
• There seem to be subtle differences on different OSs in how mlock works; users and maintainers need to be aware of these differences. Is it worth the effort?
• The best argument for deploying mlock is defense-in-depth, but I can't imagine a scenario where the attacker can access swap files but not process memory.

The code LGTM.

[ordian]

@dvdplm I'm with you on this. I don't know if there is a realistic scenario of attack that can be avoided with mlock and curious what @kirushik thinks

[kirushik]

@ordian @dvdplm, cc @gww-parity

I can't imagine a scenario where the attacker can access swap files but not process memory.

Well, it's actually a bit more realistic in context of Substrate-based chains (are we even using parity-crypto there, by the way?) than for OpenEthereum: people will be running those on shared machines or in shared environments (while running Ethereum node outside of datacenter is barely practical nowadays).

That means that those machines might have swap files/partitions physically accessible while the device is offline. This will make it possible for a purely "cold" offline attack to steal the secrets that are accidentally exposed on the hard drive.
This issue is amplified with the realities of modern-day SSDs, where you can never be 100% sure that the thing you've asked to wipe out has actually been wiped out on physical level, not just masked by the allocation firmware of the drive.

I don't want to go into the weeds of discussing what secrets might and might not be valuable for the attacker enough to satisfy getting a physical access to the powered off machine -- such assumptions are really hard to maintain in code over time. So the application of "defence in deep" here is indeed correct.
But I want to admit that the marginal benefit of mlocking the page over just not recommending/supporting systems with unencrypted swap is rather slim; so it all depends on the development, maintenance and preformance costs of mlock-ing for us.
So far I assumed that it would be relatively easy to implement, negligible in performance impact -- and if we would upstream our patches to the zeroize/secrecy/whatever crate we're using to properly free memory, then the maintenance costs would also not be prohibitive.
If that assumptions is not correct -- let's reconsider; there are probably better tasks to spend a human-week on than this one.

[ordian]

are we even using parity-crypto there, by the way?

only in secret-store AFAIK

code outside of publickey feature like aes still uses stack-based keys, don't think we can do anything about it, this was discussed elsewhere

[dvdplm]

We (me, @ordian and @gww-parity) have had another round of discussion about this and landed in a decision to close this.

@cheme @kirushik Let us know if you strongly disagree and we can re-open! :)

[gww-parity]

To give a bit more context, even I have been a bit towards using mlock, still closer to "be on the fence", as I see lack of model for decision making "mlock or not to mlock" e.g. from thread modelling point of view.

Having said that, I am looking forward for Secrets types RFCs for Rust and LLVM, in mean time if I would came across (or come up with) better modelling for mlock will reopen.