Re: [openssl-commits] [openssl] master update

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [openssl-commits] [openssl] master update

Benjamin Kaduk
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
Reply | Threaded
Open this post in threaded view
|

Re: [openssl-commits] [openssl] master update

Salz, Rich
TL;DR -- Don't read the diff, look at the revised code.

> 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.

If you can think of a more-c lear comment, let me know.  But checking against qsize is the right thing to do.

> >+            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.

No, it's like "seed_in == NULL ? 1 : 0"

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

Nope.  

> and seed_in is never read after this point.

It was set up before the loop.

> 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.

Open a ticket :)

_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl-commits] [openssl] master update

Benjamin Kaduk
On 8/28/15, 10:34, "Salz, Rich" <[hidden email]> wrote:

>TL;DR -- Don't read the diff, look at the revised code.
>
>> 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.
>
>If you can think of a more-c lear comment, let me know.  But checking
>against qsize is the right thing to do.
>
>> >+            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.
>
>No, it's like "seed_in == NULL ? 1 : 0"

Sigh, need more coffee.

Sorry for the noise :(

-Ben

>
>> I guess the intent is to declare the variable in the outer loop?
>
>Nope.  
>
>> and seed_in is never read after this point.
>
>It was set up before the loop.
>
>> 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.
>
>Open a ticket :)
>
>


_______________________________________________
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev