Bug 726315, followup from bug 542832, Patch contributed by Brian Smith, r=kaie

8 files changed, 45 insertions, 20 deletions

diff --git a/security/nss/cmd/tstclnt/tstclnt.c b/security/nss/cmd/tstclnt/tstclnt.c
index 8a8071b37..52ef9a15f 100644
--- a/security/nss/cmd/tstclnt/tstclnt.c
+++ b/security/nss/cmd/tstclnt/tstclnt.c
@@ -325,7 +325,8 @@ ownAuthCertificate(void *arg, PRFileDesc *fd, PRBool checkSig,
 {
     ServerCertAuth * serverCertAuth = (ServerCertAuth *) arg;
 
-    FPRINTF(stderr, "using asynchronous certificate validation\n");
+    FPRINTF(stderr, "%s: using asynchronous certificate validation\n",
+	    progName);
 
     PORT_Assert(serverCertAuth->shouldPause);
     PORT_Assert(!serverCertAuth->isPaused);
diff --git a/security/nss/lib/ssl/ssl.h b/security/nss/lib/ssl/ssl.h
index c3e900ac8..2e5f4e7aa 100644
--- a/security/nss/lib/ssl/ssl.h
+++ b/security/nss/lib/ssl/ssl.h
@@ -347,11 +347,14 @@ SSL_IMPORT CERTCertificate *SSL_PeerCertificate(PRFileDesc *fd);
 **
 ** If the authenticate certificate hook returns SECFailure, then the bad cert
 ** hook will be called. The bad cert handler is NEVER called if the
-** authenticate certificate hook returns SECWouldBlock.
+** authenticate certificate hook returns SECWouldBlock. If the application
+** needs to handle and/or override a bad cert, it should do so before it
+** calls SSL_AuthCertificateComplete (modifying the error it passes to
+** SSL_AuthCertificateComplete as needed).
 **
 ** See the documentation for SSL_AuthCertificateComplete for more information
 ** about the asynchronous behavior that occurs when the authenticate
-** certificate hook returns SECWouldBlock
+** certificate hook returns SECWouldBlock.
 */
 typedef SECStatus (PR_CALLBACK *SSLAuthCertificate)(void *arg, PRFileDesc *fd, 
                                                     PRBool checkSig,
@@ -772,11 +775,11 @@ extern const char *NSSSSL_GetVersion(void);
  * a connection; it does not work for the server role.
  *
  * The application must call SSL_AuthCertificateComplete with 0 as the value of
- * status parameter after it has successfully validated the peer's certificate,
- * in order to continue the SSL handshake.
+ * the error parameter after it has successfully validated the peer's
+ * certificate, in order to continue the SSL handshake.
  *
  * The application may call SSL_AuthCertificateComplete with a non-zero value
- * for status (e.g. SEC_ERROR_REVOKED_CERTIFICATE) when certificate validation
+ * for error (e.g. SEC_ERROR_REVOKED_CERTIFICATE) when certificate validation
  * fails, before it closes the connection. If the application does so, an
  * alert corresponding to the error (e.g. certificate_revoked) will be sent to
  * the peer. See the source code of the internal function
@@ -816,10 +819,16 @@ extern const char *NSSSSL_GetVersion(void);
  * Returns SECFailure on failure, SECSuccess on success. Never returns
  * SECWouldBlock. Note that SSL_AuthCertificateComplete will (usually) return
  * SECSuccess; do not interpret the return value of SSL_AuthCertificateComplete
- * as an indicator of whether it is OK to continue using the connection.
+ * as an indicator of whether it is OK to continue using the connection. For
+ * example, SSL_AuthCertificateComplete(fd, SEC_ERROR_REVOKED_CERTIFICATE) will
+ * return SECSuccess (normally), but that does not mean that the application
+ * should continue using the connection. If the application passes a non-zero
+ * value for second argument (error), or if SSL_AuthCertificateComplete returns
+ * anything other than SECSuccess, then the application should close the
+ * connection.
  */
 SSL_IMPORT SECStatus SSL_AuthCertificateComplete(PRFileDesc *fd,
-						 PRErrorCode status);
+						 PRErrorCode error);
 SEC_END_PROTOS
 
 #endif /* __ssl_h_ */
diff --git a/security/nss/lib/ssl/ssl3con.c b/security/nss/lib/ssl/ssl3con.c
index 325106e25..26abe281a 100644
--- a/security/nss/lib/ssl/ssl3con.c
+++ b/security/nss/lib/ssl/ssl3con.c
@@ -8146,7 +8146,7 @@ ssl3_AlwaysFail(sslSocket * ss)
 /* Caller must hold 1stHandshakeLock.
 */
 SECStatus
-ssl3_AuthCertificateComplete(sslSocket *ss, PRErrorCode status)
+ssl3_AuthCertificateComplete(sslSocket *ss, PRErrorCode error)
 {
     SECStatus rv;
 
@@ -8168,9 +8168,9 @@ ssl3_AuthCertificateComplete(sslSocket *ss, PRErrorCode status)
 
     ss->ssl3.hs.authCertificatePending = PR_FALSE;
 
-    if (status != 0) {
+    if (error != 0) {
 	ss->ssl3.hs.restartTarget = ssl3_AlwaysFail;
-	ssl3_SendAlertForCertError(ss, status);
+	ssl3_SendAlertForCertError(ss, error);
 	rv = SECSuccess;
     } else if (ss->ssl3.hs.restartTarget != NULL) {
 	sslRestartTarget target = ss->ssl3.hs.restartTarget;
diff --git a/security/nss/lib/ssl/ssl3ext.c b/security/nss/lib/ssl/ssl3ext.c
index 1b24eae91..8937a618e 100644
--- a/security/nss/lib/ssl/ssl3ext.c
+++ b/security/nss/lib/ssl/ssl3ext.c
@@ -241,7 +241,7 @@ static const ssl3HelloExtensionHandler clientHelloHandlers[] = {
 #endif
     { ssl_session_ticket_xtn,     &ssl3_ServerHandleSessionTicketXtn },
     { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn },
-    { ssl_next_proto_neg_xtn,     &ssl3_ServerHandleNextProtoNegoXtn },
+    { ssl_next_proto_nego_xtn,    &ssl3_ServerHandleNextProtoNegoXtn },
     { -1, NULL }
 };
 
@@ -252,7 +252,7 @@ static const ssl3HelloExtensionHandler serverHelloHandlersTLS[] = {
     /* TODO: add a handler for ssl_ec_point_formats_xtn */
     { ssl_session_ticket_xtn,     &ssl3_ClientHandleSessionTicketXtn },
     { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn },
-    { ssl_next_proto_neg_xtn,     &ssl3_ClientHandleNextProtoNegoXtn },
+    { ssl_next_proto_nego_xtn,    &ssl3_ClientHandleNextProtoNegoXtn },
     { -1, NULL }
 };
 
@@ -276,7 +276,7 @@ ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS] = {
     { ssl_ec_point_formats_xtn,   &ssl3_SendSupportedPointFormatsXtn },
 #endif
     { ssl_session_ticket_xtn,     &ssl3_SendSessionTicketXtn },
-    { ssl_next_proto_neg_xtn,     &ssl3_ClientSendNextProtoNegoXtn }
+    { ssl_next_proto_nego_xtn,    &ssl3_ClientSendNextProtoNegoXtn }
     /* any extra entries will appear as { 0, NULL }    */
 };
 
@@ -641,14 +641,14 @@ ssl3_ClientSendNextProtoNegoXtn(sslSocket * ss, PRBool append,
 
     if (append && maxBytes >= extension_length) {
 	SECStatus rv;
-	rv = ssl3_AppendHandshakeNumber(ss, ssl_next_proto_neg_xtn, 2);
+	rv = ssl3_AppendHandshakeNumber(ss, ssl_next_proto_nego_xtn, 2);
 	if (rv != SECSuccess)
 	    goto loser;
 	rv = ssl3_AppendHandshakeNumber(ss, 0, 2);
 	if (rv != SECSuccess)
 	    goto loser;
 	ss->xtnData.advertised[ss->xtnData.numAdvertised++] =
-		ssl_next_proto_neg_xtn;
+		ssl_next_proto_nego_xtn;
     } else if (maxBytes < extension_length) {
 	return 0;
     }
diff --git a/security/nss/lib/ssl/sslimpl.h b/security/nss/lib/ssl/sslimpl.h
index a6a508f97..ee5650265 100644
--- a/security/nss/lib/ssl/sslimpl.h
+++ b/security/nss/lib/ssl/sslimpl.h
@@ -1359,7 +1359,7 @@ extern void ssl_FreeSocket(struct sslSocketStr *ssl);
 extern SECStatus SSL3_SendAlert(sslSocket *ss, SSL3AlertLevel level,
 				SSL3AlertDescription desc);
 
-extern SECStatus ssl3_AuthCertificateComplete(sslSocket *ss, PRErrorCode status);
+extern SECStatus ssl3_AuthCertificateComplete(sslSocket *ss, PRErrorCode error);
 
 /*
  * for dealing with SSL 3.0 clients sending SSL 2.0 format hellos
diff --git a/security/nss/lib/ssl/sslsecur.c b/security/nss/lib/ssl/sslsecur.c
index 8622654b6..bea866e26 100644
--- a/security/nss/lib/ssl/sslsecur.c
+++ b/security/nss/lib/ssl/sslsecur.c
@@ -1488,7 +1488,7 @@ SSL_RestartHandshakeAfterServerCert(sslSocket * ss)
 
 /* See documentation in ssl.h */
 SECStatus
-SSL_AuthCertificateComplete(PRFileDesc *fd, PRErrorCode status)
+SSL_AuthCertificateComplete(PRFileDesc *fd, PRErrorCode error)
 {
     SECStatus rv;
     sslSocket *ss = ssl_FindSocket(fd);
@@ -1508,7 +1508,7 @@ SSL_AuthCertificateComplete(PRFileDesc *fd, PRErrorCode status)
 	PORT_SetError(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SSL2);
 	rv = SECFailure;
     } else {
-	rv = ssl3_AuthCertificateComplete(ss, status);
+	rv = ssl3_AuthCertificateComplete(ss, error);
     }
 
     ssl_Release1stHandshakeLock(ss);
diff --git a/security/nss/lib/ssl/sslsock.c b/security/nss/lib/ssl/sslsock.c
index bf5a97b85..c1295a7f5 100644
--- a/security/nss/lib/ssl/sslsock.c
+++ b/security/nss/lib/ssl/sslsock.c
@@ -1964,8 +1964,23 @@ ssl_Poll(PRFileDesc *fd, PRInt16 how_flags, PRInt16 *p_out_flags)
 	 * the caller to poll the socket unless there is pending write data.
 	 */
 	if (ss->lastWriteBlocked && ss->pendingBuf.len != 0) {
+	    /* Ignore any newly-received data on the socket, but do wait for
+	     * the socket to become writable again. Here, it is OK for an error
+	     * to be detected, because our logic for sending pending write data
+	     * will allow us to report the error to the caller without the risk
+	     * of the application spinning.
+	     */
 	    new_flags &= (PR_POLL_WRITE | PR_POLL_EXCEPT);
 	} else {
+	    /* Unfortunately, clearing new_flags will make it impossible for
+	     * the application to detect errors that it would otherwise be
+	     * able to detect with PR_POLL_EXCEPT, until the asynchronous
+	     * callback completes. However, we must clear all the flags to
+	     * prevent the application from spinning (alternating between
+	     * calling PR_Poll that would return PR_POLL_EXCEPT, and send/recv
+	     * which won't actually report the I/O error while we are waiting
+	     * for the asynchronous callback to complete).
+	     */
 	    new_flags = 0;
 	}
     }
diff --git a/security/nss/lib/ssl/sslt.h b/security/nss/lib/ssl/sslt.h
index 17e1ab203..543e0cb30 100644
--- a/security/nss/lib/ssl/sslt.h
+++ b/security/nss/lib/ssl/sslt.h
@@ -203,7 +203,7 @@ typedef enum {
     ssl_ec_point_formats_xtn         = 11,
 #endif
     ssl_session_ticket_xtn           = 35,
-    ssl_next_proto_neg_xtn           = 13172,
+    ssl_next_proto_nego_xtn          = 13172,
     ssl_renegotiation_info_xtn       = 0xff01	/* experimental number */
 } SSLExtensionType;