Replacing g_random_int() with openssl RAND_bytes()

[Jonathan Müller]

Hi

I recently stumbled upon this: https://github.com/meetecho/janus-gateway/blob/422e26dacd34ee22083ae77673fcc5af7a3b098f/utils.c#L77

janus_random_uint{32, 64}() are not used to generate secrets, but still, it would be nicer to generate user-exposed random numbers in a crypto-safe way, so for example users cannot gain insight about internal state.

The obvious way to do this is RAND_bytes() from openssl (as the comment also states). It is already in use here: https://github.com/meetecho/janus-gateway/blob/2d5f2be38b9c19d7affa45a929d7b7e5596dc6aa/rtp.c#L690

I'd be happy to create a PR for this. Before I start, some considerations:

• Do you think it makes sense to implement this?
  
• OpenSSL random init: The OpenSSL wiki and the man pages say manual seeding is not needed.
  • but RAND_poll() is being called in Janus plugins, e.g. here. However the return code is not checked...
  • Maybe it would be better to just check RAND_status() on Janus startup (in janus.c) and remove the rand_poll() calls from the plugins.
• How to do error handling? Even if RAND_status() checks out at startp, RAND_bytes() can of course still fail. So I guess we'd make janus_random_uint{32, 64}() return a status code, and handle that where ever it is used? Or just check-and-crash in janus_random_uint().
  

Then, another thing: The comment here says "up to 2^53", but it is actually up to 2^53-1.

Best

Jonathan

[Alessandro Toppi]

That makes sense to me, since we are already using the crypto methods for core operations (srtp_crypto_get_random).

Ideally the PR should just change the implementation of the janus_random_uint  and not the interfaces or behaviors but the observation about RAND_bytes failure is remarkable.

I guess the check of RAND_status() at startup and the removal of manual reseeding in the plugins is a good starting point.

Anyway all the changes should be done very carefully in order e.g. to not break fuzzers.

IMHO this discussion should be moved to the repo.

[Lorenzo Miniero]

Reimplementing the random-int methods would make sense to me only if they don't impact the CPU too much. We don't use those for anything critical (random session IDs, random SSRCs, a few other things), but can can use them a lot, so if cryptographically secure values require much more CPU time I'm not sure it's worth it. Maybe we could have two versions, one more secure and one that's not, depending on the target? (probably a silly idea) That said, I'm definitely open to discuss these changes in a PR!

Lorenzo