From 9af2a269cf4407b6446011450fc9e3e78d46cadc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Tue, 27 Feb 2024 17:02:06 +0100 Subject: [PATCH] Fix DTLS packets do not honor configured DTLS MTU (attempt 3) (#1345) --- CHANGELOG.md | 4 + worker/include/RTC/DtlsTransport.hpp | 3 +- worker/src/RTC/DtlsTransport.cpp | 138 ++++++++++++++++----------- 3 files changed, 86 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3404784291..e24f5e536c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +### Next + +- Fix DTLS packets do not honor configured DTLS MTU (attempt 3) ([PR #1345](https://github.com/versatica/mediasoup/pull/1345)). + ### 3.13.22 - Fix wrong publication of mediasoup NPM 3.13.21. diff --git a/worker/include/RTC/DtlsTransport.hpp b/worker/include/RTC/DtlsTransport.hpp index 0d50c3eb07..27f4bd847f 100644 --- a/worker/include/RTC/DtlsTransport.hpp +++ b/worker/include/RTC/DtlsTransport.hpp @@ -152,6 +152,8 @@ namespace RTC return this->localRole; } void SendApplicationData(const uint8_t* data, size_t len); + // This method must be public since it's called within an OpenSSL callback. + void SendDtlsData(const uint8_t* data, size_t len); private: bool IsRunning() const @@ -173,7 +175,6 @@ namespace RTC } void Reset(); bool CheckStatus(int returnCode); - void SendPendingOutgoingDtlsData(); bool SetTimeout(); bool ProcessHandshake(); bool CheckRemoteFingerprint(); diff --git a/worker/src/RTC/DtlsTransport.cpp b/worker/src/RTC/DtlsTransport.cpp index f85d108c94..b3458c901b 100644 --- a/worker/src/RTC/DtlsTransport.cpp +++ b/worker/src/RTC/DtlsTransport.cpp @@ -47,15 +47,49 @@ inline static void onSslInfo(const SSL* ssl, int where, int ret) static_cast(SSL_get_ex_data(ssl, 0))->OnSslInfo(where, ret); } +/** + * This callback is called by OpenSSL when it wants to send DTLS data to the + * endpoint. Such a data could be a full DTLS message, various DTLS messages, + * a DTLS message fragment, various DTLS message fragments or a combination of + * these. It's guaranteed (by observation) that |len| argument corresponds to + * the entire content of our BIO mem buffer |this->sslBioToNetwork| and it + * never exceeds our |DtlsMtu| limit. + */ +inline static long onSslBioOut( + BIO* bio, + int operationType, + const char* argp, + size_t len, + int /*argi*/, + long /*argl*/, + int ret, + size_t* /*processed*/) +{ + long resultOfcallback = (operationType == BIO_CB_RETURN) ? static_cast(ret) : 1; + + // This callback is called twice for write operations: + // - First one with operationType = BIO_CB_WRITE. + // - Second one with operationType = BIO_CB_RETURN | BIO_CB_WRITE. + // We only care about the former. + if ((operationType == BIO_CB_WRITE) && argp && len > 0) + { + auto* dtlsTransport = reinterpret_cast(BIO_get_callback_arg(bio)); + + dtlsTransport->SendDtlsData(reinterpret_cast(argp), len); + } + + return resultOfcallback; +} + inline static unsigned int onSslDtlsTimer(SSL* /*ssl*/, unsigned int timerUs) { - if (timerUs == 0) + if (timerUs == 0u) { - return 100000; + return 100000u; } - else if (timerUs >= 4000000) + else if (timerUs >= 4000000u) { - return 4000000; + return 4000000u; } else { @@ -71,15 +105,15 @@ namespace RTC static constexpr int DtlsMtu{ 1350 }; static constexpr int SslReadBufferSize{ 65536 }; // AES-HMAC: http://tools.ietf.org/html/rfc3711 - static constexpr size_t SrtpMasterKeyLength{ 16 }; - static constexpr size_t SrtpMasterSaltLength{ 14 }; + static constexpr size_t SrtpMasterKeyLength{ 16u }; + static constexpr size_t SrtpMasterSaltLength{ 14u }; static constexpr size_t SrtpMasterLength{ SrtpMasterKeyLength + SrtpMasterSaltLength }; // AES-GCM: http://tools.ietf.org/html/rfc7714 - static constexpr size_t SrtpAesGcm256MasterKeyLength{ 32 }; - static constexpr size_t SrtpAesGcm256MasterSaltLength{ 12 }; + static constexpr size_t SrtpAesGcm256MasterKeyLength{ 32u }; + static constexpr size_t SrtpAesGcm256MasterSaltLength{ 12u }; static constexpr size_t SrtpAesGcm256MasterLength{ SrtpAesGcm256MasterKeyLength + SrtpAesGcm256MasterSaltLength }; - static constexpr size_t SrtpAesGcm128MasterKeyLength{ 16 }; - static constexpr size_t SrtpAesGcm128MasterSaltLength{ 12 }; + static constexpr size_t SrtpAesGcm128MasterKeyLength{ 16u }; + static constexpr size_t SrtpAesGcm128MasterSaltLength{ 12u }; static constexpr size_t SrtpAesGcm128MasterLength{ SrtpAesGcm128MasterKeyLength + SrtpAesGcm128MasterSaltLength }; // clang-format on @@ -708,15 +742,19 @@ namespace RTC goto error; } - SSL_set_bio(this->ssl, this->sslBioFromNetwork, this->sslBioToNetwork); - // Set the MTU so that we don't send packets that are too large with no // fragmentation. - // TODO: This is not honored, see issue: - // https://github.com/versatica/mediasoup/issues/1100 SSL_set_mtu(this->ssl, DtlsMtu); DTLS_set_link_mtu(this->ssl, DtlsMtu); + // We want to monitor OpenSSL write operations into our |sslBioToNetwork| + // buffer so we can immediately send those DTLS bytes (containing full DTLS + // messages, or valid DTLS fragment messages, or combination of them) to + // the endpoint, and hence we honor the configured DTLS MTU. + BIO_set_callback_ex(this->sslBioToNetwork, onSslBioOut); + BIO_set_callback_arg(this->sslBioToNetwork, reinterpret_cast(this)); + SSL_set_bio(this->ssl, this->sslBioFromNetwork, this->sslBioToNetwork); + // Set callback handler for setting DTLS timer interval. DTLS_set_timer_cb(this->ssl, onSslDtlsTimer); @@ -757,7 +795,6 @@ namespace RTC { // Send close alert to the peer. SSL_shutdown(this->ssl); - SendPendingOutgoingDtlsData(); } if (this->ssl) @@ -880,7 +917,6 @@ namespace RTC SSL_set_connect_state(this->ssl); SSL_do_handshake(this->ssl); - SendPendingOutgoingDtlsData(); SetTimeout(); break; @@ -951,9 +987,6 @@ namespace RTC // Must call SSL_read() to process received DTLS data. read = SSL_read(this->ssl, static_cast(DtlsTransport::sslReadBuffer), SslReadBufferSize); - // Send data if it's ready. - SendPendingOutgoingDtlsData(); - // Check SSL status and return if it is bad/closed. if (!CheckStatus(read)) { @@ -1022,9 +1055,31 @@ namespace RTC MS_WARN_TAG( dtls, "OpenSSL SSL_write() wrote less (%d bytes) than given data (%zu bytes)", written, len); } + } + + /** + * This method is called within our |onSslBioOut| callback above. As told + * there, it's guaranteed that OpenSSL invokes that callback with all the + * bytes currently written in our BIO mem buffer |this->sslBioToNetwork| so + * we can safely reset/clear that buffer once we have sent the data to the + * endpoint. + */ + void DtlsTransport::SendDtlsData(const uint8_t* data, size_t len) + { + MS_TRACE(); + + MS_DEBUG_DEV("%zu bytes of DTLS data ready to be sent", len); - // Send data. - SendPendingOutgoingDtlsData(); + // Notify the listener. + this->listener->OnDtlsTransportSendData(this, data, len); + + // Clear the BIO buffer. + auto ret = BIO_reset(this->sslBioToNetwork); + + if (ret != 1) + { + MS_ERROR("BIO_reset() failed [ret:%d]", ret); + } } void DtlsTransport::Reset() @@ -1044,8 +1099,9 @@ namespace RTC this->timer->Stop(); // NOTE: We need to reset the SSL instance so we need to "shutdown" it, but - // we don't want to send a Close Alert to the peer, so just don't call - // SendPendingOutgoingDTLSData(). + // we don't want to send a DTLS Close Alert to the peer. However this is + // gonna happen since SSL_shutdown() will trigger a DTLS Close Alert and + // we'll have our onSslBioOut() callback called to deliver it. SSL_shutdown(this->ssl); this->localRole.reset(); @@ -1069,10 +1125,9 @@ namespace RTC { MS_TRACE(); - int err; const bool wasHandshakeDone = this->handshakeDone; - err = SSL_get_error(this->ssl, returnCode); + int err = SSL_get_error(this->ssl, returnCode); switch (err) { @@ -1182,36 +1237,6 @@ namespace RTC } } - void DtlsTransport::SendPendingOutgoingDtlsData() - { - MS_TRACE(); - - if (BIO_eof(this->sslBioToNetwork)) - { - return; - } - - int64_t read; - char* data{ nullptr }; - - read = BIO_get_mem_data(this->sslBioToNetwork, &data); // NOLINT - - if (read <= 0) - { - return; - } - - MS_DEBUG_DEV("%" PRIu64 " bytes of DTLS data ready to sent to the peer", read); - - // Notify the listener. - this->listener->OnDtlsTransportSendData( - this, reinterpret_cast(data), static_cast(read)); - - // Clear the BIO buffer. - // NOTE: the (void) avoids the -Wunused-value warning. - (void)BIO_reset(this->sslBioToNetwork); - } - bool DtlsTransport::SetTimeout() { MS_TRACE(); @@ -1684,9 +1709,6 @@ namespace RTC if (ret == 1) { - // If required, send DTLS data. - SendPendingOutgoingDtlsData(); - // Set the DTLS timer again. SetTimeout(); }