Hi Rich, Ismo,

I'm surprised this doesn't cause the compiler to warn/error out (inline)

On 8/27/15, 21:57, "Rich Salz" <

[hidden email]> wrote:

>The branch master has been updated

> via f00a10b89734e84fe80f98ad9e2e77b557c701ae (commit)

> from 3c65047d30dacca345d30269b95af4a5c413e8d1 (commit)

>

>

>- Log -----------------------------------------------------------------

>commit f00a10b89734e84fe80f98ad9e2e77b557c701ae

>Author: Ismo Puustinen <

[hidden email]>

>Date: Fri Aug 7 22:14:47 2015 -0400

>

> GH367: Fix dsa keygen for too-short seed

>

> If the seed value for dsa key generation is too short (< qsize),

> return an error. Also update the documentation.

>

> Signed-off-by: Rich Salz <

[hidden email]>

> Reviewed-by: Emilia Käsper <

[hidden email]>

>

>-----------------------------------------------------------------------

>

>diff --git a/crypto/dsa/dsa_gen.c b/crypto/dsa/dsa_gen.c

>index e030cfa..a4fae17 100644

>--- a/crypto/dsa/dsa_gen.c

>+++ b/crypto/dsa/dsa_gen.c

>@@ -132,18 +132,15 @@ int dsa_builtin_paramgen(DSA *ret, size_t bits,

>size_t qbits,

>

> bits = (bits + 63) / 64 * 64;

>

>- /*

>- * NB: seed_len == 0 is special case: copy generated seed to seed_in

>if

>- * it is not NULL.

>- */

>- if (seed_len && (seed_len < (size_t)qsize))

>- seed_in = NULL; /* seed buffer too small -- ignore */

>- if (seed_len > (size_t)qsize)

>- seed_len = qsize; /* App. 2.2 of FIPS PUB 186 allows larger

>- * SEED, but our internal buffers are

>- * restricted to 160 bits */

>- if (seed_in != NULL)

>+ if (seed_in != NULL) {

>+ if (seed_len < (size_t)qsize)

>+ return 0;

>+ if (seed_len > (size_t)qsize) {

>+ /* Don't overflow seed local variable. */

The comment and code here are a slight mismatch, since qsize is

dynamically computed (but limited to three values, the largest of which is

used to size the local variable). It's not clear that using

SHA256_DIGEST_LENGTH for the check would actually be better, though.

>+ seed_len = qsize;

>+ }

> memcpy(seed, seed_in, seed_len);

>+ }

>

> if ((ctx = BN_CTX_new()) == NULL)

> goto err;

>@@ -166,20 +163,18 @@ int dsa_builtin_paramgen(DSA *ret, size_t bits,

>size_t qbits,

>

> for (;;) {

> for (;;) { /* find q */

>- int seed_is_random;

>+ int seed_is_random = seed_in == NULL;

This part seems really bogus; seed_is_random is an int, but seed_in is

const unsigned char *; the assignment makes no sense.

>

> /* step 1 */

> if (!BN_GENCB_call(cb, 0, m++))

> goto err;

>

>- if (!seed_len) {

>+ if (seed_is_random) {

and this chunk can never execute since seed_is_random was just set to 0

(er, NULL).

I guess the intent is to declare the variable in the outer loop?

> if (RAND_bytes(seed, qsize) <= 0)

> goto err;

>- seed_is_random = 1;

> } else {

>- seed_is_random = 0;

>- seed_len = 0; /* use random seed if 'seed_in' turns

>out to

>- * be bad */

>+ /* If we come back through, use random seed next time. */

>+ seed_in = NULL;

and seed_in is never read after this point.

> }

> memcpy(buf, seed, qsize);

> memcpy(buf2, seed, qsize);

>diff --git a/doc/crypto/DSA_generate_parameters.pod

>b/doc/crypto/DSA_generate_parameters.pod

>index d2a0418..92c89a0 100644

>--- a/doc/crypto/DSA_generate_parameters.pod

>+++ b/doc/crypto/DSA_generate_parameters.pod

>@@ -23,13 +23,12 @@ Deprecated:

> DSA_generate_parameters_ex() generates primes p and q and a generator g

> for use in the DSA and stores the result in B<dsa>.

>

>-B<bits> is the length of the prime to be generated; the DSS allows a

>-maximum of 1024 bits.

>+B<bits> is the length of the prime p to be generated.

>+For lengths under 2048 bits, the length of q is 160 bits; for lengths

>+at least 2048, it is set to 256 bits.

The grammar here is slightly unusual; "for lengths of at least 2048 bits"

or "for lengths 2048 bits and larger" would feel more natural to me.

-Ben

>

>-If B<seed> is B<NULL> or B<seed_len> E<lt> 20, the primes will be

>-generated at random. Otherwise, the seed is used to generate

>-them. If the given seed does not yield a prime q, a new random

>-seed is chosen and placed at B<seed>.

>+If B<seed> is NULL, the primes will be generated at random.

>+If B<seed_len> is less than the length of q, an error is returned.

>

> DSA_generate_parameters_ex() places the iteration count in

> *B<counter_ret> and a counter used for finding a generator in

>_____

>openssl-commits mailing list

>To unsubscribe:

https://mta.openssl.org/mailman/listinfo/openssl-commits>

_______________________________________________

openssl-dev mailing list

To unsubscribe:

https://mta.openssl.org/mailman/listinfo/openssl-dev