rand() is NOT cryptographically safe ... even if flipper zero implementation happens to be

[henrygab]

Unless there's something I've missed, this app appears to use consecutive calls to rand() as the source of "randomness" for the password being generated:

flipper_passgen/passgen.c

Line 137 in 6c1121e

int x = rand() % hi;

Unfortunately, rand() is a PRNG (pseudo-random number generator). Worse, implementations have historically been really terrible. See generally, https://stackoverflow.com/questions/58067210/is-it-acceptable-to-use-rand-for-cryptographically-insecure-random-numbers.

Since you're suggesting this application is to be used to generate passwords, that definitely falls into the category where cryptographically strong randomness is desired.

If you cannot find a TRNG (true random number generator) or CSRNG (cryptographically strong random number generator) usable in the FAP, at a minimum WARN folks that this is not a cryptographically strong password generator (e.g., splash screen at startup), or just pull the repo.

[ClaraCrazy]

Meow

Yes, you have missed something @henrygab

rand() has been overridden in the fz code to use furi_hal_get_random? (something like that, tired af), which is uses the stm32wb55s hardware to generate random values, based on 4? oscillators. Its cryptographically secure as claimed by their own documentation.

Also, dev seems to be MIA, so yeah. If you wanna suggest changes or w/e. ping me, I'd keep it up unless @anakod magically reappears in here

[henrygab]

Awesome!

Would you consider using furi_hal_random_get() directly?

As to why: It would make it more clear that your password generator is not using the insecure rand() function, and perhaps reduce re-use of the code, without at least some indication that the function is not rand(), but relies upon a more secure alternative (e.g., the TRNG of the stm32wb55s).

[MichaelGrafnetter]

Hi, if I understand the code correctly, there is a modulo bias present in the password generation code. Some characters thus have higher probability to appear in the password than others:

void generate(PassGen* app)
{
	int hi = strlen(app->alphabet);
	for (int i=0; i<app->length; i++)
	{
		int x = rand() % hi;
		app->password[i]=app->alphabet[x];
	}
	app->password[app->length] = '\0';
}

This is how KeepassXC deals with the issue:

quint32 Random::randomUInt(quint32 limit)
{
    Q_ASSERT(limit <= QUINT32_MAX);
    if (limit == 0) {
        return 0;
    }

    quint32 rand;
    const quint32 ceil = QUINT32_MAX - (QUINT32_MAX % limit) - 1;

    // To avoid modulo bias make sure rand is below the largest number where rand%limit==0
    do {
        m_rng->randomize(reinterpret_cast<uint8_t*>(&rand), 4);
    } while (rand > ceil);

    return (rand % limit);
}

As @ClaraCrazy noted, rand() is implemented as follows:

int rand() {
    return (furi_hal_random_get() & RAND_MAX);
}

It thus really uses a cryptographically secure random number generator. But I would agree with @henrygab that furi_hal_random_get() should still be used instead of rand() to avoid any future confusion. Moreover, furi_hal_random_fill_buf() would probably yield better performance when generating multiple random bytes in a row, as it avoids repeated RNG hardware initialization.

[MichaelGrafnetter]

Thanks @henrygab and @anakod for fixing this!