Sequential call of `encrypt`/`dump`/`wrap` with the same cocoon object produces the same cipher text
Closed this issue · 10 comments
Cloning the StdRng object for every encrypt call brings the rng back to the initial seeded starting point. This is a security issue as when you try to create new encrypted messages with the same cocoon object, the IV does not change, and the same ciperblock is returned. See tests below.
pub const SEED: &[u8] = b"123456789abcdefghijklmnopqrstuvw";
let seed = SEED;
let mut s = [0u8; KEY_SIZE];
s.copy_from_slice(seed);
let mut rng = StdRng::from_seed(s);
let mut nonce = [0u8; 32];
for i in 0..5 {
let mut rng = rng.clone();
rng.fill_bytes(&mut nonce);
println!("Nonce: {:?}", nonce);
}Output:
Nonce: [91, 177, 20, 216, 167, 56, 166, 87, 46, 160, 25, 99, 230, 239, 202, 50, 251, 54, 132, 240, 215, 77, 131, 90, 29, 35, 172, 31, 212, 88, 131, 169]
Nonce: [91, 177, 20, 216, 167, 56, 166, 87, 46, 160, 25, 99, 230, 239, 202, 50, 251, 54, 132, 240, 215, 77, 131, 90, 29, 35, 172, 31, 212, 88, 131, 169]
Nonce: [91, 177, 20, 216, 167, 56, 166, 87, 46, 160, 25, 99, 230, 239, 202, 50, 251, 54, 132, 240, 215, 77, 131, 90, 29, 35, 172, 31, 212, 88, 131, 169]
Nonce: [91, 177, 20, 216, 167, 56, 166, 87, 46, 160, 25, 99, 230, 239, 202, 50, 251, 54, 132, 240, 215, 77, 131, 90, 29, 35, 172, 31, 212, 88, 131, 169]
Nonce: [91, 177, 20, 216, 167, 56, 166, 87, 46, 160, 25, 99, 230, 239, 202, 50, 251, 54, 132, 240, 215, 77, 131, 90, 29, 35, 172, 31, 212, 88, 131, 169]Commenting out let mut rng = rng.clone();
Output:
Nonce: [91, 177, 20, 216, 167, 56, 166, 87, 46, 160, 25, 99, 230, 239, 202, 50, 251, 54, 132, 240, 215, 77, 131, 90, 29, 35, 172, 31, 212, 88, 131, 169]
Nonce: [50, 174, 147, 111, 250, 33, 221, 25, 109, 138, 30, 5, 14, 234, 162, 123, 114, 140, 134, 5, 35, 114, 12, 205, 63, 173, 138, 48, 137, 220, 130, 30]
Nonce: [213, 229, 165, 126, 127, 72, 61, 11, 85, 159, 85, 9, 78, 12, 134, 234, 165, 82, 4, 170, 91, 11, 244, 88, 132, 73, 149, 22, 35, 102, 67, 232]
Nonce: [189, 243, 159, 198, 83, 41, 103, 147, 244, 13, 187, 2, 189, 102, 232, 90, 249, 71, 170, 30, 118, 15, 223, 75, 8, 146, 219, 161, 117, 159, 87, 42]
Nonce: [107, 167, 36, 67, 64, 222, 101, 250, 201, 107, 88, 239, 128, 232, 45, 145, 203, 68, 247, 64, 0, 126, 176, 172, 170, 113, 248, 174, 63, 27, 43, 197]Created a pull request fixing the issue, and adding some new test criteria to all 4 of the encrypt functions #23
Also opened an issue here: rust-random/rand#1345
Wow, nice finding. I left comments in the PR.
@ProjectInitiative What a nasty issue :) Here's the thing: originally, I designed API to be a one-shot, e.g. you initialize Cocoon, dump something, and leave. On the other side, sequential dumping/encrypting is a nice feature. However, changing the API to be mutable would break backward compatibility.
Options:
- We could introduce
dump_next(&mut self,...),encrypt_next(&mut self, ...). - Bumping version to
0.4.0.
Both are good, I would suggest a contingency on option 1: might be a good idea to add an crate.io advisory for < 3.x.x as I found this out by NOT using the crate in the intended way, and others might have as well while thinking their implementation was correct.
As for which to select? I am not sure, I will defer the direction of the project to project owner.
I will note from a security perspective, option 2 with a crate.io advisory would force/push dependent projects to re-evaluate and confirm the security they expect in their respective projects.
@ProjectInitiative I am keen towards moving hardly: mut + version bump to 0.4.0 + rustsec advisory for <0.3.x.
Why:
- I haven't found evidence that I deliberately planned to have an immutable API, although I found a commit, where the API became to be immutable:
fa4d1d9. - Introducing additional API calls doesn't make the old API more secure for sequential usage.
@ProjectInitiative 0.4.0 is published (https://crates.io/crates/cocoon)
Closing issue!
Security Advisory PR: rustsec/advisory-db#1805
@ProjectInitiative I just found that it was reproduced with MiniCocoon only and with Cocoon::from_seed (and others with custom RNGs and seeds) where StdRng is used! That's why I haven't found it easily in the first place. It means that cloning ThreadRng (which is used in Cocoon::new) and StdRng (which is used in MiniCocoon and Cocoon::from_seed) behaves differently, or maybe ThreadRng adds entropy every time no matter what.