Fix ssl/s3_enc.c, ssl/t1_enc.c and ssl/s3_pkt.c so that we don't

reveal whether illegal block cipher padding was found or a MAC
verification error occured.

In ssl/s2_pkt.c, verify that the purported number of padding bytes is in
the legal range.

diff --git a/CHANGES b/CHANGES
index 2206ea09956839b0fb4ef4bb2c512c0d8c88872b..66e02675b71780ac3a0e4ff515f4293f774ca7df 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,19 @@
 
  Changes between 0.9.6b and 0.9.6c  [XX xxx XXXX]
 
+  *) Fix ssl/s3_enc.c, ssl/t1_enc.c and ssl/s3_pkt.c so that we don't
+     reveal whether illegal block cipher padding was found or a MAC
+     verification error occured.  (Neither SSLerr() codes nor alerts
+     are directly visible to potential attackers, but the information
+     may leak via logfiles.)
+
+     Similar changes are not required for the SSL 2.0 implementation
+     because the number of padding bytes is sent in clear for SSL 2.0,
+     and the extra bytes are just ignored.  However ssl/s2_pkt.c
+     failed to verify that the purported number of padding bytes is in
+     the legal range.
+     [Bodo Moeller]
+
   *) OpenUNIX-8 support (Boyd Lynn Gerber <gerberb@zenez.com>)
      [Lutz Jaenicke]
 

diff --git a/ssl/s2_enc.c b/ssl/s2_enc.c
index 35acdf8276d218c53cb555b861b6692460ead259..fa2ab8dc4be7a2ec9f20f573059daec0d4269df1 100644 (file)
--- a/ssl/s2_enc.c
+++ b/ssl/s2_enc.c
@@ -111,8 +111,8 @@ err:
        }
 
 /* read/writes from s->s2->mac_data using length for encrypt and 
- * decrypt.  It sets the s->s2->padding, s->[rw]length and
- * s->s2->pad_data ptr if we are encrypting */
+ * decrypt.  It sets s->s2->padding and s->[rw]length
+ * if we are encrypting */
 void ssl2_enc(SSL *s, int send)
        {
        EVP_CIPHER_CTX *ds;

diff --git a/ssl/s2_pkt.c b/ssl/s2_pkt.c
index f2f46ff377c91888b03ee8d96ee0aa6fb472cb0f..2e451d39f01ce45df46c4a2a574969e05d3f3f00 100644 (file)
--- a/ssl/s2_pkt.c
+++ b/ssl/s2_pkt.c
@@ -130,7 +130,7 @@ static int ssl2_read_internal(SSL *s, void *buf, int len, int peek)
        unsigned char mac[MAX_MAC_SIZE];
        unsigned char *p;
        int i;
-       unsigned int mac_size=0;
+       unsigned int mac_size;
 
  ssl2_read_again:
        if (SSL_in_init(s) && !s->in_handshake)
@@ -235,17 +235,25 @@ static int ssl2_read_internal(SSL *s, void *buf, int len, int peek)
                /* Data portion */
                if (s->s2->clear_text)
                        {
+                       mac_size = 0;
                        s->s2->mac_data=p;
                        s->s2->ract_data=p;
-                       s->s2->pad_data=NULL;
+                       if (s->s2->padding)
+                               {
+                               SSLerr(SSL_F_SSL2_READ_INTERNAL,SSL_R_ILLEGAL_PADDING);
+                               return(-1);
+                               }
                        }
                else
                        {
                        mac_size=EVP_MD_size(s->read_hash);
                        s->s2->mac_data=p;
                        s->s2->ract_data= &p[mac_size];
-                       s->s2->pad_data= &p[mac_size+
-                               s->s2->rlength-s->s2->padding];
+                       if (s->s2->padding + mac_size > s->s2->rlength)
+                               {
+                               SSLerr(SSL_F_SSL2_READ_INTERNAL,SSL_R_ILLEGAL_PADDING);
+                               return(-1);
+                               }
                        }
 
                s->s2->ract_data_length=s->s2->rlength;
@@ -593,10 +601,8 @@ static int do_ssl_write(SSL *s, const unsigned char *buf, unsigned int len)
        s->s2->wact_data= &(s->s2->wbuf[3+mac_size]);
        /* we copy the data into s->s2->wbuf */
        memcpy(s->s2->wact_data,buf,len);
-#ifdef PURIFY
        if (p)
-               memset(&(s->s2->wact_data[len]),0,p);
-#endif
+               memset(&(s->s2->wact_data[len]),0,p); /* arbitrary padding */
 
        if (!s->s2->clear_text)
                {

diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c
index 8709da9175675004591b1aa7c729059c551a5792..f3a9c51404f00677e2436438260f3898cf37537c 100644 (file)
--- a/ssl/s3_enc.c
+++ b/ssl/s3_enc.c
@@ -381,8 +381,8 @@ int ssl3_enc(SSL *s, int send)
                        if (l == 0 || l%bs != 0)
                                {
                                SSLerr(SSL_F_SSL3_ENC,SSL_R_BLOCK_CIPHER_PAD_IS_WRONG);
-                               ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPT_ERROR);
-                               return(0);
+                               ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPTION_FAILED);
+                               return 0;
                                }
                        }
                
