Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DRBG random method #3789

Closed
wants to merge 1 commit into from

Conversation

@richsalz
Copy link
Contributor

commented Jun 27, 2017

Ported from the last FIPS release.
Compiles and passes the self-tests.
Tests not ported.

This is a step along the path. The other RAND-related PR's also need to get approved. And then I will work on switching this for the default PRNG.

@richsalz richsalz added the master label Jun 27, 2017

@richsalz richsalz self-assigned this Jun 27, 2017

@mattcaswell

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Not reviewed, but this seems like a great idea.

dctx->app_data = app_data;
}

size_t RAND_DRBG_get_blocklength(DRBG_CTX *dctx)

This comment has been minimized.

Copy link
@FdaSilvaYY

FdaSilvaYY Jun 27, 2017

Contributor

const DRBG_CTX *dctx

This comment has been minimized.

Copy link
@richsalz

richsalz Jun 27, 2017

Author Contributor

fixed.

return dctx->blocklength;
}

int RAND_DRBG_get_strength(DRBG_CTX *dctx)

This comment has been minimized.

Copy link
@FdaSilvaYY

FdaSilvaYY Jun 27, 2017

Contributor

const DRBG_CTX *dctx

This comment has been minimized.

Copy link
@richsalz

richsalz Jun 27, 2017

Author Contributor

fixed.

#include "fips_utl.h"

static int dparse_md(char *str)
{

This comment has been minimized.

Copy link
@FdaSilvaYY

FdaSilvaYY Jun 27, 2017

Contributor

Old formatting style revival ;) ?

This comment has been minimized.

Copy link
@mspncp

mspncp Jun 27, 2017

Contributor

I agree, an automatic code reformat could be useful, but preferrable at the end of the review process, not now. A code reformat makes it more difficult to compare Richs changes to the original FIPS code.

This comment has been minimized.

Copy link
@richsalz

richsalz Jun 27, 2017

Author Contributor

That test code hasn't been looked at yet.

This comment has been minimized.

Copy link
@richsalz

richsalz Jun 27, 2017

Author Contributor

but @mspncp, it will be reformatted and changed before I try to compile it :) Just like the other code.

}
}

