Copy Libsodium's randombytes API and use for prng seeding #27

zanebeckwith · 2017-09-19T22:17:55Z

Copy (with appropriate attribution) Libsodium's randombytes_buf functionality, for getting crypto-strength random bytes for the purposes of seeding AMCL's pseudo random number generator.

This copy takes everything in-tact from Libsodium, except the nativeclient or salsa randombytes options. The default randombytes backend, the sysrandom, is retained. As is the ability for a user to define a custom backend (e.g. for a hardware device).

closes #9

Also, this PR introduces a wrapper around AMCL's prng, so the user won't be expected to seed the rng themselves (instead, we make it very clear that they need to use a "constructor" for our prng wrapper, which uses the Libsodium randombytes functionality to seed the prng).

closes #11

A CMake option for Libsodium's USE_BLOCKING_RANDOM is also included, but not documented in the README.

codecov-io · 2017-09-19T22:22:58Z

Codecov Report

Merging #27 into master will decrease coverage by 0.31%.
The diff coverage is 90.68%.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   76.96%   76.65%   -0.32%     
==========================================
  Files          21       22       +1     
  Lines         990     1041      +51     
  Branches       59       69      +10     
==========================================
+ Hits          762      798      +36     
- Misses        208      215       +7     
- Partials       20       28       +8

