Added DRBG_HMAC & DRBG_HASH by slontis · Pull Request #6779 · openssl/openssl

slontis · 2018-07-25T03:02:58Z

defaults can now be set for master/public/private
renamed generate_counter back to reseed_counter so it is not confusing and different from standard. (The value is actually used by the Hash DRBG, so I didnt want to use generate_counter+1 in that code.)
generated new cavs data tests.

I have not attempted to create any new API's, so some values have been added into flags parameters.

i.e-
(1) The DRBG_hmac uses the same type (nid's) as DRBG_hash - It just uses an additional flag to select HMAC.
(2) Since the DRBG’s don’t actually get setup until they are accessed via …master_get0() etc, the set_defaults() can be called multiple times to set default (type and flags) for master, public and private DRBG’s before they are used.

The uninstantiate() has been changed so that it can use any new defaults for type, flags. This mainly allows testing of different set_defaults (which you can’t really do any other way since the getters are static init_once methods that have Cleanups that only run on exit). (The uninstatiate() used to just clear the internal state and reset it back to the same type and flags).
There is also a set(type, flags) that can be also used to change the type & flags to a different DRBG. The set also now calls uninstatiate if required.

Checklist

documentation is added or updated
tests are added or updated

slontis · 2018-07-25T03:42:04Z

Test Failure(s) are related to the other pull request that was submitted today (6778)

paulidale · 2018-07-25T03:46:43Z

Try make doc-nits for the documentation issue...

paulidale · 2018-07-25T06:43:06Z

I'm wondering if the label should be Post 1.1.1 rather than FIPS.

This change includes two other DRBGs, which are worthwhile as a fallback. FIPS requires a whole lot more testing etc.

I'm not fussed much either way.

mattcaswell · 2018-09-11T15:16:53Z

include/openssl/rand_drbg.h

Changing the value unnecessarily breaks ABI compat. Keep it as 1.

mattcaswell · 2018-09-11T15:33:47Z

include/openssl/rand_drbg.h

This is problematic. See #7182.

Note: issue #7182 has been fixed by #7190.

mattcaswell · 2018-09-11T15:46:34Z

crypto/rand/drbg_hash.c

Why 2011? Copy & paste error?

The code was mostly lifted from the old FOM I believe.

The date is the same as the drbg_ctr.c file.
i.e- code was copied from old FOM and updated.

mattcaswell · 2018-09-11T15:47:17Z

crypto/rand/drbg_hash.c

Multi line comments should start with a blank line like this:

/* * my multi * line comment */

mattcaswell · 2018-09-11T15:54:07Z

crypto/rand/rand_lcl.h

Renaming this to be the same as what reseed_prop_counter used to be called seems like its going to be a source of errors when we cherry pick fixes from our new version to the 1.1.1 version.

This is a bit unfortunate :) I will change it back and not match the standard :(

Well you could give it a different name that is almost the same as the standard (e.g. some suffix or other). Just calling it exactly the same as what something else was called sounds like a recipe for problems.

mattcaswell · 2018-09-11T16:34:34Z

crypto/rand/drbg_hmac.c

Describe params

mattcaswell · 2018-09-11T16:34:40Z

crypto/rand/drbg_hmac.c

Describe params

mattcaswell · 2018-09-11T16:36:32Z

crypto/rand/drbg_hmac.c

Where does this number come from?

Added the following comments:
This one is a general one added up higher..
/* These are taken from SP 800-90 10.1 Table 2 */

/* Maximum number of bits per request = 2^19 = 2^16 bytes */

mattcaswell · 2018-09-11T16:39:47Z

crypto/rand/drbg_lib.c

else on same line as }

mattcaswell · 2018-09-11T16:40:19Z

crypto/rand/drbg_lib.c

Here we get a RANDerr call but nowhere else!

Do you mean there are no RANDerr calls in drbg_ctr/drbg_hmac/drbg_hash?

Yes - that's my point. It seems inconsistent that there are no RANDerr calls elsewhere, but there is here. Not sure what the answer is - just pointing it out.

mattcaswell · 2018-09-25T10:03:02Z

crypto/rand/drbg_hash.c

I suspect some compilers will complain about the truncation here (size_t -> unsigned char) - even though we know it is safe. Might need a cast on each of these four lines, e.g.
tmp[tmp_sz++] = (unsigned char)(num_bits_returned & 0xff);

mattcaswell · 2018-09-25T10:10:09Z

crypto/rand/drbg_hash.c

elseon same line as }. Also we need {} around the return 0 because we use them above (if you use {} on one branch, you must use them on all branches).

mattcaswell · 2018-09-25T10:12:55Z

crypto/rand/drbg_hash.c

We never use OPENSSL_assert anymore (at least in the libraries). Use ossl_assert instead (or possibly just assert).

mattcaswell · 2018-09-25T10:14:38Z

crypto/rand/drbg_hash.c

Possible cast required here (unsigned char)?

mattcaswell · 2018-09-25T10:40:16Z

crypto/rand/drbg_hash.c

On all the other lines above you've used ctx, but here its hash->ctx. Why?

mattcaswell · 2018-09-25T10:43:01Z

crypto/rand/drbg_hash.c

Possible casts needed?

mattcaswell · 2018-09-25T11:11:17Z

crypto/rand/drbg_lib.c

Why the blank lines here?

mattcaswell · 2018-09-25T14:05:46Z

crypto/rand/drbg_lib.c

Yes - that's my point. It seems inconsistent that there are no RANDerr calls elsewhere, but there is here. Not sure what the answer is - just pointing it out.

mattcaswell · 2018-09-25T14:17:42Z

