Use crypto safe random numbers

[jmfotokite]

This is a continuation of the discussion in https://groups.google.com/g/meetecho-janus/c/dIVe_v1xaxM.

It is not yet a ready-to-merge PR, more of a suggestion.

Some considerations:

• creates a new janus_random_uint64_javacript_safe() method to make it explicit that those numbers are safe to use in JS
• but do use janus_random_uint64() in the other places (like when generating an UUID)
• make "crypto safe random" optional? This might sound like a good idea at first, because it can theoretically fail (unlike the pseudo random g_random_int()). BUT:
  • it is easier to have only one behavior in the code
  • pretty much any system in use nowadays depends on having working crypto safe random numbers, a lot stuff would not work otherwise (SSL etc)
  • the same is true for Janus, e.g. the DTLS implementation
• in terms of CPU usage, I doubt that this is an issue. We are not using random stuff at a great rate, and a quick test shows that, on my Thinkpad, I can generate "good" random data quickly enough: cat /dev/urandom | pv > /dev/null --> 0:00:05 [ 197MiB/s] [ <=> ]

[lminiero]

Thanks! A few comments:

• I don't think we need a janus_random_uint64_javacript_safe, as it's a breaking signature name change for something that won't be really used: you just updated the core, but we use random uint64 values all over the plugins; besides, instead than modifying the call name EVERYWHERE, I'd rather the method defaults to capping at 2^53-1, and maybe have another method (with a different name) with no cap instead, in case it's needed by anything (nothing that comes to mind)
• the automated build failed a couple of times due to a missing RAND_bytes() availability: this means that there are build configurations we support where this may happen

[lminiero]

the automated build failed a couple of times due to a missing RAND_bytes() availability: this means that there are build configurations we support where this may happen

Look like the problem is mjr2pcap (and possibly other postprocessing tools that are compiled later), that uses these random generators too, is not linked to ssl/crypto, and so fails.

[atoppi]

Thanks for the effort @jmfotokite

1. What if we make crpyto-safe RNG an option when configuring janus, not changing the signatures all over like @lminiero mentioned
2. I agree on the low impact in terms of CPU. This code will be executed only on the signaling path.
3. We need to double-check if the fuzzers are still working after this change

[jmfotokite]

I don't think we need a janus_random_uint64_javacript_safe

OK. We could also name it janus_random_uint52() or something. I just though it was very wrong to have a janus_random_uint64() method which returns only 52 bite. In the end, it is your call of course. I can understand if you do not want to change the signature name because 3rd party plugins depend on it or similar. What should we call the "real" rand64 method then?

What if we make crpyto-safe RNG an option when configuring janus, not changing the signatures all over like lminiero mentioned

Those are two orthogonal things. The crypto-safe RNG has nothing to do with the signature change.

[lminiero]

What should we call the "real" rand64 method then?

No need to be overly creative, I guess something like janus_random_uint64_full() works 🤭

Those are two orthogonal things. The crypto-safe RNG has nothing to do with the signature change.

I agree, it would just change their implementation: we do the same capping to 2^53-1 in the current version as well. In case this configure-level thing is added, though, it would mean not add a dependency to the crypto stuff where it's not needed in some cases though (e.g., mjr2pcap when not using the crypto-safe version).

[atoppi]

My idea was to use a macro defined in configure.
In the (default) case the macro is not defined everything is exactly like it is now.
Conversely, should the macro be defined, the random_uint methods will use the crypto stuff.
In any case the signatures of the involved methods do not change and, as we are already discussing, we could just introduce a new method that will give a full uint64 random in case something would need it.

[jmfotokite]

The signatures are reverted now.

My idea was to use a macro defined in configure.
...
Conversely, should the macro be defined, the random_uint methods will use the crypto stuff.

So, should I add a --enable-crypto-safe-random option to the build config (and adapt the docstrings)?

[atoppi]

@jmfotokite I was just trying to collect feedback.
If @lminiero agrees to moving to crypto-safe implementation, then I guess we do not need the configuration.

[atoppi]

@jmfotokite the linking with libcrypto is missing.
Please apply the attached patch. It should fix compilation for post-processor and fuzzers.

fix_crypto_rng_pr.txt

[jmfotokite]

Thanks! Let's hope it works now 🙏

[atoppi]

For sake of completeness, here is a small benchmark that compares the two approaches

#include <stdio.h>
#include <inttypes.h>
#include <time.h>
#include <glib.h>
#include <openssl/rand.h>

static guint64 janus_random_uint64(void);
static guint64 janus_random_uint64_crypto(void);

guint64 janus_random_uint64(void) {
	guint64 num = g_random_int() & 0x1FFFFF;
	num = (num << 32) | g_random_int();
	return num;
}

guint64 janus_random_uint64_crypto(void) {
	guint64 ret = 0;
	if (RAND_bytes((void *)&ret, sizeof(ret)) != 1) {
		printf("\tOpenSSL RAND_bytes() failed\n");
		exit(1);
	}
	ret = ret & 0x1FFFFFFFFFFFFF;
	return ret;
}

int main(void) 
{	
	const int ITERS=1000000;
	
	clock_t tic = clock();
	for(int i=0; i<ITERS; i++) {
		janus_random_uint64();
		//printf("random1 = %" PRIu64 "\n", val);
	}
	clock_t toc = clock();
	printf("[\tg_random_int\t] : %d iterations took %f seconds\n", ITERS, (double)(toc - tic) / CLOCKS_PER_SEC);
	
	tic = clock();
	for(int j=0; j<ITERS; j++) {
		janus_random_uint64_crypto();
		//printf("random2 = %" PRIu64 "\n", val);
	}
	toc = clock();
	printf("[\tRAND_bytes\t] : %d iterations took %f seconds\n", ITERS, (double)(toc - tic) / CLOCKS_PER_SEC);
} 

Iterating the RNG functions 100,000 times:

[	g_random_int	] : 1000000 iterations took 0.042884 seconds
[	RAND_bytes	] : 1000000 iterations took 1.255055 seconds

So yeah, the crypto-safe functions have more impact on the CPU, but the time needed to sample a value is still very low (0,0125 ms on average).

[jmfotokite]

Wow, I did not expect a 30x slowdown... but yes, we are actually not using janus_random_uint64() a lot, so that's fine.

[atoppi]

I've made some tests with echotest, videoroom and streaming.
Everything seems to work as expected.

[lminiero]

Just added some notes inline, thanks again!

[lminiero]

Looks good, thanks! I just added a note on logging, and then it's good to go for me.

[jmfotokite]

(obviously this dirty commit history should be squash-merged)

[lminiero]

Thanks, this looks good to me now! Merging then, thanks for the huge work 👍