gh-libtom/libtomcrypt
Watch
1
0
Fork
You've already forked libtomcrypt
 0
Code Issues 42 Pull requests 17 Releases 9 Wiki Activity

Improve Fortuna PRNG #363

Merged
sjaeckel merged 5 commits from improve/fortuna into develop 2018-04-09 21:23:20 -05:00
Conversation 7 Commits 5 Files changed 7 +189 -36
sjaeckel commented 2018-03-23 17:37:53 -05:00 (Migrated from github.com)
  • documentation is added or updated
  • tests are added or updated

This updates the Fortuna implementation to be more in-line with the original specification.

It adds the fortuna_add_random_event() function, an alternative to fortuna_add_entropy() and makes fortuna_update_seed() public.

Additionally it fixes a bug in the reseed rate-limit which should only happen after pool0_len >= 64 AND rate-limit expired which was an OR before, c.f. Chapter 9.5.5 of [1].

Or simplified as stated by @tyzoid in IRC, the new behavior is "...if we've got enough entropy in the buffer, and it's been long enough that we probably should reseed, then we will do so" whereas the old buggy behavior was "...pretty much constantly reseeding with an empty buffer".

* [x] documentation is added or updated * [x] tests are added or updated This updates the Fortuna implementation to be more in-line with the original specification. It adds the `fortuna_add_random_event()` function, an alternative to `fortuna_add_entropy()` and makes `fortuna_update_seed()` public. Additionally it fixes a bug in the reseed rate-limit which should only happen after `pool0_len >= 64` _AND_ `rate-limit expired` which was an _OR_ before, c.f. Chapter 9.5.5 of [[1]]. Or simplified as stated by @tyzoid in IRC, the new behavior is "...if we've got enough entropy in the buffer, and it's been long enough that we probably should reseed, then we will do so" whereas the old buggy behavior was "...pretty much constantly reseeding with an empty buffer". [1]: https://www.schneier.com/academic/paperfiles/fortuna.pdf
karel-m commented 2018-03-27 04:30:12 -05:00 (Migrated from github.com)

Not that I am against this PR, I only need more time for review, stay tuned.

Not that I am against this PR, I only need more time for review, stay tuned.
tyzoid (Migrated from github.com) reviewed 2018-04-04 12:36:52 -05:00
tyzoid (Migrated from github.com) left a comment

LGTM Otherwise.

LGTM Otherwise.
src/prngs/fortuna.c
@ -64,0 +80,4 @@
cur_time -= CONST64(116444736000000000); /* subtract epoch in microseconds */
cur_time /= 1000; /* nanoseconds -> microseconds */
#endif
return cur_time / 100;
tyzoid (Migrated from github.com) commented 2018-04-04 12:36:33 -05:00

cur_time here is uninitialized if neither _POSIX_C_SOURCE >= 200112L or _WIN32 is defined. I'd recommend either initializing it to 0 on line 68, or adding an additional #else before line 82, with an #error trigger to positively alert to a problem.

`cur_time` here is uninitialized if neither `_POSIX_C_SOURCE >= 200112L` or `_WIN32` is defined. I'd recommend either initializing it to `0` on line 68, or adding an additional `#else` before line 82, with an `#error` trigger to positively alert to a problem.
tyzoid (Migrated from github.com) reviewed 2018-04-04 13:25:53 -05:00
src/headers/tomcrypt_custom.h
@ -366,0 +378,4 @@
* LTC_FORTUNA_RESEED_RATELIMIT_TIMED
* is defined.
*/
#undef LTC_FORTUNA_RESEED_RATELIMIT_TIMED
tyzoid (Migrated from github.com) commented 2018-04-04 13:25:44 -05:00

We should warn if we #undef this constant here, because it indicates a fault in user-code.

We should warn if we `#undef` this constant here, because it indicates a fault in user-code.
tyzoid (Migrated from github.com) reviewed 2018-04-04 13:27:04 -05:00
src/prngs/fortuna.c
@ -64,0 +80,4 @@
cur_time -= CONST64(116444736000000000); /* subtract epoch in microseconds */
cur_time /= 1000; /* nanoseconds -> microseconds */
#endif
return cur_time / 100;
tyzoid (Migrated from github.com) commented 2018-04-04 13:27:03 -05:00

As pointed out by @sjaeckel on IRC, this isn't a problem, because src/headers/tomcrypt_custom.h on line 362 checks to make sure that these constants are defined.

As pointed out by @sjaeckel on IRC, this isn't a problem, because `src/headers/tomcrypt_custom.h` on line 362 checks to make sure that these constants are defined.
sjaeckel (Migrated from github.com) reviewed 2018-04-07 20:04:21 -05:00
src/headers/tomcrypt_custom.h
@ -366,0 +378,4 @@
* LTC_FORTUNA_RESEED_RATELIMIT_TIMED
* is defined.
*/
#undef LTC_FORTUNA_RESEED_RATELIMIT_TIMED
sjaeckel (Migrated from github.com) commented 2018-04-07 20:04:21 -05:00

done

done
tyzoid (Migrated from github.com) approved these changes 2018-04-09 10:14:37 -05:00
tyzoid (Migrated from github.com) left a comment

LGTM.

LGTM.
karel-m (Migrated from github.com) approved these changes 2018-04-09 15:54:59 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
tyzoid
karel-m
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
feature

   
help wanted

   
improve API

   
invalid

   
needs documentation

   
not-planned
This ticket is not planned to be implemented and/or fixed from the core team.

  
rtfm

   
stale

   
support

   
wishlist
No labels
bug
duplicate
enhancement
feature
help wanted
improve API
invalid
needs documentation
not-planned
rtfm
stale
support
wishlist
Milestone
Clear milestone
No items
No milestone
next
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-libtom/libtomcrypt!363
No description provided.