From: Bodo Möller <bodo@openssl.org>
Date: Thu, 20 Sep 2001 18:35:52 +0000 (+0000)
Subject: Fix ssl/s3_enc.c, ssl/t1_enc.c and ssl/s3_pkt.c so that we don't
X-Git-Tag: OpenSSL_0_9_6c~26^2~217
X-Git-Url: https://git.openssl.org/?p=openssl.git;a=commitdiff_plain;h=ee60d9fb282030be3f25e951b86d74d8f2dd1bdd

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 4986190821..ffe4c9cba6 100644
--- 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 1aacae1380..91e56eba91 100644
--- 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 4340404b8e..1bb0ef1e8f 100644
--- 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 ab63b6c8fb..13ef517731 100644
--- 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 8166fc1dfa..e0b13d99b9 100644
--- 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 538d11a0c0..88060ad6d8 100644
--- 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 b04a794f13..a979c355f8 100644
--- 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 26410b9b2a..0fdd38dd24 100644
--- 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 9686edde75..81583ecae9 100644
--- 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;