Add os random prng by eadmund · Pull Request #82 · froydnj/ironclad

eadmund · 2017-01-31T01:07:42Z

I have not regenerated the HTML documentation, but have updated the source for the docs.

This is a pretty big set of changes; I'm glad to explain my reasoning behind them.

Addresses #79

There are now three different PRNG classes: the default OS-provided PRNG (/dev/urandom on Unix & CryptGenRandom on Windows); the Fortuna PRNG & the Fortuna generator. Almost every single Ironclad user should just use the default OS-provided PRNG; if one wishes to have a userspace PRNG (which is generally considered to be a Bad Idea™) then one might wish to use the Fortuna PRNG. Finally, the Fortuna generator can be used as a seeded, predictable PRNG, perhaps for use with Monte Carlo simulations in need of high-quality random-like data. Do _not_ use the Fortuna generator on its own to generate key material! The API has been simplified: rather than the previous plethora of INTERNAL- methods, there is now a single mandatory method (PRNG-RANDOM-DATA) and two optional methods: PRNG-RESEED & PRNG-SEED-LENGTH. Overlong parameter names have been shortened. Internal names have been disambiguated (e.g. POOL → FORTUNA-POOL). The source has been reorganised.

Once corrections were made, the generator test vector worked perfectly. The Fortuna test vectors had incorrectly-sized seeds; after switching to properly-sized seeds, I updated the test vectors. I also updated MAKE-PRNG to not assume that it's making a Fortuna PRNG. Finally, the tests always passed, due to RT requiring an error vice a nil; this is now corrected.

It's better to use the OS-provided PRNG.

eadmund · 2017-01-31T01:10:32Z

  (:nicknames :crypto)
  (:shadow null)
-  (:import-from #:nibbles 
+  (:import-from #:nibbles


My editor automatically removes trailing whitespace. Sorry if you consider this clutter in the PR.

eadmund · 2017-01-31T01:14:33Z

   #:ed25519-key-x #:ed25519-key-y

   ;; pseudo-random number generators
-   #:pseudo-random-number-generator #:list-all-prngs #:make-prng #:random-data


It was a tough call to remove #:PSEUDO-RANDOM-NUMBER-GENERATOR, since it was exported and hence part of the external interface. However, it was only exported to allow user-defined PRNG classes, but the methods necessary to actually define PRNG classes were never exported, and thus any code which did extend PSEUDO-RANDOM-NUMBER-GENERATOR was relying on internals.

eadmund · 2017-01-31T01:14:33Z

-   #:pseudo-random-number-generator #:list-all-prngs #:make-prng #:random-data
-   #:read-os-random-seed #:read-seed #:write-seed #:fortuna-prng
-   #:add-random-event #:fortuna #:strong-random #:random-bits #:*prng*
+   #:list-all-prngs #:make-prng #:random-data #:read-os-random-seed


I really wanted to remove READ-OS-RANDOM-SEED too, but it was exported before and I've tried to be as conservative as possible — but no more — in these changes.

eadmund · 2017-01-31T01:15:28Z

 for use as a password salt."
-  (let ((prng (or *prng* (make-prng :fortuna :seed :random))))
-    (random-data size prng)))
+  (random-data size))


With the default OS PRNG, there's no need to instantiate a random one each time.

This also has the advantage of not blocking on Linux, which has a broken /dev/random

