diff options
-rw-r--r-- | CHANGES | 13 | ||||
-rw-r--r-- | ssl/s2_enc.c | 4 | ||||
-rw-r--r-- | ssl/s2_pkt.c | 20 | ||||
-rw-r--r-- | ssl/s3_enc.c | 11 | ||||
-rw-r--r-- | ssl/s3_pkt.c | 45 | ||||
-rw-r--r-- | ssl/ssl.h | 2 | ||||
-rw-r--r-- | ssl/ssl2.h | 2 | ||||
-rw-r--r-- | ssl/ssl_err.c | 2 | ||||
-rw-r--r-- | ssl/t1_enc.c | 16 |
9 files changed, 80 insertions, 35 deletions
@@ -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 35acdf8276..fa2ab8dc4b 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 f2f46ff377..2e451d39f0 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 8709da9175..f3a9c51404 100644 --- 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 9ab76604a6..7e2607133b 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); @@ -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 f8b56afb6b..298e651602 100644 --- 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 1ae3333407..3f7c01539e 100644 --- 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 a0758e9261..062c56d7d1 100644 --- 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; |