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

Use libsodium's CSPRNG instead of OpenSSL's #1706

Merged
merged 6 commits into from Feb 5, 2017

Conversation

@paragonie-scott
Copy link
Contributor

commented Oct 29, 2016

Closes #1632.

@daira

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2016

Concept ACK; need to check for other calls into OpenSSL and whether the change of argument type is ok.

@daira daira added this to the 1.0.1 stabilization milestone Oct 29, 2016

@daira daira self-assigned this Oct 29, 2016

@daira daira added the review needed label Oct 29, 2016

@afk11

This comment has been minimized.

Copy link

commented Oct 29, 2016

Concept ACK

@paragonie-scott

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2016

It looks like the two ways this is being invoked are:

  • sizeof(something) (more common)
  • integer constant

getrandbytes

Since sizeof returns a size_t and the libsodium function expects a size_t, this seemed like it would be more consistent.

@ebfull

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

Concept ACK.

@TakahT

This comment has been minimized.

Copy link

commented Nov 3, 2016

Concept ACK

@str4d
Copy link
Contributor

left a comment

utACK, but this PR should also remove OpenSSL's PRNG seeding:

  • RandAddSeed() and RandAddSeedPerfmon() (and all places that call them)
  • qt/winshutdownmonitor.cpp (The if block containing RAND_event())
  • util.cpp (the calls to RAND_screen() and RAND_cleanup())

Some or all of the above might be replaceable with randombytes_stir(), but for the default generator this should not be necessary.

@@ -20,7 +20,7 @@ void RandAddSeedPerfmon();
/**
* Functions to gather random data via the OpenSSL PRNG

This comment has been minimized.

Copy link
@str4d

str4d Nov 17, 2016

Contributor

via the libsodium PRNG

@str4d

str4d approved these changes Jan 4, 2017

@str4d

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2017

Addressed my own comments, so two more ACKs required.

@str4d str4d requested review from daira and ebfull Jan 4, 2017

@str4d str4d assigned str4d and unassigned str4d Jan 4, 2017

@daira

daira approved these changes Jan 13, 2017

Copy link
Contributor

left a comment

utACK. Does not change coverage.

LogPrintf("%s: OpenSSL RAND_bytes() failed with error: %s\n", __func__, ERR_error_string(ERR_get_error(), NULL));
assert(false);
}
randombytes_buf(buf, (size_t) num);

This comment has been minimized.

Copy link
@daira

daira Jan 13, 2017

Contributor

The cast to size_t isn't necessary. Does not block.

@@ -12,15 +12,9 @@
#include <stdint.h>

/**
* Seed OpenSSL PRNG with additional entropy data
* Functions to gather random data via the libsodium PRNG

This comment has been minimized.

Copy link
@daira

daira Jan 13, 2017

Contributor

RNG, not PRNG (since it's nondeterministically seeded). CSPRNG would also be fine.

@bitcartel bitcartel self-requested a review Jan 16, 2017

@ebfull ebfull self-assigned this Jan 18, 2017

@bitcartel
Copy link
Contributor

left a comment

(review below)

@bitcartel
Copy link
Contributor

left a comment

NACK - doesn't build. I rebased on master locally. Using gcc 6.2.0.

libzcash.a(libzcash_a-Note.o): In function `zero_after_free_allocator<char>::deallocate(char*, unsigned long)':
/support/allocators/zeroafterfree.h:40: undefined reference to `memory_cleanse(void*, unsigned long)'
Makefile:3153: recipe for target 'zcash/CreateJoinSplit' failed

That function calls into OpenSSL. See support/cleanse.cpp

#include "cleanse.h"

#include <openssl/crypto.h>

void memory_cleanse(void *ptr, size_t len)
{
    OPENSSL_cleanse(ptr, len);
}
@ebfull

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2017

Build issues here, let's delay this to 1.0.6.

@ebfull ebfull modified the milestones: 1.0.6, 1.0.5 Jan 20, 2017

@daira

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2017

Drat, so close. You want sodium_memzero to replace OPENSSL_cleanse: https://download.libsodium.org/doc/helpers/memory_management.html

@daira

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

Incidentally, support/allocators seems to be duplicating functionality available in libsodium (same doc page as above), so it might be worth comparing the two implementations to see if libsodium's is more secure.

@str4d

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

@daira

You want sodium_memzero to replace OPENSSL_cleanse: https://download.libsodium.org/doc/helpers/memory_management.html

I intentionally didn't make that change in order to keep the scope of this PR restricted to the PRNG. But I guess that code implicitly relies on a dependency inclusion that I'm removing in this PR.

The correct fix for this would be to figure out the missing inclusion and add it. But since we're planning on removing OpenSSL anyway, I'll just make this change here.

Incidentally, support/allocators seems to be duplicating functionality available in libsodium (same doc page as above), so it might be worth comparing the two implementations to see if libsodium's is more secure.

Agreed; I've opened #2033.

paragonie-security and others added some commits Oct 29, 2016

Update random.h
While I'm making an argument for better consistency, I might as well be self-consistent.
Remove OpenSSL PRNG reseeding
Per https://download.libsodium.org/doc/generating_random_data/ reseeding the
default libsodium PRNG is not required.
@str4d

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

Wait, I was reading @bitcartel's error all wrong. This has nothing to do with the removal of OpenSSL (and changing to libsodium's call didn't help); the linker can't find memory_cleanse(void*, unsigned long) itself.

@str4d

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

Rebased on 1.0.5 so that this PR contained the problem, and then fixed it.

By removing the memory_cleanse calls from src/random.cpp, the function never needed to be resolved inside libbitcoin_util.a. This uncovered the underlying bug, which was that the LDADD arguments for CreateJoinSplit were in the incorrect order.

The gcc linker parses its arguments in-order (left-to-right), and unused functions are ignored. By having libbitcoin_util.a before libzcash.a, and having no prior call to memory_cleanse, the unresolved calls to memory_cleanse in libzcash.a were never resolved.

@daira

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2017

utACK for the linker fix. But we do want to make that change to use sodium_memzero anyway, right? (It's fine if that is in a different PR.)

@str4d

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2017

@daira I'd definitely prefer a different PR. If nowhere else, we'd have to do it in the final PR that removes OpenSSL entirely.

@str4d str4d requested a review from bitcartel Jan 25, 2017

@str4d

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2017

AFAICT this PR just needs @bitcartel to check that the issue he raised has been resolved.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

ACK

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2017

📌 Commit 4752335 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2017

⌛️ Testing commit 4752335 with merge 499e34f...

zkbot added a commit that referenced this pull request Feb 5, 2017

Auto merge of #1706 - paragonie:master, r=bitcartel
Use libsodium's CSPRNG instead of OpenSSL's

Closes #1632.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit 4752335 into zcash:master Feb 5, 2017

1 check passed

homu Test successful
Details
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.