From 801ab68eac3f442ec10688f17ad9f76f873fb76f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 26 Feb 2019 07:06:57 +0000 Subject: [PATCH] Rewrite invent_firstbits(). Instead of repeatedly looping on the random number generator until it comes up with two values that have a large enough product, the new version guarantees only one use of random numbers, by first counting up all the possible pairs of values that would work, and then inventing a single random number that's used as an index into that list. I've done the selection from the list using constant-time techniques, not particularly because I think key generation can be made CT in general, but out of sheer habit after the last few months, and who knows, it _might_ be useful. While I'm at it, I've also added an option to make sure the two firstbits values differ by at least a given value. For RSA, I set that value to 2, guaranteeing that even if the smaller prime has a very long string of 1 bits after the firstbits value and the larger has a long string of 0, they'll still have a relative difference of at least 2^{-12}. Not that there was any serious chance of the primes having randomly ended up so close together as to make the key in danger of factoring, but it seems like a silly thing to leave out if I'm rewriting the function anyway. --- ssh.h | 2 +- sshdssg.c | 2 +- sshprime.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++------- sshrsag.c | 8 +++- 4 files changed, 101 insertions(+), 16 deletions(-) diff --git a/ssh.h b/ssh.h index 2fcef6fa..c07fd9aa 100644 --- a/ssh.h +++ b/ssh.h @@ -1172,7 +1172,7 @@ int eddsa_generate(struct eddsa_key *key, int bits, progfn_t pfn, mp_int *primegen( int bits, int modulus, int residue, mp_int *factor, int phase, progfn_t pfn, void *pfnparam, unsigned firstbits); -void invent_firstbits(unsigned *one, unsigned *two); +void invent_firstbits(unsigned *one, unsigned *two, unsigned min_separation); /* * Connection-sharing API provided by platforms. This function must diff --git a/sshdssg.c b/sshdssg.c index cece7b0f..091a51e6 100644 --- a/sshdssg.c +++ b/sshdssg.c @@ -57,7 +57,7 @@ int dsa_generate(struct dss_key *key, int bits, progfn_t pfn, pfn(pfnparam, PROGFN_READY, 0, 0); unsigned pfirst, qfirst; - invent_firstbits(&pfirst, &qfirst); + invent_firstbits(&pfirst, &qfirst, 0); /* * Generate q: a prime of length 160. */ diff --git a/sshprime.c b/sshprime.c index 8f54d271..33000273 100644 --- a/sshprime.c +++ b/sshprime.c @@ -365,7 +365,8 @@ mp_int *primegen( /* * Invent a pair of values suitable for use as 'firstbits' in the - * above function, such that their product is at least 2. + * above function, such that their product is at least 2, and such + * that their difference is also at least min_separation. * * This is used for generating both RSA and DSA keys which have * exactly the specified number of bits rather than one fewer - if you @@ -377,19 +378,97 @@ mp_int *primegen( * the lines of 'Hey, I asked PuTTYgen for a 2048-bit key and I only * got 2047 bits! Bug!' */ -void invent_firstbits(unsigned *one, unsigned *two) +static inline unsigned firstbits_b_min( + unsigned a, unsigned lo, unsigned hi, unsigned min_separation) +{ + /* To get a large enough product, b must be at least this much */ + unsigned b_min = (lo*lo + a - 1) / a; + /* Now enforce a hi) + b_min = hi; + return b_min; +} + +void invent_firstbits(unsigned *one, unsigned *two, unsigned min_separation) { /* - * Our criterion is that any number in the range [one,one+1) - * multiplied by any number in the range [two,two+1) should have - * the highest bit set. It should be clear that we can trivially - * test this by multiplying the smallest values in each interval, - * i.e. the ones we actually invented. + * We'll pick 12 initial bits (number selected at random) for each + * prime, not counting the leading 1. So we want to return two + * values in the range [2^12,2^13) whose product is at least 2^24. + * + * Strategy: count up all the viable pairs, then select a random + * number in that range and use it to pick a pair. + * + * To keep things simple, we'll ensure a < b, and randomly swap + * them at the end. */ - do { - uint8_t bytes[2]; - random_read(bytes, 2); - *one = 0x100 | bytes[0]; - *two = 0x100 | bytes[1]; - } while (*one * *two < 0x20000); + const unsigned lo = 1<<12, hi = 1<<13, minproduct = lo*lo; + unsigned a, b; + + /* + * Count up the number of prefixes of b that would be valid for + * each prefix of a. + */ + mp_int *total = mp_new(32); + for (a = lo; a < hi; a++) { + unsigned b_min = firstbits_b_min(a, lo, hi, min_separation); + mp_add_integer_into(total, total, hi - b_min); + } + + /* + * Make up a random number in the range [0,2*total). + */ + mp_int *mlo = mp_from_integer(0), *mhi = mp_new(32); + mp_lshift_fixed_into(mhi, total, 1); + mp_int *randval = mp_random_in_range(mlo, mhi); + mp_free(mlo); + mp_free(mhi); + + /* + * Use the low bit of randval as our swap indicator, leaving the + * rest of it in the range [0,total). + */ + unsigned swap = mp_get_bit(randval, 0); + mp_rshift_fixed_into(randval, randval, 1); + + /* + * Now do the same counting loop again to make the actual choice. + */ + a = b = 0; + for (unsigned a_candidate = lo; a_candidate < hi; a_candidate++) { + unsigned b_min = firstbits_b_min(a_candidate, lo, hi, min_separation); + unsigned limit = hi - b_min; + + unsigned b_candidate = b_min + mp_get_integer(randval); + unsigned use_it = 1 ^ mp_hs_integer(randval, limit); + a ^= (a ^ a_candidate) & -use_it; + b ^= (b ^ b_candidate) & -use_it; + + mp_sub_integer_into(randval, randval, limit); + } + + mp_free(randval); + + /* + * Check everything came out right. + */ + assert(lo <= a); + assert(a < hi); + assert(lo <= b); + assert(b < hi); + assert(a * b >= minproduct); + assert(b >= a + min_separation); + + /* + * Last-minute optional swap of a and b. + */ + unsigned diff = (a ^ b) & (-swap); + a ^= diff; + b ^= diff; + + *one = a; + *two = b; } diff --git a/sshrsag.c b/sshrsag.c index 5794435d..d70ac6b4 100644 --- a/sshrsag.c +++ b/sshrsag.c @@ -63,8 +63,14 @@ int rsa_generate(RSAKey *key, int bits, progfn_t pfn, * and e to be coprime, and (q-1) and e to be coprime, but in * general that's slightly more fiddly to arrange. By choosing * a prime e, we can simplify the criterion.) + * + * We give a min_separation of 2 to invent_firstbits(), ensuring + * that the two primes won't be very close to each other. (The + * chance of them being _dangerously_ close is negligible - even + * more so than an attacker guessing a whole 256-bit session key - + * but it doesn't cost much to make sure.) */ - invent_firstbits(&pfirst, &qfirst); + invent_firstbits(&pfirst, &qfirst, 2); mp_int *p = primegen(bits / 2, RSA_EXPONENT, 1, NULL, 1, pfn, pfnparam, pfirst); mp_int *q = primegen(bits - bits / 2, RSA_EXPONENT, 1, NULL,