Create a DRBG for s2n #61
Conversation
This change refactors s2n's entropy handling to be able to use a
DRBG. The DRBG included is simply the stream output of AES256-CTR
and is not yet conformant with NIST SP 800-90A, which will come
in a subsequent commit.
Highlights:
s2n_drbg emulates the NIST DRBG API and generates bits and
automatically reseeds itself.
s2n_get_public_random_data / s2n_get_private_random_data
have been added, each backed by different DRBG instantiations.
s2n_get_random_data -> s2n_get_urandom_data
all of the random generators have been factored to produce
blobs
The TLS code has been modified to use the public random data
where appropriate.
The OpenSSL over-ride now uses the private random data.
The public/private DRBGs are kept in thread-local storage:
so every thread gets its own automatically.
Defensiveness against fork()'ing has been added, including
support for minherit() where available.
Tests have been added and tweak, including full coverage
of the multi-thread and multi-process cases.
This change makes our DRBG conformant with NIST SP800-90A and now matches the NIST test vectors.
Conflicts: error/s2n_errno.c error/s2n_errno.h
| { | ||
| AES_KEY ctx; | ||
|
|
||
| AES_set_encrypt_key(key, 128, &ctx); |
There was a problem hiding this comment.
do we need to call aes_free(&ctx) when we are done with it?
There was a problem hiding this comment.
So there's no aes_free() because it's a flat struct that isn't malloc()'d, but this is a good call out. I've restructured things to move the AES_KEY into the drbg struct. I've added an s2n_drbg_wipe call to zero that key and all other DRBG context, and I now wipe the DRBGs from s2n_cleanup().
|
first pass i was mainly looking at the code layout, looks good. i'll go through again with the NIST docs at my side - oh and i trust you constructed your test vectors correctly! :) |
This change also creates a define for S2N_DRBG_BLOCK_SIZE for the underlying CTR block size.
This change moves the AES_KEY context to the drbg struct, rather than on a function stack, and adds an s2n_drbg_wipe() call to explicitly wipe all of the DRBG state. The public/private DRBGs are now also wiped when s2n_cleanup is called.
This commit makes the DRBG useable with BoringSSL and fixes a few
Ubuntu GCC warnings/errors:
* DRBG code now uses the EVP interface, which is the only
working interface that BoringSSL provides
* Fix several zero initialization and cast warnings detected
by Ubuntu GCC
Conflicts: tests/unit/s2n_handshake_test.c
|
Just need to check your nist vectors :) i'll email you the results of the valgrind, but looks like we are leaking the cipher ctx, in the test at least. a call to s2n_drbg_wipe might be in order at the end of the test, here is an example: ==32339== 3,432 bytes in 13 blocks are definitely lost in loss record 9 of 9 |
| GUARD(s2n_drbg_update(drbg, &blob)); | ||
|
|
||
| drbg->bytes_used = 0; | ||
| drbg->generation += 1; |
There was a problem hiding this comment.
do we want to ensure we don't overflow this?
although unlikely, it's still possible. what is the impact?
There was a problem hiding this comment.
The generation variable isn't exposed by any API and it's only used in one place: the unit test. The reason it's there is so that a unit test could cover the case where we cross the 2^35 boundary and check that we reseed the DRBG. I think it's ok to let it overflow. It will overflow only after a DRBG has created 2^97 bits of data.
|
Ok, I like that better - tested your test vectors and they are all correct :) merging! |
|
Thanks! I appreciate the review and especially for digging through the NIST docs and test vectors. I know it makes the review at least 10 times more work than reading the code alone! |
This change also creates a define for S2N_DRBG_BLOCK_SIZE for the underlying CTR block size.
1. Add necessary cbmc-proofs.txt file to contracts-based proof directories.
2. Update PROOF_UID for some contracts-based proofs.
3. Remove occurrences where text was appended to variables, such as this one:
FUNCTION_NAME = __CPROVER_file_local_s2n_hash_c_s2n_evp_hash_copy
PROOF_UID = $(FUNCTION_NAME)_with_contracts
HARNESS_ENTRY = $(PROOF_UID)_harness
HARNESS_FILE = $(HARNESS_ENTRY).c
The above approach doesn't yield the expected variables of:
PROOF_UID = __CPROVER_file_local_s2n_hash_c_s2n_evp_hash_copy_with_contracts
I changed all "$(VARIABLE_NAME)" occurrences with the string itself.
)" This reverts commit 4001add.
1. Add necessary cbmc-proofs.txt file to contracts-based proof directories.
2. Update PROOF_UID for some contracts-based proofs.
3. Remove occurrences where text was appended to variables, such as this one:
FUNCTION_NAME = __CPROVER_file_local_s2n_hash_c_s2n_evp_hash_copy
PROOF_UID = $(FUNCTION_NAME)_with_contracts
HARNESS_ENTRY = $(PROOF_UID)_harness
HARNESS_FILE = $(HARNESS_ENTRY).c
The above approach doesn't yield the expected variables of:
PROOF_UID = __CPROVER_file_local_s2n_hash_c_s2n_evp_hash_copy_with_contracts
I changed all "$(VARIABLE_NAME)" occurrences with the string itself.
)" This reverts commit 4001add.
1. Add necessary cbmc-proofs.txt file to contracts-based proof directories.
2. Update PROOF_UID for some contracts-based proofs.
3. Remove occurrences where text was appended to variables, such as this one:
FUNCTION_NAME = __CPROVER_file_local_s2n_hash_c_s2n_evp_hash_copy
PROOF_UID = $(FUNCTION_NAME)_with_contracts
HARNESS_ENTRY = $(PROOF_UID)_harness
HARNESS_FILE = $(HARNESS_ENTRY).c
The above approach doesn't yield the expected variables of:
PROOF_UID = __CPROVER_file_local_s2n_hash_c_s2n_evp_hash_copy_with_contracts
I changed all "$(VARIABLE_NAME)" occurrences with the string itself.
)" This reverts commit 4001add.
1. Add necessary cbmc-proofs.txt file to contracts-based proof directories.
2. Update PROOF_UID for some contracts-based proofs.
3. Remove occurrences where text was appended to variables, such as this one:
FUNCTION_NAME = __CPROVER_file_local_s2n_hash_c_s2n_evp_hash_copy
PROOF_UID = $(FUNCTION_NAME)_with_contracts
HARNESS_ENTRY = $(PROOF_UID)_harness
HARNESS_FILE = $(HARNESS_ENTRY).c
The above approach doesn't yield the expected variables of:
PROOF_UID = __CPROVER_file_local_s2n_hash_c_s2n_evp_hash_copy_with_contracts
I changed all "$(VARIABLE_NAME)" occurrences with the string itself.
)" This reverts commit 4001add.
This pull request adds a NIST SP800-90A conformant CTR_DRBG, which removes /dev/urandom as our bottleneck. Some high-lights: