Bug 1879954 - exempt errors after Server Hello from Xyber retry logic. r=keeler

jschanck · jschanck · commit de02dc50a7fb · 2024-02-15T21:01:33.000Z
Differential Revision: https://phabricator.services.mozilla.com/D201638
diff --git a/security/manager/ssl/NSSSocketControl.cpp b/security/manager/ssl/NSSSocketControl.cpp
@@ -38,6 +38,7 @@ NSSSocketControl::NSSSocketControl(const nsCString& aHostName, int32_t aPort,
       mNotedTimeUntilReady(false),
       mEchExtensionStatus(EchExtensionStatus::kNotPresent),
       mSentXyberShare(false),
+      mHasTls13HandshakeSecrets(false),
       mIsShortWritePending(false),
       mShortWritePendingByte(0),
       mShortWriteOriginalAmount(-1),
diff --git a/security/manager/ssl/NSSSocketControl.h b/security/manager/ssl/NSSSocketControl.h
@@ -125,6 +125,16 @@ class NSSSocketControl final : public CommonSocketControl {
     return mSentXyberShare;
   }
 
+  void SetHasTls13HandshakeSecrets() {
+    COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
+    mHasTls13HandshakeSecrets = true;
+  }
+
+  bool HasTls13HandshakeSecrets() {
+    COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
+    return mHasTls13HandshakeSecrets;
+  }
+
   bool GetJoined() {
     COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
     return mJoined;
@@ -296,6 +306,7 @@ class NSSSocketControl final : public CommonSocketControl {
   bool mNotedTimeUntilReady;
   EchExtensionStatus mEchExtensionStatus;  // Currently only used for telemetry.
   bool mSentXyberShare;
+  bool mHasTls13HandshakeSecrets;
 
   // True when SSL layer has indicated an "SSL short write", i.e. need
   // to call on send one or more times to push all pending data to write.
diff --git a/security/manager/ssl/nsNSSCallbacks.cpp b/security/manager/ssl/nsNSSCallbacks.cpp
@@ -1137,3 +1137,16 @@ void HandshakeCallback(PRFileDesc* fd, void* client_data) {
   infoObject->NoteTimeUntilReady();
   infoObject->SetHandshakeCompleted();
 }
+
+void SecretCallback(PRFileDesc* fd, PRUint16 epoch, SSLSecretDirection dir,
+                    PK11SymKey* secret, void* arg) {
+  // arg must be set to an NSSSocketControl* in SSL_SecretCallback
+  MOZ_ASSERT(arg);
+  NSSSocketControl* infoObject = (NSSSocketControl*)arg;
+  if (epoch == 2 && dir == ssl_secret_read) {
+    // |secret| is the server_handshake_traffic_secret. Set a flag to indicate
+    // that the Server Hello has been processed successfully. We use this when
+    // deciding whether to retry a connection in which a Xyber share was sent.
+    infoObject->SetHasTls13HandshakeSecrets();
+  }
+}
diff --git a/security/manager/ssl/nsNSSCallbacks.h b/security/manager/ssl/nsNSSCallbacks.h
@@ -17,6 +17,7 @@
 #include "mozpkix/pkix.h"
 #include "mozpkix/pkixtypes.h"
 #include "nsIX509Cert.h"
+#include "ssl.h"
 
 using mozilla::OriginAttributes;
 using mozilla::TimeDuration;
@@ -27,6 +28,8 @@ class nsILoadGroup;
 char* PK11PasswordPrompt(PK11SlotInfo* slot, PRBool retry, void* arg);
 
 void HandshakeCallback(PRFileDesc* fd, void* client_data);
+void SecretCallback(PRFileDesc* fd, PRUint16 epoch, SSLSecretDirection dir,
+                    PK11SymKey* secret, void* arg);
 SECStatus CanFalseStartCallback(PRFileDesc* fd, void* client_data,
                                 PRBool* canFalseStart);
 
diff --git a/security/manager/ssl/nsNSSIOLayer.cpp b/security/manager/ssl/nsNSSIOLayer.cpp
@@ -446,7 +446,7 @@ bool retryDueToTLSIntolerance(PRErrorCode err, NSSSocketControl* socketInfo) {
   }
 
   if (!socketInfo->IsPreliminaryHandshakeDone() &&
-      socketInfo->SentXyberShare()) {
+      !socketInfo->HasTls13HandshakeSecrets() && socketInfo->SentXyberShare()) {
     nsAutoCString errorName;
     const char* prErrorName = PR_ErrorToName(err);
     if (prErrorName) {
@@ -1279,6 +1279,9 @@ static PRFileDesc* nsSSLIOLayerImportFD(PRFileDesc* fd,
       SECSuccess) {
     return nullptr;
   }
+  if (SSL_SecretCallback(sslSock, SecretCallback, infoObject) != SECSuccess) {
+    return nullptr;
+  }
   if (SSL_SetCanFalseStartCallback(sslSock, CanFalseStartCallback,
                                    infoObject) != SECSuccess) {
     return nullptr;
diff --git a/security/manager/ssl/tests/unit/test_faulty_server.js b/security/manager/ssl/tests/unit/test_faulty_server.js
@@ -73,50 +73,70 @@ add_task(
     skip_if: () => AppConstants.MOZ_SYSTEM_NSS,
   },
   async function testRetryXyber() {
-    const retryDomains = [
-      "xyber-net-interrupt.example.com",
-      "xyber-alert-unexpected.example.com",
-    ];
+    const retryDomain = "xyber-net-interrupt.example.com";
 
     Services.prefs.setBoolPref("security.tls.enable_kyber", true);
-    Services.prefs.setCharPref("network.dns.localDomains", retryDomains);
+    Services.prefs.setCharPref("network.dns.localDomains", [retryDomain]);
     Services.prefs.setIntPref("network.http.speculative-parallel-limit", 0);
 
-    for (let i = 0; i < retryDomains.length; i++) {
-      // Get the number of xyber / x25519 callbacks prior to making the request
-      // ssl_grp_kem_xyber768d00 = 25497
-      // ssl_grp_ec_curve25519 = 29
-      let countOfXyber = handlerCount("/callback/25497");
-      let countOfX25519 = handlerCount("/callback/29");
-      let chan = makeChan(`https://${retryDomains[i]}:8443`);
-      let [, buf] = await channelOpenPromise(chan, CL_ALLOW_UNKNOWN_CL);
-      ok(buf);
-      // The server will make a xyber768d00 callback for the initial request, and
-      // then an x25519 callback for the retry. Both callback counts should
-      // increment by one.
-      equal(
-        handlerCount("/callback/25497"),
-        countOfXyber + 1,
-        "negotiated xyber768d00"
-      );
-      equal(
-        handlerCount("/callback/29"),
-        countOfX25519 + 1,
-        "negotiated x25519"
-      );
-    }
+    // Get the number of xyber / x25519 callbacks prior to making the request
+    // ssl_grp_kem_xyber768d00 = 25497
+    // ssl_grp_ec_curve25519 = 29
+    let countOfXyber = handlerCount("/callback/25497");
+    let countOfX25519 = handlerCount("/callback/29");
+    let chan = makeChan(`https://${retryDomain}:8443`);
+    let [, buf] = await channelOpenPromise(chan, CL_ALLOW_UNKNOWN_CL);
+    ok(buf);
+    // The server will make a xyber768d00 callback for the initial request, and
+    // then an x25519 callback for the retry. Both callback counts should
+    // increment by one.
+    equal(
+      handlerCount("/callback/25497"),
+      countOfXyber + 1,
+      "negotiated xyber768d00"
+    );
+    equal(handlerCount("/callback/29"), countOfX25519 + 1, "negotiated x25519");
     if (!mozinfo.socketprocess_networking) {
       // Bug 1824574
       equal(
         1,
         await Glean.tls.xyberIntoleranceReason.PR_END_OF_FILE_ERROR.testGetValue(),
         "PR_END_OF_FILE_ERROR telemetry accumulated"
       );
-      equal(
-        1,
-        await Glean.tls.xyberIntoleranceReason.SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE.testGetValue(),
-        "SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE telemetry accumulated"
-      );
     }
   }
 );
+
+add_task(
+  {
+    skip_if: () => AppConstants.MOZ_SYSTEM_NSS,
+  },
+  async function testNoRetryXyber() {
+    const retryDomain = "xyber-alert-after-server-hello.example.com";
+
+    Services.prefs.setBoolPref("security.tls.enable_kyber", true);
+    Services.prefs.setCharPref("network.dns.localDomains", [retryDomain]);
+    Services.prefs.setIntPref("network.http.speculative-parallel-limit", 0);
+
+    // Get the number of xyber / x25519 / p256 callbacks prior to making the request
+    // ssl_grp_kem_xyber768d00 = 25497
+    // ssl_grp_ec_curve25519 = 29
+    let countOfXyber = handlerCount("/callback/25497");
+    let countOfX25519 = handlerCount("/callback/29");
+    let chan = makeChan(`https://${retryDomain}:8443`);
+    let [req] = await channelOpenPromise(chan, CL_EXPECT_FAILURE);
+    equal(req.status, 0x805a2f4d); // psm::GetXPCOMFromNSSError(SSL_ERROR_HANDSHAKE_FAILED)
+    // The server will make a xyber768d00 callback for the initial request and
+    // the client should not retry.
+    equal(
+      handlerCount("/callback/25497"),
+      countOfXyber + 1,
+      "negotiated xyber768d00"
+    );
+    equal(
+      handlerCount("/callback/29"),
+      countOfX25519,
+      "did not negotiate x25519"
+    );
+  }
+);
diff --git a/security/manager/ssl/tests/unit/tlsserver/cmd/FaultyServer.cpp b/security/manager/ssl/tests/unit/tlsserver/cmd/FaultyServer.cpp
@@ -39,7 +39,8 @@ const char* kHostZeroRttAlertUnexpected = "0rtt-alert-unexpected.example.com";
 const char* kHostZeroRttAlertDowngrade = "0rtt-alert-downgrade.example.com";
 
 const char* kHostXyberNetInterrupt = "xyber-net-interrupt.example.com";
-const char* kHostXyberAlertUnexpected = "xyber-alert-unexpected.example.com";
+const char* kHostXyberAlertAfterServerHello =
+    "xyber-alert-after-server-hello.example.com";
 
 const char* kCertWildcard = "default-ee";
 
@@ -55,7 +56,7 @@ const FaultyServerHost sFaultyServerHosts[]{
     {kHostZeroRttAlertUnexpected, kCertWildcard, ZeroRtt},
     {kHostZeroRttAlertDowngrade, kCertWildcard, ZeroRtt},
     {kHostXyberNetInterrupt, kCertWildcard, Xyber},
-    {kHostXyberAlertUnexpected, kCertWildcard, Xyber},
+    {kHostXyberAlertAfterServerHello, kCertWildcard, Xyber},
     {nullptr, nullptr},
 };
 
@@ -199,8 +200,8 @@ void SecretCallbackFailXyber(PRFileDesc* fd, PRUint16 epoch,
       // The client will see this as a PR_END_OF_FILE / NS_ERROR_NET_INTERRUPT
       // error.
       ss->recordWriteCallback = FailingWriteCallback;
-    } else if (!strcmp(host->mHostName, kHostXyberAlertUnexpected)) {
-      SSL3_SendAlert(ss, alert_fatal, no_alert);
+    } else if (!strcmp(host->mHostName, kHostXyberAlertAfterServerHello)) {
+      SSL3_SendAlert(ss, alert_fatal, close_notify);
     }
   }
 }