Impacted Files	Coverage Δ
src/credential_BN254.c	`45.78% <100%> (ø)`	⬆️
test/benchmarks.c	`100% <100%> (ø)`	⬆️
test/sign-and-verify-tests.c	`100% <100%> (ø)`	⬆️
src/signature_BN254.c	`45.33% <100%> (ø)`	⬆️
test/schnorr-tests.c	`100% <100%> (ø)`	⬆️
test/credential-tests.c	`100% <100%> (ø)`	⬆️
test/issuer_keypair-tests.c	`100% <100%> (ø)`	⬆️
src/issuer_keypair_BN254.c	`20% <100%> (ø)`	⬆️
src/member_keypair_BN254.c	`15.38% <100%> (ø)`	⬆️
src/internal/schnorr.c	`93.54% <100%> (ø)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 476903c...144afb9. Read the comment docs.

drbild · 2017-09-20T17:11:02Z

I'm conflicted on the libsodium import. It pulls in a bunch of stuff we don't use (various functions and macros in util.h) and all the support for various environments that we don't really care about yet (Windows, emscripten, etc.) adds a lot of noise. It is also a lot of duplicate code that will require fairly frequent updating from the upstream libsodium source. This isn't just one small function/algorithm we've copied.

Some other options to consider (I'm not writing off the current approach - just want to discuss these):

Stick with the AMCL approach: require our users to provide an implementation of ecdaa_prng. Suggest in the docs how to use libsodium for that implementation.
Provide a libsodium-based ecdaa_prng by default, but require users to link against libsodium themselves. Then we won't have to maintain copies of all that code. I'm not sure how much of a header we would need to copy (hopefully none - just redeclare one or functions with no environment-specific ifdefs).
Copy the libsodium code, but remove all the portions that aren't relevant to us, to reduce the maintenance burden. E.g., just keep the stuff for Linux, not Windows, emscripten, etc.

drbild · 2017-09-20T16:52:44Z

include/ecdaa/prng.h

+ *
+ * No dynamic memory allocation is performed.
+ */
+void ecdaa_prng_construct(struct ecdaa_prng *prng_in);


Call this ecdaa_prng_init. I believe the _init prefix (and _free suffix if any cleanup is needed) are de facto standards for C (de)constructor naming.

drbild · 2017-09-20T16:53:54Z

src/amcl-extensions/big_256_56.h

@@ -24,9 +24,10 @@
 extern "C" {
 #endif

+struct ecdaa_prng;


Not really part of this review, but I'm just noticing that many of the headers redeclare the structs as opaque, rather than importing the defining header? What's the reason for this?

That started with a circular dependency I had in an early version of this project, and I kept doing it just as a nod to compilation time reduction. But, this is a tiny project, so that's really not an issue. I can change it easily if you think it confuses things.

Haha, my reason for importing headers was going to be that "it helps ensure circular dependencies don't creep in".

I think eventually standard imports will be better, but that's an easy change to make down the line.

zanebeckwith · 2017-09-20T18:15:08Z

Yea, I definitely agree. I think this stems from a misunderstanding I had of a comment you made last week sometime. My preference is actually something akin to option 2, but I misunderstood and thought you recommended pulling in Libsodium's code.

Here's my proposal:

Keep the ecdaa_prng essentially in-tact, but make the randombytes_buf function that it calls to seed the rng a static function pointer (like Libsodium's implementation struct pointer). By default, this points to (the system-installed) Libsodium function.

But, the user can call something like ecdaa_prng_override_seed_function to set that pointer to their own implementation. Linking to Libsodium can then be controlled by a CMake option like USE_DEFAULT_RNG_SEED_FUNCTION, which by default would be ON.

drbild · 2017-09-20T18:55:15Z

Gotcha. I made that comment thinking we'd only have to copy a function or two, not realizing the extent of the libsodium implementation.

Your proposal sounds good. Maybe call the option USE_LIBSODIUM_RNG_SEED_FUNCTION ("LIBSODIUM" instead of "DEFAULT", to make clear that this requires libsodium.

zanebeckwith · 2017-09-20T19:02:13Z

Yea, I thought it would be simpler, too. When I realized it was more intricate, I started following your option 3 above, then got a little frightened I was pulling a bull-in-a-chinashop without understanding how it all works. Hence the wholesale copying.

I just pushed a few commits implementing this new version. It's not building on travis at the moment, because I have to install libsodium there. But, let me know if this looks more like what you're thinking.

drbild · 2017-09-20T19:06:15Z

Yes, looks good to me.

The global for the seed function bothers me a bit, but I think is better (easier to use and more likely to be used correctly) than requiring ecdaa_prng_init to take a seed function as an argument.

zanebeckwith · 2017-09-20T19:14:23Z

I was just thinking about which of those I prefer. I hate global stuff like this, and taking it as an argument would also allow for per-signature setting, if that were ever required.

We could have two functions for ecdaa_prng_init: ecdaa_prng_init (which takes no arguments), and ecdaa_prng_init_nondefault_seed(ecdaa_prng_seed_function_type seed_func)

That way, most people would just use the default one. But, if you want, you have to go out of your way to use a non-default one.

drbild · 2017-09-20T19:17:21Z

Good point. I still suggest keeping your current version. It's got the simplest API (1. Set the seed function at program start. 2. init each prng when created) The *_init_nondefault_seed variant can be added down the line if needed (it's nice to know if can be added though).

zanebeckwith · 2017-09-20T21:43:35Z

👍
As discussed above, the most-recent version uses a file-scoped function pointer with a setter function, with the pointer set to randombytes_buf by default.

drbild · 2017-09-20T22:10:58Z

src/prng.c

+            fputs("Unable to initialize libsodium random seed source. Aborting", stderr);
+            abort();
+        }
+#if defined(__linux__) && defined(RNDGETENTCNT)


I don't think this section belongs in a low-level crypto library - for the same reasons that it's not included in libsodium by default.

How the program should respond to low entropy is application-specific. For many services, the right answer might be to keep running, but refuse connections until sufficient entropy is available.

May be better to let the ecdaa_init() have the same semantics as the sodium_init() return code. Then copy the that code snippet from the libsodium readme to the ecdaa readme.

drbild · 2017-09-20T22:13:14Z

src/prng.c

+void ecdaa_prng_init(struct ecdaa_prng *prng_in)
+{
+#ifdef USE_LIBSODIUM_RNG_SEED_FUNCTION
+    sodium_init_if_needed();


Can this be put in ecdaa_init() instead of here? Then we don't need to track the libsodium init state. If the user isn't using libsodium, ecdaa_init() will just be a no-op for now.

zanebeckwith · 2017-09-21T16:41:48Z

After discussion offline, this PR has been updated to use the following design:

No more file-scope function pointer
No more abort called in prng initialization function
Two variants of prng init function:
- Default one, calls Libsodium's randombytes_buf
- 'Custom' one, which takes a pointer to a function that produces crypto random bytes for seeding
All functions taking a pointer to a prng first check its initialized flag, and abort if not initialized

drbild · 2017-09-21T16:35:57Z

include/ecdaa/prng.h

+ * 0 on success
+ * return value of `custom_func`, if non-zero
+ */
+int ecdaa_prng_init_custom(struct ecdaa_prng *prng_in, ecdaa_prng_custom_seed_function_type custom_func);


Maybe just take the seed directly, rather than a seed function? My gut feeling is that will be the easiest API for users.

I want to make sure we actually get AMCL_SEED_SIZE bytes of seed. I can imagine someone thinking "well, bytes is an appropriate seed size, so that's what I'll pass in" without realizing we're requesting a specific size, then we try to read more bytes than they gave us.

We can document the requirement by taking the seed as char seed[AMCL_SEED_SIZE], but we have no way of determining if that's what is actually passed in.

Maybe I'm being anal, and this is a caveat emptor situation. And, yea, I can see that having to pass in a function with a specific signature can be a pain.

No, after thinking about it more, I think you're right. I think I'm trying to hard to tell the user what to do, and making the API hard to use.

I agree now.

Can we take both the pointer and size as arguments, and return an error if the size is less than AMCL_SEED_SIZE?

drbild · 2017-09-21T16:38:51Z

src/prng.c

+            ret = -1;
+            break;
+        }
+#if defined(__linux__) && defined(RNDGETENTCNT)


Move this block to a separate function.

This check should also go before the call to sodium_init. Otherwise the entropy might have been low when sodium_init was run, but high when the check is being made.

drbild · 2017-09-21T16:40:25Z

src/prng.c

+#include <linux/random.h>
+#endif  // defined(__linux__)
+
+int ecdaa_prng_init(struct ecdaa_prng *prng_in)


This implementation should call ecdaa_prng_init_custom internally, to avoid code duplication. Generate the seed and call ecdaa_prng_init_custom with it.

I think this hinges on the signature of ecdaa_prng_init_custom, as per above.

Even if we keep the function arg for ecdaa_prng_init_custom, ecdaa_prng_init should call through to ecdaa_prng_init_custom. After all, we're just providing a default libsodium-based variant. If using ecdaa_prng_init_custom for that is too difficult for us, then what hope does an end user have? It would imply that the ecdaa_prng_init_custom API needs more work.

Agreed. Implementation has been updated now.

drbild · 2017-09-21T17:46:22Z

include/ecdaa/prng.h

+#define AMCL_SEED_SIZE 128  // As recommended in AMCL's "rand.c"
+
+enum ecdaa_prng_initialized {
+    ECDAA_PRNG_INITIALIZED_YES,


Explicitly define the numeric values and make sure the ECDAA_PRNG_INITIALIZED_NO is 0. As it is right now, I think that on systems the zero new memory allocations, a new ecdaa_prng.initialized will default to 0, which is ECDAA_PRNG_INITIALIZED_YES.

Good catch.

drbild · 2017-09-21T18:18:29Z

src/amcl-extensions/big_256_56.c

 {
+    if (ECDAA_PRNG_INITIALIZED_YES != prng->initialized)


Can all of these checks be moved into one function - an accessor for ecdaa_prng->impl? Then we don't have to remember to check the initialized state in so many places; just the once in the accessor.

And actually, in this particular case, because it is a amcl-extension it should take the csprng directly, not a ecdaa_prng. Ideally acml-extensions shouldn't depend on ecdaa code.

That's a good idea.
👍

drbild · 2017-09-21T18:20:14Z

src/amcl-extensions/big_256_56.c

    BIG_256_56 curve_order;
    BIG_256_56_rcopy(curve_order, CURVE_Order_BN254);

-    BIG_256_56_randomnum(*big_out, curve_order, rng);
+    BIG_256_56_randomnum(*big_out, curve_order, &prng->impl);


Continuing the prior comment, replace this with

BIG_256_56_randomnum(*big_out, curve_order, ecdaa_prng_get_csprng(prng));

where ecdaa_prng_get_csprng(...) returns csprng* or calls abort().

I made all the functions take ecdaa_prng rather than letting the amcl-extensions take the bare one, to make it easier to see where the bare one is getting used.

If I move to the accessor discussed above, that would mean a grep of the source code would immediately show us where the bare csprng is being used directly.

Just grep on everything except acml-extensions (or throw a | grep -v acml-extensions on the end of your shell command to filter out those matches).

It depends on what is meant by acml-extensions. In my mind, those are things that would ideally be provided by the relevant modules in acml (and might sometime be contributed back). So to the main ecdaa code, functions from acml or acml-extensions should be indistinguishable.

Hmm, I hadn't been thinking of amcl-extensions that way. I'm sure I understand the advantage of thinking of it that way. Is that what others would expect from such a sub-directory?

There's only one function in amcl-extensions that uses the csprng, and thus really only one place in our code where we actually use an rng: big_256_56_random_mod_order. The implementation of that function may change (cf. #1 ), so maybe that should get lifted out of amcl-extensions?

I guess I'm a little confused as the to the point of big256_56_random_mod_order() in the first place. Why not just call BIG_256_56_randomnum(*big_out, CURVE_Order_BN254, rng) directly? I don't see the purpose of the BIG_256_56_rcopy call.

As always, after thinking about it, I think I see your point. It's nice to not have the cognitive friction of having two conventions when using AMCL-related code in our project. Besides, if we end up wanting to submit any of our extensions to the AMCL project for inclusion, this makes it easier.

Hmm, weird, I didn't see your question about rcopy when I left my previous comment.

That's copying from AMCL's examples. Concretely, it has to do with const vs non-const. The r seems to indicate ROM in their naming scheme. It does look like simply casting away const might work, but the fact that they explicitly do the copy in their examples makes me wonder if they're trying to be careful there (cf. my worries in #4 ).

So, at the moment, it's simply a wrapper to do the copy from "ROM" then call _randomnum, but I don't feel confident removing it.

drbild · 2017-09-21T18:28:13Z

src/prng.c

+    do {
+        ret = check_entropy();
+        if (0 != ret)
+            break;


I'd personally use gotos rather than do/while/break to avoid the extra level of indentation, but that's a personal preference.

ret = check_entropy(); if (0 != ret) goto out; ... prng_in->initialized = ECDAA_PRNG_INITIALIZED_YES; out: return ret;

The do-while construct appears in other crypto software I've seen, that's why I adopted it. I kind of like the extra indentation, because it clearly shows what's wrapped (like a try-catch block).

drbild · 2017-09-21T20:41:16Z

include/ecdaa/prng.h

+ *
+ * Does not do any heap deallocation.
+ */
+void ecdaa_prng_clean(struct ecdaa_prng *prng);


Rename to ecdaa_prng_free()to match C convention?

After addressing this, I say merge it. The public API seems to be good.

The nature of the inner usage can evolve (what the acml-extensions conventions should be will become more clear as support for other curves are added. Whether get_csrng should be prng_get_csrng or ecdaa_prng_get_csrng will become more clear when naming conventions for internal code are decided. For now there are bigger fish to fry.)

I avoided free here to keep from insinuating memory is being freed. But, maybe making people think that would be good (more people have been burned by memory leaks than by side-channel attacks, at least that they know of).

Yea, bigger fish indeed.

I spoke with Michael Scott from AMCL a fair amount today, getting advice on using their csprng. Definitely feel more confident in their implementation and our usage of it now. So, I'm go for this PR, other than your input on free vs clean.

zanebeckwith · 2017-09-22T00:10:54Z

Awesome, thanks so much for all the help and sorry for all the back and forth!

zanebeckwith requested a review from drbild September 19, 2017 22:17

drbild reviewed Sep 20, 2017

View reviewed changes

zanebeckwith force-pushed the libsodium-randombytes branch 13 times, most recently from 6cd72bb to 3fae763 Compare September 20, 2017 21:41

drbild approved these changes Sep 20, 2017

View reviewed changes

zanebeckwith force-pushed the libsodium-randombytes branch 2 times, most recently from 33793d3 to eb4a344 Compare September 20, 2017 22:11

drbild reviewed Sep 20, 2017

View reviewed changes

zanebeckwith force-pushed the libsodium-randombytes branch from eb4a344 to 974586b Compare September 20, 2017 22:31

zanebeckwith force-pushed the libsodium-randombytes branch from 848bbc2 to 3f036c8 Compare September 21, 2017 16:34

drbild reviewed Sep 21, 2017

View reviewed changes

zanebeckwith force-pushed the libsodium-randombytes branch 4 times, most recently from 6705bcc to 391124b Compare September 21, 2017 17:46

drbild reviewed Sep 21, 2017

View reviewed changes

Zane Beckwith added 3 commits September 21, 2017 18:56

Add a wrapper for AMCL's csprng, and always seed it.

a8a7dc1

Add libsodium package to travis.

208752d

Update README to reflect prng struct wrapper.

144afb9

zanebeckwith force-pushed the libsodium-randombytes branch from cbfe1ff to 144afb9 Compare September 21, 2017 23:56

zanebeckwith merged commit b953cd8 into xaptum:master Sep 22, 2017

zanebeckwith deleted the libsodium-randombytes branch September 22, 2017 00:13

Copy Libsodium's randombytes API and use for prng seeding #27

Copy Libsodium's randombytes API and use for prng seeding #27

Conversation

zanebeckwith commented Sep 19, 2017

codecov-io commented Sep 19, 2017 • edited Loading

Codecov Report

drbild commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanebeckwith commented Sep 20, 2017

drbild commented Sep 20, 2017

zanebeckwith commented Sep 20, 2017

drbild commented Sep 20, 2017

zanebeckwith commented Sep 20, 2017

drbild commented Sep 20, 2017

zanebeckwith commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanebeckwith commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drbild Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drbild Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanebeckwith commented Sep 22, 2017

codecov-io commented Sep 19, 2017 •

edited

Loading

drbild Sep 21, 2017 •

edited

Loading

drbild Sep 21, 2017 •

edited

Loading