Security fix: Vaudenay timing attack on CBC.

An advisory will be posted to the web.  Expect a release within the hour.

2 files changed, 41 insertions, 18 deletions

diff --git a/CHANGES b/CHANGES
index 8d600f0638..cf7444a1a9 100644
--- a/CHANGES
+++ b/CHANGES
@@ -2,9 +2,17 @@
  OpenSSL CHANGES
  _______________
 
- Changes between 0.9.6h and 0.9.6i  [xx XXX xxxx]
+ Changes between 0.9.6h and 0.9.6i  [19 Feb 2003]
 
-  *)
+  *) In ssl3_get_record (ssl/s3_pkt.c), minimize information leaked
+     via timing by performing a MAC computation even if incorrrect
+     block cipher padding has been found.  This is a countermeasure
+     against active attacks where the attacker has to distinguish
+     between bad padding and a MAC verification error. (CAN-2003-0078)
+
+     [Bodo Moeller; problem pointed out by Brice Canvel (EPFL),
+     Alain Hiltgen (UBS), Serge Vaudenay (EPFL), and
+     Martin Vuagnoux (EPFL, Ilion)]
 
  Changes between 0.9.6g and 0.9.6h  [5 Dec 2002]
 
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index f52303c7c3..caf975d688 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -238,6 +238,8 @@ static int ssl3_get_record(SSL *s)
 	unsigned int mac_size;
 	int clear=0;
 	size_t extra;
+	int decryption_failed_or_bad_record_mac = 0;
+	unsigned char *mac = NULL;
 
 	rr= &(s->s3->rrec);
 	sess=s->session;
@@ -353,8 +355,11 @@ again:
 			/* SSLerr() and ssl3_send_alert() have been called */
 			goto err;
 
-		/* otherwise enc_err == -1 */
-		goto decryption_failed_or_bad_record_mac;
+		/* Otherwise enc_err == -1, which indicates bad padding
+		 * (rec->length has not been changed in this case).
+		 * To minimize information leaked via timing, we will perform
+		 * the MAC computation anyway. */
+		decryption_failed_or_bad_record_mac = 1;
 		}
 
 #ifdef TLS_DEBUG
@@ -380,28 +385,46 @@ printf("\n");
 			SSLerr(SSL_F_SSL3_GET_RECORD,SSL_R_PRE_MAC_LENGTH_TOO_LONG);
 			goto f_err;
 #else
-			goto decryption_failed_or_bad_record_mac;
+			decryption_failed_or_bad_record_mac = 1;
 #endif			
 			}
 		/* check the MAC for rr->input (it's in mac_size bytes at the tail) */
-		if (rr->length < mac_size)
+		if (rr->length >= mac_size)
 			{
+			rr->length -= mac_size;
+			mac = &rr->data[rr->length];
+			}
+		else
+			{
+			/* record (minus padding) is too short to contain a MAC */
 #if 0 /* OK only for stream ciphers */
 			al=SSL_AD_DECODE_ERROR;
 			SSLerr(SSL_F_SSL3_GET_RECORD,SSL_R_LENGTH_TOO_SHORT);
 			goto f_err;
 #else
-			goto decryption_failed_or_bad_record_mac;
+			decryption_failed_or_bad_record_mac = 1;
+			rr->length = 0;
 #endif
 			}
-		rr->length-=mac_size;
 		i=s->method->ssl3_enc->mac(s,md,0);
-		if (memcmp(md,&(rr->data[rr->length]),mac_size) != 0)
+		if (mac == NULL || memcmp(md, mac, mac_size) != 0)
 			{
-			goto decryption_failed_or_bad_record_mac;
+			decryption_failed_or_bad_record_mac = 1;
 			}
 		}
 
+	if (decryption_failed_or_bad_record_mac)
+		{
+		/* A separate 'decryption_failed' alert was introduced with TLS 1.0,
+		 * SSL 3.0 only has 'bad_record_mac'.  But unless a decryption
+		 * failure is directly visible from the ciphertext anyway,
+		 * we should not reveal which kind of error occured -- this
+		 * might become visible to an attacker (e.g. via a logfile) */
+		al=SSL_AD_BAD_RECORD_MAC;
+		SSLerr(SSL_F_SSL3_GET_RECORD,SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC);
+		goto f_err;
+		}
+
 	/* r->length is now just compressed */
 	if (s->expand != NULL)
 		{
@@ -443,14 +466,6 @@ printf("\n");
 
 	return(1);
 
-decryption_failed_or_bad_record_mac:
-	/* Separate 'decryption_failed' alert was introduced with TLS 1.0,
-	 * SSL 3.0 only has 'bad_record_mac'.  But unless a decryption
-	 * failure is directly visible from the ciphertext anyway,
-	 * we should not reveal which kind of error occured -- this
-	 * might become visible to an attacker (e.g. via logfile) */
-	al=SSL_AD_BAD_RECORD_MAC;
-	SSLerr(SSL_F_SSL3_GET_RECORD,SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC);
 f_err:
 	ssl3_send_alert(s,SSL3_AL_FATAL,al);
 err: