Randomize() calls Crop(), though it may not be needed

[irwir]

https://github.com/weidai11/cryptopp/blob/899dea907103075cf7e68b16dc81aadd4ce83749/integer.cpp#L3525 By definition nbytes should be 1 or greater.

[noloader]

Thanks @irwir.

Yeah, something looks a little sideways.

Looking at it:

if (nbytes)
    buf[0] = (byte)Crop(buf[0], nbits % 8);

It looks like the intention is to keep the low order bits, and mask-out the upper bits. And if nbits % 8 == 0, then keep all the bits.

I'm wondering if it should not be:

if (nbits % 8)
    buf[0] = (byte)Crop(buf[0], nbits % 8);

It looks like the code has used nbytes since at least Crypto++ 5.6.2. See https://github.com/weidai11/cryptopp/blob/789f81f048c9bc472c7e7596f49b02eadd6fc1fd/integer.cpp#L3200

I'm wondering if it not was by design. Wei makes very few mistakes. I would be surprised to learn of one.

@mouse07410, any thoughts?

[irwir]

https://github.com/weidai11/cryptopp/blob/899dea907103075cf7e68b16dc81aadd4ce83749/integer.cpp#L3522 It might make more sense if instead of questionable nbits/8 + 1 it was more obvious (nbits + 7)/8

[noloader]

So the description of Randomize says (https://github.com/weidai11/cryptopp/blob/master/integer.h#L447):

The random integer created is uniformly distributed over [0, 2^bitCount].

If someone passes 0 bits, then the the caller should get either 0 or 1 because 2^0 = 1. If we switch to (nbits + 7)/8, then we can only return 0. But I am not sure the existing method handles that properly, either.

Let me look at it some more.

[noloader]

Yeah, we don't handle that case properly now:

$ cat test.cxx
#include <iostream>
#include <iomanip>
#include "integer.h"
#include "osrng.h"

int main(int argc, char* argv[])
{
    using namespace CryptoPP;

    AutoSeededRandomPool prng;
    Integer n;

    for (size_t i=0; i<128; i++)
    {
      n.Randomize(prng, 0);
      std::cout << (n==0 ? "0" : "1") << " ";
    }
    std::cout << std::endl;
    return 0;
}

$ g++ -o test.exe -g2 -O3 test.cxx ./libcryptopp.a
$ ./test.exe
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0

[irwir]

https://github.com/weidai11/cryptopp/blob/899dea907103075cf7e68b16dc81aadd4ce83749/gf2n.cpp#L82-L89 Without digging too deep into the code, my guess is that the check for nbytes is to be removed. nbits % 8 is the shift count for bit mask. If zero bits were requested, Crop will return 0.

[irwir]

The random integer created is uniformly distributed over [0, 2^bitCount].

[0, 2^bitCount[ or [0, 2^bitCount) Or like this: [0, 2^bitCount-1]

[noloader]

This should be cleared at Commit 6ac6668b2976.