Bug 1817164 - refresh nsHttpTransacation's nsITransportSecurityInfo if reads/writes after the handshake fail r=jschanck,necko-reviewers,valentin

This ensures the transaction's security info has the correct error code.

Differential Revision: https://phabricator.services.mozilla.com/D170258

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -710,7 +710,10 @@ nsresult nsHttpTransaction::ReadRequestSegment(nsIInputStream* stream,
 
   nsHttpTransaction* trans = (nsHttpTransaction*)closure;
   nsresult rv = trans->mReader->OnReadSegment(buf, count, countRead);
-  if (NS_FAILED(rv)) return rv;
+  if (NS_FAILED(rv)) {
+    trans->MaybeRefreshSecurityInfo();
+    return rv;
+  }
 
   LOG(("nsHttpTransaction::ReadRequestSegment %p read=%u", trans, *countRead));
 
@@ -735,12 +738,7 @@ nsresult nsHttpTransaction::ReadSegments(nsAHttpSegmentReader* reader,
 
   if (!mConnected && !m0RTTInProgress) {
     mConnected = true;
-    nsCOMPtr<nsITLSSocketControl> tlsSocketControl;
-    mConnection->GetTLSSocketControl(getter_AddRefs(tlsSocketControl));
-    if (tlsSocketControl) {
-      MutexAutoLock lock(mLock);
-      tlsSocketControl->GetSecurityInfo(getter_AddRefs(mSecurityInfo));
-    }
+    MaybeRefreshSecurityInfo();
   }
 
   mDeferredSendProgress = false;
@@ -817,7 +815,10 @@ nsresult nsHttpTransaction::WritePipeSegment(nsIOutputStream* stream,
   // OK, now let the caller fill this segment with data.
   //
   rv = trans->mWriter->OnWriteSegment(buf, count, countWritten);
-  if (NS_FAILED(rv)) return rv;  // caller didn't want to write anything
+  if (NS_FAILED(rv)) {
+    trans->MaybeRefreshSecurityInfo();
+    return rv;  // caller didn't want to write anything
+  }
 
   LOG(("nsHttpTransaction::WritePipeSegment %p written=%u", trans,
        *countWritten));
@@ -1400,13 +1401,7 @@ void nsHttpTransaction::Close(nsresult reason) {
     connReused = mConnection->IsReused();
     isHttp2or3 = mConnection->Version() >= HttpVersion::v2_0;
     if (!mConnected) {
-      // Try to get TLSSocketControl for this transaction.
-      nsCOMPtr<nsITLSSocketControl> tlsSocketControl;
-      mConnection->GetTLSSocketControl(getter_AddRefs(tlsSocketControl));
-      if (tlsSocketControl) {
-        MutexAutoLock lock(mLock);
-        tlsSocketControl->GetSecurityInfo(getter_AddRefs(mSecurityInfo));
-      }
+      MaybeRefreshSecurityInfo();
     }
   }
   mConnected = false;
@@ -3007,12 +3002,7 @@ nsresult nsHttpTransaction::Finish0RTT(bool aRestart,
   } else if (!mConnected) {
     // this is code that was skipped in ::ReadSegments while in 0RTT
     mConnected = true;
-    nsCOMPtr<nsITLSSocketControl> tlsSocketControl;
-    mConnection->GetTLSSocketControl(getter_AddRefs(tlsSocketControl));
-    if (tlsSocketControl) {
-      MutexAutoLock lock(mLock);
-      tlsSocketControl->GetSecurityInfo(getter_AddRefs(mSecurityInfo));
-    }
+    MaybeRefreshSecurityInfo();
   }
   return NS_OK;
 }

netwerk/protocol/http/nsHttpTransaction.h

@@ -294,6 +294,17 @@ class nsHttpTransaction final : public nsAHttpTransaction,
 
   bool HandleWebTransportResponse(uint16_t aStatus);
 
+  void MaybeRefreshSecurityInfo() {
+    MutexAutoLock lock(mLock);
+    if (mConnection) {
+      nsCOMPtr<nsITLSSocketControl> tlsSocketControl;
+      mConnection->GetTLSSocketControl(getter_AddRefs(tlsSocketControl));
+      if (tlsSocketControl) {
+        tlsSocketControl->GetSecurityInfo(getter_AddRefs(mSecurityInfo));
+      }
+    }
+  }
+
  private:
   class UpdateSecurityCallbacks : public Runnable {
    public:

security/manager/ssl/tests/mochitest/browser/browser_clientAuth_connection.js

@@ -172,7 +172,8 @@ async function testHelper(
   prefValue,
   expectedURL,
   expectCallingChooseCertificate,
-  options = undefined
+  options = undefined,
+  expectStringInPage = undefined
 ) {
   gClientAuthDialogs.chooseCertificateCalled = false;
   await SpecialPowers.pushPrefEnv({
@@ -203,6 +204,17 @@ async function testHelper(
     "chooseCertificate should have been called if we were expecting it to be called"
   );
 
+  if (expectStringInPage) {
+    let pageContent = await SpecialPowers.spawn(
+      win.gBrowser.selectedBrowser,
+      [],
+      async function() {
+        return content.document.body.textContent;
+      }
+    );
+    Assert.ok(pageContent.includes(expectStringInPage));
+  }
+
   await win.close();
 
   // This clears the TLS session cache so we don't use a previously-established
@@ -232,7 +244,12 @@ add_task(async function testCertNotChosenByUser() {
   await testHelper(
     "Ask Every Time",
     "about:neterror?e=nssFailure2&u=https%3A//requireclientcert.example.com/",
-    true
+    true,
+    undefined,
+    // bug 1818556: ssltunnel doesn't behave as expected here on Windows
+    AppConstants.platform != "win"
+      ? "SSL_ERROR_RX_CERTIFICATE_REQUIRED_ALERT"
+      : undefined
   );
   cars.clearRememberedDecisions();
 });