Daiki Ueno <u...@gnu.org> writes:
> From: Daiki Ueno <du...@redhat.com>
Comments and questions on patch 1/2:
> index 135542f..035074c 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -110,6 +110,7 @@ nettle_SOURCES = aes-decrypt-internal.c aes-decrypt.c \
> md2.c md2-meta.c md4.c md4-meta.c \
> md5.c md5-compress.c md5-compat.c md5-meta.c \
> memeql-sec.c memxor.c memxor3.c \
> + mgf1.c mgf1-sha256.c mgf1-sha384.c mgf1-sha512.c \
> nettle-meta-aeads.c nettle-meta-armors.c \
> nettle-meta-ciphers.c nettle-meta-hashes.c \
> pbkdf2.c pbkdf2-hmac-sha1.c pbkdf2-hmac-sha256.c \
> @@ -144,6 +145,7 @@ hogweed_SOURCES = sexp.c sexp-format.c \
> pkcs1.c pkcs1-encrypt.c pkcs1-decrypt.c \
> pkcs1-rsa-digest.c pkcs1-rsa-md5.c pkcs1-rsa-sha1.c \
> pkcs1-rsa-sha256.c pkcs1-rsa-sha512.c \
> + pss.c pss-sha256.c pss-sha512.c \
> rsa.c rsa-sign.c rsa-sign-tr.c rsa-verify.c \
> rsa-pkcs1-sign.c rsa-pkcs1-sign-tr.c rsa-pkcs1-verify.c \
> rsa-md5-sign.c rsa-md5-sign-tr.c rsa-md5-verify.c \
> @@ -194,9 +196,10 @@ HEADERS = aes.h arcfour.h arctwo.h asn1.h blowfish.h \
> md2.h md4.h \
> md5.h md5-compat.h \
> memops.h memxor.h \
> + 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.
> 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).
> --- /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).
> +
> +int
> +mgf1(void *seed, void *state, const struct nettle_hash *hash,
I think seed should be declared as const void *.
> + size_t mask_length, uint8_t *mask)
> +{
> + TMP_DECL(h, uint8_t, NETTLE_MAX_HASH_DIGEST_SIZE);
> + size_t i, blocks;
> + uint8_t c[4], *p;
> +
> + TMP_ALLOC(h, hash->digest_size);
> +
> + blocks = (mask_length + hash->digest_size - 1) / hash->digest_size;
> + for (i = 0, p = mask; i < blocks; i++, p += hash->digest_size)
> + {
> + c[0] = (i >> 24) & 0xFF;
> + c[1] = (i >> 16) & 0xFF;
> + c[2] = (i >> 8) & 0xFF;
> + c[3] = i & 0xFF;
Use the WRITE_UINT32 macro.
> + 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.
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.
> --- /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.
> +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.
> --- /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).
> + nettle_mgf_func mgf,
> + size_t salt_length, const uint8_t *salt,
> + const uint8_t *digest)
> +{
> + TMP_GMP_DECL(em, uint8_t);
> + uint8_t pad[8];
> + size_t key_size = (bits + 7) / 8;
> + size_t j;
> +
> + TMP_GMP_ALLOC(em, key_size);
> +
> + if (key_size < hash->digest_size + salt_length + 2)
> + {
> + TMP_GMP_FREE(em);
> + return 0;
> + }
> +
> + /* Compute M'. */
> + hash->init(state);
> + memset(pad, 0, 8);
> + hash->update(state, 8, pad);
> + hash->update(state, hash->digest_size, digest);
> + hash->update(state, salt_length, salt);
> +
> + /* Store H in EM, right after maskedDB. */
> + hash->digest(state, hash->digest_size, em + key_size - hash->digest_size -
> 1);
> +
> + /* Compute dbMask. */
> + hash->init(state);
> + hash->update(state, hash->digest_size, em + key_size - hash->digest_size -
> 1);
> +
> + mgf(state, key_size - hash->digest_size - 1, em);
> +
> + /* Compute maskedDB and store it in front of H in EM. */
> + for (j = 0; j < key_size - salt_length - hash->digest_size - 2; j++)
> + em[j] ^= 0;
> + em[j++] ^= 1;
> + memxor(em + j, salt, salt_length);
> + j += salt_length;
> +
> + /* Store the trailer field following H. */
> + j += hash->digest_size;
> + *(em + j) = 0xbc;
> +
> + /* Clear the leftmost 8 * emLen - emBits of the leftmost octet in EM. */
> + *em &= pss_masks[(8 * key_size - bits)];
> +
> + nettle_mpz_set_str_256_u(m, key_size, em);
> + TMP_GMP_FREE(em);
> + return 1;
> +}
> +
> +int
> +pss_verify(mpz_t m, size_t bits,
> + void *state, const struct nettle_hash *hash,
> + nettle_mgf_func mgf,
> + size_t salt_length,
> + const uint8_t *digest)
> +{
It would be nice with some comments summarizing how pss_encode and
pss_verify relate.
> + /* 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?
> + 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(...));
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.
Regards,
/Niels
--
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
_______________________________________________
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
