feat(ciphers+fuzz): Add experimental feature to use a keyed version of SHAKE256 instead of Blake2b. #581
Conversation
|
@DavidNiehues Can you rebase the pull request? Its set to prevent me from doing so. |
fuzz/fuzz_targets/blake2b.rs
Outdated
| @@ -1,18 +1,18 @@ | |||
| #![no_main] | |||
| # ![no_main] | |||
There was a problem hiding this comment.
I think this file needs to be renamed then. No need to do this right now, but I think it makes sense to allow both the sha3 and broken blake2b hash functions to coexist (feature flag "hash-blake2b", "hash-sha3") to allow things like fuzzing and tests to use them directly
There was a problem hiding this comment.
Good point, at least the file name is misleading as of now.
ciphers/src/subtle/keyed_shake256.rs
Outdated
| // we append the length of the input to the end of the input. | ||
| shake256.update(&(32u32.to_be_bytes())); | ||
| shake256.finalize_xof().read(tmp.as_mut()); | ||
| copy_slice(tmp.as_ref()).to(out); |
There was a problem hiding this comment.
Can we get rid of the temp variable? Copy_slice needs to assert anyway so we might also use try_into to just cast the input to a &mut [u8; 32] to feed it into finalize directly.
There was a problem hiding this comment.
Sure thing. Not sure why I didn't do it that way directly.
ciphers/src/subtle/keyed_shake256.rs
Outdated
| let mut tmp = Zeroizing::new([0u8; OUT_LEN]); | ||
| // Following the NIST recommendations in Appendix A.2 of the FIPS 202 standard, | ||
| // we append the length of the input to the end of the input. | ||
| shake256.update(&(32u32.to_be_bytes())); |
There was a problem hiding this comment.
Why big endian? Is this the length of the key?
There was a problem hiding this comment.
It's the output length and thus should also use the OUT_LEN constant. The encoding in big endian is arbitrary, it could be any other reasonable encoding of the output length just as well.
There was a problem hiding this comment.
Lets use little endian then; we generally use little endian in most places in rosenpass.
ciphers/src/subtle/keyed_shake256.rs
Outdated
| shake256.update(data); | ||
|
|
||
| let mut tmp = Zeroizing::new([0u8; OUT_LEN]); | ||
| // Following the NIST recommendations in Appendix A.2 of the FIPS 202 standard, |
There was a problem hiding this comment.
Good, but then I think we need to make this part of the name of the module.
Is there any particular reason for this recommendation by the way? Sounds somewhat like cruft but I do not know.
There was a problem hiding this comment.
Not too familiar with this scheme, but I was curious as well so I looked up the spec:
A.2 Additional Consideration for Extendable-Output Functions
[...]
if an attacker is able to induce one of the parties to use a different value for keylength,
say 168 bits, but the same value for keymaterial, then the two parties will end up with the
following keys:
SHAKE128(keymaterial, 112) = fg
SHAKE128(keymaterial, 168) = fgh,
where the bolded letters of the digest represent 56-bit strings, e.g., the parts of a Triple DES key.
Because of the structure of Triple DES, these keys are vulnerable to attack
[...]
Source: https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.202.pdf (pages 24/25, i.e., 32/33 in the PDF)
Technically there's no section called "appendix", so if that's the right part you could consider linking to it? Alternatively, summarizing the reason briefly might help as well. (Otherwise, disregard my comment)
There was a problem hiding this comment.
Why: The reason to include this is what @phildremi quoted. Since SHAKE256 is a XOF, two outputs with different lengths but the same input will be identcal up to the length of the shorter output, which can lead to related key attacks. I am not aware of any vulnerability that would be introduced by not dong this given that rosenpass does use domain separation and only uses a fixed key and output length of 32 bytes. I added it as a safeguard to make the implementation more robust with regards to future changes. If you prefer it that way, we can of course do without incorporating the output length.
Appendix A.2 Indeed, while A.x, B.x, ... usually refers to an appendix, everywhere in the standard, it is referred to as a section. I'll thus update the reference.
ciphers/src/subtle/keyed_shake256.rs
Outdated
| pub const OUT_MAX: usize = OUT_LEN; | ||
|
|
||
| /// Provides a keyed hash function based on SHAKE256. To work for the protocol, the output length | ||
| /// and key length are fixed to 32 bytes (also see [KEN_LEN] and [OUT_LEN]). |
There was a problem hiding this comment.
| /// and key length are fixed to 32 bytes (also see [KEN_LEN] and [OUT_LEN]). | |
| /// and key length are fixed to 32 bytes (also see [KEY_LEN] and [OUT_LEN]). |
There was a problem hiding this comment.
@phildremi Thank you for the help!
5046a8a to
6727fa7
Compare
…f SHAKE256 instead of Blake2b.
6727fa7 to
9b183ae
Compare
This PR adds support for using a keyed version of SHAKE256, truncated to 32 bytes, instead of the incorrect HMAC with Blake2b as the PRF for key derivation for rosenpass.
This feature is a compile time flag as of now, namely the flag "experiment_sha3".
Since there is also fuzzing for Blake2b, this PR also disables this fuzzing if the feature "experiment_sha3" is disabled.