Allocate DRBG additional data pool from non-secure memory #9423

bernd-edlinger · 2019-07-20T09:29:14Z

@paulidale @mspncp how about this?

crypto/rand/drbg_lib.c

crypto/rand/rand_lib.c

paulidale · 2019-07-20T11:47:13Z

Definitely a step in the right direction.

mspncp

First impression is good. Only one remark and one question.

Your commit message definitely deserves more text ;-)
Is there a particular reason why you go from one extreme (secure memory) to the other (no cleaning at all)? I'd prefer if you would keep the cleaning.

crypto/rand/drbg_lib.c

crypto/rand/rand_lib.c

crypto/rand/drbg_lib.c

bernd-edlinger · 2019-07-20T13:37:17Z

No real reason, but it is odd that the non-initialized memory causes a msan failure.
The idea was that the memory shall be initialized by the entropy that is added, not
by a memset, but the memset can easily hide a bug from valgrind.
In the moment it looks like a compiler-bug.

crypto/rand/drbg_lib.c

The additional data allocates 12K per DRBG instance in the secure memory, which is not necessary. Also nonces are not considered secret. [extended tests]

bernd-edlinger · 2019-07-20T15:03:16Z

Reverted the test code, and zeroized the additional data pool as before.
It is a bit odd that msan raises bogus errors and forces unnecessary initializing.

crypto/include/internal/rand_int.h

mspncp · 2019-07-21T19:04:30Z

Reverted the test code, and zeroized the additional data pool as before.

From the security perspective, only the erasing of the buffer before freeing it is important. If you prefer malloc to zalloc for performance reasons, and if you manage to fix the asan complaint, I won't mind.

bernd-edlinger · 2019-07-22T05:59:22Z

Unfortunately I was not able to fix the memory sanitizer at this time,
valgrind did not agree and did never print any warnings at all.

crypto/rand/rand_lib.c

paulidale · 2019-07-22T09:54:57Z

Did anything come of the suggestion to use the entropy argument to determine secure storage or not?

bernd-edlinger · 2019-07-22T10:05:42Z

Yes, it still makes sense in case DRBG is created with RAND_DRBG_new:
#9423 (comment)

bernd-edlinger · 2019-07-22T10:06:37Z

However we agreed, that the public DRBG will not use non-secure memory.

paulidale · 2019-07-22T10:10:51Z

