I apologize if this has been discussed recently, but the archive on the website is still busted, and thus does not contain any messages sent to the list during the last year and change.
While tracing through the code for HKDF, I noticed a rather disturbing pattern: BouncyCastle's classes are not only _very_ happy about leaving key material sitting around on the heap, they actually make it impossible to prevent this from happening! A perfect example of this behavior is `KeyParameter` -- once you pass a key to a KeyParameter ctor, a copy of the key data will be placed on the heap, and there is no guaranteed way to clear it out (note that even garbage collection won't necessarily do it, depending how much space the compaction phase reclaims.) In a similar vein, KeyParameter has no way to copy the key into a specific destination; you have to call GetKey() to get a _new_ array, then copy from that new array to your destination, then clear out the temporary array. (Otherwise, you now have two copies of the key on the heap, and thus twice as many chances for it to be stolen somehow.) I propose that the BouncyCastle library make an reasonable effort to ensure the following: - That any objects that could be storing sensitive data, such as key material, implement IDisposable, with the implementation clearing out the array(s) inside - That in a majority of circumstances, unnecessary copies of key material can be avoided - That all intermediate copies of key material are cleared/destroyed before abandonment For a particularly egregious case, look at `HkdfBytesGenerator.Init`: at least _six pieces_ of secret key material get stored on the heap, with no way to prevent it, or clear them out when finished: - HkdfParameters ctor takes the IKM and clones it (undestroyable KM 1) - HkdfBytesGenerator.Init (by the way, unconditionally casting the argument to a different public type: code smell) - When not extracting (uncommon): Calls GetIkm(), which clones the array (undestroyable KM 2), then constructs a KeyParameter(), which clones the array again (undestroyable KM 3) - When extracting (common): Calls GetIkm(), which clones the array (undestroyable KM 2), then does the hashing and constructs a KeyParameter(), which clones the array (undestroyable KM 3) - Either way, HMac.Init casts its argument to KeyParameter (another code smell) and calls GetKey(), which clones the array *again* (undestroyable KM 4) - HMac's `inputPad` and `outputBuf` contain two copies of the internal key, each XORed by (different) constants (undestroyable KM 5 and 6) - If you actually generate a key, the last block's worth of data (so, 256 bits if you use SHA256, which is probably your entire key) is left around inside currentT. This would be undestroyable KM #7, except that for the current implementation, you can actually destroy it by calling GenerateBytes a second time, which will overwrite it with the next block. To me, this is unacceptable; I certainly expect it would be unacceptable to many others. If Heartbleed and Spectre/Meltdown have taught us anything, it's that leaving stray key material around and hoping nobody can find it is a losing proposition. -- -- Stevie-O Real programmers use COPY CON PROGRAM.EXE
