Add HC-128 #210

pitdicker · 2017-12-16T13:57:00Z

Implement the HC-128 cryptographically secure RNG. See dhardy#53.

It is similar to ISAAC, but proven to be secure and (in many cases) faster.

I tested locally that the first 65536 result of this RNG match the reference implementation for a couple of random seeds.

This is based on top of #209.

dhardy · 2018-01-02T14:38:20Z

@pitdicker can you rebase please? Sorry, I must have rebased #209.

pitdicker · 2018-01-02T14:51:49Z

No problem, hope I got it right...

pitdicker · 2018-01-03T15:08:23Z

Not sure what the problem with travis is

dhardy

sorry, wrote these comments the other day but never finished reviewing

dhardy · 2018-01-02T16:03:14Z

src/prng/hc128.rs

+///      ["Some Results On Analysis And Implementation Of HC-128 Stream Cipher"]
+///      (http://library.isical.ac.in:8080/jspui/bitstream/123456789/6636/1/TH431.pdf).
+#[derive(Clone)]
+pub struct Hc128Rng {


I guess this is the wrong time to bring it up, but we should think about the naming scheme. I know Rust will complain, but HC128 might be preferable (ugh, camel case applied to acronyms — if HC is even an acronym)! Okay, ignore this for now but lets open an issue.

I don't really mind. ISAAC is already IsaacRng. In the standard library ARC is Arc, RC is Rc etc.

dhardy · 2018-01-02T16:04:46Z

src/prng/hc128.rs

+///
+/// HC-128 is an array based RNG. In this it is similar to RC-4 and ISAAC before
+/// it, but those have never been proven cryptographically secure (or have even
+/// been broken).


if you're going to mention that it was broken, please be more specific (RC-4, or even a link)

I'll reword it. For RC-4 it seemed like 'common knowledge', but I'll try to find a nice link. Maybe https://tools.ietf.org/html/rfc7465 "Prohibiting RC4 Cipher Suites".

dhardy · 2018-01-06T15:47:17Z

src/prng/hc128.rs

 ///
 /// HC-128 is an array based RNG. In this it is similar to RC-4 and ISAAC before
 /// it, but those have never been proven cryptographically secure (or have even
-/// been broken).
+/// been sgnificantly coompromised, as in the case of RC-4 [5]).


I have my first mechanical keyboard now, and it takes some getting used to 😄

dhardy

Wow, quite impressive code here! Sorry it took me so long to go through it.

There are a few things mentioned I'm not sure about (only one I'm really concerned about is seeding with a slice, but also might be worth looking at next_u64).

As for the HC-128 generator itself, I read through the paper. I didn't follow the analysis, but it appeared to only be one of several possible things (e.g. there is no analysis on period at all in that paper). I notice you also linked a thesis on the subject, maybe I'll have a look sometime, but I trust you already studied this well.

dhardy · 2018-01-06T18:02:27Z

src/prng/hc128.rs

