[stream] Harden KDF for ChaCha20Poly1305 cipher #1021
Conversation
stream/src/public_key/cipher.rs
Outdated
| let salt = hasher.finalize(); | ||
|
|
||
| // HKDF-Extract: creates a pseudorandom key (PRK) | ||
| let prk = Hkdf::<ISha256>::new(Some(salt.as_ref()), ikm); |
There was a problem hiding this comment.
I think prk is not zeroized
There was a problem hiding this comment.
I believe prk does its own internal zeroization when its dropped but I'll have to double-check on that
There was a problem hiding this comment.
Yes, please. hkdf does not have zeroization, hmac has it, but it is a separate feature.
There was a problem hiding this comment.
pub struct Hkdf<H: OutputSizeUser, I: HmacImpl<H> = Hmac<H>> {
hmac: I::Core,
_pd: PhantomData<H>,
}
Since we use sha2::Sha256 as H and don't specify I, it defaults to hmac::Hmac<sha2::Sha256>. This means that Hkdf itself doesn't have to implement zeroize on drop, only hmac does (PhantomData is inconsequential). Therefore, hmac::Hmac<sha2::Sha256>::Core must ultimately implement zeroize on drop
There was a problem hiding this comment.
I'm having a difficult time tracing the code that deep but my strong suspicion is that the library ultimately takes care of the zeroize on drop since hkdf and hmac are in the RustCrypto github
|
I also opened one minor issue - #1031 - if you think it is valid, it would be great to fix it in this PR. |
|
#1031 I will do in a separate PR as I think it's unrelated enough that it doesn't need to be associated with this PR |
There was a problem hiding this comment.
I suppose it may make sense to put ciphers in cryptography eventually...
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1021 +/- ##
==========================================
+ Coverage 90.96% 90.97% +0.01%
==========================================
Files 191 192 +1
Lines 53587 54092 +505
==========================================
+ Hits 48743 49210 +467
- Misses 4844 4882 +38
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Thanks to @dnkolegov-ar for finding these issues
Fixes #1013
Fixes #1014
Fixes #1016
Updates the
ChaCha20Poly1305cipher derivation for thecommonware-stream::public_keymodule:hkdfandsha2::Sha256to derive the cipher from the Diffie-Hellman shared secret (rather than the shared secret directly)