Fix bug 160207. Make TLS implementation resistant to timing attacks on MOZILLA_1_3_BRANCH
authorwtc%netscape.com
Tue, 25 Feb 2003 00:41:29 +0000
branchMOZILLA_1_3_BRANCH
changeset 4204 6022170c5a7bcd67015dab8a26e2d117852de901
parent 4188 9de3cb96187345b29ffb307402f7da11b9697e8d
child 4205 ce961c69b3aa3aeb07d173ec3c53401aa90147c2
child 14016 db54461abbbb5ca19328f0c07db9ffd06973fed9
push idunknown
push userunknown
push dateunknown
bugs160207
Fix bug 160207. Make TLS implementation resistant to timing attacks on CBC block mode cipher suites in TLS. See bug for details. Tag: MOZILLA_1_3_BRANCH
security/nss/lib/ssl/ssl3con.c
--- a/security/nss/lib/ssl/ssl3con.c
+++ b/security/nss/lib/ssl/ssl3con.c
@@ -7388,19 +7388,20 @@ ssl3_HandleHandshake(sslSocket *ss, sslB
  */
 SECStatus
 ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText, sslBuffer *databuf)
 {
 const ssl3BulkCipherDef *cipher_def;
     ssl3State *          ssl3 			= ss->ssl3;
     ssl3CipherSpec *     crSpec;
     SECStatus            rv;
-    unsigned int         hashBytes;
+    unsigned int         hashBytes		= MAX_MAC_LENGTH + 1;
     unsigned int         padding_length;
     PRBool               isTLS;
+    PRBool               padIsBad               = PR_FALSE;
     SSL3ContentType      rType;
     SSL3Opaque           hash[MAX_MAC_LENGTH];
 
     PORT_Assert( ssl_HaveRecvBufLock(ss) );
 
     if (ssl3 == NULL) {
 	ssl_GetSSL3HandshakeLock(ss);
 	rv = ssl3_InitState(ss);
@@ -7425,16 +7426,17 @@ const ssl3BulkCipherDef *cipher_def;
 
     databuf->len = 0; /* filled in by decode call below. */
     if (databuf->space < MAX_FRAGMENT_LENGTH) {
 	rv = sslBuffer_Grow(databuf, MAX_FRAGMENT_LENGTH + 2048);
 	if (rv != SECSuccess) {
 	    SSL_DBG(("%d: SSL3[%d]: HandleRecord, tried to get %d bytes",
 		     SSL_GETPID(), ss->fd, MAX_FRAGMENT_LENGTH + 2048));
 	    /* sslBuffer_Grow has set a memory error code. */
+	    /* Perhaps we should send an alert. (but we have no memory!) */
 	    return SECFailure;
 	}
     }
 
     PRINT_BUF(80, (ss, "ciphertext:", cText->buf->buf, cText->buf->len));
 
     ssl_GetSpecReadLock(ss); /******************************************/
 
@@ -7450,70 +7452,67 @@ const ssl3BulkCipherDef *cipher_def;
     }
     /* decrypt from cText buf to databuf. */
     rv = crSpec->decode(
 	crSpec->decodeContext, databuf->buf, (int *)&databuf->len,
 	databuf->space, cText->buf->buf, cText->buf->len);
 
     PRINT_BUF(80, (ss, "cleartext:", databuf->buf, databuf->len));
     if (rv != SECSuccess) {
+	int err = ssl_MapLowLevelError(SSL_ERROR_DECRYPTION_FAILURE);
 	ssl_ReleaseSpecReadLock(ss);
-	ssl_MapLowLevelError(SSL_ERROR_DECRYPTION_FAILURE);
-	SSL3_SendAlert(ss, alert_fatal, 
-		       isTLS ? decryption_failed : bad_record_mac);
-	ssl_MapLowLevelError(SSL_ERROR_DECRYPTION_FAILURE);
+	SSL3_SendAlert(ss, alert_fatal,
+	               isTLS ? decryption_failed : bad_record_mac);
+	PORT_SetError(err);
 	return SECFailure;
     }
 
     /* If it's a block cipher, check and strip the padding. */
     if (cipher_def->type == type_block) {
 	padding_length = *(databuf->buf + databuf->len - 1);
 	/* TLS permits padding to exceed the block size, up to 255 bytes. */
-	if (padding_length + crSpec->mac_size >= databuf->len)
-	    goto bad_pad;
+	if (padding_length + 1 + crSpec->mac_size > databuf->len)
+	    padIsBad = PR_TRUE;
 	/* if TLS, check value of first padding byte. */
-	if (padding_length && isTLS && padding_length != 
-		*(databuf->buf + databuf->len - 1 - padding_length))
-	    goto bad_pad;
-	databuf->len -= padding_length + 1;
-	if (databuf->len <= 0) {
-bad_pad:
-	    /* must not hold spec lock when calling SSL3_SendAlert. */
-	    ssl_ReleaseSpecReadLock(ss);
-	    /* SSL3 & TLS must send bad_record_mac if padding check fails. */
-	    SSL3_SendAlert(ss, alert_fatal, bad_record_mac);
-	    PORT_SetError(SSL_ERROR_BAD_BLOCK_PADDING);
-	    return SECFailure;
-	}
-    }
-
-    /* Check the MAC. */
-    if (databuf->len < crSpec->mac_size) {
-    	/* record is too short to have a valid mac. */
-	goto bad_mac;
-    }
-    databuf->len -= crSpec->mac_size;
+	else if (padding_length && isTLS && 
+	         padding_length != 
+	                 *(databuf->buf + databuf->len - (padding_length + 1)))
+	    padIsBad = PR_TRUE;
+	else
+	    databuf->len -= padding_length + 1;
+    }
+
+    /* Remove the MAC. */
+    if (databuf->len >= crSpec->mac_size)
+	databuf->len -= crSpec->mac_size;
+    else
+    	padIsBad = PR_TRUE;	/* really macIsBad */
+
+    /* compute the MAC */
     rType = cText->type;
     rv = ssl3_ComputeRecordMAC(
-    	crSpec, (ss->sec.isServer) ? crSpec->client.write_mac_context
+	crSpec, (ss->sec.isServer) ? crSpec->client.write_mac_context
 				    : crSpec->server.write_mac_context,
 	rType, cText->version, crSpec->read_seq_num, 
 	databuf->buf, databuf->len, hash, &hashBytes);
     if (rv != SECSuccess) {
+	int err = ssl_MapLowLevelError(SSL_ERROR_MAC_COMPUTATION_FAILURE);
 	ssl_ReleaseSpecReadLock(ss);
-	ssl_MapLowLevelError(SSL_ERROR_MAC_COMPUTATION_FAILURE);
+	SSL3_SendAlert(ss, alert_fatal, bad_record_mac);
+	PORT_SetError(err);
 	return rv;
     }
 
-    if (hashBytes != (unsigned)crSpec->mac_size ||
+    /* Check the MAC */
+    if (hashBytes != (unsigned)crSpec->mac_size || padIsBad || 
 	PORT_Memcmp(databuf->buf + databuf->len, hash, crSpec->mac_size) != 0) {
-bad_mac:
 	/* must not hold spec lock when calling SSL3_SendAlert. */
 	ssl_ReleaseSpecReadLock(ss);
 	SSL3_SendAlert(ss, alert_fatal, bad_record_mac);
+	/* always log mac error, in case attacker can read server logs. */
 	PORT_SetError(SSL_ERROR_BAD_MAC_READ);
 
 	SSL_DBG(("%d: SSL3[%d]: mac check failed", SSL_GETPID(), ss->fd));
 
 	return SECFailure;
     }
 
     ssl3_BumpSequenceNumber(&crSpec->read_seq_num);