+        self.counter1024 = self.counter1024.wrapping_add(16);
+    }
+
+    fn sixteen_steps(&mut self) {


So this is the same code as update excepting where results are written and the counter wrapping at the end? I suppose you can't just call update(&mut state.state) because of the borrow checker, but I suspect in C one could do that. Oh well, I guess this will have to do.

This was already different from the C code because the borrow checker was quite a problem for ISAAC and HC-128 and all the different places that index into to table. And yes, I couldn''t get the borrow checker happy here...

dhardy · 2018-01-06T18:07:04Z

src/prng/hc128.rs

+                (y << 32) | x
+            }
+        } else if index >= 16 {
+            self.state.update(&mut self.results);


Wouldn't it be better to move this check to the beginning of the function, as in next_u32?

The three-way split in this function has these routes:

no values left: update

one u32 left: use that one and update

all other situations: just index normally.

This optimisation is what makes it faster than just using next_u32 two times.

dhardy · 2018-01-06T18:10:08Z

src/prng/hc128.rs

+            (y << 32) | x
+        } else {
+            let x = self.results[15] as u64;
+            self.state.update(&mut self.results);


Nothing in the spec of Rng says we can't throw away extra numbers, so you could just update if index >= 15 at the beginning. Would be simpler, and would align future output if a u32 was taken then a long sequence of u64.

In fact, you could possibly go further and always throw away a number if unaligned, then you should be able to use the direct copy on all CPUs? It's difficult to benchmark this kind of thing since it's very usage-dependent, but it would simplify code.

I modified the benchmark when writing this to test the impact. For example in the gen_uint marco I added a single next_u32 just after creating rng, so that all calls to next_u64 would take the suboptimal path. And if I remember right, the performance impact was minimal.

I have played with the thought of throwing away parts of the result depending on the alignment of the output buffer in fill_bytes. But that would make the results depend on memory alignment,something we may not be able to guarantee across compilations. Or are we speaking about something else?

Sorry, you're right that the Hc128Rng type may not be aligned for u64 values anyway, and yes, we need consistent output across platforms.

dhardy · 2018-01-06T18:18:36Z

src/prng/hc128.rs

+            let dest_u32: &mut [u32] = unsafe {
+                slice::from_raw_parts_mut(
+                    dest[filled..].as_mut_ptr() as *mut u8 as *mut u32,
+                                      16)


I've used the array_ref! macro a few times myself. Maybe we really should copy it into rand? Not important.

Ah, this is where unaligned copies happen.

I wrote this before we had it. I don't think about it enough yet to be honest, maybe just have to see it a few more times in action. Should I modify this?

No, array_ref is not in rand yet. Maybe we should add it; not sure, and not sure if it should be exported. It would probably be more useful in rand-core if we do export the macro.

dhardy · 2018-01-06T18:22:01Z

src/prng/hc128.rs

+    // be safe and fast.
+    // This improves performance by about 12%.
+    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+    fn fill_bytes(&mut self, dest: &mut [u8]) {


Not entirely sure how I feel about this. Pretty cool that you can do this but a fair amount extra complexity. Well, we have it now!

For ISAAC I would not think it worth it, but here it is directly filling at most 64 bytes at a time. It seems to me requesting that amount or more is not uncommon. So the 12% improvement seemed worth the complexity. It had the consequence that Hc128 had to be a separate struct inside Hc128Rng. So the complexity cost is higher than that single function...

The struct separation is good for readability anyway.

dhardy · 2018-01-06T18:23:40Z

src/prng/hc128.rs

+    /// 256 bits in length, matching the 128 bit `key` followed by 128 bit `iv`
+    /// when HC-128 where to be used as a stream cipher.
+    fn from_seed(seed: &'a [u32]) -> Hc128Rng {
+        assert!(seed.len() == 8);


Why use a slice when the length must be fixed? Why not use an array ref? I know the current convention is to use a slice, but we don't need to respect that.

Oh, we discussed that! Fixed it in some branch, but forget to update here. https://github.com/dhardy/rand/pull/77/files#diff-9c520b54b9848356a0cf51d6f963749cL87

Should I change the seed type to [u8, 32], or make it [u32; 8]? With an u8 seed I would also need to include the read_u32_into function.

pitdicker · 2018-01-06T20:07:46Z

As for the HC-128 generator itself, I read through the paper. I didn't follow the analysis, but it appeared to only be one of several possible things (e.g. there is no analysis on period at all in that paper). I notice you also linked a thesis on the subject, maybe I'll have a look sometime, but I trust you already studied this well.

I have read it, but a lot of it was over my head... I believe periods are a bit of a fuzzy thing with this super-large array based RNG's. The average cycle length should be 2^(1024*32-1) = 2^32767. And there might be cases where the cycle length is shorter, but that is maybe impossibly hard to prove.

dhardy · 2018-01-06T20:08:07Z

On a related note, I want to add SeedableRng::from_rng to replace the current Rand impls. It could be merged before or after this I guess, but needs to be before my work on distributions (which needs to be before rand-core stuff). You can do so if you get there first...

pitdicker · 2018-01-06T20:10:38Z

I would welcome the new seeding code, I see it as one of the big improvements. But from_rng depends on the new error handling?

pitdicker · 2018-01-06T20:15:58Z

Cycle length is good to document, I will add it.

dhardy · 2018-01-08T15:12:12Z

src/prng/hc128.rs

        };

        // run the cipher 1024 steps
        for _ in 0..64 { state.state.sixteen_steps() };
        state.state.counter1024 = 0;
-
-        // Prepare the first set of results
-        state.state.update(&mut state.results);


Why did this code get dropped?

I also noticed a similar difference with ISAAC when merging upstream into my branch; not sure why it was there.

Isn't it better to do as much as possible during init?

To be honest, I don't completely remember. I had reasons for it when working on the BlockRngCore stuff, this change to the seeding was part of the 'functional' changes.

ChaCha generated the set of results on first use, while ISAAC and HC-128 did on initialization. So it seemed a good idea to pick one default.

Generating results on first use made the seeding function for slightly simpler, it is just initializing a struct:

impl<R> SeedableRng for BlockRng<R> where R: BlockRngCore + SeedableRng, { type Seed = R::Seed; fn from_seed(seed: Self::Seed) -> Self { let results_empty = R::Results::default(); Self { core: R::from_seed(seed), index: results_empty.as_ref().len(), // generate on first use results: results_empty, } } fn from_rng<Rseed: Rng>(rng: Rseed) -> Result<Self, Error> { let results_empty = R::Results::default(); Ok(Self { core: R::from_rng(rng)?, index: results_empty.as_ref().len(), // generate on first use results: results_empty, }) } }

I feel like I had a better reason, but can't remember it.

dhardy · 2018-01-08T15:15:13Z

This has been open for three weeks now (mostly because it took me a while to review). Any final comments?

There's one new comment I just made; once that's resolved I'll merge.

pitdicker · 2018-01-08T16:57:28Z

Any final comments?

Happy to see this merged 😄

Add HC-128

pitdicker changed the title ~~Hc 128 up~~ Add HC-128 Dec 16, 2017

pitdicker force-pushed the hc-128_up branch 2 times, most recently from 5ce0a1f to ca3bc7e Compare December 16, 2017 14:39

dhardy mentioned this pull request Dec 17, 2017

Release strategy #205

Closed

dhardy mentioned this pull request Jan 2, 2018

Add Serde #189

Merged

pitdicker force-pushed the hc-128_up branch from ca3bc7e to 2fb08f4 Compare January 2, 2018 14:50

Add HC-128 RNG

918d481

pitdicker force-pushed the hc-128_up branch from 1613987 to 918d481 Compare January 2, 2018 15:07

dhardy reviewed Jan 4, 2018

View reviewed changes

dhardy reviewed Jan 6, 2018

View reviewed changes

Small improvements to HC-128 documentation

30df574

pitdicker force-pushed the hc-128_up branch from b0c5368 to 30df574 Compare January 6, 2018 16:18

dhardy reviewed Jan 6, 2018

View reviewed changes

Seed with an array stead of a slice

7f42739

pitdicker force-pushed the hc-128_up branch from 2896dc7 to 7f42739 Compare January 8, 2018 12:56

dhardy reviewed Jan 8, 2018

View reviewed changes

Fix documentation warnings.

254a9f7

dhardy merged commit 3d1dbb6 into rust-random:master Jan 10, 2018

dhardy mentioned this pull request Jan 12, 2018

Tracker: planned changes for 0.5 #232

Closed

33 tasks

pitdicker deleted the hc-128_up branch January 21, 2018 15:31

pitdicker pushed a commit that referenced this pull request Apr 4, 2018

Merge pull request #210 from pitdicker/hc-128_up

5281685

Add HC-128

Add HC-128 #210

Add HC-128 #210

Conversation

pitdicker commented Dec 16, 2017

dhardy commented Jan 2, 2018

pitdicker commented Jan 2, 2018

pitdicker commented Jan 3, 2018

dhardy left a 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

dhardy left a 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

pitdicker commented Jan 6, 2018

dhardy commented Jan 6, 2018 • edited Loading

pitdicker commented Jan 6, 2018

pitdicker commented Jan 6, 2018

Choose a reason for hiding this comment

pitdicker Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

dhardy commented Jan 8, 2018

pitdicker commented Jan 8, 2018

dhardy commented Jan 6, 2018 •

edited

Loading

pitdicker Jan 8, 2018 •

edited

Loading