Prepare the RAND code for moving into the FIPS module by mattcaswell · Pull Request #9039 · openssl/openssl

mattcaswell · 2019-05-29T15:24:23Z

This is mostly about changing the way the RAND code initialises itself so that it is based on OPENSSL_CTX instead.

(Needed in order to move the RAND code into the FIPS provider - see #9035)

levitte · 2019-06-03T11:14:08Z

crypto/init.c

This could be implemented using a provider wide set_params...

levitte · 2019-06-03T11:17:32Z

At a first quick look through, this looks ok, but I frankly don't know this code enough. @mspncp, would you mind having a look?

levitte · 2019-06-03T11:17:48Z

Rebase needed for CIs

mattcaswell · 2019-06-03T12:18:57Z

Rebased as requested.

mspncp · 2019-06-03T23:17:47Z

@mspncp, would you mind having a look?

I started taking a look and immediately my gut instinct tells me that it's not happy about this proliferation of underscore_ex functions introduced for passing OPENSSL_CTX * pointers around. In particular, extending public API calls like RAND_bytes to RAND_bytes_ex feels wrong to me. But I need some time to figure out what precisely disturbs me and whether the problem could be addressed differently...

paulidale · 2019-06-04T04:56:36Z

_ctx ?

mspncp · 2019-06-04T05:00:07Z

_ctx ?

Might be an improvement over the "Microsoft disease" of adding exes to API functions. But I think the problem lies deeper...

paulidale · 2019-06-04T06:09:24Z

_ctx ?

Might be an improvement over the "Microsoft disease" of adding exes to API functions. But I think the problem lies deeper...

I'm inclined to agree. The question is what alternative is there?

A per thread context might be workable but represents a significant departure from where things were headed.

mspncp · 2019-06-04T06:39:42Z

I'm inclined to agree. The question is what alternative is there?

A per thread context might be workable ...

My thoughts go in this direction. However, it will still be a library context not a thread context. Only the pointer to the current library context will be implemented as thread local variable and there needs to be a simple and consistent way to activate a new context at library boundaries. (These activations need to be stacked.) I'll try to figure out more details, but I first need to understand the current state of the implementation better.

... but represents a significant departure from where things were headed.

Well, it's not too late yet to change things:

No matter how far down the wrong road you've gone, turn back.
                                             Turkish proverb
[You'll know whether the road is wrong if you carefully listen
 to your conscience.]

(taken from @t8m's email footer)

mattcaswell · 2019-06-04T09:46:13Z

For most APIs (such as EVP) its not too much of a problem. There are plenty of existing FOO_CTX objects that get passed to the various function calls, and therefore you only need a relatively small number of additional functions to associate those FOO_CTX objects with an OPENSSL_CTX - and then suddenly all those functions have the required access to OPENSSL_CTX.

The RAND API is a little different because there is no RAND_CTX concept - so there are more functions that need to change to add support for it. Personally I don't see this as too much of a problem.

I'm not particularly fond of the "_ex" naming scheme, so I'm certainly open to alternative suggestions.

mattcaswell · 2019-06-04T09:56:17Z

Rebased again due to yet more conflicts with master

mspncp · 2019-06-04T10:22:18Z

For most APIs (such as EVP) its not too much of a problem.

I see. That makes things a lot simpler and will (hopefully) make thread-local implicit library contexts dispensable.

The RAND API is a little different because there is no RAND_CTX concept - so there are more functions that need to change to add support for it.

If this is your only problem, then the solution is easy: Just use RAND_DRBG_bytes() instead of RAND_bytes(): contrary to the legacy RAND API, the RAND_DRBG API does have a context, it is the RAND_DRBG struct itself (a.k.a rand_drbg_st):

openssl/include/openssl/ossl_typ.h

Line 124 in ff8029c

typedef struct rand_drbg_st RAND_DRBG;

The RAND_DRBG structure is opaque and it is easy to add a backward pointer from every RAND_DRBG struct to the OPENSSL_CTX struct which owns it.

openssl/crypto/rand/rand_lcl.h

Lines 193 to 197 in 8094a69

    
           struct rand_drbg_st { 
        
               CRYPTO_RWLOCK *lock; 
        
               RAND_DRBG *parent; 
        
               int secure; /* 1: allocated on the secure heap, 0: otherwise */ 
        
               int type; /* the nid of the underlying algorithm */

Note: In the times of the Fips Object Module 2.0, the RAND_DRBG struct used to be called a "drbg context" ...

openssl/fips/rand/fips_rand.h

Line 76 in d11e6a4

typedef struct drbg_ctx_st DRBG_CTX;

... but this terminology was dropped by me in favor of the newer "OpenSSL class" pattern (CLASS_NAME_method_name(CLASS_NAME *obj, ...)).

mspncp · 2019-06-04T10:27:25Z

Providers should stick to using the RAND_DRBG api directly and the legacy RAND api should remain for application use only.

Then the only thing which remains to do is to redirect the RAND methods to the FIPS provider DRBGs when FIPS mode is enabled.

mattcaswell · 2019-06-04T10:32:41Z

Providers should stick to using the RAND_DRBG api directly and the legacy RAND api should remain for application use only.

Hmm. Right. That does make sense. I'll look into that.

beldmit · 2019-06-04T10:44:39Z

It's worth mentioning that in Russia we must (and even MUST) to provide our own certified RNGs. Currently we are able to do it via engines, and we need this possibility via providers.

mattcaswell · 2019-06-04T10:48:37Z

It's worth mentioning that in Russia we must (and even MUST) to provide our own certified RNGs. Currently we are able to do it via engines, and we need this possibility via providers.

That is the plan (although you don't get it through this PR).

mspncp · 2019-06-04T11:11:56Z

Side note: The OPENSSL_CTX typedef-struct fits nicely into the new OpenSSL "class" pattern. Having this in mind, the awkward underscore_ex functions

RAND_DRBG *RAND_DRBG_get0_master_ex(OPENSSL_CTX *ctx);
RAND_DRBG *RAND_DRBG_get0_public_ex(OPENSSL_CTX *ctx);
RAND_DRBG *RAND_DRBG_get0_private_ex(OPENSSL_CTX *ctx);

naturally turn into accessors for the OPENSSL_CTX class:

RAND_DRBG *OPENSSL_CTX_get_master_drbg(OPENSSL_CTX *ctx);
RAND_DRBG *OPENSSL_CTX_get_public_drbg(OPENSSL_CTX *ctx);
RAND_DRBG *OPENSSL_CTX_get_private_drbg(OPENSSL_CTX *ctx);

(The zero in get0 is not necessary IMO, the get could even be dropped entirely. Feel free to choose your favorite variation of the suggested names.)

mspncp · 2019-06-04T11:12:45Z

Side note 2: Most of the modern style OpenSSL "classes" follow the OSSL_CLASSNAME naming pattern, i.e. they use OSSL_ as the common prefix.

For that reason (and for brevity), I would suggest to rename OPENSSL_CTX to OSSL_CTX (and the lowercase versions accordingly). If you agree with me, I volunteer to carry out the straightforward renaming task.

richsalz · 2019-06-04T13:01:16Z

I'm not particularly fond of the "_ex" naming scheme, so I'm certainly open to alternative suggestions.

It is what is used throughout OpenSSL. Please don't add a new convention; the API is hard enough to understand already.

levitte · 2019-06-04T13:48:18Z

I'm not particularly fond of the "_ex" naming scheme, so I'm certainly open to alternative suggestions.

It is what is used throughout OpenSSL. Please don't add a new convention; the API is hard enough to understand already.

side note: I've always wondered what will happen when we need to extend a function that already has _ex at the end...

levitte · 2019-06-04T13:49:23Z

Side note 2: Most of the modern style OpenSSL "classes" follow the OSSL_CLASSNAME naming pattern, i.e. they use OSSL_ as the common prefix.

OSSL_ and OPENSSL_ are both acceptable.

mspncp · 2019-06-04T14:12:50Z

OSSL_ and OPENSSL_ are both acceptable.

@levitte what is the correct interpretation of your statement?

You have no preference and don't mind if I change the name from OPENSSL_CTX to OSSL_CTX.
You would prefer if I'd leave the names as they are, since both are acceptable.

mspncp · 2019-06-04T14:18:46Z

I'm not particularly fond of the "_ex" naming scheme, so I'm certainly open to alternative suggestions.

It is what is used throughout OpenSSL. Please don't add a new convention; the API is hard enough to understand already.

I don't intend to create new conventions, but I'd rather avoid adding meaningless _ex suffixes which IMHO should be used only as last resort if you run out of ideas for naming your functions properly ;-). See #9039 (comment) for an example how it could be done better.

levitte · 2019-06-04T14:22:20Z

I'm not strictly opposed to OSSL_CTX over OPENSSL_CTX, but I don't see that as a necessity either. In other words, such a name change is very low on my priority list.

mattcaswell · 2019-06-04T14:50:01Z

(The zero in get0 is not necessary IMO, the get could even be dropped entirely. Feel free to choose your favorite variation of the suggested names.)

Why do you think it is unnecessary? It seems to fit the intended meaning here?

In preparation for moving the RAND code into the FIPS module we make drbg_lib.c OPENSSL_CTX aware.

This is in preparation for moving this code inside the FIPS module.

It was previously rand_lib but it makes more sense in drbg_lib.c since all the functions that use this lock are only ever called from drbg_lib.c We add some FIPS_MODE defines in preparation for later moving this code into the FIPS module.

Various functions have been added that take an OPENSSL_CTX parameter as a result of moving the RAND code into the FIPS module. We document all of those functions.

mspncp · 2019-06-04T15:24:06Z

Why do you think it is unnecessary? It seems to fit the intended meaning here?

Unnecessary only in the sense that DRBG's are never up-rev'ed, so there is no need to distinguish between those two cases. But if you prefer the get0 because you think it is more explicit, that's ok for me.

mattcaswell · 2019-06-04T15:27:14Z

To me it implies more than just whether an object has been up-ref'd or not. It implies ownership. With a get0 you should not free the returned object explicitly. With a get1 you should. That applies whether or not the object in question is ref counted or not.

So, I prefer to keep it because it makes the intended memory semantics clearer.

levitte · 2019-06-04T15:42:27Z

Quite frankly, the get0/get1 and set0/set1 naming scheme isn't very intuitive. We who have been part of the project for a while know what they mean (and still get it wrong at times), but for someone new to the mind-set? It's kinda horrible, really...

richsalz · 2019-06-04T15:45:47Z

I agree it's not great but don't change it, fix it by adding more docs and cross-referencing all manpages with a [sg]et[01] API to point to the new man7 page.

mspncp · 2019-06-04T16:03:59Z

Once you get accustomed to it, it is a help. And it is an improvement, because before this convention was introduced, there were far too many other functions introduced with unclear memory semantics. So I agree with @mattcaswell and @richsalz that we should better stick to it.

mattcaswell · 2019-06-04T16:35:55Z

I've removed the parts of this PR that added a lot of the _ex functions and restricted the changes to just the RAND_DRBG API. This will mean that any code brought into the FIPS module will have to be updated to use that API instead...but that's ok.

I also renamed the functions as suggested to OPENSSL_CTX_get0_public_drbg() etc.

slontis · 2019-06-05T03:06:50Z

doc/man3/RAND_DRBG_new.pod


 #include <openssl/rand_drbg.h>

+ RAND_DRBG *RAND_DRBG_new_ex(OPENSSL_CTX *ctx,


RAND_DRBG_ctx_new() maybe?

Hmm. I'm dubious about adding that as a new convention. What if I later have to extend FOO_CTX_new() to add an OPENSSL_CTX param? Calling it FOO_CTX_ctx_new sounds odd.

slontis · 2019-06-05T03:22:34Z

crypto/rand/drbg_lib.c

+        return NULL;
+
+    if (!CRYPTO_THREAD_init_local(&dgbl->private_drbg, NULL))
+        goto err1;


I would prefer seeing the CRYPTO_THREAD_cleanup_local() implementations checked for NULL, rather than doing this. It looks like basic code :)

I don't believe this is possible. &dgbl->private_drbg here will never be NULL even if it has not been initialised by CRYPTO_THREAD_init_local. The contents of dgbl->private_drbg might have a zero value because it has been "zalloc'd". But we do not know whether 0 is an invalid value or not for the local platform threading scheme - so it would not be correct for CRYPTO_THREAD_cleanup_local to check for 0.

slontis · 2019-06-05T03:33:25Z

crypto/rand/drbg_lib.c

+    return dgbl;
+
+ err3:
+    CRYPTO_THREAD_cleanup_local(&dgbl->public_drbg);


drbg_ossl_ctx_free() instead?

Not possible because I need to have the different entry points for the error handing (err1/err2/err3).

crypto/rand/drbg_lib.c

slontis · 2019-06-05T04:58:49Z

test/drbgtest.c

 static size_t crngt_case, crngt_idx;

-static int crngt_entropy_cb(unsigned char *buf, unsigned char *md,
+static int crngt_entropy_cb(OPENSSL_CTX *ctx, unsigned char *buf,


Hmm - is this a backwards compatibility breaking change?

No. This callback is only exposed because drbgtest.c includes rand_lcl.h

slontis · 2019-06-05T06:26:49Z

test/drbgtest.c

    crngt_case = n % crngt_num_cases;
    crngt_idx = 0;
    crngt_get_entropy = &crngt_entropy_cb;
-    if (!TEST_true(rand_crngt_init()))


What is the effect of removing these crngt calls?

The CRNGT code should be automatically initialised when it attempts to get its data out of the OPENSSL_CTX for the first time. Similarly it is automatically cleaned up when the OPENSSL_CTX gets freed. Therefore there is no need to explicitly initialise and clean-up here.

mattcaswell · 2019-06-05T07:50:11Z

Fix up commit pushed addressing the style nits. All other comments responded to above.

levitte

Travis failure not relevant

mattcaswell · 2019-06-07T11:58:01Z

Pushed. Thanks!

In preparation for moving the RAND code into the FIPS module we make drbg_lib.c OPENSSL_CTX aware. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #9039)

This is in preparation for moving this code inside the FIPS module. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #9039)

It was previously rand_lib but it makes more sense in drbg_lib.c since all the functions that use this lock are only ever called from drbg_lib.c We add some FIPS_MODE defines in preparation for later moving this code into the FIPS module. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #9039)

Various functions have been added that take an OPENSSL_CTX parameter as a result of moving the RAND code into the FIPS module. We document all of those functions. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #9039)

mattcaswell mentioned this pull request May 29, 2019

Make the RAND code available from within the FIPS module #9035

Closed

levitte reviewed Jun 3, 2019

View reviewed changes

crypto/init.c Outdated

Copy link

Member

levitte Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be implemented using a provider wide set_params...

mattcaswell force-pushed the prepare-rand branch from b557710 to f2953d6 Compare June 3, 2019 12:18

mattcaswell force-pushed the prepare-rand branch from f2953d6 to 578ac61 Compare June 4, 2019 09:56

mattcaswell added 2 commits June 4, 2019 16:12

Convert drbg_lib to use OPENSSL_CTX for its global data

0f5bf9d

In preparation for moving the RAND code into the FIPS module we make drbg_lib.c OPENSSL_CTX aware.

Make the rand_crng code OPENSSL_CTX aware

f821063

This is in preparation for moving this code inside the FIPS module.

mattcaswell added 2 commits June 4, 2019 16:12

Move the rand_nonce_lock code into drbg_lib.c

2cb0ffc

It was previously rand_lib but it makes more sense in drbg_lib.c since all the functions that use this lock are only ever called from drbg_lib.c We add some FIPS_MODE defines in preparation for later moving this code into the FIPS module.

Add documentation for new functions taking an OPENSSL_CTX parameter

01b8b27

Various functions have been added that take an OPENSSL_CTX parameter as a result of moving the RAND code into the FIPS module. We document all of those functions.

mattcaswell force-pushed the prepare-rand branch from 578ac61 to 01b8b27 Compare June 4, 2019 16:32

slontis reviewed Jun 5, 2019

View reviewed changes

crypto/rand/drbg_lib.c Outdated Show resolved Hide resolved

slontis reviewed Jun 5, 2019

View reviewed changes

crypto/rand/drbg_lib.c Outdated Show resolved Hide resolved

slontis reviewed Jun 5, 2019

View reviewed changes

fixup! Move the rand_nonce_lock code into drbg_lib.c

8ab05e8

mattcaswell mentioned this pull request Jun 7, 2019

test/evp_test.c: try fetching algorithms #9106

Closed

levitte approved these changes Jun 7, 2019

View reviewed changes

mattcaswell closed this Jun 7, 2019

levitte pushed a commit that referenced this pull request Jun 7, 2019

Make the rand_crng code OPENSSL_CTX aware

4e297b7

This is in preparation for moving this code inside the FIPS module. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #9039)

mattcaswell mentioned this pull request Jun 7, 2019

Make the threading code provider aware #9040

Closed


		#include <openssl/rand_drbg.h>

		RAND_DRBG RAND_DRBG_new_ex(OPENSSL_CTX ctx,

Uh oh!

Conversation

mattcaswell commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Jun 3, 2019

Uh oh!

levitte commented Jun 3, 2019

Uh oh!

mattcaswell commented Jun 3, 2019

Uh oh!

mspncp commented Jun 3, 2019

Uh oh!

paulidale commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

paulidale commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

mattcaswell commented Jun 4, 2019

Uh oh!

mattcaswell commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

mattcaswell commented Jun 4, 2019

Uh oh!

beldmit commented Jun 4, 2019

Uh oh!

mattcaswell commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

richsalz commented Jun 4, 2019

Uh oh!

levitte commented Jun 4, 2019

Uh oh!

levitte commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

levitte commented Jun 4, 2019

Uh oh!

mattcaswell commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

mattcaswell commented Jun 4, 2019

Uh oh!

levitte commented Jun 4, 2019

Uh oh!

richsalz commented Jun 4, 2019

Uh oh!

mspncp commented Jun 4, 2019

Uh oh!

mattcaswell commented Jun 4, 2019

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!

Uh oh!

mattcaswell commented May 29, 2019 •

edited

Loading