security fix from openssl 0.9.7a:

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)

2 files changed, 62 insertions, 32 deletions

diff --git a/src/lib/libssl/s3_pkt.c b/src/lib/libssl/s3_pkt.c
index 6ccea9aee5..3f88429e79 100644
--- a/src/lib/libssl/s3_pkt.c
+++ b/src/lib/libssl/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:
diff --git a/src/lib/libssl/src/ssl/s3_pkt.c b/src/lib/libssl/src/ssl/s3_pkt.c
index 6ccea9aee5..3f88429e79 100644
--- a/src/lib/libssl/src/ssl/s3_pkt.c
+++ b/src/lib/libssl/src/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: