add GetRandBytes() as wrapper for RAND

Diapolo commented at 12:40 pm on June 24, 2014: none

add a small wrapper in util around RAND_bytes() and replace with GetRandBytes() in the code to log errors from calling RAND_bytes()
remove OpenSSL header rand.h where no longer needed
move rand functions from util to new random.h/.cpp

laanwj commented at 12:44 pm on June 24, 2014: member

I like this idea! It makes dependency on OpenSSL[for randomness] less widespread, and focused on a single place where it could easier be swapped out.

theuni commented at 2:27 pm on June 24, 2014: member

Agreed. I’d like it even more if it were moved out of util and into a new file, so that util would drop that dependency.

I should think that a simple RAII class for seeding and teardown would be pretty easy to whip up?

laanwj commented at 2:38 pm on June 24, 2014: member

What do you mean with a RAII class for seeding and teardown? Isn’t that global state, not local state?

theuni commented at 5:54 pm on June 24, 2014: member

@laanwj Yes openssl’s rand stuff is global, but others may not be. And it doesn’t hurt to scope, so that we know it’s always seeded and valid. But don’t worry about it here, we can discuss that later/elsewhere.

My request to have it moved to another file still stands, though.

laanwj commented at 10:27 am on June 25, 2014: member

@theuni So to be concrete, to introduce a random.cpp + random.h? Agreed on that. All the Rand* functions could move there.

sipa commented at 10:52 am on June 25, 2014: member

How about putting that in crypto/?

laanwj commented at 11:02 am on June 25, 2014: member

I thought about that, but didn’t propose it, that would make crypto dependent on OpenSSL :p

theuni commented at 4:45 pm on June 25, 2014: member

@laanwj Yes, that’s what I had in mind.

-1 to moving to crypto. crypto stands alone with no dependencies, this doesn’t seem like a good enough reason to change that.

Diapolo commented at 11:40 am on June 26, 2014: none

Agreed, I’ll rework this and move RandXYZ() from util to a new random.

Diapolo commented at 12:46 pm on June 26, 2014: none

@laanwj @theuni I moved the rand functions from util to new random.h/.cpp, can you check?

in src/util.cpp: in 62935b4236 outdated

70@@ -70,6 +71,7 @@
71 #include <boost/program_options/detail/config_file.hpp>
72 #include <boost/program_options/parsers.hpp>
73 #include <openssl/crypto.h>
74+#include <openssl/err.h>

Diapolo commented at 12:51 pm on June 26, 2014:

This will be removed in the next rebase!

in src/random.cpp: in 62935b4236 outdated

49+}
50+
51+bool GetRandBytes(unsigned char *buf, int num)
52+{
53+    if (RAND_bytes(buf, num) == 0) {
54+        LogPrint("rand", "%s : OpenSSL RAND_bytes() failed with error: %s\n", __func__, ERR_error_string(ERR_get_error(), NULL));

Diapolo commented at 1:14 pm on June 26, 2014:

I’m going to remove the "rand" in the next rebase as this seems important to log!

laanwj commented at 1:16 pm on June 26, 2014:

Agreed!

jgarzik commented at 1:53 pm on June 26, 2014: contributor

Bonus points: most of the uses involve vector. It would be nice to have a wrapper or two to provide type-safe interfaces, permitting

0    vector<unsigned char> v;
1    GetRandBytes(v, 32);
2versus
3    vector<unsigned char> v;
4    GetRandBytes(&v[0], 32);

Less margin for error.

Smarter types permit better safety checks. If you pass in a vector, you may check .size() prior to storing a range of data. Better checks – particularly in a security sensitive area where buffer overflows may occur – are always welcome.

laanwj commented at 2:08 pm on June 26, 2014: member

I agree that is useful to do in the future, But I think we should keep the scope of this pull down to the code movement and a trivial wrapper.

Diapolo commented at 7:15 am on June 27, 2014: none

@laanwj Rebased after the recent change to RandAddSeedPerfmon(), can you test?

Diapolo commented at 8:58 am on June 27, 2014: none

Added required changes to canonical_tests.cpp, which I forgot…

Diapolo commented at 9:11 am on June 27, 2014: none

Fixed crypto_tests.cpp.

in src/random.cpp: in 1115121081 outdated

0@@ -0,0 +1,121 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 The Bitcoin developers
3+// Distributed under the MIT/X11 software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#include "random.h"
7+
8+#include "util.h" // for LogPrint()

theuni commented at 4:20 pm on June 27, 2014:

This re-adds the dependency that we were breaking.

laanwj commented at 4:55 pm on June 27, 2014:

What dependency? How do you mean? I don’t think we can avoid an util.cpp-random.cpp cross-dependency unless we split up util.cpp further into a logging part and the part-that-uses-random…

theuni commented at 5:12 pm on June 27, 2014:

I don’t see any reason for stand-alone helper functions to be using program state and internal functions. For example:

0bool GetRandBytes(unsigned char *buf, int num)

should become

0bool GetRandBytes(unsigned char *buf, int num, std::string& error)

Then the caller decides how to handle it, in this case using LogPrintf() if the result is false;

Ignore my complaint for now. I’ll add it to the list of stuff I’d like to get to.

laanwj commented at 8:01 pm on June 27, 2014:

I don’t like returning an error/warning string from a low-level utility function. That just moves the burden to every caller site. IMO the function should be allowed to log if necessary.

Diapolo commented at 9:11 pm on June 27, 2014:

I also think it complicates things, if we just return an error an let the caller decide what to do. The dependency we broke is that we don’t need the OpenSSL rand.h in util.h (+ many others) and can replace OpenSSL rand.h in random.h by something better in the future. I also think logging is THAT basic, that every file should be able to do that. I have been thinking about a better place for the OpenSSL init, so that util.h doesn’t need ANY of the OpenSSL headers (perhaps init.cpp/.h).

Anyway, it would be nice if this could be tested, reviewed so I can continue with other things :).

in src/random.h: in 1115121081 outdated

15+
16+#ifndef WIN32
17+#include <sys/time.h>
18+#endif
19+
20+inline int64_t GetPerformanceCounter()

laanwj commented at 6:17 am on June 28, 2014:

BTW; this function can be moved to the implementation file and made static. It’s only used in random seeding. I think this means that the header doesn’t need to include compat.h and time.h anymore.

Diapolo commented at 6:04 am on June 30, 2014:

Guess you are right, will update later…

in src/util.cpp: in 1115121081 outdated

5@@ -6,6 +6,7 @@
6 #include "util.h"
7 
8 #include "chainparamsbase.h"
9+#include "random.h"

laanwj commented at 6:15 am on June 30, 2014:

Util.cpp no longer needs random.h after this, right?

Diapolo commented at 6:17 am on June 30, 2014:

The problem is the OpenSSL init code, which uses RandAddSeed().

laanwj commented at 7:00 am on June 30, 2014:

Ah yes, we need to move the OpenSSL init code at some point, you already mentioned that sorry.

Diapolo commented at 6:29 am on June 30, 2014: none

@laanwj See third commit, GetPerformanceCounter() was moved.

in src/wallet.cpp: in 9a14a122b5 outdated

361@@ -363,13 +362,15 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
362     RandAddSeedPerfmon();
363 
364     vMasterKey.resize(WALLET_CRYPTO_KEY_SIZE);
365-    RAND_bytes(&vMasterKey[0], WALLET_CRYPTO_KEY_SIZE);
366+    if (!GetRandBytes(&vMasterKey[0], WALLET_CRYPTO_KEY_SIZE))
367+        return false;

laanwj commented at 7:01 am on June 30, 2014:

+1 on checking error return values here

in src/net.h: in 9a14a122b5 outdated

12@@ -13,6 +13,7 @@
13 #include "mruset.h"
14 #include "netbase.h"
15 #include "protocol.h"
16+#include "random.h"

laanwj commented at 7:06 am on June 30, 2014:

Bleh that we need this here at all (I know why, there’s a GetRand call in EndMessage(), at some point we should move the non-trivial functions from net.h to net.cpp, but not in this pull)

in src/random.cpp: in 9a14a122b5 outdated

82+#endif
83+}
84+
85+bool GetRandBytes(unsigned char *buf, int num)
86+{
87+    if (RAND_bytes(buf, num) == 0) {

laanwj commented at 7:09 am on June 30, 2014:

The function can also return -1 if they are not supported by the current RAND method (see https://www.openssl.org/docs/crypto/RAND_bytes.html). So I’d prefer != 1 instead to explicitly check for succes instead of not failure.

Diapolo commented at 12:41 pm on June 30, 2014:

Agreed, will update. Edit: Updated!

Diapolo commented at 12:45 pm on June 30, 2014: none

@laanwj I rebased, so that we have only 2 commits left. Last and only change was the check for -1 from RAND_bytes(). Also needed to add changes to skiplist_test.cpp.

in src/random.cpp: in 884d6e7a84 outdated

35+void RandAddSeed()
36+{
37+    // Seed with CPU performance counter
38+    int64_t nCounter = GetPerformanceCounter();
39+    RAND_add(&nCounter, sizeof(nCounter), 1.5);
40+    memset(&nCounter, 0, sizeof(nCounter));

Diapolo commented at 11:22 am on July 1, 2014:

We could use OPENSSL_cleanse() here and remove the cstring include I guess. Not sure if a secure erase is needed here, but we also do that when adding the performance data as entropy source.

laanwj commented at 11:43 am on July 1, 2014:

That would be consistent, but no need to do it in this pull, let’s leave code movements and functional changes separate.

Diapolo commented at 10:21 am on July 2, 2014: none

@laanwj Perhaps you can also help me find files, which could now also work without util.h. @sipa @gmaxwell @gavinandresen @jgarzik Can you also help testing and reviewing.

Diapolo commented at 6:13 am on July 7, 2014: none

Any more testing done yet?

laanwj commented at 8:27 am on July 7, 2014: member

This mostly needs code review. Testing is also great (I’m using these changes on my (walletless) nodes) but changes to randomness aren’t very well testable. I’d feel better if at least one more person could take a thorough look at this.

add GetRandBytes() as wrapper for RAND_bytes()

- add a small wrapper in util around RAND_bytes() and replace with
  GetRandBytes() in the code to log errors from calling RAND_bytes()
- remove OpenSSL header rand.h where no longer needed

001a53d742

move rand functions from util to new random.h/.cpp 6354935c48

make RandAddSeed() use OPENSSL_cleanse()

- removes the cstring include and is also used in RandAddSeedPerfmon()

4eedf4ffee

BitcoinPullTester commented at 8:29 am on July 9, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4400_4eedf4ffeea6a3734f245f785a8d82d69634dccd/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj merged this on Jul 14, 2014

laanwj closed this on Jul 14, 2014

laanwj referenced this in commit 6513a9f703 on Jul 14, 2014

gmaxwell commented at 11:02 am on July 14, 2014: contributor

Sorry for the late comments but:

" if (RAND_bytes(buf, num) == 0) "

My manpage here says “RAND_bytes() returns 1 on success, 0 otherwise. The error code can be obtained by ERR_get_error(3). RAND_pseudo_bytes() returns 1 if the bytes generated are cryptographically strong, 0 otherwise. Both functions return -1 if they are not supported by the current RAND method.”

Shouldn’t we best testing != 1 ? And perhaps we should be asserting on failure?

laanwj commented at 11:07 am on July 14, 2014: member

We already check against 1: RAND_bytes(buf, num) != 1 (I had the same comment)

Not sure about asserting on failure, returning failures to the caller is already a big leap from what we used to do: ignore failures.

gmaxwell commented at 11:35 am on July 14, 2014: contributor

Sorry, I was reading the actual commits in order out of git and missed it in the pull. I can’t think of any case we’d not like to fail, and a failure of the RNG can be a fatal total loss of security.

Diapolo deleted the branch on Jul 15, 2014

MarcoFalke locked this on Sep 8, 2021

add GetRandBytes() as wrapper for RAND_bytes() #4400