Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.54k stars 2.6k forks source link

Get rid of rand() in library code #6427

Open gilles-peskine-arm opened 2 years ago

gilles-peskine-arm commented 2 years ago

library/rsa.c uses rand() in its self-test code. This is fine, since it's only a test, but it limits portability (some embedded platforms don't have rand()) and it causes the occasional false positive by static analyzers. (We'd even like to get rid of rand() in the test framework).

What to replace it with? RSA needs a RNG with statistical properties that aren't completely absurd (e.g. all zeros won't do), but since this is test code it doesn't have to be anywhere near crypto quality. We'd like not to create adherence to other modules, so using CTR_DRBG or HMAC_DRBG is not acceptable. Maybe a minimalistic LSFR with a fixed seed?

Follow-ups: get rid of rand() in the benchmark program, in the UDP proxy, and in the fuzzing programs. Whatever solution we pick for the RSA self-tests is likely acceptable there.

tom-cosgrove-arm commented 2 years ago

Just take a standard open source implementation of rand() such as in https://cvsweb.openbsd.org/src/lib/libc/stdlib/rand.c?rev=1.18&content-type=text/x-cvsweb-markup and give it a suitable name such as mbedtls_test_pseudo_rand()

mpg commented 2 years ago

@tom-cosgrove-arm this particular implementation seems quite specific to OpenBSD (use of arc4random). But I agree with your general point. Actually, in the past I've done more or less this for the EC J-PAKE self-tests.

I quite strongly feel that the new function should not be exposed as part of the library API. Otherwise, whatever amount of warnings we give, I fear some people will end up using it in production. I think it's OK to have one copy in the library (exposed in an internal header), present only when MBEDTLS_SELF_TEST is defined, and then one or more copies in programs.

Note that in the medium/long term, when PSA Crypto becomes mandatory, everyone (programs, self-test functions, test suites) can just use psa_generate_random() and be done with it except when repeatability is desired (which it often isn't, or we wouldn't be using rand() in the first place - the only exception probably is udp_proxy). So, we're mostly talking about an interim solution until we get to that point, and I suggest we don't overthink it.

tom-cosgrove-arm commented 2 years ago

this particular implementation seems quite specific to OpenBSD (use of arc4random)

@mpg Sorry, I should have been more specified - I was suggesting using the deterministic algorithm from that file:

int
rand_r(u_int *seed)
{
    *seed = *seed * 1103515245 + 12345;
    return (*seed & RAND_MAX);
}

I quite strongly feel that the new function should not be exposed as part of the library API

Yes, it should be part of test support, named accordingly (hence my suggestion mbedtls_test_pseudo_rand(), but something like mbedtls_test_unsafe_rand() works too).

repeatability is desired (which it often isn't, or we wouldn't be using rand() in the first place

Ah - I'd assumed we'd only be using rand() where we did want repeatability (known seed to srand()

mpg commented 2 years ago

Ah - I'd assumed we'd only be using rand() where we did want repeatability (known seed to srand()

Yes, and that's what we're doing in udp_proxy. But I think we want to get rid of srand() for the same reasons we want to get rid of rand().

I quite strongly feel that the new function should not be exposed as part of the library API

Yes, it should be part of test support, named accordingly (hence my suggestion mbedtls_test_pseudo_rand(), but something like mbedtls_test_unsafe_rand() works too).

But the problem is, we want to be able to call that function from library functions, namely the various mbedtls_xxx_selftest() functions. And we can't really have the library depend on the test support. So, where should it be defined and declared?

gilles-peskine-arm commented 2 years ago

Note that in the medium/long term, when PSA Crypto becomes mandatory, everyone (programs, self-test functions, test suites) can just use psa_generate_random()

Unfortunately, self-tests cannot use psa_generate_random because they typically need to run before the RNG is initialized. If you have an administrative requirement to include self-tests, they must run before the first use of the algorithm. It's administratively permissible run the AES and CTR_DRBG self-tests, then initialize an RNG using CTR_DRBG, and then run the RSA self-tests, but it's technically awkward to arrange: it's easier to run all the self-tests in one go. Also you might want to perform some RSA signature verification in a bootloader with no entropy source, which requires running the RSA self-tests without an entropy source (this could be solved by separating RSA verification self-tests from RSA private-key self tests, but we don't offer this interface).

gilles-peskine-arm commented 2 years ago

Ah - I'd assumed we'd only be using rand() where we did want repeatability (known seed to srand()

Actually, we mostly don't call srand or call srand(1) which is equivalent to not calling it. Only the UDP proxy uses a seed that's configurable at runtime and we want it to be repeatable (on a given platform, and it's sometimes been annoying that it's not repeatable across platforms). So where we're currently using rand, we either want repeatability or at least don't mind repeatability. (Which is unsurprising since it's always for testing.)