fix the resize function to actually zero the buffer as advertised #1
Conversation
Ignore failing doctest and add tests that check resize and clear (a special case thereof) for their advertised behavior of overwriting memory which ostensibly contains secrets. Running these tests currently demonstrates that the shrink case of resize does not in fact zero memory due to a logic error.
Due to an error in the order in which operations were performed, the third argument to memset in the shrink case of resize was always 0. Re-order operations so that the intended behavior is achieved.
zero the defunct buffer after copying the contents to the newly-allocated buffer when reallocing in the resize method. This change (or one very similar to it) was made in the original upstream pijul repository, which seems to have disappeared recently. I have reproduced it from memory as my own commit for the purposes of fixing the bug.
|
My newly-created gitlab account didn't have permissions to create a fork on the redox instance, sorry for making a PR against a mirror. |
|
It would be better to fix this at upstream (Pijul project) than here. This is just an outdated fork of cc @P-E-Meunier, you may want to take a look at this. |
|
Wow, I didn't know other people were using this! |
|
I found (what I assume was) the original repository on nest.pijul.org and found that the realloc case was fixed (I mentioned so in the commit message), but the shrink case still wasn't. If you're still using this library in anu, you may want to double-check that it behaves correctly. I would've submitted this patch to the pijul codebase but nest.pijul.org seems to have disappeared around the time that nest.anu.dev came up, and I'm unable to find the anu/pijul source on the new nest. If you could point me to it, I'd happily submit patches there. I haven't thoroughly audited the rest of the code for issues either, it may be worth a look-over. |
|
Oh, I see. It isn't indeed fixed in my version, and is indeed an issue. Thanks. |
|
Was surprised to see this still open while looking through my PRs, but looking back at this, it seems this crate still hasn't been updated in redox, and there still seem to be some packages using it? I think realistically they should just be migrated to use zeroize at this point, but maybe pull the fix from upstream in the meantime? |
|
It looks like I was the one who forked the crate. I wouldn't have been doing anything with it outside of redox_users, which uses zeroize now. Unless somebody else used it someplace that I'm not aware of (or forgot about), then this is probably not in use anymore. |
|
Searching the org here on github for remaining references turned up https://github.com/redox-os/thrussh/blob/master/thrussh/Cargo.toml#L56. I recall seeing another dependency as well, but I don't remember how I did the search last week. EDIT: https://github.com/search?q=org%3Aredox-os+cryptovec+filename%3ACargo.toml&type=Code&ref=advsearch&l=&l= suggests just Regardless, thanks for taking a look. |
|
Redox moved to GitLab years ago, thus if you still want to merge it, move this pull request to there. If you still want to contribute to Redox, talk with us in the Matrix chat. |
The resize function did not guarantee an overwrite of the underlying buffer in either of the cases where it should have.
Fix the issues and add unit tests to check these invariants.