roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.06k stars 213 forks source link

Add OpenSSL dependency #364

Closed gavv closed 8 months ago

gavv commented 4 years ago

Problem

Earlier we implemented CSPRNG using libuv (#102).

Now we're slowly removing libuv dependency. On the other hand, we will likely employ OpenSSL for DTLS support (#318), and OpenSSL provides CSPRNG as well, which we could use.

Acceptance criteria

The following solution is suggested:

Moved to separate task:

Info

About target directories: https://roc-streaming.org/toolkit/docs/internals/code_structure.html#targets

An example of adding a new dependency can be found in #246 and #265.

Implementation

jsomwaru commented 3 years ago

You think I can take this?

gavv commented 3 years ago

You're welcome!

jsomwaru commented 3 years ago

If openssl is enabled should I remove target_libuv as target directory? Is the current target_libuv not supposed to have any conflicts with the OpenSSL target directory and why you specified to create the new tartget_libuv_csprn?

gavv commented 3 years ago

Hi, sorry for late reply.

If openssl is enabled should I remove target_libuv as target directory?

No, we have a lot of other code using libuv except PRNG implementation.

Is the current target_libuv not supposed to have any conflicts with the OpenSSL target directory and why you specified to create the new tartget_libuv_csprn?

The only conflict will be secure_random().

If user specified --disable-csprng, we'll use secure_random from target_nocsprng. Otherwise, if OpenSSL is enabled, we'll use secure_random from target_openssl. Otherwise, if libuv version is at least 1.33.0, we'll use secure_random from target_libuv_csprng. Otherwise, build will fail.

At the same time, target_libuv is always enabled and we use other code from it.

adrianopol commented 1 year ago

Hi, I may take this one.

gavv commented 1 year ago

Awesome

gavv commented 1 year ago

I've updated task description

gavv commented 1 year ago

@adrianopol We also have benchmarks for secure_random (bench_random.cpp). It would be interesting to compare performance before and after integrating OpenSSL.

gavv commented 1 year ago

Summary on OpenSSL versions:

https://www.openssl.org/policies/releasestrat.html

gavv commented 1 year ago

I've temporary removed secure_random() because it caused regression on some ARM boards. See details in this commit: 729ba8bd42ec86028021ce9f66303e902c62d2ae

In this issue, we'll have to re-implement secure_random() using OpenSSL, re-integrate it into the three classes where it was used, and re-add test & benchmark; basically we can just revert that commit and replace implementation.

It's important that the new implementation should be non-blocking and should handle lack of entropy in kernel (no matter temporary or permanent). When there is no entropy, it should fall back to fast_random.

We also should consider using kernel entropy only as as seed instead of reading it each time; probably OpenSSL already does it for us?

zx2c4 commented 1 year ago

There's no need to use OpenSSL here. Just use getrandom(0). If you are satisfied with non cryptographic usage, you can use getrandom(GRND_INSECURE) to never block at boot. But recent kernels shouldn't have a starvation problem anyway, so getrandom(0) is probably fine.

gavv commented 1 year ago

@zx2c4 Hi! Yep, we could use it, but it would be non-POSIX (e.g. apple does not support it IIRC), so we would need to craft several implementations.

On the other hand, we anyway need OpenSSL for DTLS and (partially) SRTP, so why not to use is for this as well? It already has all necessary ifdefs for platform-specific stuff.

But recent kernels shouldn't have a starvation problem anyway, so getrandom(0) is probably fine.

Oh, great. Seems there are also workarounds, I didn't know.

Though, I'd still prefer to support older kernels too without need for additional configuration. E.g. many people have old SBCs or vendor distros. One of my boxes have this problem it and it's accumulating entropy very slowly, I guess because it almost doesn't have peripherals.

zx2c4 commented 1 year ago

macOS and all the BSDs and glibc and such all have getentropy(). Recent glibc on Linux has arc4random()which also macOS and all the BSDs have. So you could use one of those.

Or, just read from /dev/urandom as a fallback?

gavv commented 1 year ago

I digged into it a bit more and it seems that OpenSSL does not expose a non-blocking random query. It has RAND_status which is intended to check whether PRNG was seeded, but it seems it also invokes blocking version of getrandom(), so it wont fix starvation problem.

On the other hand, using OpenSSL still seems to be a more practical approach:


Then, given a second thought to the problem with starvation, it seems that silently switching to insecure RNG is not a good idea (I guess it may be considered as a vulnerability?)

Initially, we need CSPRNG to fulfill RTP requirements, which are actually relevant only when RTP is encrypted (e.g. inside DTLS or SRTP). On the other hand, if we use DTLS or SRTP, we anyway need to wait for entropy.

So, a better fix for this problem would be the following:

Benefits of this approach:

zx2c4 commented 1 year ago

it calls kernel only once to seed and then uses user-space CSPRNG, which seems much better approach (we don't need to go to kernel each time; we don't waste system entropy which is probably generating slowly on some embedded systems)

This part is very much false. User-space CSPRNGs have generally worse security characteristics. And there is no such thing as "wasting system entropy". Once initialized, you can use getrandom() or /dev/urandom as much as you want, with no negative effects.

e.g. getentropy is usually available on Linux but not if we're using uClibc, which instead has arc4random

glibc now has arc4random too. (It's just a wrapper around getrandom()). So maybe just use that?

gavv commented 1 year ago

This part is very much false. User-space CSPRNGs have generally worse security characteristics. And there is no such thing as "wasting system entropy". Once initialized, you can use getrandom() or /dev/urandom as much as you want, with no negative effects.

Oh I see. Does it mean that if N processes read /dev/(u)random, it does not matter what is N and additional readers wont reduce amount of high quality entropy received by others?

glibc now has arc4random too. (It's just a wrapper around getrandom()). So maybe just use that?

Seems that's a very recent addition.. Even my debian stable does not have it yet. Also not sure about musl. Or e.g. qnx :)

Yeah I understand that getrandom and arc4random will cover most of the cases and we can have fallback(s), but we still will need a bunch of ifdefs, and on the other hand why bother at all if OpenSSL already has them all?

BTW here are ifdefs from libuv: https://github.com/libuv/libuv/blob/47e0c5c575e92a25e0da10fc25b2732942c929f3/src/random.c

zx2c4 commented 1 year ago

Oh I see. Does it mean that if N processes read /dev/(u)random, it does not matter what is N and additional readers wont reduce amount of high quality entropy received by others?

Correct.

BTW here are ifdefs from libuv

wireguard-tools is fairly lowkey: https://git.zx2c4.com/wireguard-tools/tree/src/genkey.c#n31

gavv commented 1 year ago

wireguard-tools is fairly lowkey: https://git.zx2c4.com/wireguard-tools/tree/src/genkey.c#n31

Nice!

adrianopol commented 1 year ago

Output of ./build/3rdparty/x86_64-pc-linux-gnu/gcc-11.3.0-release/openssl-3.0.7/src/openssl-3.0.7/Configure LIST:

``` BC-32 BS2000-OSD BSD-aarch64 BSD-generic32 BSD-generic64 BSD-ia64 BSD-riscv64 BSD-sparc64 BSD-sparcv8 BSD-x86 BSD-x86-elf BSD-x86_64 Cygwin Cygwin-i386 Cygwin-i486 Cygwin-i586 Cygwin-i686 Cygwin-x86 Cygwin-x86_64 DJGPP MPE/iX-gcc OS390-Unix UEFI UEFI-x86 UEFI-x86_64 UWIN VC-CE VC-WIN32 VC-WIN32-ARM VC-WIN32-ARM-UWP VC-WIN32-ONECORE VC-WIN32-UWP VC-WIN64-ARM VC-WIN64-ARM-UWP VC-WIN64A VC-WIN64A-ONECORE VC-WIN64A-UWP VC-WIN64A-masm VC-WIN64I aix-cc aix-gcc aix64-cc aix64-gcc aix64-gcc-as android-arm android-arm64 android-armeabi android-mips android-mips64 android-x86 android-x86_64 android64 android64-aarch64 android64-mips64 android64-x86_64 bsdi-elf-gcc cc darwin-i386 darwin-i386-cc darwin-ppc darwin-ppc-cc darwin64-arm64 darwin64-arm64-cc darwin64-ppc darwin64-ppc-cc darwin64-x86_64 darwin64-x86_64-cc gcc haiku-x86 haiku-x86_64 hpux-ia64-cc hpux-ia64-gcc hpux-parisc-cc hpux-parisc-gcc hpux-parisc1_1-cc hpux-parisc1_1-gcc hpux64-ia64-cc hpux64-ia64-gcc hpux64-parisc2-cc hpux64-parisc2-gcc hurd-x86 ios-cross ios-xcrun ios64-cross ios64-xcrun iossimulator-xcrun iphoneos-cross irix-mips3-cc irix-mips3-gcc irix64-mips4-cc irix64-mips4-gcc linux-aarch64 linux-alpha-gcc linux-aout linux-arm64ilp32 linux-armv4 linux-c64xplus linux-elf linux-generic32 linux-generic64 linux-ia64 linux-latomic linux-mips32 linux-mips64 linux-ppc linux-ppc64 linux-ppc64le linux-sparcv8 linux-sparcv9 linux-x32 linux-x86 linux-x86-clang linux-x86_64 linux-x86_64-clang linux32-s390x linux64-loongarch64 linux64-mips64 linux64-riscv64 linux64-s390x linux64-sparcv9 mingw mingw64 nonstop-nse nonstop-nse_64 nonstop-nse_64_put nonstop-nse_g nonstop-nse_g_tandem nonstop-nse_put nonstop-nse_spt nonstop-nse_spt_floss nonstop-nsv nonstop-nsx nonstop-nsx_64 nonstop-nsx_64_put nonstop-nsx_g nonstop-nsx_g_tandem nonstop-nsx_put nonstop-nsx_spt nonstop-nsx_spt_floss sco5-cc sco5-gcc solaris-sparcv7-cc solaris-sparcv7-gcc solaris-sparcv8-cc solaris-sparcv8-gcc solaris-sparcv9-cc solaris-sparcv9-gcc solaris-x86-gcc solaris64-sparcv9-cc solaris64-sparcv9-gcc solaris64-x86_64-cc solaris64-x86_64-gcc tru64-alpha-cc tru64-alpha-gcc uClinux-dist uClinux-dist64 unixware-2.0 unixware-2.1 unixware-7 unixware-7-gcc vms-alpha vms-alpha-p32 vms-alpha-p64 vms-ia64 vms-ia64-p32 vms-ia64-p64 vms-x86_64 vos-gcc vxworks-mips vxworks-ppc405 vxworks-ppc60x vxworks-ppc750 vxworks-ppc750-debug vxworks-ppc860 vxworks-ppcgen vxworks-simlinux ```
adrianopol commented 1 year ago

Hello @zx2c4 , thank you for your proposals! As @gavv has already said, getting random numbers is only the first functionality needed from OpenSSL; we also plan to use TLS and stuff in future.

Do you have any feasible complains about using this library in the project? I remember that several years ago OpenSSL suffered from it's very unclean code base which led to a row of high-severity CVE's. Some guys even forked it as LibreSSL in order to remove the most dangerous parts. Actually, I stopped monitoring the situation about security status of OpenSSL and its improvements after v1.1.1.

zx2c4 commented 1 year ago

In general, I would stay away from OpenSSL for RNG functionality and just use getrandom()/getentropy(). Or if you want a /dev/urandom fallback, do what https://git.zx2c4.com/wireguard-tools/tree/src/genkey.c#n31 does. If you're eventually going to add TLS, you can evaluate which TLS library to use at that time, but even then, just use boring old getrandom()/getentropy() for random bytes.

gavv commented 1 year ago

@adrianopol I've added OpenSSL to _distfiles

adrianopol commented 1 year ago

PR: #524

adrianopol commented 1 year ago

@adrianopol I've added OpenSSL to _distfiles

great, do I need to change anything?

gavv commented 1 year ago

great, do I need to change anything?

Nope

gavv commented 8 months ago

Fixed everything related to scons & CI, will open a new task for the function itself.