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 4986190821ce5736b41e5032e44cdd8024cd86e6..ffe4c9cba6e40c2b70921570dcc6def5c4e1bf56 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -12,6 +12,19 @@
          *) applies to 0.9.6a/0.9.6b/0.9.6c and 0.9.7
          +) applies to 0.9.7 only
 
+  *) 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]
+
   +) Add some demos for certificate and certificate request creation.
      [Steve Henson]
 

diff --git a/ssl/s2_enc.c b/ssl/s2_enc.c
index 1aacae1380acfea6df93b51bc1ddf34346432109..91e56eba916a1714989ead6a1a324de8f266e532 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 4340404b8e940e4c2163addc8b39b3ac96273000..1bb0ef1e8f6acb8bfeae3b113df5f99d4d7fec31 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 ab63b6c8fb31a7c9440ad40864157e074d66eeba..13ef517731392e3f19ae5d7da4bf97642544e0f7 100644 (file)
--- a/ssl/s3_enc.c
+++ b/ssl/s3_enc.c
@@ -393,8 +393,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;
                                }
                        }
                
@@ -407,9 +407,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 8166fc1dfaaaaf87fb2fb772f8510c4b42a371fd..e0b13d99b99b195f983874ceefcdd9abd806bb21 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 538d11a0c0ecce69f86efeb2a0253eafd0f92e55..88060ad6d8102cbe173512c2be7e1dff3c87ca7b 100644 (file)
--- a/ssl/ssl.h
+++ b/ssl/ssl.h
@@ -1474,6 +1474,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
@@ -1484,6 +1485,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_INVALID_CHALLENGE_LENGTH                  158
 #define SSL_R_INVALID_COMMAND                           280
 #define SSL_R_INVALID_PURPOSE                           278

diff --git a/ssl/ssl2.h b/ssl/ssl2.h
index b04a794f13ad78a5f2b60060897ccf225c44a4ee..a979c355f8a8d2b40b45b870c4d5f1741dbb7059 100644 (file)
--- a/ssl/ssl2.h
+++ b/ssl/ssl2.h
@@ -189,7 +189,6 @@ typedef struct ssl2_state_st
        unsigned char *ract_data;
        unsigned char *wact_data;
        unsigned char *mac_data;
-       unsigned char *pad_data;
 
        unsigned char *read_key;
        unsigned char *write_key;

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 26410b9b2aec6a1f5e05489b383aa509cc6b6395..0fdd38dd244acb2cf6d3f288608728f512f5e6f8 100644 (file)
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -259,6 +259,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"},
@@ -269,6 +270,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_INVALID_CHALLENGE_LENGTH          ,"invalid challenge length"},
 {SSL_R_INVALID_COMMAND                   ,"invalid command"},
 {SSL_R_INVALID_PURPOSE                   ,"invalid purpose"},

diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c
index 9686edde75a2243e8486c958668fe11c85c3cdaf..81583ecae9f770c60f6bb96cae3644682f4c1056 100644 (file)
--- a/ssl/t1_enc.c
+++ b/ssl/t1_enc.c
@@ -517,8 +517,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;
                                }
                        }
                
@@ -550,17 +550,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;