Commit ee60d9fb authored by Bodo Möller's avatar Bodo Möller
Browse files

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.
parent be6d7700

CHANGES

+13 −0
Original line number Diff line number Diff line
@@ -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]

ssl/s2_enc.c

+2 −2
Original line number Diff line number Diff line
@@ -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;

ssl/s2_pkt.c

+13 −7
Original line number Diff line number Diff line
@@ -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)

		{

ssl/s3_enc.c

+6 −5
Original line number Diff line number Diff line
@@ -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;

			}

ssl/s3_pkt.c

+33 −12
Original line number Diff line number Diff line
@@ -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);