Provide a better random password generator

[bertogg]

The code used to generate random passwords for the Password Safe feature uses qrand(), which is simply the Qt wrapper on top of the standard rand() libc function.

That function provides pseudo-random numbers and is not designed to be cryptographically secure. From the GNU libc documentation:

The numbers generated are not truly random; typically, they form a sequence that repeats periodically, with a period so large that you can ignore it for ordinary purposes. The random number generator works by remembering a seed value which it uses to compute the next random number and also to compute a new seed. Although the generated numbers look unpredictable within one run of a program, the sequence of numbers is exactly the same from one run to the next.

Since the Nitrokey itself comes with a random number generator, I wonder if it isn't possible to use it for this purpose? Otherwise you could just simply use /dev/urandom on Linux and equivalent methods on other systems.

On top of that I also noticed that since there are 95 printable characters and they are selected with qrand() % 95 then it means that some characters have more chances of being selected than others.

[szszszsz]

Hi! Good catch! Indeed, qrand() should be replaced with the recommended now QRandomGenerator, which after brief read sounds similar to C++11 (or later) random generators. I think it is better to use Qt's equivalents to maintain cross-platform code, than using the urandom file directly (or alternatively the C++ standard equivalent). Gathering data from smart card is of course a great idea, and might be introduced later via the firmware. In the future Nitrokey App should be able to talk directly to the smart card itself.

On top of that I also noticed that since there are 95 printable characters and they are selected with qrand() % 95 then it means that some characters have more chances of being selected than others.

Do you mean with the improper (not perfectly random) numbers distribution? In that case yes. With the generator replaced it should work as-is though.

[bertogg]

Good catch! Indeed, qrand() should be replaced with the recommended now QRandomGenerator

Hi,

yes it looks like QRandomGenerator is a good option.

Do you mean with the improper (not perfectly random) numbers distribution? In that case yes. With the generator replaced it should work as-is though.

No, I mean something else.

Let's say you get a random number N between 0 and 255, and you have 94 characters in PasswordCharSpace (I incorrectly said 95 in my original post).

If you do N % 94 you'll indeed get a valid index for PasswordCharSpace, but not all characters have the same probability: characters with index 0 to 67 appear three times, those with index 68 to 94 appear twice.

You get PasswordCharSpace[0] with N == 0, N == 94 and N == 188

You get PasswordCharSpace[72] only with N == 72 and N == 166

QRandomGenerator::generate() returns a 32-bit unsigned integer, so the difference in probability would be negligible in this case. It's easy to fix, you only need the largest number max for which max % 94 == 93. It would be something like this:

uint32_t max = UINT32_MAX - UINT32_MAX % PasswordCharSpace.length() - 1;
do {
   x = get_random_number();
} while (x > max);

However I just checked and QRandomGenerator has a bounded(min, max) method, so that's probably the best way to go.

[szszszsz]

I see. Surely I agree if that would be the case. However in the current implementation the returned value of qrand() is int, and it is saved in an int typed variable, which should be at least 32 bit. https://github.com/Nitrokey/nitrokey-app/blob/a92d46b4c923e431bafb71cf55bc3abd3a9bb405/src/ui/mainwindow.cpp#L1495-L1511 Anyway, the random number generator has to be replaced. I have marked this to be fixed in the next release. I understand, that with your snippet you are making the distribution exactly uniform. I guess such is already provided in the discussed Qt's replacement (it should be at least, since it is probably an std wrapper).

[bertogg]

Right, with a 32-bit (signed) int the problem is not very important. I suppose Qt does the right thing anyway.