commit 3ab702580d7cbfe739740a1bd95c9f0f1bf9ac1a
parent 4dbbb7733ec95063e8b10d88c56c5eb8e524d2bf
Author: Alexandru Marc <amarc@mozilla.com>
Date: Mon, 6 Oct 2025 15:52:52 +0300
Revert "Bug 1932591 - WebRTC DTLS1.3 to enable sending a PQ key share and put the PQ share as the top priority in Nightly/Early Beta r=webrtc-reviewers,djackson,bwc" for causing bc failures @ ServoUtils.h
This reverts commit 172b7fa7a9518648caec7f93524e05a1252731d5.
Diffstat:
4 files changed, 48 insertions(+), 200 deletions(-)
diff --git a/dom/media/webrtc/tests/mochitests/test_peerConnection_glean.html b/dom/media/webrtc/tests/mochitests/test_peerConnection_glean.html
@@ -223,70 +223,7 @@
},
async function checkDtlsKEA() {
- // "media.peerconnection.dtls.enable_pq_hybrid_kex" is enabled by default
- // "send_mlkem_keyshare" enabled in Nightly and Early Beta
- // By default we send 2 key shares, PQ and X25519
- // PQ Key share (ECDH Hybrid) has a higher preference, so it will be chosen as KEA
- const pc1 = new RTCPeerConnection();
- const pc2 = new RTCPeerConnection();
- await gleanResetTestValues();
- // SSL Handshake Key Exchange Algorithm (null=0, rsa=1, dh=2, ecdh=4, ecdh_hybrid=8)
- let keyExchangeValue = await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue() || 0;
- is(keyExchangeValue, 0, "Expected no keyExchange distribution defined");
-
- const stream = await navigator.mediaDevices.getUserMedia({ video: true });
- pc1.addTrack(stream.getTracks()[0]);
-
- await connect(pc1, pc2, 32000, "DTLS connected", true, true);
- // This telemetry happens on STS/socket process
- await gleanFlushChildren();
-
- let count1_0 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["0"] || 0;
- let count1_1 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["1"] || 0;
- let count1_2 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["2"] || 0;
- let count1_4 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["4"] || 0;
- let count1_8 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["8"] || 0;
- is(count1_0, 0, "Expected 0 connections using NULL");
- is(count1_1, 0, "Expected 0 connections using RSA");
- is(count1_2, 0, "Expected 0 connections using DH");
- is(count1_4, 0, "Expected 0 connections using ECDH");
- is(count1_8, 2, "Expected 2 connections using ECDH Hybrid");
- },
-
- async function checkDtlsKEA_DTLSBelow13() {
- // DTLS1.2 does not use Kyber
- // In this case, X25519 (ECDH) key share will be used
- await withPrefs([["media.peerconnection.dtls.version.max", 771]],
- async () => {
- const pc1 = new RTCPeerConnection();
- const pc2 = new RTCPeerConnection();
- await gleanResetTestValues();
- // SSL Handshake Key Exchange Algorithm (null=0, rsa=1, dh=2, ecdh=4, ecdh_hybrid=8)
- let keyExchangeValue = await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue() || 0;
- is(keyExchangeValue, 0, "Expected no keyExchange distribution defined");
-
- const stream = await navigator.mediaDevices.getUserMedia({ video: true });
- pc1.addTrack(stream.getTracks()[0]);
-
- await connect(pc1, pc2, 32000, "DTLS connected", true, true);
- // This telemetry happens on STS/socket process
- await gleanFlushChildren();
-
- let count1_0 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["0"] || 0;
- let count1_1 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["1"] || 0;
- let count1_2 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["2"] || 0;
- let count1_4 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["4"] || 0;
- let count1_8 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["8"] || 0;
- is(count1_0, 0, "Expected 0 connections using NULL");
- is(count1_1, 0, "Expected 0 connections using RSA");
- is(count1_2, 0, "Expected 0 connections using DH");
- is(count1_4, 2, "Expected 2 connections using ECDH");
- is(count1_8, 0, "Expected 0 connections using ECDH Hybrid");
- })},
-
- async function checkDtlsKEA_DTLS13DisablePQ() {
- await withPrefs([["media.peerconnection.dtls.enable_pq_hybrid_kex", false]],
- async () => {
+ // Currently, ssl_grp_ec_curve25519 is the default option (ECDH)
const pc1 = new RTCPeerConnection();
const pc2 = new RTCPeerConnection();
await gleanResetTestValues();
@@ -311,73 +248,7 @@
is(count1_2, 0, "Expected 0 connections using DH");
is(count1_4, 2, "Expected 2 connections using ECDH");
is(count1_8, 0, "Expected 0 connections using ECDH Hybrid");
- })},
-
- async function checkDtlsKEA_DTLS13DisablePQEnablePQShare() {
- // Safety measures, when PQ is disabled, even if the sending ml-kem share is enabled
- // it should not be sent.
- await withPrefs([["media.peerconnection.dtls.enable_pq_hybrid_kex", false],
- ["media.peerconnection.dtls.send_mlkem_keyshare", true]
- ],
- async () => {
- const pc1 = new RTCPeerConnection();
- const pc2 = new RTCPeerConnection();
- await gleanResetTestValues();
- // SSL Handshake Key Exchange Algorithm (null=0, rsa=1, dh=2, ecdh=4, ecdh_hybrid=8)
- let keyExchangeValue = await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue() || 0;
- is(keyExchangeValue, 0, "Expected no keyExchange distribution defined");
-
- const stream = await navigator.mediaDevices.getUserMedia({ video: true });
- pc1.addTrack(stream.getTracks()[0]);
-
- await connect(pc1, pc2, 32000, "DTLS connected", true, true);
- // This telemetry happens on STS/socket process
- await gleanFlushChildren();
-
- let count1_0 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["0"] || 0;
- let count1_1 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["1"] || 0;
- let count1_2 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["2"] || 0;
- let count1_4 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["4"] || 0;
- let count1_8 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["8"] || 0;
- is(count1_0, 0, "Expected 0 connections using NULL");
- is(count1_1, 0, "Expected 0 connections using RSA");
- is(count1_2, 0, "Expected 0 connections using DH");
- is(count1_4, 2, "Expected 2 connections using ECDH");
- is(count1_8, 0, "Expected 0 connections using ECDH Hybrid");
- })},
-
- async function checkDtlsKEA_DTLS13EnablePQDisablePQShare() {
- // We will still advertise PQ, but we won't send a key share.
- // See bug 1992457.
- await withPrefs([["media.peerconnection.dtls.enable_pq_hybrid_kex", true],
- ["media.peerconnection.dtls.send_mlkem_keyshare", false]
- ],
- async () => {
- const pc1 = new RTCPeerConnection();
- const pc2 = new RTCPeerConnection();
- await gleanResetTestValues();
- // SSL Handshake Key Exchange Algorithm (null=0, rsa=1, dh=2, ecdh=4, ecdh_hybrid=8)
- let keyExchangeValue = await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue() || 0;
- is(keyExchangeValue, 0, "Expected no keyExchange distribution defined");
-
- const stream = await navigator.mediaDevices.getUserMedia({ video: true });
- pc1.addTrack(stream.getTracks()[0]);
-
- await connect(pc1, pc2, 32000, "DTLS connected", true, true);
- // This telemetry happens on STS/socket process
- await gleanFlushChildren();
-
- let count1_0 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["0"] || 0;
- let count1_1 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["1"] || 0;
- let count1_2 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["2"] || 0;
- let count1_4 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["4"] || 0;
- let count1_8 = (await GleanTest.webrtcdtls.keyExchangeAlgorithm.testGetValue()).values["8"] || 0;
- is(count1_0, 0, "Expected 0 connections using NULL");
- is(count1_1, 0, "Expected 0 connections using RSA");
- is(count1_2, 0, "Expected 0 connections using DH");
- is(count1_4, 2, "Expected 2 connections using ECDH");
- is(count1_8, 0, "Expected 0 connections using ECDH Hybrid");
- })},
+ },
async function checkRTCRtpSenderCount() {
const pc = new RTCPeerConnection();
diff --git a/dom/media/webrtc/transport/transportlayerdtls.cpp b/dom/media/webrtc/transport/transportlayerdtls.cpp
@@ -593,62 +593,39 @@ bool TransportLayerDtls::Setup() {
return false;
}
- // If the version is DTLS1.3 and pq in enabled, we will indicate support for
- // ML-KEM
- bool enable_mlkem =
- maxVersion_ >= Version::DTLS_1_3 &&
- Preferences::GetBool("media.peerconnection.dtls.enable_pq_hybrid_kex");
+ unsigned int additional_shares =
+ StaticPrefs::security_tls_client_hello_send_p256_keyshare();
- bool send_mlkem =
- Preferences::GetBool("media.peerconnection.dtls.send_mlkem_keyshare");
+ const SSLNamedGroup namedGroups[] = {
+ ssl_grp_ec_curve25519, ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1,
+ ssl_grp_ffdhe_2048, ssl_grp_ffdhe_3072,
+ // Advertise MLKEM support but do not pro-actively send a keyshare
- if (!enable_mlkem && send_mlkem) {
- MOZ_MTLOG(ML_NOTICE,
- "The PQ preferences are inconsistent. ML-KEM support will not be "
- "advertised, nor will the ML-KEM key share be sent.");
- }
-
- std::vector<SSLNamedGroup> namedGroups;
+ // Mlkem must stay the last in the list because if we don't support it
+ // the amount of supported_groups will be sent without it.
+ ssl_grp_kem_mlkem768x25519};
- if (enable_mlkem && send_mlkem) {
- // RFC 8446: client_shares: A list of offered KeyShareEntry values in
- // descending order of client preference.
- // {ssl_grp_kem_mlkem768x25519}, {ssl_grp_ec_curve2551} key shared to be
- // sent ML-KEM has the highest preference, so it's sent first.
- namedGroups = {ssl_grp_kem_mlkem768x25519, ssl_grp_ec_curve25519,
- ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1,
- ssl_grp_ffdhe_2048, ssl_grp_ffdhe_3072};
-
- if (SECSuccess != SSL_SendAdditionalKeyShares(ssl_fd.get(), 1)) {
- MOZ_MTLOG(ML_ERROR, "Couldn't set up additional key shares");
- return false;
- }
- }
- // Else we don't send any additional key share
- else if (enable_mlkem && !send_mlkem) {
- // Here the order of the namedGroups is different than in the first if
- // {ssl_grp_ec_curve25519} is first because it's the key share
- // that will be sent by default
- namedGroups = {ssl_grp_ec_curve25519, ssl_grp_kem_mlkem768x25519,
- ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1,
- ssl_grp_ffdhe_2048, ssl_grp_ffdhe_3072};
- }
- // ml_kem is disabled
- // {ssl_grp_ec_curve25519} will be send as a default key_share
- else {
- namedGroups = {ssl_grp_ec_curve25519, ssl_grp_ec_secp256r1,
- ssl_grp_ec_secp384r1, ssl_grp_ffdhe_2048,
- ssl_grp_ffdhe_3072};
+ size_t numGroups = std::size(namedGroups);
+ if (!(StaticPrefs::security_tls_enable_kyber() &&
+ StaticPrefs::media_webrtc_enable_pq_dtls() &&
+ maxVersion_ >= Version::DTLS_1_3)) {
+ // Excluding the last group of the namedGroups.
+ numGroups -= 1;
}
- rv = SSL_NamedGroupConfig(ssl_fd.get(), namedGroups.data(),
- std::size(namedGroups));
+ rv = SSL_NamedGroupConfig(ssl_fd.get(), namedGroups, numGroups);
if (rv != SECSuccess) {
MOZ_MTLOG(ML_ERROR, "Couldn't set named groups");
return false;
}
+ if (SECSuccess !=
+ SSL_SendAdditionalKeyShares(ssl_fd.get(), additional_shares)) {
+ MOZ_MTLOG(ML_ERROR, "Couldn't set up additional key shares");
+ return false;
+ }
+
// Certificate validation
rv = SSL_AuthCertificateHook(ssl_fd.get(), AuthCertificateHook,
reinterpret_cast<void*>(this));
@@ -722,16 +699,16 @@ bool TransportLayerDtls::SetupAlpn(UniquePRFileDesc& ssl_fd) const {
// builds, but can be disabled with prefs and they aren't on in our unit tests
// since that uses NSS default configuration.
//
-// Only override prefs to comply with MUST statements in the security-arch
-// doc. Anything outside this list is governed by the usual combination of
-// policy and user preferences.
+// Only override prefs to comply with MUST statements in the security-arch doc.
+// Anything outside this list is governed by the usual combination of policy
+// and user preferences.
static const uint32_t EnabledCiphers[] = {
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA};
-// Disable all NSS suites modes without PFS or with old and rusty
-// ciphersuites. Anything outside this list is governed by the usual
-// combination of policy and user preferences.
+// Disable all NSS suites modes without PFS or with old and rusty ciphersuites.
+// Anything outside this list is governed by the usual combination of policy
+// and user preferences.
static const uint32_t DisabledCiphers[] = {
// Bug 1310061: disable all SHA384 ciphers until fixed
TLS_AES_256_GCM_SHA384,
@@ -863,8 +840,8 @@ std::vector<uint16_t> TransportLayerDtls::GetDefaultSrtpCiphers() {
ciphers.push_back(kDtlsSrtpAeadAes256Gcm);
ciphers.push_back(kDtlsSrtpAes128CmHmacSha1_80);
#ifndef NIGHTLY_BUILD
- // To support bug 1491583 lets try to find out if we get bug reports if we
- // no longer offer this in Nightly builds.
+ // To support bug 1491583 lets try to find out if we get bug reports if we no
+ // longer offer this in Nightly builds.
ciphers.push_back(kDtlsSrtpAes128CmHmacSha1_32);
#endif
@@ -893,8 +870,8 @@ void TransportLayerDtls::StateChange(TransportLayer* layer, State state) {
LAYER_INFO << "Lower layer is now open; starting TLS");
timer_->Cancel();
timer_->SetTarget(target_);
- // Async, since the ICE layer might need to send a STUN response, and
- // we don't want the handshake to start until that is sent.
+ // Async, since the ICE layer might need to send a STUN response, and we
+ // don't want the handshake to start until that is sent.
timer_->InitWithNamedFuncCallback(
TimerCallback, this, 0, nsITimer::TYPE_ONE_SHOT,
"TransportLayerDtls::TimerCallback"_ns);
@@ -1025,8 +1002,8 @@ bool TransportLayerDtls::CheckAlpn() {
return !alpn_.empty();
case SSL_NEXT_PROTO_NO_OVERLAP:
- // This only happens if there is a custom NPN/ALPN callback installed
- // and that callback doesn't properly handle ALPN.
+ // This only happens if there is a custom NPN/ALPN callback installed and
+ // that callback doesn't properly handle ALPN.
MOZ_MTLOG(ML_ERROR, LAYER_INFO << "error in ALPN selection callback");
return false;
@@ -1039,8 +1016,8 @@ bool TransportLayerDtls::CheckAlpn() {
std::string chosen(chosenAlpn, chosenAlpnLen);
MOZ_MTLOG(ML_NOTICE, LAYER_INFO << "Selected ALPN string: " << chosen);
if (alpn_allowed_.find(chosen) == alpn_allowed_.end()) {
- // Maybe our peer chose a protocol we didn't offer (when we are client),
- // or something is seriously wrong.
+ // Maybe our peer chose a protocol we didn't offer (when we are client), or
+ // something is seriously wrong.
std::ostringstream ss;
for (auto i = alpn_allowed_.begin(); i != alpn_allowed_.end(); ++i) {
ss << (i == alpn_allowed_.begin() ? " '" : ", '") << *i << "'";
@@ -1249,8 +1226,8 @@ PRBool TransportLayerDtls::WriteSrtpXtn(PRFileDesc* fd,
if (message == ssl_hs_client_hello) {
MOZ_ASSERT(self->role_ == CLIENT);
MOZ_ASSERT(self->enabled_srtp_ciphers_.size(), "Haven't enabled SRTP");
- // We will take 2 octets for each cipher, plus a 2 octet length and 1
- // octet for the length of the empty MKI.
+ // We will take 2 octets for each cipher, plus a 2 octet length and 1 octet
+ // for the length of the empty MKI.
if (max_len < self->enabled_srtp_ciphers_.size() * 2 + 3) {
MOZ_ASSERT(false, "Not enough space to send SRTP extension");
return false;
@@ -1380,8 +1357,8 @@ SECStatus TransportLayerDtls::HandleSrtpXtn(
MOZ_ASSERT(self->role_ == SERVER);
if (self->enabled_srtp_ciphers_.empty()) {
// We don't have SRTP enabled, which is probably bad, but no sense in
- // having the handshake fail at this point, let the client decide if
- // this is a problem.
+ // having the handshake fail at this point, let the client decide if this
+ // is a problem.
return SECSuccess;
}
diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml
@@ -12161,6 +12161,12 @@
value: false
mirror: always
+# This pref disables using PQ crypto for WebRTC DTLS code.
+- name: media.webrtc.enable_pq_dtls
+ type: RelaxedAtomicBool
+ value: @IS_EARLY_BETA_OR_EARLIER@
+ mirror: always
+
# This pref controls whether dispatch testing-only events.
- name: media.webvtt.testing.events
type: bool
diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js
@@ -319,12 +319,6 @@ pref("media.videocontrols.keyboard-tab-to-all-controls", true);
// 770 = DTLS 1.0, 771 = DTLS 1.2, 772 = DTLS 1.3
pref("media.peerconnection.dtls.version.min", 771);
pref("media.peerconnection.dtls.version.max", 772);
- pref("media.peerconnection.dtls.enable_pq_hybrid_kex", true);
- #ifdef EARLY_BETA_OR_EARLIER
- pref("media.peerconnection.dtls.send_mlkem_keyshare", true);
- #else
- pref("media.peerconnection.dtls.send_mlkem_keyshare", false);
- #endif
pref("media.peerconnection.sctp.default_max_streams", 2048);
