feat: add zeroizing read helper for sensitive deserialization#1057
feat: add zeroizing read helper for sensitive deserialization#1057Jr-kenny wants to merge 4 commits into
Conversation
|
Automated check (CONTRIBUTING.md) Findings:
Recommendations:
Next steps:
|
huitseeker
left a comment
There was a problem hiding this comment.
thanks for tackling this!
| impl Deserializable for SecretKey { | ||
| fn read_from<R: ByteReader>(source: &mut R) -> Result<Self, DeserializationError> { | ||
| let byte_vector: [u8; SK_LEN] = source.read_array()?; | ||
| let byte_vector = read_sensitive_array::<SK_LEN, _>(source)?; |
There was a problem hiding this comment.
I think this fixes the serialized SK_LEN buffer, but this Falcon path still decodes secret coefficients into the f, g, and big_f temporaries below. Those are ordinary Vec/Polynomial values, and I do not see a Drop impl for Polynomial, only Zeroize. Should those decoded temporaries also be wrapped or wiped?
There was a problem hiding this comment.
Good catch. The Vec<i8> temporaries coming out of decode_i8 were easy to cover, I've wrapped them in Zeroizing in 53587db so the raw coefficient bytes get wiped on every exit path now.
The Polynomial side turned out to be a deeper hole than it looks. FalconFelt doesn't implement Zeroize at all, so the generic impl on Polynomial can't touch Polynomial<FalconFelt>. And it's actually a bit worse than no Drop impl: polynomial.rs has a manual impl<F: Zeroize> ZeroizeOnDrop for Polynomial<F> {} with nothing backing it, and since ZeroizeOnDrop is just a marker trait, that's a wipe-on-drop promise that never actually runs. Even if both of those were fixed, computing big_g goes through fft().hadamard_div().hadamard_mul().ifft() and building the basis negates polynomials, all of which allocate intermediates carrying secret coefficients that read_from has no handle on.
So doing this properly means giving FalconFelt a Zeroize impl, backing the marker with a real Drop, and going over the falcon math ops for those intermediate allocations. That felt too big to bolt onto this PR, so I'd file it as a follow-up issue and take it on, shout if you'd rather see it handled here. Thanks!
There was a problem hiding this comment.
Filed the follow-up as #1059 with the full picture. While writing it up I found the gap is wider than the read path: write_into drops the encoded secret key unwiped behind a stale comment, and generate_seed leaks an unwiped copy of it on every signature. I have the fix building and passing the falcon suite locally, ready to open once assigned.
53587db to
ba7b5ae
Compare
ba7b5ae to
4a5d507
Compare
Deserializing a secret key reads its bytes into a stack buffer, and each read_from was responsible for remembering to zeroize that buffer before every return path. Some forgot: the ecdsa path leaked the buffer on the early return when from_slice rejects the bytes, and the falcon and poseidon2 AEAD secret keys never wiped their read buffers at all. Add read_sensitive_array to utils, which returns the array wrapped in Zeroizing so the buffer is wiped on drop no matter how the function exits. Switch the four sensitive read_from impls (ecdsa, eddsa, falcon secret key, poseidon2 AEAD secret key) over to it and drop the manual zeroize calls. One honest limit, also noted on the helper doc: read_array returns by value, so the single move into the wrapper is not wiped. That residual is the same one the manual calls had. Closing it fully would need a ByteReader method that reads into a caller-provided buffer. Closes 0xMiden#593
Wrap the f, g, and big_F byte vectors produced by decode_i8 in Zeroizing so the raw secret coefficients are wiped when read_from returns, on both the success and error paths.
4a5d507 to
20fbb02
Compare
Describe your changes
Closes #593. Follows up on my comment there, opening this so the actual diff is easier to judge than a description.
Deserializing a secret key reads its bytes into a stack buffer, and each
read_fromhad to remember to callzeroize()on that buffer before every return path. Going through the sensitiveread_fromimpls, some forgot:ecdsa_k256_keccak::SecretKey::read_fromzeroized on success but leaked the buffer on the early return whenfrom_slicerejects the bytesfalcon512_poseidon2::SecretKey::read_fromnever wiped itsSK_LENbuffer on any path, success includedSecretKey::read_fromnever wiped its buffer eitherfrom_bytesis infallibleThis adds
read_sensitive_arraytoutils, which returns the array wrapped inZeroizingso the buffer is wiped on drop no matter how the calling function exits, and switches the four impls over to it. The manualzeroize()calls go away, and a forgotten cleanup in a futureread_fromstops being possible by construction.One limit I want to be straight about (also documented on the helper):
read_arrayreturns by value, so the single move into theZeroizingwrapper is not wiped. That residual is the same one the manual calls had. Closing it fully would need aByteReadermethod that reads into a caller-provided&mut [u8], which felt like a bigger conversation than this fix.While in there I noticed
falcon512read_fromcallsdecode_i8(...).unwrap()for thegandbig_Fchunks (onlyfgets a proper error). If those can fail on malformed input that's a panic on attacker-supplied bytes, but I didn't want to widen this PR before asking. Happy to file it separately if you think it's reachable.Testing
miden-cryptolib suite passes (648 tests), clippy cleanChecklist before requesting a review
nextaccording to naming convention.