Segfault when NULL is returned from SSL_get_selected_srtp_profile

[mqp]

I've experienced a number of segfaults coming out of janus_dtls_srtp_incoming_msg, here: https://github.com/meetecho/janus-gateway/blob/master/dtls.c#L737

Janus commit from recent master (3d0d248e9f8be4ed793c881513e7e87cb431d19e), OpenSSL v1.0.2g, libsrtp v2.1.0, Ubuntu 17.10

My core dumps inform me that srtp_profile is NULL, causing a crash immediately thereafter. The documentation seems to think that NULL means that no profile has been negotiated, but I'm not informed enough about how the DTLS handshake and code works to understand in what cases that might occur, yet, or what to do about it.

So far I don't know how to reproduce the issue -- I'll keep trying.

[lminiero]

The DTLS handshake in this case is supposed to negotiate which SRTP profile to agree upon, e.g., AES_CM_128_HMAC_SHA1_32 or others. It should really never return NULL, as Janus offers something that should always succeed:

https://github.com/meetecho/janus-gateway/blob/master/dtls.c#L351

We recently added support for AES-GCM as well, but it's only enabled if your OpenSSL supports it, and again it's not used anyway if the browser doesn't support it (it's only available in Chrome behind a flag):

https://github.com/meetecho/janus-gateway/blob/master/dtls.c#L349

It was probably my mistake to assume it would never be NULL, so we'll have to add a check for that. Which devices/endpoints caused the crashes in your experience?

[mqp]

I don't know yet what devices might be causing the problem. I'm hosting a public instance that restarts itself if it dies, so I only find out about the problem later by looking to see whether any core dumps have popped up.

My OpenSSL on the server doesn't support AES-GCM right now and Janus doesn't advertise it. I'm pretty sure of that because I was looking into it as part of testing cipher suites for performance the other day.

I'll let you know if I can find more info -- maybe I can look at the logs and figure out something about what the client's opinion of the negotiation was.

[lminiero]

Added a check so this is at least catched.

[lminiero]

Thinking about this, I guess this might happen if the client simply initiates a DTLS connection but never exchanges SRTP crypto information: in that case in older versions it would have caused SSL_export_keying_material to fail (but not crash), while we now check the SRTP profile as it dictates how many bytes we must export.

[mqp]

Interesting, that sounds like something that might be feasible to simulate. Thanks for adding the graceful handling, that's just what I was going to do.

[lminiero]

Closing as this should be fixed, please let me know if it's still an issue.