nisse@lysator.liu.se (Niels Möller) writes:
Thank you for the detailed comments. Please find attached the updated patches.
mgf1.h \mgf1.h is intended as a public, rather than internal, header? Maybe rename to pss-mgf1.h, unless you foresee some non-pss use for it.
RSA-OAEP could also use it, though I am not sure if it is worth being supported.
index 0000000..11e908c --- /dev/null +++ b/mgf1-sha256.c +int +mgf1_sha256(const struct sha256_ctx *hash, size_t mask_length, uint8_t *mask)
Rename first argument "seed", for consistency with the mgf1 function. "hash" is generally used for struct nettle_hash, using it for a context struct is a bit confusing. (And similarly for the other functions mgf1_sha* functions).
I removed mgf1_sha* functions and made the mgf1 function more generic, based on the suggestion below for pss_sha*_encode. The interface now looks like:
void mgf1(const void *seed, const struct nettle_hash *hash, size_t length, uint8_t *mask);
--- /dev/null +++ b/mgf1.c @@ -0,0 +1,72 @@ +/* mgf1.c
+#define MGF1_MIN(a,b) ((a) < (b) ? (a) : (b))
Could consider moving the definition to macros.h or nettle-internal.h (I'm a bit surprised a macro like this isn't defined already, from a quick search, there's only GMP_MIN in mini-gmp.c, which isn't visible here).
After rewriting the loop, the macro is no longer necessary. I just removed it.
+int +mgf1(void *seed, void *state, const struct nettle_hash *hash,
I think seed should be declared as const void *.
Sure, fixed.
c[0] = (i >> 24) & 0xFF;c[1] = (i >> 16) & 0xFF;c[2] = (i >> 8) & 0xFF;c[3] = i & 0xFF;Use the WRITE_UINT32 macro.
Done.
memcpy(state, seed, hash->context_size);hash->update(state, 4, c);hash->digest(state, hash->digest_size, h);memcpy(p, h, MGF1_MIN(hash->digest_size, mask_length - (p - mask)));No need for the second memcpy, just pass the desired length to hash->digest.
Done.
Also, I'd consider rewriting the loop to decrement mask_length as you go (and rename it to just length). Then you may also be able to eliminate the blocks variable. You might also want to handle the final iteration specially (there are a couple of ways to do that, not sure what's cleanest), then you get rid of the MIN conditional. You might be able to get some ideas from the pbkdf2 function.
And unless you really need the original value of mask around, I think it makes the code simpler to update it throughout the loop too, and eliminate the extra loop variable p.
Thank you, those changes made the code much simpler.
--- /dev/null +++ b/pss-sha256.c @@ -0,0 +1,64 @@ +/* pss.c
I admit filenames in this place are of questionable utility. But this one is not correct.
I removed pss-sha*.c, in favor of consolidating them into the generic pss_{encode,verify}_mgf1 functions.
+int +pss_sha256_encode(mpz_t m, size_t bits,
size_t salt_length, const uint8_t *salt,const uint8_t *digest)+{
- struct sha256_ctx state;
- return pss_encode(m, bits,
&state, &nettle_sha256,(nettle_mgf_func *) mgf1_sha256,Since you pass &nettle_sha256 as an argument here, do you really need the specialized function mgf1_sha256 at all? Couldn't pss_encode use the generic mgf1 directly? If this loss of generality seems like a problem, pss_encode could be renamed to pss_encode_mgf1.
Done.
--- /dev/null +++ b/pss.c +int +pss_encode(mpz_t m, size_t bits,
void *state, const struct nettle_hash *hash,Is state needed by the callers? If not, allocate it locally here (using TMP_ALLOC and hash->context_size. If we need a NETTLE_MAX_HASH_CONTEXT_SIZE, we could add that in some way, preferably as a separate patch with some sanity check which could go in testsuite/meta-hash-test).
I am attaching a separate patch for this.
It would be nice with some comments summarizing how pss_encode and pss_verify relate.
Done.
- /* Check if H' = H. */
- if (memcmp(h2, h, hash->digest_size) != 0)
- {
TMP_GMP_FREE(em);return 0;- }
You could add a fail: label and use goto, to avoid repeating this block lots of times. Also there are lots of different ways this function can fail. What are the consequences if one of the falure cases is handled incorrectly, do we need tests for them all?
--- /dev/null +++ b/testsuite/pss-test.c @@ -0,0 +1,35 @@ +#include "testutils.h"
+#include "pss.h"
+void +test_main(void) +{
- struct tstring *salt;
- struct tstring *digest;
- mpz_t m;
- mpz_t expected;
- int ret;
- mpz_init(m);
- mpz_init(expected);
- salt = SHEX("11223344556677889900");
- /* From sha256-test.c */
- digest = SHEX("ba7816bf8f01cfea 414140de5dae2223"
"b00361a396177a9c b410ff61f20015ad");- ret = pss_sha256_encode(m, 1024, salt->length, salt->data, digest->data);
- ASSERT(ret == 1);
- mpz_set_str(expected,
"76b9a52705c8382c5367732f993184eff340b6305c9f73e7e308c8""004fcc15cbbaab01e976bae4b774628595379a2d448a36b3ea6fa8""353b97eeea7bdac93b4b7807ac98cd4b3bebfb31f3718e1dd3625f""227fbb8696606498e7070e21c3cbbd7386ea20eb81ac7927e0c6d1""d7788826a63af767f301bcc05dd65b00da862cbc", 16);Nice with unit tests for this function too. Thanks! Are there any official test vectors?
There are (historical?) test vectors from RSA for these intermediate primitives: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1/pkcs-1v2-1d2-vec.zip
The reason I didn't use them was that those test vectors use SHA-1 as the underlying hash algorithm, while the previous patch only provides SHA-2 variants of the pss_*_encode() functions.
Now that the generic pss_encode_mgf1() is provided, it is possible to use SHA-1 in the tests, so I just added them.
- ASSERT(mpz_cmp(m, expected) == 0);
- ret = pss_sha256_verify(m, 1024, salt->length, digest->data);
- ASSERT(ret == 1);
Simpler with just ASSERT(pss_sha256_verify(...));
Done.
Some test also for the failure case is desirable. Three reasonably simple checks are to try flipping some bit of m, digest or salt and check that it returns failure.
Done.
Regards,