bug 528288 - spdy - disallow server cert changes through renegotiation when using spdy r=bsmith sr=honzab
authorPatrick McManus <mcmanus@ducksong.com>
Fri, 02 Dec 2011 10:28:57 -0500
changeset 81188 0c7ba908f2fd9ce258f3adf169f708126a9319dc
parent 81187 c80e1a5653cb39d1393a769505bae7efe60c9258
child 81189 9aed66c3a561683acbe305a6cb0e8d74fb5fa24e
push id21564
push usermak77@bonardo.net
push dateSat, 03 Dec 2011 11:10:17 +0000
treeherdermozilla-central@a68c96c1d8e0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmith, honzab
bugs528288
milestone11.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 528288 - spdy - disallow server cert changes through renegotiation when using spdy r=bsmith sr=honzab patch 13
security/manager/ssl/src/SSLServerCertVerification.cpp
--- a/security/manager/ssl/src/SSLServerCertVerification.cpp
+++ b/security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -360,20 +360,91 @@ PSM_SSL_BlacklistDigiNotar(CERTCertifica
     // let's see if we want to worsen the error code to revoked.
     PRErrorCode revoked_code = PSM_SSL_DigiNotarTreatAsRevoked(serverCert, serverCertChain);
     return (revoked_code != 0) ? revoked_code : SEC_ERROR_UNTRUSTED_ISSUER;
   }
 
   return 0;
 }
 
+// This function assumes that we will only use the SPDY connection coalescing
+// feature on connections where we have negotiated SPDY using NPN. If we ever
+// talk SPDY without having negotiated it with SPDY, this code will give wrong
+// and perhaps unsafe results.
+//
+// Returns SECSuccess on the initial handshake of all connections, on
+// renegotiations for any connections where we did not negotiate SPDY, or on any
+// SPDY connection where the server's certificate did not change.
+//
+// Prohibit changing the server cert only if we negotiated SPDY,
+// in order to support SPDY's cross-origin connection pooling.
+
+static SECStatus
+BlockServerCertChangeForSpdy(nsNSSSocketInfo *infoObject,
+                             CERTCertificate *serverCert)
+{
+  // Get the existing cert. If there isn't one, then there is
+  // no cert change to worry about.
+  nsCOMPtr<nsIX509Cert> cert;
+  nsCOMPtr<nsIX509Cert2> cert2;
+
+  nsRefPtr<nsSSLStatus> status = infoObject->SSLStatus();
+  if (!status) {
+    // If we didn't have a status, then this is the
+    // first handshake on this connection, not a
+    // renegotiation.
+    return SECSuccess;
+  }
+  
+  status->GetServerCert(getter_AddRefs(cert));
+  cert2 = do_QueryInterface(cert);
+  if (!cert2) {
+    NS_NOTREACHED("every nsSSLStatus must have a cert"
+                  "that implements nsIX509Cert2");
+    PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
+    return SECFailure;
+  }
+
+  // Filter out sockets that did not neogtiate SPDY via NPN
+  nsCAutoString negotiatedNPN;
+  nsresult rv = infoObject->GetNegotiatedNPN(negotiatedNPN);
+  NS_ASSERTION(NS_SUCCEEDED(rv),
+               "GetNegotiatedNPN() failed during renegotiation");
+
+  if (NS_SUCCEEDED(rv) && !negotiatedNPN.Equals(NS_LITERAL_CSTRING("spdy/2")))
+    return SECSuccess;
+
+  // If GetNegotiatedNPN() failed we will assume spdy for safety's safe
+  if (NS_FAILED(rv))
+    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
+           ("BlockServerCertChangeForSpdy failed GetNegotiatedNPN() call."
+            " Assuming spdy.\n"));
+
+  // Check to see if the cert has actually changed
+  CERTCertificate * c = cert2->GetCert();
+  NS_ASSERTION(c, "very bad and hopefully impossible state");
+  bool sameCert = CERT_CompareCerts(c, serverCert);
+  CERT_DestroyCertificate(c);
+  if (sameCert)
+    return SECSuccess;
+
+  // Report an error - changed cert is confirmed
+  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
+         ("SPDY Refused to allow new cert during renegotiation\n"));
+  PR_SetError(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED, 0);
+  return SECFailure;
+}
+
 SECStatus
 SSLServerCertVerificationJob::AuthCertificate(
   nsNSSShutDownPreventionLock const & nssShutdownPreventionLock)
 {
+  if (BlockServerCertChangeForSpdy(mSocketInfo, mCert) != SECSuccess)
+    return SECFailure;
+
   if (mCert->serialNumber.data &&
       mCert->issuerName &&
       !strcmp(mCert->issuerName, 
         "CN=UTN-USERFirst-Hardware,OU=http://www.usertrust.com,O=The USERTRUST Network,L=Salt Lake City,ST=UT,C=US")) {
 
     unsigned char *server_cert_comparison_start = mCert->serialNumber.data;
     unsigned int server_cert_comparison_len = mCert->serialNumber.len;