[openssl-dev] [openssl-commits] [openssl] master update

[Kaduk, Ben]

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" <rsalz at openssl.org> wrote:

>The branch master has been updated
>       via  f00a10b89734e84fe80f98ad9e2e77b557c701ae (commit)
>      from  3c65047d30dacca345d30269b95af4a5c413e8d1 (commit)
>
>
>- Log -----------------------------------------------------------------
>commit f00a10b89734e84fe80f98ad9e2e77b557c701ae
>Author: Ismo Puustinen <ismo.puustinen at intel.com>
>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 <rsalz at akamai.com>
>    Reviewed-by: Emilia Käsper <emilia at openssl.org>
>
>-----------------------------------------------------------------------
>
>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
>