@@ -395,9 +395,10 @@ int ssl3_enc(SSL *s, int send)
                         * padding bytes (except that last) are arbitrary */
                        if (i > bs)
                                {
-                               SSLerr(SSL_F_SSL3_ENC,SSL_R_BLOCK_CIPHER_PAD_IS_WRONG);
-                               ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPT_ERROR);
-                               return(0);
+                               /* Incorrect padding. SSLerr() and ssl3_alert are done
+                                * by caller: we don't want to reveal whether this is
+                                * a decryption error or a MAC verification failure. */
+                               return -1;
                                }
                        rec->length-=i;
                        }

diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 9ab76604a64534192a45569a6f90257487b2135a..7e2607133b2e0848d90962e17a5486fe22db482f 100644 (file)
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -231,7 +231,7 @@ static int ssl3_read_n(SSL *s, int n, int max, int extend)
 static int ssl3_get_record(SSL *s)
        {
        int ssl_major,ssl_minor,al;
-       int n,i,ret= -1;
+       int enc_err,n,i,ret= -1;
        SSL3_RECORD *rr;
        SSL_SESSION *sess;
        unsigned char *p;
@@ -342,16 +342,23 @@ again:
        /* decrypt in place in 'rr->input' */
        rr->data=rr->input;
 
-       if (!s->method->ssl3_enc->enc(s,0))
+       enc_err = s->method->ssl3_enc->enc(s,0);
+       if (enc_err <= 0)
                {
-               al=SSL_AD_DECRYPT_ERROR;
-               goto f_err;
+               if (enc_err == 0)
+                       /* SSLerr() and ssl3_send_alert() have been called */
+                       goto err;
+
+               /* otherwise enc_err == -1 */
+               goto decryption_failed_or_bad_record_mac;
                }
+
 #ifdef TLS_DEBUG
 printf("dec %d\n",rr->length);
 { unsigned int z; for (z=0; z<rr->length; z++) printf("%02X%c",rr->data[z],((z+1)%16)?' ':'\n'); }
 printf("\n");
 #endif
+
        /* r->length is now the compressed data plus mac */
        if (    (sess == NULL) ||
                (s->enc_read_ctx == NULL) ||
@@ -364,25 +371,30 @@ printf("\n");
 
                if (rr->length > SSL3_RT_MAX_COMPRESSED_LENGTH+extra+mac_size)
                        {
+#if 0 /* OK only for stream ciphers (then rr->length is visible from ciphertext anyway) */
                        al=SSL_AD_RECORD_OVERFLOW;
                        SSLerr(SSL_F_SSL3_GET_RECORD,SSL_R_PRE_MAC_LENGTH_TOO_LONG);
                        goto f_err;
+#else
+                       goto decryption_failed_or_bad_record_mac;
+#endif                 
                        }
                /* check the MAC for rr->input (it's in mac_size bytes at the tail) */
                if (rr->length < mac_size)
                        {
+#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;
+#endif
                        }
                rr->length-=mac_size;
                i=s->method->ssl3_enc->mac(s,md,0);
                if (memcmp(md,&(rr->data[rr->length]),mac_size) != 0)
                        {
-                       al=SSL_AD_BAD_RECORD_MAC;
-                       SSLerr(SSL_F_SSL3_GET_RECORD,SSL_R_BAD_MAC_DECODE);
-                       ret= -1;
-                       goto f_err;
+                       goto decryption_failed_or_bad_record_mac;
                        }
                }
 
@@ -427,6 +439,15 @@ printf("\n");
        if (rr->length == 0) goto again;
 
        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:
@@ -1164,7 +1185,7 @@ void ssl3_send_alert(SSL *s, int level, int desc)
        s->s3->alert_dispatch=1;
        s->s3->send_alert[0]=level;
        s->s3->send_alert[1]=desc;
-       if (s->s3->wbuf.left == 0) /* data still being written out */
+       if (s->s3->wbuf.left == 0) /* data still being written out? */
                ssl3_dispatch_alert(s);
        /* else data is still being written out, we will get written
         * some time in the future */
@@ -1183,9 +1204,9 @@ int ssl3_dispatch_alert(SSL *s)
                }
        else
                {
-               /* If it is important, send it now.  If the message
-                * does not get sent due to non-blocking IO, we will
-                * not worry too much. */
+               /* Alert sent to BIO.  If it is important, flush it now.
+                * If the message does not get sent due to non-blocking IO,
+                * we will not worry too much. */
                if (s->s3->send_alert[0] == SSL3_AL_FATAL)
                        (void)BIO_flush(s->wbio);
 

diff --git a/ssl/ssl.h b/ssl/ssl.h
index 6e67506a7557a4580a149d78a1f8657871ebaefa..81b06341a458789b32cb2e47f57b39f08cb28141 100644 (file)
--- a/ssl/ssl.h
+++ b/ssl/ssl.h
@@ -1405,6 +1405,7 @@ void ERR_load_SSL_strings(void);
 #define SSL_R_DATA_BETWEEN_CCS_AND_FINISHED             145
 #define SSL_R_DATA_LENGTH_TOO_LONG                      146
 #define SSL_R_DECRYPTION_FAILED                                 147
+#define SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC       1109
 #define SSL_R_DH_PUBLIC_VALUE_LENGTH_IS_WRONG           148
 #define SSL_R_DIGEST_CHECK_FAILED                       149
 #define SSL_R_ENCRYPTED_LENGTH_TOO_LONG                         150
@@ -1415,6 +1416,7 @@ void ERR_load_SSL_strings(void);
 #define SSL_R_GOT_A_FIN_BEFORE_A_CCS                    154
 #define SSL_R_HTTPS_PROXY_REQUEST                       155
 #define SSL_R_HTTP_REQUEST                              156
+#define SSL_R_ILLEGAL_PADDING                           1110
 #define SSL_R_INTERNAL_ERROR                            157
 #define SSL_R_INVALID_CHALLENGE_LENGTH                  158
 #define SSL_R_INVALID_COMMAND                           280

diff --git a/ssl/ssl2.h b/ssl/ssl2.h
index f8b56afb6b20ef37a005f975aecbbbbbd3de2997..298e6516021ad540a45ceb72cbbefb751cf4085e 100644 (file)
--- a/ssl/ssl2.h
+++ b/ssl/ssl2.h
@@ -189,7 +189,7 @@ typedef struct ssl2_state_st
        unsigned char *ract_data;
        unsigned char *wact_data;
        unsigned char *mac_data;
-       unsigned char *pad_data;
+       unsigned char *pad_data_UNUSED; /* only for binary compatibility with 0.9.6b */
 
        unsigned char *read_key;
        unsigned char *write_key;

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 1ae333340794c3b128be73461c56fef376309157..3f7c01539eac0368b0f0a6dda22b805df2b573c7 100644 (file)
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -258,6 +258,7 @@ static ERR_STRING_DATA SSL_str_reasons[]=
 {SSL_R_DATA_BETWEEN_CCS_AND_FINISHED     ,"data between ccs and finished"},
 {SSL_R_DATA_LENGTH_TOO_LONG              ,"data length too long"},
 {SSL_R_DECRYPTION_FAILED                 ,"decryption failed"},
+{SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC,"decryption failed or bad record mac"},
 {SSL_R_DH_PUBLIC_VALUE_LENGTH_IS_WRONG   ,"dh public value length is wrong"},
 {SSL_R_DIGEST_CHECK_FAILED               ,"digest check failed"},
 {SSL_R_ENCRYPTED_LENGTH_TOO_LONG         ,"encrypted length too long"},
@@ -268,6 +269,7 @@ static ERR_STRING_DATA SSL_str_reasons[]=
 {SSL_R_GOT_A_FIN_BEFORE_A_CCS            ,"got a fin before a ccs"},
 {SSL_R_HTTPS_PROXY_REQUEST               ,"https proxy request"},
 {SSL_R_HTTP_REQUEST                      ,"http request"},
+{SSL_R_ILLEGAL_PADDING                   ,"illegal padding"},
 {SSL_R_INTERNAL_ERROR                    ,"internal error"},
 {SSL_R_INVALID_CHALLENGE_LENGTH          ,"invalid challenge length"},
 {SSL_R_INVALID_COMMAND                   ,"invalid command"},

diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c
index a0758e92614a85fe161c5eece3015975463a740d..062c56d7d15fbee288ce866a1f872530e756f82b 100644 (file)
--- a/ssl/t1_enc.c
+++ b/ssl/t1_enc.c
@@ -452,8 +452,8 @@ int tls1_enc(SSL *s, int send)
                        if (l == 0 || l%bs != 0)
                                {
                                SSLerr(SSL_F_TLS1_ENC,SSL_R_BLOCK_CIPHER_PAD_IS_WRONG);
-                               ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPT_ERROR);
-                               return(0);
+                               ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPTION_FAILED);
+                               return 0;
                                }
                        }
                
@@ -476,17 +476,17 @@ int tls1_enc(SSL *s, int send)
                         * All of them must have value 'padding_length'. */
                        if (i > (int)rec->length)
                                {
-                               SSLerr(SSL_F_TLS1_ENC,SSL_R_BLOCK_CIPHER_PAD_IS_WRONG);
-                               ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPTION_FAILED);
-                               return(0);
+                               /* Incorrect padding. SSLerr() and ssl3_alert are done
+                                * by caller: we don't want to reveal whether this is
+                                * a decryption error or a MAC verification failure. */
+                               return -1;
                                }
                        for (j=(int)(l-i); j<(int)l; j++)
                                {
                                if (rec->data[j] != ii)
                                        {
-                                       SSLerr(SSL_F_TLS1_ENC,SSL_R_DECRYPTION_FAILED);
-                                       ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPTION_FAILED);
-                                       return(0);
+                                       /* Incorrect padding */
+                                       return -1;
                                        }
                                }
                        rec->length-=i;