Thanks for the link. I had to rush through too much today and missed the outcome :(

I agree that the DRBG should be in secure memory in addition to the entropy.

paulidale

I'm assuming that the Travis failures aren't relevant (they didn't appear to be).

mspncp · 2019-07-22T10:17:13Z

Did anything come of the suggestion to use the entropy argument to determine secure storage or not?

I agree with @bernd-edlinger. I wasn't aware anymore that we have now two flavors of DRBG: a secure one and an insecure one.

openssl/crypto/rand/drbg_lib.c

Lines 473 to 482 in efa4add

    
           RAND_DRBG *RAND_DRBG_new(int type, unsigned int flags, RAND_DRBG *parent) 
        
           { 
        
               return RAND_DRBG_new_ex(NULL, type, flags, parent); 
        
           } 
        
           RAND_DRBG *RAND_DRBG_secure_new_ex(OPENSSL_CTX *ctx, int type, 
        
                                              unsigned int flags, RAND_DRBG *parent) 
        
           { 
        
               return rand_drbg_new(ctx, 1, type, flags, parent); 
        
           }

(In fact, it was me who added the functions in 4f9dabb, but it was only during code cleanup, so I did not really introduce the two flavors.)

So to be consistent, the secure flag should be inherited from the DRBG to its pool, agreed?

bernd-edlinger · 2019-07-22T10:19:00Z

@mspncp are your concerns resolved ?
If yes, I can go ahead and merge this in the afternoon, so @paulidale can continue with his PR.

bernd-edlinger · 2019-07-22T10:20:30Z

crypto/rand/rand_lib.c

@@ -149,7 +149,7 @@ size_t rand_drbg_get_entropy(RAND_DRBG *drbg,
        pool = drbg->seed_pool;
        pool->entropy_requested = entropy;
    } else {
-        pool = rand_pool_new(entropy, min_len, max_len);
+        pool = rand_pool_new(entropy, drbg->secure, min_len, max_len);


So to be consistent, the secure flag should be inherited from the DRBG to its pool, agreed?

Yes. That happens here.

mspncp

Looks good to me now. In particular the locations which I marked explicitly.

mspncp · 2019-07-22T10:21:12Z

crypto/rand/rand_crng_test.c

@@ -110,7 +110,7 @@ size_t rand_crngt_get_entropy(RAND_DRBG *drbg,
    if (crngt_glob == NULL)
        return 0;

-    if ((pool = rand_pool_new(entropy, min_len, max_len)) == NULL)
+    if ((pool = rand_pool_new(entropy, 1, min_len, max_len)) == NULL)


mspncp · 2019-07-22T10:21:30Z

crypto/rand/rand_lib.c

@@ -149,7 +149,7 @@ size_t rand_drbg_get_entropy(RAND_DRBG *drbg,
        pool = drbg->seed_pool;
        pool->entropy_requested = entropy;
    } else {
-        pool = rand_pool_new(entropy, min_len, max_len);
+        pool = rand_pool_new(entropy, drbg->secure, min_len, max_len);


mspncp · 2019-07-22T10:22:06Z

crypto/rand/rand_lib.c

@@ -331,7 +335,7 @@ int RAND_poll(void)
        RAND_POOL *pool = NULL;

        /* fill random pool and seed the current legacy RNG */
-        pool = rand_pool_new(RAND_DRBG_STRENGTH,
+        pool = rand_pool_new(RAND_DRBG_STRENGTH, 1,


bernd-edlinger · 2019-07-22T11:38:50Z

Merged as 1372560. Thanks!

The additional data allocates 12K per DRBG instance in the secure memory, which is not necessary. Also nonces are not considered secret. [extended tests] Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #9423)

paulidale · 2019-07-22T11:45:01Z

I agree with @bernd-edlinger. I wasn't aware anymore that we have now two flavors of DRBG: a secure one and an insecure one.

I hadn't noticed either. I'm having difficulty thinking of a plausible use case for a non-secure DRBG. Random numbers are fundamental to security and need to be produced in the safest manner possible. Being able to predict future output would be very bad.

I feel that the internal state should always be in secure memory as should the entropy input. I can live with the other inputs not being in secure memory (although it could be argued that the additional input is a kind of weak entropy).

Anyway, this might be for another PR.

bernd-edlinger · 2019-07-22T11:50:12Z

Was there something with the deterministic ecdsa signature, where a drbg would
be fed with predictable input and produces the deteministic R for the ECDSA?

paulidale · 2019-07-22T12:13:47Z

Yes, it reuses one of the DRBG constructions.
The reference escapes me at present.

bernd-edlinger added the branch: master Merge to master branch label Jul 20, 2019

bernd-edlinger force-pushed the alloc_adin_pool_from_non_secure_mem branch 2 times, most recently from 9ce1491 to 50769dc Compare July 20, 2019 11:31

paulidale reviewed Jul 20, 2019

View reviewed changes

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

crypto/rand/drbg_lib.c Show resolved Hide resolved

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

bernd-edlinger force-pushed the alloc_adin_pool_from_non_secure_mem branch from 50769dc to 1bf8253 Compare July 20, 2019 11:53

mspncp requested changes Jul 20, 2019

View reviewed changes

bernd-edlinger commented Jul 20, 2019

View reviewed changes

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

bernd-edlinger commented Jul 20, 2019

View reviewed changes

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

bernd-edlinger force-pushed the alloc_adin_pool_from_non_secure_mem branch from 1bf8253 to dbf2013 Compare July 20, 2019 13:42

bernd-edlinger commented Jul 20, 2019

View reviewed changes

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

bernd-edlinger force-pushed the alloc_adin_pool_from_non_secure_mem branch 2 times, most recently from 301c7a5 to cc00073 Compare July 20, 2019 14:24

Allocate DRBG additional data pool from non-secure memory

efa4add

The additional data allocates 12K per DRBG instance in the secure memory, which is not necessary. Also nonces are not considered secret. [extended tests]

bernd-edlinger force-pushed the alloc_adin_pool_from_non_secure_mem branch from cc00073 to efa4add Compare July 20, 2019 14:55

mspncp reviewed Jul 21, 2019

View reviewed changes

crypto/include/internal/rand_int.h Show resolved Hide resolved

paulidale mentioned this pull request Jul 22, 2019

Make rand_pool buffers more dynamic in their sizing. #9428

Closed

2 tasks

bernd-edlinger commented Jul 22, 2019

View reviewed changes

crypto/rand/rand_lib.c Show resolved Hide resolved

bernd-edlinger commented Jul 22, 2019

View reviewed changes

crypto/rand/rand_lib.c Show resolved Hide resolved

paulidale approved these changes Jul 22, 2019

View reviewed changes

bernd-edlinger commented Jul 22, 2019

View reviewed changes

mspncp approved these changes Jul 22, 2019

View reviewed changes

bernd-edlinger closed this Jul 22, 2019

paulidale mentioned this pull request Jul 23, 2019

Backport dynamic rand_pool buffering to 1.1.1 #9448

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allocate DRBG additional data pool from non-secure memory #9423

Allocate DRBG additional data pool from non-secure memory #9423

bernd-edlinger commented Jul 20, 2019

paulidale commented Jul 20, 2019

mspncp left a comment •

edited

Loading

bernd-edlinger commented Jul 20, 2019

bernd-edlinger commented Jul 20, 2019

mspncp commented Jul 21, 2019

bernd-edlinger commented Jul 22, 2019

paulidale commented Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

paulidale commented Jul 22, 2019

paulidale left a comment

mspncp commented Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

bernd-edlinger Jul 22, 2019

mspncp left a comment

mspncp Jul 22, 2019

mspncp Jul 22, 2019

mspncp Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

paulidale commented Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

paulidale commented Jul 22, 2019

Allocate DRBG additional data pool from non-secure memory #9423

Allocate DRBG additional data pool from non-secure memory #9423

Conversation

bernd-edlinger commented Jul 20, 2019

paulidale commented Jul 20, 2019

mspncp left a comment • edited Loading

Choose a reason for hiding this comment

bernd-edlinger commented Jul 20, 2019

bernd-edlinger commented Jul 20, 2019

mspncp commented Jul 21, 2019

bernd-edlinger commented Jul 22, 2019

paulidale commented Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

paulidale commented Jul 22, 2019

paulidale left a comment

Choose a reason for hiding this comment

mspncp commented Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

bernd-edlinger Jul 22, 2019

Choose a reason for hiding this comment

mspncp left a comment

Choose a reason for hiding this comment

mspncp Jul 22, 2019

Choose a reason for hiding this comment

mspncp Jul 22, 2019

Choose a reason for hiding this comment

mspncp Jul 22, 2019

Choose a reason for hiding this comment

bernd-edlinger commented Jul 22, 2019

paulidale commented Jul 22, 2019

bernd-edlinger commented Jul 22, 2019

paulidale commented Jul 22, 2019

mspncp left a comment •

edited

Loading