Patch for RandomNumberGenerator::GenerateIntoBufferedTransformation bug

[Jeffrey Walton]

Testing of RandomNumberGenerator::GenerateWord32 revealed a bug in GenerateBlock.

GenerateBlock calls GenerateIntoBufferedTransformation. GenerateIntoBufferedTransformation, in turn, calls, GenerateBlock. Ad infinitum.

This patch fixes the circularity by calling OS_GenerateRandomBlock using the OS's default entropy pool for userspace in a non-blocking mode.

Another way to address t is to have GenerateIntoBufferedTransformation throw an Exception with type set to NOT_IMPLEMENTED. But I think that course will be less useful.

Any comments or suggestions?

**********

$ cat cryptlib.diff
diff --git a/cryptlib.cpp b/cryptlib.cpp
index a9ed290..ad173e2 100644
--- a/cryptlib.cpp
+++ b/cryptlib.cpp
@@ -277,13 +277,15 @@ void RandomNumberGenerator::DiscardBytes(size_t n)
     GenerateIntoBufferedTransformation(TheBitBucket(), DEFAULT_CHANNEL, n);
 }
 
+// Avoid bringing in <osrng.h>, which depends on <cryptlib.h>
+extern void OS_GenerateRandomBlock(bool, byte*, size_t);
 void RandomNumberGenerator::GenerateIntoBufferedTransformation(BufferedTransformation &target, const std::string &channel, lword length)
 {
     FixedSizeSecBlock<byte, 256> buffer;
     while (length)
     {
         size_t len = UnsignedMin(buffer.size(), length);
-        GenerateBlock(buffer, len);
+        OS_GenerateRandomBlock(false, buffer, len);
         target.ChannelPut(channel, buffer, len);
         length -= len;
     }

[Jeffrey Walton]

Here's an updated patch. cryptlib.cpp already includes <osrng.h>, so there's no need for the hack.

$ git diff cryptlib.cpp
diff --git a/cryptlib.cpp b/cryptlib.cpp
index a9ed290..228f18a 100644
--- a/cryptlib.cpp
+++ b/cryptlib.cpp
@@ -252,11 +252,9 @@ byte RandomNumberGenerator::GenerateByte()
 
 word32 RandomNumberGenerator::GenerateWord32(word32 min, word32 max)
 {
-       word32 range = max-min;
+       word32 value, range = max-min;
        const int maxBits = BitPrecision(range);
 
-       word32 value;
-
        do
        {
                GenerateBlock((byte *)&value, sizeof(value));
@@ -282,10 +280,10 @@ void RandomNumberGenerator::GenerateIntoBufferedTransforma

        FixedSizeSecBlock<byte, 256> buffer;
        while (length)
        {

-               size_t len = UnsignedMin(buffer.size(), length);
-               GenerateBlock(buffer, len);
-               target.ChannelPut(channel, buffer, len);
-               length -= len;
+               const size_t segmentLen = UnsignedMin(buffer.size(), length);
+               OS_GenerateRandomBlock(false, buffer, segmentLen);
+               target.ChannelPut(channel, buffer, segmentLen);
+               length -= segmentLen;

[Jean-Pierre Münch]

IMO, don't change RandomNumberGenerator.

Am 08.07.2015 um 17:26 schrieb Jeffrey Walton:

Testing of RandomNumberGenerator::GenerateWord32 revealed a bug in GenerateBlock.

GenerateBlock calls GenerateIntoBufferedTransformation. GenerateIntoBufferedTransformation, in turn, calls, GenerateBlock. Ad infinitum.

This is by design, as far as I can tell.

This patch fixes the circularity by calling OS_GenerateRandomBlock using the OS's default entropy pool for userspace in a non-blocking mode.

We already agreed that using "plain" OS's RNG isn't something we don't want, so we should step away from providing it.
Furthermore the RNG class by itself is supposed to be non-working. It is like an abstract base class which may be used with lower-level implementations like X917 or RandomPool.

Another way to address t is to have GenerateIntoBufferedTransformation throw an Exception with type set to NOT_IMPLEMENTED. But I think that course will be less useful.

This class by itself isn't intended for direct usage, without real underlying primitives, so this would be the better choice to emphasize to use a properly implemented class.

Any comments or suggestions?

I'll give a short answer on what I think is the rationale behind the behavior you discovered.

The RNG class has a problem: It needs to provide two basic interfaces for random number generation. Everything else is built on these two functions. So what to do? Make both virtual? This is unnecessary, an implementor would likely only implement one of them, so why force him to write a wrapper himself?

The design choice behind this "bug" seems to be, that an implementor can choose whether to implement GenerateBlock() or to implement GenerateIntoBufferedTransformation(). If he implements either of those, he's fine. If he misses to implement any of these he'll see his test failing and will notice that he has to implement one of them.

So please leave it as it is right now (5.6.2)

BR

JPM

**********

$ cat cryptlib.diff
diff --git a/cryptlib.cpp b/cryptlib.cpp
index a9ed290..ad173e2 100644
--- a/cryptlib.cpp
+++ b/cryptlib.cpp
@@ -277,13 +277,15 @@ void RandomNumberGenerator::DiscardBytes(size_t n)
     GenerateIntoBufferedTransformation(TheBitBucket(), DEFAULT_CHANNEL, n);
 }
 
+// Avoid bringing in <osrng.h>, which depends on <cryptlib.h>
+extern void OS_GenerateRandomBlock(bool, byte*, size_t);
 void RandomNumberGenerator::GenerateIntoBufferedTransformation(BufferedTransformation &target, const std::string &channel, lword length)
 {
     FixedSizeSecBlock<byte, 256> buffer;
     while (length)
     {
         size_t len = UnsignedMin(buffer.size(), length);
-        GenerateBlock(buffer, len);
+        OS_GenerateRandomBlock(false, buffer, len);
         target.ChannelPut(channel, buffer, len);
         length -= len;
     }

--
--
You received this message because you are subscribed to the "Crypto++ Users" Google Group.
To unsubscribe, send an email to cryptopp-user...@googlegroups.com.
More information about Crypto++ and this group is available at http://www.cryptopp.com.
---
You received this message because you are subscribed to the Google Groups "Crypto++ Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cryptopp-user...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jeffrey Walton]

On Wednesday, July 8, 2015 at 4:50:11 PM UTC-4, jean-pierre.muench wrote:

IMO, don't change RandomNumberGenerator.

OK, fair enough.

Am 08.07.2015 um 17:26 schrieb Jeffrey Walton:

Testing of RandomNumberGenerator::GenerateWord32 revealed a bug in GenerateBlock.

GenerateBlock calls GenerateIntoBufferedTransformation. GenerateIntoBufferedTransformation, in turn, calls, GenerateBlock. Ad infinitum.

This is by design, as far as I can tell.

But the thing that has always troubled me is, one (or both) are not pure virtuals.
 

This patch fixes the circularity by calling OS_GenerateRandomBlock using the OS's default entropy pool for userspace in a non-blocking mode.

We already agreed that using "plain" OS's RNG isn't something we don't want, so we should step away from providing it.
Furthermore the RNG class by itself is supposed to be non-working. It is like an abstract base class which may be used with lower-level implementations like X917 or RandomPool.

Another way to address t is to have GenerateIntoBufferedTransformation throw an Exception with type set to NOT_IMPLEMENTED. But I think that course will be less useful.

This class by itself isn't intended for direct usage, without real underlying primitives, so this would be the better choice to emphasize to use a properly implemented class.

OK. Another option is to make  GenerateIntoBufferedTransformation a pure virtual.

Any comments or suggestions?

I'll give a short answer on what I think is the rationale behind the behavior you discovered.

The RNG class has a problem: It needs to provide two basic interfaces for random number generation. Everything else is built on these two functions. So what to do? Make both virtual? This is unnecessary, an implementor would likely only implement one of them, so why force him to write a wrapper himself?

The design choice behind this "bug" seems to be, that an implementor can choose whether to implement GenerateBlock() or to implement GenerateIntoBufferedTransformation(). If he implements either of those, he's fine. If he misses to implement any of these he'll see his test failing and will notice that he has to implement one of them.

So please leave it as it is right now (5.6.2)

OK, will do.

Jeff

[Jeffrey Walton]

On Wednesday, July 8, 2015 at 11:26:53 AM UTC-4, Jeffrey Walton wrote:

Testing of RandomNumberGenerator::GenerateWord32 revealed a bug in GenerateBlock.

GenerateBlock calls GenerateIntoBufferedTransformation. GenerateIntoBufferedTransformation, in turn, calls, GenerateBlock. Ad infinitum.

This patch fixes the circularity by calling OS_GenerateRandomBlock using the OS's default entropy pool for userspace in a non-blocking mode.

Another way to address t is to have GenerateIntoBufferedTransformation throw an Exception with type set to NOT_IMPLEMENTED. But I think that course will be less useful.

Any comments or suggestions?

This has come up again offlist. Someone reported the same issue against 5.6.3rc4 while debugging a SHA failure with a bleeding edge GCC.

I opened an issue against it at "Crash in RandomNumberGenerator::GenerateWord32 due to stack recursion" (https://github.com/weidai11/cryptopp/issues/38).

I also dug up an old email between Wei and I about it. Here's what Wei had to say about it:

    Yeah, you're not supposed to use it directly. It's just
    meant to define the interface that other RNGs are
    supposed to implement, and includes some helper
    functions. I should probably make it so that it can't
    be instantiated.

I think we should remove the implementations for GenerateIntoBufferedTransforma and GenerateBlock, and turn them into pure virtuals.

Any thoughts or objections?

Jeff