From be5422db22a496b1a463d141c1d093a46f056f9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 22 Feb 2024 12:31:07 +0100 Subject: [PATCH 1/6] Fix DTLS fragment exceeds MTU size if the certificate is large Fixes #1100 ### Details I cannot believe that what I've done works. So let me first explain what the problem is (which is perfectly explained in issue #1100), I hope I'm right: As commented in https://github.com/versatica/mediasoup/issues/1100#issuecomment-1959002237, the problem we had is: - When it comes to send DTLS data to the remote client, `DtlsTransport::SendPendingOutgoingDtlsData()` in mediasoup calls `read = BIO_get_mem_data(...)`, and obtained `read` maybe higher than our `DtlsMtu = 1350` (for example in DTLS Server Hello message if our certificate is very large). Let's say that `read` is 3000 bytes which is higher than 1350 so problems here. - But we enable all the MTU related stuff in OpenSSL DTLS in `DtlsTransport` so we **know** (and this is proven, see https://github.com/versatica/mediasoup/issues/1100#issuecomment-1959002237) that those 3000 bytes contain **N fragments** of a DTLS packet (in this case Server Hello). - But we don't know how long each fragment is so we cannot read and send them separately. And we don't know it because `read = BIO_get_mem_data(...)` returns the **total** size as explained above. What does this PR? - In `DtlsTransport::SendPendingOutgoingDtlsData()` we call `read = BIO_get_mem_data(...)` as usual. - If `read <= DtlsMtu` then nothing changes, we notify the listener with the full data. - If `read > DtlsMtu` then we split the total `data` into fragments of max `DtlsMtu` bytes and notify the parent separately with each of them. - So the parent (`WebRtcTransport`) will send each fragment into a separate UDP datagram. - And for whatever reason it **WORKS!** ``` DtlsTransport::SendPendingOutgoingDtlsData() | data to be sent is longer than DTLS MTU, fragmenting it [len:6987, DtlsMtu:1350] +248ms DtlsTransport::SendPendingOutgoingDtlsData() | sending fragment [offset:0, len:1350] +0ms DtlsTransport::SendPendingOutgoingDtlsData() | sending fragment [offset:1350, len:1350] +0ms DtlsTransport::SendPendingOutgoingDtlsData() | sending fragment [offset:2700, len:1350] +0ms DtlsTransport::SendPendingOutgoingDtlsData() | sending fragment [offset:4050, len:1350] +0ms DtlsTransport::SendPendingOutgoingDtlsData() | sending fragment [offset:5400, len:1350] +0ms DtlsTransport::SendPendingOutgoingDtlsData() | sending fragment [offset:6750, len:237] +0ms ``` An interesting fact is that, if I remove `SSL_OP_NO_QUERY_MTU` flag in the call to `SSL_CTX_set_options()`, then things don't work and the client/browser keeps sending DTLS Client Hello forever, meaning that the DTLS data it's receiving from mediasoup is invalid. So this is magic. How is possible that this works? AFAIU OpenSSL is generating valid DTLS **fragments** but it doesn't expose them to the application separately (see the problem description above). So does this PR work by accident??? Maybe not! Probably OpenSSL is generating DTLS fragments of **exactly** our given `DtlsMtu` size (which BTW makes sense since we call `SSL_set_mtu(this->ssl, DtlsMtu)` and `DTLS_set_link_mtu(this->ssl, DtlsMtu)`), so if we split the total DTLS data to be sent into fragments of `DtlsMtu` **exact** sizes, then we are **indeed** taking **valid** DTLS fragments, so we can send them into separate UDP datagrams to the client. **IS IT???** **DOES IT MAKE ANY SENSE???** IMHO it does, but I did **NOT** expect this to work. --- worker/src/RTC/DtlsTransport.cpp | 37 ++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/worker/src/RTC/DtlsTransport.cpp b/worker/src/RTC/DtlsTransport.cpp index 3bb8a9657c..7af0d6933f 100644 --- a/worker/src/RTC/DtlsTransport.cpp +++ b/worker/src/RTC/DtlsTransport.cpp @@ -1189,10 +1189,8 @@ namespace RTC return; } - int64_t read; char* data{ nullptr }; - - read = BIO_get_mem_data(this->sslBioToNetwork, &data); // NOLINT + const int64_t read = BIO_get_mem_data(this->sslBioToNetwork, &data); // NOLINT if (read <= 0) { @@ -1201,9 +1199,36 @@ namespace RTC 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)); + if (read <= DtlsMtu) + { + // Notify the listener. + this->listener->OnDtlsTransportSendData( + this, reinterpret_cast(data), static_cast(read)); + } + else + { + MS_WARN_DEV( + "data to be sent is longer than DTLS MTU, fragmenting it [len:%" PRIi64 ", DtlsMtu:%i]", + read, + DtlsMtu); + + size_t offset{ 0u }; + + while (offset < read) + { + auto* fragmentData = reinterpret_cast(data + offset); + auto fragmentLen = static_cast(read) - offset >= DtlsMtu + ? DtlsMtu + : static_cast(read) - offset; + + MS_DEBUG_DEV("sending fragment [offset:%zu, len:%zu]", offset, fragmentLen); + + // Notify the listener. + this->listener->OnDtlsTransportSendData(this, fragmentData, fragmentLen); + + offset += fragmentLen; + } + } // Clear the BIO buffer. // NOTE: the (void) avoids the -Wunused-value warning. From 82efb1a6bf33cfae5a78d1d5ec26b9d911f7f1e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 22 Feb 2024 12:43:59 +0100 Subject: [PATCH 2/6] Use MS_WARN_TAG --- worker/src/RTC/DtlsTransport.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/worker/src/RTC/DtlsTransport.cpp b/worker/src/RTC/DtlsTransport.cpp index 7af0d6933f..530f358f24 100644 --- a/worker/src/RTC/DtlsTransport.cpp +++ b/worker/src/RTC/DtlsTransport.cpp @@ -1207,7 +1207,8 @@ namespace RTC } else { - MS_WARN_DEV( + MS_WARN_TAG( + dtls, "data to be sent is longer than DTLS MTU, fragmenting it [len:%" PRIi64 ", DtlsMtu:%i]", read, DtlsMtu); From 35e93163c322a4f50f6ffd768b0d8770b825fbda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 22 Feb 2024 13:05:45 +0100 Subject: [PATCH 3/6] Add a comment --- worker/src/RTC/DtlsTransport.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/worker/src/RTC/DtlsTransport.cpp b/worker/src/RTC/DtlsTransport.cpp index 530f358f24..6f489a64fa 100644 --- a/worker/src/RTC/DtlsTransport.cpp +++ b/worker/src/RTC/DtlsTransport.cpp @@ -1,5 +1,5 @@ #define MS_CLASS "RTC::DtlsTransport" -// #define MS_LOG_DEV_LEVEL 3 +#define MS_LOG_DEV_LEVEL 3 #include "RTC/DtlsTransport.hpp" #include "Logger.hpp" @@ -1207,6 +1207,8 @@ namespace RTC } else { + // Here we do a dangerous thing that works. + // Explained in https://github.com/versatica/mediasoup/pull/1343. MS_WARN_TAG( dtls, "data to be sent is longer than DTLS MTU, fragmenting it [len:%" PRIi64 ", DtlsMtu:%i]", From 8e13c4af11fbd652c878de6c16c53e289f62bfe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 22 Feb 2024 13:06:15 +0100 Subject: [PATCH 4/6] Oppps --- worker/src/RTC/DtlsTransport.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worker/src/RTC/DtlsTransport.cpp b/worker/src/RTC/DtlsTransport.cpp index 6f489a64fa..a741eadb7c 100644 --- a/worker/src/RTC/DtlsTransport.cpp +++ b/worker/src/RTC/DtlsTransport.cpp @@ -1,5 +1,5 @@ #define MS_CLASS "RTC::DtlsTransport" -#define MS_LOG_DEV_LEVEL 3 +// #define MS_LOG_DEV_LEVEL 3 #include "RTC/DtlsTransport.hpp" #include "Logger.hpp" From 0dc7dce2a99c0a13a34e95c9663b98b7fd5a46de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 22 Feb 2024 14:46:48 +0100 Subject: [PATCH 5/6] Add proper inline documentation --- worker/src/RTC/DtlsTransport.cpp | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/worker/src/RTC/DtlsTransport.cpp b/worker/src/RTC/DtlsTransport.cpp index a741eadb7c..d5d3e3c72d 100644 --- a/worker/src/RTC/DtlsTransport.cpp +++ b/worker/src/RTC/DtlsTransport.cpp @@ -1207,9 +1207,32 @@ namespace RTC } else { - // Here we do a dangerous thing that works. - // Explained in https://github.com/versatica/mediasoup/pull/1343. - MS_WARN_TAG( + // Here we have a DTLS message with total size higher than our DtlsMtu + // value. Such a big DTLS message is, in fact, various DTLS message + // fragments. Each DTLS message fragment must be sent in a single + // UDP datagram or TCP framed message (althought N DTLS message fragments + // can be sent together because they are framed). So the question is: how + // to split this big data we have here into valid DTLS message fragments? + // + // Here the trick: + // - We called SSL_CTX_set_options() with SSL_OP_NO_QUERY_MTU (among other + // flags). + // - We called SSL_set_mtu(this->ssl, DtlsMtu). + // - We called DTLS_set_link_mtu(this->ssl, DtlsMtu). + // + // So we know that OpenSSL will split DTLS messages bigger than DtlsMtu + // into DtlsMtu long chunks (of course the last chunk maybe smaller). + // So assuming that (and it behaves that way), we can do the reverse + // operation here and split this big data into chunks of DtlsMtu (except + // the last one, of couse), so it's guaranteed that each chunk contains + // a valid DTLS message fragment. So we notify the parent by passing each + // chunk to it, so it will encapsulate it into a single UDP datagram or + // framed TCP message. + // + // PR with more information about this: + // https://github.com/versatica/mediasoup/pull/1343. + + MS_DEBUG_TAG( dtls, "data to be sent is longer than DTLS MTU, fragmenting it [len:%" PRIi64 ", DtlsMtu:%i]", read, From 60103c49af639223c94312812867006d89d6a93f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Thu, 22 Feb 2024 15:42:52 +0100 Subject: [PATCH 6/6] Improve inline doc --- worker/src/RTC/DtlsTransport.cpp | 61 ++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 14 deletions(-) diff --git a/worker/src/RTC/DtlsTransport.cpp b/worker/src/RTC/DtlsTransport.cpp index d5d3e3c72d..40e4f0d2e5 100644 --- a/worker/src/RTC/DtlsTransport.cpp +++ b/worker/src/RTC/DtlsTransport.cpp @@ -832,9 +832,9 @@ namespace RTC } MS_DUMP(""); - MS_DUMP(" state : %s", state.c_str()); - MS_DUMP(" role : %s", role.c_str()); - MS_DUMP(" handshake done: : %s", this->handshakeDone ? "yes" : "no"); + MS_DUMP(" state: %s", state.c_str()); + MS_DUMP(" role: %s", role.c_str()); + MS_DUMP(" handshake done: %s", this->handshakeDone ? "yes" : "no"); MS_DUMP(""); } @@ -1207,12 +1207,13 @@ namespace RTC } else { - // Here we have a DTLS message with total size higher than our DtlsMtu - // value. Such a big DTLS message is, in fact, various DTLS message - // fragments. Each DTLS message fragment must be sent in a single - // UDP datagram or TCP framed message (althought N DTLS message fragments - // can be sent together because they are framed). So the question is: how - // to split this big data we have here into valid DTLS message fragments? + // Here we have data containing one or more DTLS messages with total size + // higher than our DtlsMtu value. These DTLS messages are, in fact, DTLS + // message fragments (various fragments conform a DTLS message). Each DTLS + // message fragment must be sent in a single UDP datagram or TCP framed + // message (although various DTLS message fragments can be sent together + // because they are framed). So the question is: How to split this big + // data we have here into valid DTLS message fragments? // // Here the trick: // - We called SSL_CTX_set_options() with SSL_OP_NO_QUERY_MTU (among other @@ -1221,14 +1222,46 @@ namespace RTC // - We called DTLS_set_link_mtu(this->ssl, DtlsMtu). // // So we know that OpenSSL will split DTLS messages bigger than DtlsMtu - // into DtlsMtu long chunks (of course the last chunk maybe smaller). - // So assuming that (and it behaves that way), we can do the reverse - // operation here and split this big data into chunks of DtlsMtu (except - // the last one, of couse), so it's guaranteed that each chunk contains - // a valid DTLS message fragment. So we notify the parent by passing each + // into DtlsMtu bytes long chunks (of course the last chunk maybe smaller). + // So assuming that (and it behaves that way), we can follow the reverse + // logic here and split this big data into chunks of DtlsMtu (except the + // last one, of couse), so it's guaranteed that each chunk will contain a + // valid DTLS message fragment. So we notify the parent by passing each // chunk to it, so it will encapsulate it into a single UDP datagram or // framed TCP message. // + // There is an scenario in which this logic would fail: + // + // - DtlsMtu is 1350 bytes. + // - OpenSSL generates 2 sequential DTLS messages to be sent to client. + // - First message is 2500 bytes long. So OpenSSL splits it into 2 DTLS + // message fragments: + // 1. First DTLS message fragment is 1350 (DtlsMtu) bytes long. + // 2. Second DTLS message fragment is 1150. + // - Second message is 500 bytes long (so no fragments needed). + // - So in SendPendingOutgoingDtlsData() we have 3000 bytes to send in + // total and there are 3 DTLS messages: + // 1. The first fragment of the first message: 1350 bytes. + // 2. The second fragment of the first message: 1150 bytes. + // 3. The second DTLS message: 500 bytes. + // - Following the above logic, SendPendingOutgoingDtlsData() will split + // those 3000 bytes as follows: + // 1. First 1350 bytes: Here we are exactly taking the first fragment of + // the first message, so all good. + // 2. Next 1350 bytes: Here we are reading the 1150 bytes of the second + // fragment of the first message plus the first 200 bytes of the + // second message. This is NOT good. + // 3. Next 300 bytes: The remaining 300 bytes of the second message. + // This is not good. + // - Client will reject the second and third UDP packets since they do not + // contain one or more valid DTLS full messages or DTLS message + // fragments. + // + // However, by experimenting I see that OpenSSL doesn't generate messages + // like those and, anyway, we may only need to send DTLS messages bigger + // than DtlsMtu during the handshake and only if our certificate is big, + // and in this scenario the problematic above sequence doesn't happen. + // // PR with more information about this: // https://github.com/versatica/mediasoup/pull/1343.