eadmund · 2017-01-31T01:17:53Z

                                         event))
    (incf (slot-value pool 'length) (length event))))

-(defmethod internal-write-seed (path (pseudo-random-number-generator


This was moved into prng.lisp, since it's actually common to any seedable PRNG.

eadmund · 2017-01-31T01:21:13Z

+            (open #P"/dev/urandom" :element-type 'unsigned-byte)))
+    (assert (>= (read-sequence seq (slot-value prng 'source)) num-bytes))
+    seq)
+  ;; FIXME: this is _untested_!


One major issue here is that only SBCL is supported on Windows. I have obtained a Windows computer and will attempt to figure out how to call CryptGenRandom from other implementations.

If support for other implementations on Windows is required, this PR needs to remain open.

eadmund · 2017-01-31T01:21:38Z

+  (let ((seq (make-array num-bytes :element-type 'unsigned-byte)))
+    (unless (slot-boundp prng 'source)
+      (setf (slot-value prng 'source)
+            (open #P"/dev/urandom" :element-type 'unsigned-byte)))


Reading from /dev/urandom is the Right Thing™ generally.

Here the :element-type being 'unsigned-byte makes the function return a (simple-vector n), which causes some type issues with functions requiring a (simple-array (unsigned-byte 8) (n)) (e.g. key pair generation).

Changing :element-type 'unsigned-byte to :element-type '(unsigned-byte 8) solves the problem.

eadmund · 2017-01-31T01:22:41Z

@@ -27,27 +20,42 @@

 (defmethod make-prng :around (name &key (seed :random))


I really, really wanted to delete MAKE-PRNG, but it was an exported part of Ironclad's interface.

It might actually be a good idea to delete it, to force people not to allocate their own Fortuna generators but instead just use the OS one.

eadmund · 2017-01-31T01:23:54Z

-       (reseed (slot-value prng 'generator) seed)
-       (incf (slot-value prng 'reseed-count)))
-      (t (error "SEED must be an octet vector, pathname indicator, :random or :urandom")))
+    (unless (eq name :fortuna-generator)


A FORTUNA-GENERATOR cannot be reseeded at start; it must be initially seeded once before it may be reseeded. This type-specific check is really ugly.

eadmund · 2017-01-31T01:25:12Z

       do (crypto:add-random-event source pool-id event prng))
-    (equalp expected-sequence
-            (crypto:random-data num-bytes prng))))
+    (assert (equalp expected-sequence


These tests were never actually being checked!

eadmund · 2017-02-14T03:18:40Z

    seq)
  ;; FIXME: this is _untested_!
  #+(and win32 sb-dynamic-core)(sb-win32:crypt-gen-random num-bytes)
-  #-(or unix (and win32 sb-dynamic-core))(error "Your platform does not have a supported random source."))


The repetition between this & OS-RANDOM-SEED is pretty ugly at this point, I'll admit.

Tested the existing SBCL support. Implemented & tested support for Clozure Common Lisp, Allegro Common Lisp & LispWorks. These all use RtlGenRandom.

glv2 · 2017-04-06T13:28:47Z

+        ((typep seed 'simple-octet-vector)
+         (prng-reseed seed prng)
+         (incf (slot-value prng 'reseed-count)))
+        (t (error "SEED must be an octet vector, pathname indicator, :random or :urandom"))))


Making a Fortuna PRNG with a :random seed doesn't work anymore because of a failed assertion:

(ironclad:make-prng :fortuna :seed :urandom) The assertion (= #1=(LENGTH IRONCLAD::SEED) IRONCLAD::+FORTUNA-SEED-LENGTH+) failed with #1# = 0, IRONCLAD::+FORTUNA-SEED-LENGTH+ = 64. [Condition of type SIMPLE-ERROR]

If allowing only a determined octet vector as seed is the expected behaviour (to create a deterministic pseudo-random sequence of bytes), it could be useful to change the documentation and error strings of the make-prng function and remove the references to :random and :urandom as they won't be used neither by Fortuna nor by OS-PRNG.

Fixed; this was due to PRNG-SEED-LENGTH not being defined for FORTUNA-PRNG.

I've also added a test so that this won't happen again.

Per froydnj#82 (comment)

glv2 · 2017-04-25T12:29:25Z

 (in-package :crypto-tests)

 (rtest:deftest :prng-fortuna (run-test-vector-file :prng *prng-tests*) t)
+(rtest:deftest :prng-fortuna-urandom (let ((prng (crypto:make-prng :fortuna :seed urandom)))


Shouldn't it be :seed :urandom instead of :seed urandom?

You're right. Any idea why the Travis CI build passed?

Looks like the tests exit successfully even with an error: https://travis-ci.org/froydnj/ironclad/jobs/224327258

I've fixed the URANDOM bug, but someone who knows more about Travis CI & RT needs to figure out how to exit non-zero on errors …

I think I figured out how to make test runs actually fail, but some existing tests fail in CCL: https://travis-ci.org/froydnj/ironclad/jobs/225603790

Should I revert and open an issue for fixing testing and those tests, or …?

Some tests fail in CCL 1.10 (the version used by the travis-ci script), but everything works fine in CCL 1.11 (the version on my system).

Per froydnj#82 (comment)

Per froydnj/ironclad#82 (comment) committer: Guillaume LE VAILLANT <guillaume.le.vaillant@openmailbox.org>

Robert A. Uhl added 6 commits January 24, 2017 18:51

Separate generic PRNG from Fortuna

d5e561b

Fix compilation errors

3cad220

Remove internal use of Fortuna PRNG

e96773a

It's better to use the OS-provided PRNG.

Update documentation

ce87b80

eadmund commented Jan 31, 2017

View reviewed changes

eadmund commented Feb 14, 2017

View reviewed changes

Implemented Windows support

cdbe43f

Tested the existing SBCL support. Implemented & tested support for Clozure Common Lisp, Allegro Common Lisp & LispWorks. These all use RtlGenRandom.

eadmund force-pushed the add-os-random-prng branch from fc4c5a6 to cdbe43f Compare February 14, 2017 03:19

glv2 reviewed Apr 6, 2017

View reviewed changes

Robert A. Uhl added 3 commits April 21, 2017 05:51

Switch PRNG-RANDOM-DATA to (UNSIGNED-BYTE 8)

9feb73e

Per froydnj#82 (comment)

Define PRNG-SEED-LENGTH for Fortuna

bb3ea02

Add test of (MAKE-PRNG :FORTUNA :SEED URANDOM)

a9a8988

glv2 reviewed Apr 25, 2017

View reviewed changes

Robert A. Uhl added 2 commits April 25, 2017 07:18

Fix PRNG-FORTUNA-URANDOM test

fc8ae00

Attempt to exit with non-zero status on test failure

9757bf7

glv2 pushed a commit to glv2/ironclad that referenced this pull request Apr 28, 2017

Switch PRNG-RANDOM-DATA to (UNSIGNED-BYTE 8)

fa7b7d4

Per froydnj#82 (comment)

eadmund mentioned this pull request Jun 26, 2017

Add os random prng glv2 glv2/ironclad#3

Closed

richardwesthaver pushed a commit to richardwesthaver/ironclad that referenced this pull request Mar 3, 2026

Switch PRNG-RANDOM-DATA to (UNSIGNED-BYTE 8)

335f661

Per froydnj/ironclad#82 (comment) committer: Guillaume LE VAILLANT <guillaume.le.vaillant@openmailbox.org>

		@@ -27,27 +20,42 @@

		(defmethod make-prng :around (name &key (seed :random))

Conversation

eadmund commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eadmund Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eadmund Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eadmund Apr 21, 2017 •

edited

Loading

eadmund Apr 25, 2017 •

edited

Loading