include/openssl/rand_drbg.h

Why no 0x04or 0x08 flag?

I have moved them down - I wanted to logically seperate the last 3 flags from the other flags - but it doesnt really matter.

I think I will leave the RANDerr call alone for now.. I think this is probably an issue across the entire code base that needs to be addressed.

…ter/public/private + renamed generate_counter back to reseed_counter + generated new cavs data tests

mattcaswell

Approved. Ping @openssl/committers for second review

paulidale · 2018-09-27T21:24:58Z

Merged. Thanks everyone.

…ter/public/private + renamed generate_counter back to reseed_counter + generated new cavs data tests Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #6779)

kroeckx · 2018-09-27T21:46:00Z

doc/man3/RAND_DRBG_new.pod

+calls are required to set the types to different values. If none of these 3
+flags are used, then the same type and flags are used for all 3 DRBG's in the
+B<drbg> chain (<master>, <public> and <private>). The default used if this
+method is not called is to use RAND_DRBG_FLAGS.


This last sentence isn't making much sense to me.

Maybe it's not related to the 3 options above, and you want to write that after the =back? And it's probably more useful to say that the default is that all 3 DRBGs act as none of the options are set.

I could probably just shift it up to where it says
'If this method is not called then the default type is given by RAND_DRBG_TYPE.'

and change it to..

'If this method is not called then the default type is given by NID_aes_256_ctr and the default flags are zero.'

Added PR #7326

kroeckx · 2018-09-27T21:48:02Z

doc/man3/RAND_DRBG_new.pod

+NID_sha256, NID_sha384, NID_sha512, NID_sha512_224, NID_sha512_256,
+NID_sha3_224, NID_sha3_256, NID_sha3_384 or NID_sha3_512.
+
+If this method is not called then the default type is given by RAND_DRBG_TYPE.


This really doesn't tell anything, maybe explicitly say NID_aes_256_ctr?

kroeckx · 2018-09-27T21:56:53Z

crypto/rand/drbg_lib.c

    drbg->meth->uninstantiate(drbg);
-    return RAND_DRBG_set(drbg, drbg->type, drbg->flags);
+
+    /* The reset uses the default values for type and flags */


The commit comment explains why it does this. i.e-

The uninstantiate() has been changed so that it can use any new defaults for type, flags. This mainly allows testing of different set_defaults (which you can’t really do any other way since the getters are static init_once methods that have Cleanups that only run on exit). (The uninstatiate() used to just clear the internal state and reset it back to the same type and flags).
There is also a set(type, flags) that can be also used to change the type & flags to a different DRBG. The set also now calls uninstatiate if required.

The commit message doesn't explain it, it's rather short.

The effect of this is if you did a RAND_DRBG_set(), and you uninstantiate it, it gets overridden by defaults now, which might be unexpected.

kroeckx · 2018-09-27T21:59:06Z

crypto/rand/drbg_lib.c


    if (drbg->reseed_interval > 0) {
-        if (drbg->generate_counter >= drbg->reseed_interval)
+        if (drbg->reseed_gen_counter > drbg->reseed_interval)


Is the > vs >= some bug fix?

The counter starts at 1 instead of zero now.
(The value is actually used by the Hash DRBG)

kroeckx added the FIPS label Jul 25, 2018

slontis mentioned this pull request Jul 25, 2018

DRBG update #6533

Closed

mattcaswell added this to the Assessed milestone Aug 2, 2018

slontis force-pushed the drbg branch from 0763de7 to 4525d48 Compare August 22, 2018 04:48

mattcaswell modified the milestones: Assessed, Post 1.1.1 Aug 27, 2018

paulidale added the after 1.1.1 label Aug 27, 2018

This was referenced Sep 5, 2018

DRBG: Using a weaker strength DRBG as a source is not currently supported. #7118

Closed

DRBG: Prediction Resistance is not currently supported. #7119

Closed

slontis force-pushed the drbg branch from 4525d48 to 2e3c707 Compare September 5, 2018 07:02

mattcaswell mentioned this pull request Sep 11, 2018

RAND_DRBG_USED_FLAGS is problematic for ABI compat #7182

Closed

mattcaswell requested changes Sep 11, 2018

View reviewed changes

slontis force-pushed the drbg branch 2 times, most recently from dc1a287 to 17000d0 Compare September 17, 2018 01:00

mattcaswell requested changes Sep 25, 2018

View reviewed changes

Added DRBG_HMAC & DRBG_HASH + Added defaults for setting DRBG for mas…

d7f64f9

…ter/public/private + renamed generate_counter back to reseed_counter + generated new cavs data tests

slontis force-pushed the drbg branch from 17000d0 to d7f64f9 Compare September 27, 2018 01:29

mattcaswell approved these changes Sep 27, 2018

View reviewed changes

paulidale approved these changes Sep 27, 2018

View reviewed changes

paulidale closed this Sep 27, 2018

kroeckx reviewed Sep 27, 2018

View reviewed changes

mattcaswell mentioned this pull request Oct 15, 2018

Add EVP_MAC API #7393

Closed

2 tasks

mspncp mentioned this pull request Mar 22, 2020

test/drbgtest.c: Fix error check test #11195

Closed

1 task

Uh oh!

Comments

Conversation

slontis commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

slontis commented Jul 25, 2018

Uh oh!

paulidale commented Jul 25, 2018

Uh oh!

paulidale commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

paulidale commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slontis commented Jul 25, 2018 •

edited

Loading

slontis Sep 28, 2018 •

edited

Loading

slontis Sep 28, 2018 •

edited

Loading