kyber: Don't store shared secret on stack, and allow it to be zeroized
The current signature of both encapsulate and decapsulate (fn encapsulate(...) -> Result<... [u8; 32] ...>, fn decapsulate(...) -> Result<[u8; 32], ...> stores the secret on the stack. This means it will potentially be copied around many times in memory, depending on how optimizations happen to generate the final code.
It is recommended to zero out cryptographic secrets when done with them/before other software has a chance to read that memory. This is currently not really possible with pqc_kyber and the current function signatures.
I recommend, and would be very interested in improving this. I implemented something very similar for classic-mceliece-rust recently. All secrets were bundled up in dedicated containers that allowed both borrowed and owned versions of the data, but that was never copied around in the memory, and that automatically cleared themselves on drop. See here for one example: https://github.com/Colfenor/classic-mceliece-rust/blob/bb57cd4f97ae2d092b8b1bfbc88c49507eca2892/src/lib.rs#L283-L333
The same goes for the SecretKey type really. It should implement Zeroize + ZeroizeOnDrop.
About this issue
- Original URL
- State: open
- Created a year ago
- Comments: 15 (12 by maintainers)
Zeroisation draft: https://github.com/Argyle-Software/kyber/pull/67
Posted a draft implementation as a basic starting point, from what I can see it clears everything important on my x86 system but copy elision behaviour isn’t guaranteed on various platforms and even different compilers. The draft mainly focuses on internal transient secrets, especially the secret key polynomial from which the secret key is easily derived.
It’s not intended to be merged but just to put it out there for the internals.
Looking at the Classic McEliece code, keen for the same design of passing a buffer, but prefer to keep it on the stack as default behaviour. Happy to provide a separate boxed version too, there’s likely a fair bit of overhead with heap allocation.
That McEliece code you linked doesn’t use Pin? That can still end up copied in various ways. The boxed version looks fine though, from a very brief check they don’t handle zeroisation of intermediate secrets from which a key can be derived it seems.
Apologies for the delay on this Linus, there’s a long overdue API redesign tied into all these changes and would prefer to do that all at once to cause minimal disruption. For instance: Keypair shouldn’t have any public fields, the public key field itself is redundant and can be derived at runtime with no cost. The Kex structs also have send fields which should be merged into an enum for space savings, etc. Alas, such stuff deserves it’s own issue.