static size_t drbg_get_entropy(DRBG_CTX *dctx, unsigned char **pout,

This comment has been minimized.

Copy link
@mspncp

mspncp Jun 27, 2017

Contributor

The original fips_get_entropy() contained a bug which was reported by Lee D Gibbins ldgibbons@avaya.com on rt

[rt.openssl.org #4055] FIPS Object Module User Guide corrections needed for (*get_entropy)()

(now GitHub Issue #2443) In contrary to the title, it was not an documentation issue but a real implementation bug, as it was confirmed by Steve Marquess in

(The reply went only to [openssl-dev], not to [rt #4055] so it is missing in GitHub Issue #2443)

Since the bug could not be fixed, because the FIPS code was frozen, Steve Marquess just added a documentation of the bug to section 6.1.1 of the OpenSSL FIPS user guide on 2015-09-30:

2015-09-30 Section 6.1.1, expanded discussion of the entropy callback (thanks to Lee D Gibbins ldgibbons@avaya.com) 

Now since the code now is being defrosted, this is the opportunity to finally fix this bug.

return 1;
}

static int drbg_ctr_generate(DRBG_CTX *dctx,

This comment has been minimized.

Copy link
@mspncp

mspncp Jun 27, 2017

Contributor

Rich, There is also a bug in drbg_ctr_generate() which I reported privately to Dr. Henson in Sept 2016 and which went unanswered. The report is too long to be reproduced here, so I will send you a copy.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

@paulidale
Copy link
Contributor

left a comment

I don't think we even want to define dual EC.

@paulidale
Copy link
Contributor

left a comment

We don't want to even define dual EC

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

We definitely don't! Did I leave it in somewhere?

long entlen, noncelen, perslen, adinlen;
int df = 0;

enum dtype { DRBG_NONE, DRBG_CTR, DRBG_HASH, DRBG_HMAC, DRBG_DUAL_EC }

This comment has been minimized.

Copy link
@paulidale

paulidale Jun 28, 2017

Contributor

Could we not define dual Ec.

This comment has been minimized.

Copy link
@paulidale

paulidale Jun 28, 2017

Contributor

I figured out how to comment on a line of code eventually :)

Worth a grep too.

else if (strstr(buf, "HMAC_DRBG"))
drbg_type = DRBG_HMAC;
else if (strstr(buf, "Dual_EC_DRBG"))
drbg_type = DRBG_DUAL_EC;

This comment has been minimized.

Copy link
@paulidale

paulidale Jun 28, 2017

Contributor

here too

default:
ret = -2;
break;
case NID_sha1:

This comment has been minimized.

Copy link
@paulidale

paulidale Jun 28, 2017

Contributor

Is it worth considering not including the SHA1 based DRBGs? This and hmacWithSHA1 below.

The known (and foreseeable) weakness in SHA1 is unlikely to impact its use as a DRBG but paranoia is bliss.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

return 1;
}

DRBG_CTX *RAND_DRBG_get_default(void)

This comment has been minimized.

Copy link
@mspncp

mspncp Jun 28, 2017

Contributor

Default DRBG should be initialized with sensible entropy callbacks

The original FIPS 2.0 DRBG is somewhat incomplete because it implements only the deterministic part and relies on the caller to provide entropy callbacks during FIPS DRBG initialisation. These callbacks were provided by the OpenSSL 1.0.2 crypto library during the FIPS DRBG initialization (see the end of rand_lib.c)

This code has been removed from master during the FIPS code cleanup in 700b4a4. As a consequence, the DRBG_CTX currently returned by RAND_DRBG_get_default() (hence also the RAND_METHOD returned by the RAND_drbg()) is unusable unless the user (i.e., the programmer) provides entropy callbacks.

I think it would be a good idea if do_ossl_drbg_init() would take care of providing sensible defaults. You could take the removed code as a starting point.

@richsalz richsalz force-pushed the richsalz:add-drbg-rand branch from 49c8c66 to 7d089dd Jun 28, 2017

Waiting for the second bugfix and the initiatialisation.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2017

@richsalz richsalz force-pushed the richsalz:add-drbg-rand branch 3 times, most recently from 8a42554 to bf8d426 Jun 30, 2017

@richsalz richsalz changed the title WIP: Add DRBG random method Add DRBG random method Jul 7, 2017

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2017

Ready for review now.
Note that the API will be changing. don't use yet.

@richsalz richsalz force-pushed the richsalz:add-drbg-rand branch from bf8d426 to 2f14f19 Jul 7, 2017

@@ -4342,6 +4342,21 @@ OSSL_STORE_INFO_set0_NAME_description 4284 1_1_1 EXIST::FUNCTION:
OSSL_STORE_INFO_get1_NAME_description 4285 1_1_1 EXIST::FUNCTION:
OSSL_STORE_do_all_loaders 4286 1_1_1 EXIST::FUNCTION:
OSSL_STORE_LOADER_get0_engine 4287 1_1_1 EXIST::FUNCTION:
OPENSSL_fork_prepare 4288 1_1_1 EXIST:UNIX:FUNCTION:
OPENSSL_fork_parent 4289 1_1_1 EXIST:UNIX:FUNCTION:
OPENSSL_fork_child 4290 1_1_1 EXIST:UNIX:FUNCTION:

This comment has been minimized.

Copy link
@levitte

levitte Jul 7, 2017

Member

Not sure why you're moving those... no biggie, since these symbols are not part of any release yet, but essentially bad form, gives me hickups...

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2017

@richsalz richsalz force-pushed the richsalz:add-drbg-rand branch from 2f14f19 to bbba482 Jul 7, 2017

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

@paulidale I squashed all previous commits and added one more. It moves function names to private space, removes some unused functions, etc. Please re-review.

@paulidale
Copy link
Contributor

left a comment

A couple of trivial things which probably don't all need addressing. I'll approve with or without.

@@ -0,0 +1,50 @@
/*
* Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.

This comment has been minimized.

Copy link
@paulidale

paulidale Jul 17, 2017

Contributor

2017 ?

@@ -107,7 +107,8 @@ DEFINE_STACK_OF(void)
# define CRYPTO_EX_INDEX_BIO 12
# define CRYPTO_EX_INDEX_APP 13
# define CRYPTO_EX_INDEX_UI_METHOD 14
# define CRYPTO_EX_INDEX__COUNT 15
# define CRYPTO_EX_INDEX_DRBG 15
# define CRYPTO_EX_INDEX__COUNT 16

This comment has been minimized.

Copy link
@paulidale

paulidale Jul 17, 2017

Contributor

What an evil macro name with double underscore.
It's a public API change to fix, so changing it mightn't be possible.

@@ -67,7 +67,8 @@ DEPRECATEDIN_1_1_0(int RAND_event(UINT, WPARAM, LPARAM))

int ERR_load_RAND_strings(void);

# ifdef __cplusplus
#ifdef __cplusplus

This comment has been minimized.

Copy link
@paulidale

paulidale Jul 17, 2017

Contributor

Shouldn't the space after the # stay?

This comment has been minimized.

Copy link
@paulidale

paulidale Jul 17, 2017

Contributor

A couple more right above too.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

@kroeckx

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

This doesn't seem to contain any documentation update?

@kroeckx

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

I would really like to see minimal documentation for each function, and this seems to add a lot of new functions without any documentation.

@kroeckx
Copy link
Member

left a comment

documentation

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

@kroeckx , think of it like the PACKET or WPACKET functions in the SSL library. These are purely internal functions that will (soon) be called by the existing RAND API. Users should not call them. Documentation, since all we have is public manpages, is not appropriate.

}

dctx->max_request = 1 << 16;
dctx->reseed_interval = 1 << 24;

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 18, 2017

Member

Is there a reason those values are different than in the SP800-90A?

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 18, 2017

Author Contributor

They are what is/was in the OpenSSL FIPS source code.

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 19, 2017

Member

I don't find this a good answer. At least in the case of max_request it's the difference between bits and bytes.

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 19, 2017

Author Contributor

I'm sorry, I dont have a better answer for you; I was just a code-monkey copying our previous FIPS code. I suggest you ask Steve and/or Andy directly.

This comment has been minimized.

Copy link
@mspncp

mspncp Jul 19, 2017

Contributor

I think, the value max_request = 2^{16} bytes is correct. On page 49 of NIST.SP.800-90Ar1.pdf) the bound for max_number_of_bits_per_request = min(B, 2^{19}) bits = 2^{19} bits = 2^{19-3} bytes.

As for the reseed_interval: IMHO the NIST paper describes an abstract mathematical model of an RNG and the OpenSSL DRGB is a concrete implementation of this model. As such its parameters need to remain within the bounds given on page 49. Citing from the bottom of that page:

Note that the claimed security strength for CTR_DRBG depends on limiting the total number of
generate requests and the bits provided per generate request according to the table above.

So in my opinion it is not a violation of the NIST requirements if the DRBG reseeds more often than required. I am using the FIPS DRBG myself and have set the reseed_interval to 1024 requests.

Note: Interestingly, the original FIPS_drbg_set_reseed_interval() in the FIPS Object Module did not contain any bounds check to enforce the NIST bounds. So anyone could call this function from unvalidated code (e.g. from the FIPS capable libcrypto library) to set the reseed interval to an illegal value. And its renamed descendant RAND_DRBG_set_reseed_interval() does not check any bounds either.

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 19, 2017

Member

I have no problem with making those numbers shorter than what is documented there, but if the numbers are different I have to wonder if there there is a reason why. And "we like it shorter" is a good enough reason for me.

But I think something like RAND_DRBG_set_reseed_interval() should enforce a limit, since we actually have something that says it shouldn't be more than that.

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 19, 2017

Author Contributor

that's a good idea, I'll make a new PR. should it be between 1 and 1<<24 ?

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 19, 2017

Member

The values from SP800-90A has 2^48 for the AES based ones and 2^32 for the 3 key TDEA. Maybe a parameter in the ctx?

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 19, 2017

Author Contributor

Please see #3970

# include <openssl/hmac.h>
# include <openssl/ec.h>
# include "include/internal/rand.h"

/* we require 256 bits of randomness */
# define RANDOMNESS_NEEDED (256 / 8)

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 18, 2017

Member

Where does this fixed value come from?

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 18, 2017

Author Contributor

It goes all the way back to openssl 0.9.8. But 128 or 256 is generally believed to be sufficient if the CSPRNG is good quality. (Ours wasn't, but this PR is a step on the way to making it so)

#include "internal/thread_once.h"

/*
* Mapping of SP800-90 DRBGs to OpenSSL RAND_METHOD

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 18, 2017

Member

That should probably be "NIST SP 800-90A"

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 18, 2017

Author Contributor

Yes, and I will make that change and assume the approval still holds.

}

/*
* Process a complete block using BCC algorithm of SPP 800-90 10.4.3

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 18, 2017

Member

SP, not SPP, and the A is also missing.

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 18, 2017

Member

And the section is 10.3.3 in revision 1.

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 18, 2017

Author Contributor

Wlil update, thanks.

@kroeckx

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

PACKET and WPACKET are documented, as comment in the header files.

@kroeckx

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

It also seems to export a lot of function for something that's internal only.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

I can add more comments to internal/rand.h in a later PR if you want.

Yes, there are several new internal functions; many are for the KAT tests, some are for the "deployment model" of the NIST document (instantiate, uninstanstate), and some will be used in the crypto and ssl library.

@richsalz richsalz force-pushed the richsalz:add-drbg-rand branch 2 times, most recently from 5321b4e to f845122 Jul 18, 2017

@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

Many comments added, per @kroeckx request.
Sorry to ask for yet another review. :(

Add DRBG random method
Ported from the last FIPS release, with DUAL_EC and SHA1 and the
self-tests removed.  Since only AES-CTR is supported, other code
simplifications were done.  Removed the "entropy blocklen" concept.

Moved internal functions to new include/internal/rand.h.

@richsalz richsalz force-pushed the richsalz:add-drbg-rand branch from 17480ff to 9ff657e Jul 19, 2017

levitte pushed a commit that referenced this pull request Jul 19, 2017
Add DRBG random method
Ported from the last FIPS release, with DUAL_EC and SHA1 and the
self-tests removed.  Since only AES-CTR is supported, other code
simplifications were done.  Removed the "entropy blocklen" concept.

Moved internal functions to new include/internal/rand.h.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #3789)
@richsalz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2017

Whew. And another step completed, thanks!

@richsalz richsalz closed this Jul 19, 2017

@richsalz richsalz deleted the richsalz:add-drbg-rand branch Jul 19, 2017

/* Should never happen */
n = 16;
}
for (i = 0; i < 16; i++)

This comment has been minimized.

Copy link
@kaduk

kaduk Jul 19, 2017

Contributor

Is this loop limit supposed to be 'n' instead of literal 16? (The previous assignments to 'n' seem to be dead code, otherwise.)

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 19, 2017

Author Contributor

Good question. @snhenson this code comes from the FIPS release. Is the 16 wrong, or the dead code before the loop?

This comment has been minimized.

Copy link
@kaduk

kaduk Jul 19, 2017

Contributor

The 16 is wrong; the conceptual CTR_DRBG_Update algorithm assumes the provided input is exactly seedlen bytes long, but our API permits implicit zero padding (since xor with 0s is the identity function). So we need to only xor as much as it is claimed that we are provided, since I see no API contract in the ctr_XOR/ctr_update usage that implies we will always be passed properly-sized objects.

*p++ = (inlen >> 8) & 0xff;
*p++ = inlen & 0xff;

/* NB keylen is at most 32 bytes */

This comment has been minimized.

Copy link
@kaduk

kaduk Jul 19, 2017

Contributor

bytes or bits?

This comment has been minimized.

Copy link
@kroeckx

kroeckx Jul 19, 2017

Member

I think we use the key in bytes, and that it's 16, 24 or 32 bytes. But I'm assuming there might be other problems with bits vs bytes problems, as already commented before.

This comment has been minimized.

Copy link
@kaduk

kaduk Jul 19, 2017

Contributor

Sure. I just ask because we just assigned to *p++ four times, i.e., consuming 32 bits of storage, having written a length to the buffer being used as AES input. So this comment would make a lot of sense as saying that we only needed those four octets written.

@mspncp

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2017

@kaduk you can find the bounds on page 49 of NIST.SP.800-90Ar1.pdf. There the key length is given in bits, here it is in bytes.

@kroeckx The code in this PR is more less the original code from the FIPS object module. And indeed the FIPS module contained a real bug mixing bits and bytes, in the implementation of fips_get_entropy() (This was reported by Lee D Gibbons on rt (See my first review of crypto/rand/drbg_lib.c in this PR for details). But this is the only bits vs. bytes bug I know of.

void (*cleanup_entropy)(DRBG_CTX *ctx, unsigned char *out, size_t olen),
size_t (*get_nonce)(DRBG_CTX *ctx, unsigned char **pout,
int entropy, size_t min_len, size_t max_len),
void (*cleanup_nonce)(DRBG_CTX *ctx, unsigned char *out, size_t olen)

This comment has been minimized.

Copy link
@kaduk

kaduk Jul 19, 2017

Contributor

If I understand correctly, our style seems to be leaning towards typedefs for function types when used as function arguments. But maybe I understand incorrectly...

This comment has been minimized.

Copy link
@richsalz

richsalz Jul 19, 2017

Author Contributor

yes we have been doing that. note that this PR is closed. feel free to open and issue or make your own PR

This comment has been minimized.

Copy link
@kaduk

kaduk Jul 19, 2017

Contributor

Okay, #3971

@mspncp mspncp referenced this pull request Aug 25, 2017
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.