781787 - Make sure libSRTP has enough randomness

Status: RESOLVED WORKSFORME

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

libSRTP seems to want some randomness and by default uses rand(). There
are compilation flags to make it uses system strong randomness.

1. Do we need randomness here?
2. If so, how do we make sure it exists on all platforms

Comment 1

[Eric Rescorla (:ekr)]

Note: I don't believe this is used in anything production yet, but it *is* security related.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

It uses /dev/urandom (or /dev/random), if they exist.  Fallback is to rand_s (Windows) or rand (linux).  I'm guessing Mac follows linux.

Random values appear to ONLY be used in tests (mostly test of randomness), and in aes_128_cbc_hmac_sha1_96_* (in crypto/ae_xfm/xfm.c).  As best I can tell, aes_128_cbc_hmac_sha1_96_* aren't used anywhere in the code.  They use random IVs for encryption; the normal crypto modes of SRTP (AES_ICM, and ) set the IV to the ssrc and the sequence number, both of which are supposed to be randomly initialized outside of srtp (and this is just for the IV; the key itself is passed in).

So, if this analysis is correct, the only thing to check is the generation of initial sequence numbers and SSRC values.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Randell Jesup [:jesup] from comment #2)

> So, if this analysis is correct, the only thing to check is the generation
> of initial sequence numbers and SSRC values.

And to note, those are outside of the SRTP code, in the webrtc code (media/webrtc/trunk). 

The encryption keys (assuming the spec sticks to DTLS-SRTP) come from DTLS.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

It will use /dev/urandom only if we make some change to the tree to cause DEV_URANDOM to be defined at the right times, since we're currently not using the libSRPT configure.in that would normally do that, at least in the non-cross-compiling case.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

No reason for this to be security-sensitive.  All that really needs doing here is to make an upstream change to disable the random number test

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to Randell Jesup [:jesup] from comment #5)
> All that really needs doing here is to make an upstream change to disable the random number test

Randell: is this still the case? I just filed (duplicate) bug 1081434 because I saw "WARNING: no real random source present!" on stderr while making a Loop call.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Upstream has changed a fair bit; we should consider choosing a stable point to grab from. There are some other real fixes (though nothing major, and perhaps not affecting how we use it at all).

However, per above, the warning is totally spurious for our usage, so there's little urgency.

Comment 9

[Nils Ohlmeier [:drno]]

Attachment: Bug 781787: turn on SRTP rand_s support for Windows

Review commit: https://reviewboard.mozilla.org/r/202988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202988/

Comment 10

[Nils Ohlmeier [:drno]]

With the landing of bug 1230759 the warning on the command line is gone, because the rng code has dis-appeared from the upstream code base.

I did check that the calls to rand/rand_s are used to only verify that the cipher algorithms pointed to internally in the library appear to generate random output (e.g. to prevent that an implementation points to a null cipher for encryption).

Having said that I find it very strange that there is only a call to rand(), but nowhere any proper initialization via srand(). I'm not sure if that is on purpose or an oversight.

Comment 11

[Chris Peterson [:cpeterson]]

If we enable HAVE_RAND_S, we should keep a close eye looking for new crash reports coming from rand_s. We've had spikes of rand_s crashes over the years caused by anti-virus and malware software hooks in advapi32.dll:

bug 694344
bug 723447
bug 1167248
bug 1240589
bug 1369361
bug 1409444

Comment 12

[Nils Ohlmeier [:drno]]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> If we enable HAVE_RAND_S, we should keep a close eye looking for new crash
> reports coming from rand_s. We've had spikes of rand_s crashes over the
> years caused by anti-virus and malware software hooks in advapi32.dll:

Interesting. I was not aware of that. Since the code is falling back to the generic rand() that makes me wonder if the anti-virus software leaves that alone, or did these just unnoticed?

I though the missing HAVE_RAND_S was just a simple oversight from when we switched from configure to moz.build. I don't feel strong to turn it on.

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Nils Ohlmeier [:drno] from comment #12)
> Interesting. I was not aware of that. Since the code is falling back to the
> generic rand() that makes me wonder if the anti-virus software leaves that
> alone, or did these just unnoticed?

I don't think the anti-virus software is purposely intercepting rand_s(). I think they are intercepting loads of advapi32.dll and rand_s() just happens to be a function in advapi32.dll that is called early. rand() is in a different DLL.

> I though the missing HAVE_RAND_S was just a simple oversight from when we
> switched from configure to moz.build. I don't feel strong to turn it on.

IIUC, rand_s() is only called by srtp_cipher_rand(), which is only called by srtp_cipher_type_test(), which is only used to self-test that SRTP ciphers can correctly encrypt and decrypt some random data. If that is the case, then the random data doesn't need to "super random" and regular rand() is probably good enough.

Comment 14

[Nils Ohlmeier [:drno]]

With the landing of bug 1479665 libsrtp now uses cipher algorithm from NSS. Plus the fact that multiple people verified that the randomness was only ever used in the self tests I think we can close this.

If anyone should dis-agree feel free to re-open.