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 on ServoUtils.h

chorotan · chorotan · commit f3fa57aea11c · 2025-10-06T15:05:38.000Z
This reverts commit b82a54f.
diff --git a/browser/components/enterprisepolicies/Policies.sys.mjs b/browser/components/enterprisepolicies/Policies.sys.mjs
@@ -2097,7 +2097,7 @@ export var Policies = {
     onBeforeAddons(manager, param) {
       setAndLockPref("network.http.http3.enable_kyber", param);
       setAndLockPref("security.tls.enable_kyber", param);
-      setAndLockPref("media.peerconnection.dtls.enable_pq_hybrid_kex", param);
+      setAndLockPref("media.webrtc.enable_pq_dtls", param);
     },
   },
 
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);
 
diff --git a/toolkit/components/nimbus/FeatureManifest.yaml b/toolkit/components/nimbus/FeatureManifest.yaml
@@ -4843,7 +4843,7 @@ pqcrypto:
       type: boolean
       setPref:
         branch: default
-        pref: media.peerconnection.dtls.enable_pq_hybrid_kex
+        pref: media.webrtc.enable_pq_dtls
       description: >-
         Whether to enable mlkem768x25519 for DTLS in WebRTC.
 
