Hello Niels, Simo,
Thanks for the comments. I've updated MR addressing part of them (see inline for the details).
Simo Sorce simo@redhat.com writes:
On Mon, 2024-01-15 at 21:43 +0100, Niels Möller wrote:
Thanks, I've had a look, and it looks pretty good to me. Some comments and questions:
- For tests, would it make some with some test that check that encryption with a given message and randomness gives the expected output? Even better if there are any authoritative testcases for that?
I would be happy to add if there are any, even if they are not so authoritative, though I wasn't even able to find ones with compatible license, in particular with SHA-2 being used as an underlying hash algorithm for MGF-1.
- Project Wycheproof (Apache 2.0): https://github.com/google/wycheproof/blob/master/testvectors/rsa_oaep_2048_s...
- Python Cryptography (Apache 2.0 and BSD): https://cryptography.io/en/latest/development/custom-vectors/rsa-oaep-sha2/
In any case, I'll try to check against those vectors manually outside the Nettle repository to ensure the correctness.
- Is it useful to have oaep_decode_mgf1 and oaep_encode_mgf1 advertised as public functions, or would it be better to make them internal?
Made them internal functions.
- Do you see any reasonable (i.e., with a net gain in maintainability) way to share more code between _oaep_sec_decrypt_variable and _pkcs1_sec_decrypt_variable?
I did review this part, and to me it seem like it is more maintainable to keep them separate, they already are tricky as it is, adding more variability sounds to me would just make them more complex and difficult to reason about.
I agree with that, considering potential optimization opportunities by the compiler.
- For oaep_decode_mgf1, oaep_encode_mgf1, maybe one could let the caller allocate and pass in the appropriate hashing context? Would be easy to do, e.g., in rsa_oaep_sha512_decrypt. But it looks like that would be inconsistent with pss_mgf1, though (which looks like it needs a separate hashing context).
Done.
- I think it was a design mistake to represent RSA ciphertexts as mpz_t rather then octet strings in Nettle's original RSA interfaces. I wonder if it would make sense to let the new functions take octet strings instead?
Done.
Regards,