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 4986190..ffe4c9c 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 1aacae1..91e56eb 100644
--- a/ssl/s2_enc.c
+++ b/ssl/s2_enc.c
@@ -111,8 +111,8 @@
}
/* 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 4340404..1bb0ef1 100644
--- a/ssl/s2_pkt.c
+++ b/ssl/s2_pkt.c
@@ -130,7 +130,7 @@
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 @@
/* 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 @@
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 ab63b6c..13ef517 100644
--- a/ssl/s3_enc.c
+++ b/ssl/s3_enc.c
@@ -393,8 +393,8 @@
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 @@
* 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 8166fc1..e0b13d9 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -231,7 +231,7 @@
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 @@
/* 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 @@
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 @@
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 @@
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 @@
}
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 538d11a..88060ad 100644
--- a/ssl/ssl.h
+++ b/ssl/ssl.h
@@ -1474,6 +1474,7 @@
#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 @@
#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 b04a794..a979c35 100644
--- a/ssl/ssl2.h
+++ b/ssl/ssl2.h
@@ -189,7 +189,6 @@
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 26410b9..0fdd38d 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -259,6 +259,7 @@
{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 @@
{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 9686edd..81583ec 100644
--- a/ssl/t1_enc.c
+++ b/ssl/t1_enc.c
@@ -517,8 +517,8 @@
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 @@
* 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;
