[openssl-dev] [openssl.org #3592] bug report. Crash. Critical? Security bug?
Вячеслав Бадалян via RT
rt at openssl.org
Wed Dec 10 03:32:56 UTC 2014
Hello. I begin test you patch. I attach to mail patched version of you
patch wthat may clear added current SRPM of Centos 6
2014-12-03 5:16 GMT+03:00 Вячеслав Бадалян <v.badalyan at open-bs.ru>:
> Thanks! I need time to test it... i will try answer at this week
>
> 2014-12-02 19:37 GMT+03:00 Matt Caswell via RT <rt at openssl.org>:
>
>> On Tue Dec 02 17:31:05 2014, v.badalyan at open-bs.ru wrote:
>> > if you send patch i can add it to SRPM build and try results
>> >
>> The patch is attached. However you may have problems with this approach.
>> I have
>> built the patch for 1.0.1e (which is the version you originally said you
>> were
>> running). However any additional patches that have been applied to the
>> SRPM
>> could cause the patch to fail to apply (and it is quite a large patch). I
>> can
>> also supply a patch against the latest 1.0.1j or OpenSSL_1_0_1-stable
>> from git
>> if you prefer.
>>
>> Matt
>>
>>
>
>
> --
> С уважением,
> Бадалян Вячеслав Борисович
>
> ООО "Открытые бизнес-решения"
> Технический директор
> +7 (495) 666-0-111
> http://www.open-bs.ru
>
--
С уважением,
Бадалян Вячеслав Борисович
ООО "Открытые бизнес-решения"
Технический директор
+7 (495) 666-0-111
http://www.open-bs.ru
-------------- next part --------------
>From a0473d82c6ad0d4f5f1e2f778a20ae374317720a Mon Sep 17 00:00:00 2001
From: Matt Caswell <matt at openssl.org>
Date: Mon, 1 Dec 2014 11:10:38 +0000
Subject: [PATCH] MTU fixes patch
---
apps/s_client.c | 16 +++++++-
apps/s_server.c | 18 ++++++++-
crypto/bio/bio.h | 4 ++
crypto/bio/bss_dgram.c | 46 ++++++++++++++++++++--
ssl/d1_both.c | 103 +++++++++++++++++++++++++++++++------------------
ssl/d1_lib.c | 32 +++++++++++++--
ssl/dtls1.h | 4 ++
ssl/ssl.h | 8 ++++
ssl/ssl_lib.c | 13 -------
ssl/ssl_locl.h | 3 +-
10 files changed, 184 insertions(+), 63 deletions(-)
diff --git a/apps/s_client.c b/apps/s_client.c
index 3ba6605..9f4be61 100644
--- a/apps/s_client.c
+++ b/apps/s_client.c
@@ -1307,10 +1307,22 @@ re_start:
BIO_ctrl(sbio, BIO_CTRL_DGRAM_SET_SEND_TIMEOUT, 0, &timeout);
}
- if (socket_mtu > 28)
+ if (socket_mtu)
{
+ if(socket_mtu < DTLS_get_link_min_mtu(con))
+ {
+ BIO_printf(bio_err,"MTU too small. Must be at least %d\n",
+ DTLS_get_link_min_mtu(con));
+ BIO_free(sbio);
+ goto shut;
+ }
SSL_set_options(con, SSL_OP_NO_QUERY_MTU);
- SSL_set_mtu(con, socket_mtu - 28);
+ if(!DTLS_set_link_mtu(con, socket_mtu))
+ {
+ BIO_printf(bio_err, "Failed to set MTU\n");
+ BIO_free(sbio);
+ goto shut;
+ }
}
else
/* want to do MTU discovery */
diff --git a/apps/s_server.c b/apps/s_server.c
index 8198d7f..a6f090b 100644
--- a/apps/s_server.c
+++ b/apps/s_server.c
@@ -2035,10 +2035,24 @@ static int sv_body(char *hostname, int s, unsigned char *context)
BIO_ctrl(sbio, BIO_CTRL_DGRAM_SET_SEND_TIMEOUT, 0, &timeout);
}
- if (socket_mtu > 28)
+ if (socket_mtu)
{
+ if(socket_mtu < DTLS_get_link_min_mtu(con))
+ {
+ BIO_printf(bio_err,"MTU too small. Must be at least %d\n",
+ DTLS_get_link_min_mtu(con));
+ ret = -1;
+ BIO_free(sbio);
+ goto err;
+ }
SSL_set_options(con, SSL_OP_NO_QUERY_MTU);
- SSL_set_mtu(con, socket_mtu - 28);
+ if(!DTLS_set_link_mtu(con, socket_mtu))
+ {
+ BIO_printf(bio_err, "Failed to set MTU\n");
+ ret = -1;
+ BIO_free(sbio);
+ goto err;
+ }
}
else
/* want to do MTU discovery */
diff --git a/crypto/bio/bio.h b/crypto/bio/bio.h
index 05699ab..32eba71 100644
--- a/crypto/bio/bio.h
+++ b/crypto/bio/bio.h
@@ -175,6 +175,8 @@ extern "C" {
#define BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT 45 /* Next DTLS handshake timeout to
* adjust socket timeouts */
+#define BIO_CTRL_DGRAM_GET_MTU_OVERHEAD 49
+
#ifndef OPENSSL_NO_SCTP
/* SCTP stuff */
#define BIO_CTRL_DGRAM_SCTP_SET_IN_HANDSHAKE 50
@@ -607,6 +609,8 @@ int BIO_ctrl_reset_read_request(BIO *b);
(int)BIO_ctrl(b, BIO_CTRL_DGRAM_GET_PEER, 0, (char *)peer)
#define BIO_dgram_set_peer(b,peer) \
(int)BIO_ctrl(b, BIO_CTRL_DGRAM_SET_PEER, 0, (char *)peer)
+#define BIO_dgram_get_mtu_overhead(b) \
+ (unsigned int)BIO_ctrl((b), BIO_CTRL_DGRAM_GET_MTU_OVERHEAD, 0, NULL)
/* These two aren't currently implemented */
/* int BIO_get_ex_num(BIO *bio); */
diff --git a/crypto/bio/bss_dgram.c b/crypto/bio/bss_dgram.c
index 8990909..0e5a52f 100644
--- a/crypto/bio/bss_dgram.c
+++ b/crypto/bio/bss_dgram.c
@@ -454,6 +454,36 @@ static int dgram_write(BIO *b, const char *in, int inl)
return(ret);
}
+static long dgram_get_mtu_overhead(bio_dgram_data *data)
+ {
+ long ret;
+
+ switch (data->peer.sa.sa_family)
+ {
+ case AF_INET:
+ /* Assume this is UDP - 20 bytes for IP, 8 bytes for UDP */
+ ret = 28;
+ break;
+#if OPENSSL_USE_IPV6
+ case AF_INET6:
+#ifdef IN6_IS_ADDR_V4MAPPED
+ if (IN6_IS_ADDR_V4MAPPED(&data->peer.sa_in6.sin6_addr))
+ /* Assume this is UDP - 20 bytes for IP, 8 bytes for UDP */
+ ret = 28;
+ else
+#endif
+ /* Assume this is UDP - 40 bytes for IP, 8 bytes for UDP */
+ ret = 48;
+ break;
+#endif
+ default:
+ /* We don't know. Go with the historical default */
+ ret = 28;
+ break;
+ }
+ return ret;
+ }
+
static long dgram_ctrl(BIO *b, int cmd, long num, void *ptr)
{
long ret=1;
@@ -630,23 +660,24 @@ static long dgram_ctrl(BIO *b, int cmd, long num, void *ptr)
#endif
break;
case BIO_CTRL_DGRAM_GET_FALLBACK_MTU:
+ ret = -dgram_get_mtu_overhead(data);
switch (data->peer.sa.sa_family)
{
case AF_INET:
- ret = 576 - 20 - 8;
+ ret += 576;
break;
#if OPENSSL_USE_IPV6
case AF_INET6:
#ifdef IN6_IS_ADDR_V4MAPPED
if (IN6_IS_ADDR_V4MAPPED(&data->peer.sa_in6.sin6_addr))
- ret = 576 - 20 - 8;
+ ret += 576;
else
#endif
- ret = 1280 - 40 - 8;
+ ret += 1280;
break;
#endif
default:
- ret = 576 - 20 - 8;
+ ret += 576;
break;
}
break;
@@ -847,6 +878,9 @@ static long dgram_ctrl(BIO *b, int cmd, long num, void *ptr)
ret = 0;
break;
#endif
+ case BIO_CTRL_DGRAM_GET_MTU_OVERHEAD:
+ ret = dgram_get_mtu_overhead(data);
+ break;
default:
ret=0;
break;
@@ -1367,6 +1401,10 @@ static long dgram_sctp_ctrl(BIO *b, int cmd, long num, void *ptr)
* Returns always 1.
*/
break;
+ case BIO_CTRL_DGRAM_GET_MTU_OVERHEAD:
+ /* We allow transport protocol fragmentation so this is irrelevant */
+ ret = 0;
+ break;
case BIO_CTRL_DGRAM_SCTP_SET_IN_HANDSHAKE:
if (num > 0)
data->in_handshake = 1;
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index de8bab8..0781ca5 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -156,9 +156,8 @@ static unsigned char bitmask_start_values[] = {0xff, 0xfe, 0xfc, 0xf8, 0xf0, 0xe
static unsigned char bitmask_end_values[] = {0xff, 0x01, 0x03, 0x07, 0x0f, 0x1f, 0x3f, 0x7f};
/* XDTLS: figure out the right values */
-static unsigned int g_probable_mtu[] = {1500 - 28, 512 - 28, 256 - 28};
+static const unsigned int g_probable_mtu[] = {1500, 512, 256};
-static unsigned int dtls1_guess_mtu(unsigned int curr_mtu);
static void dtls1_fix_message_header(SSL *s, unsigned long frag_off,
unsigned long frag_len);
static unsigned char *dtls1_write_message_header(SSL *s,
@@ -219,29 +218,48 @@ dtls1_hm_fragment_free(hm_fragment *frag)
OPENSSL_free(frag);
}
+static int dtls1_query_mtu(SSL *s)
+{
+ if(s->d1->link_mtu)
+ {
+ s->d1->mtu = s->d1->link_mtu-BIO_dgram_get_mtu_overhead(SSL_get_wbio(s));
+ s->d1->link_mtu = 0;
+ }
+
+ /* AHA! Figure out the MTU, and stick to the right size */
+ if (s->d1->mtu < dtls1_min_mtu(s))
+ {
+ if(!(SSL_get_options(s) & SSL_OP_NO_QUERY_MTU))
+ {
+ s->d1->mtu =
+ BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_QUERY_MTU, 0, NULL);
+
+ /* I've seen the kernel return bogus numbers when it doesn't know
+ * (initial write), so just make sure we have a reasonable number */
+ if (s->d1->mtu < dtls1_min_mtu(s))
+ {
+ /* Set to min mtu */
+ s->d1->mtu = dtls1_min_mtu(s);
+ BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_SET_MTU,
+ s->d1->mtu, NULL);
+ }
+ }
+ else
+ return 0;
+ }
+ return 1;
+}
+
/* send s->init_buf in records of type 'type' (SSL3_RT_HANDSHAKE or SSL3_RT_CHANGE_CIPHER_SPEC) */
int dtls1_do_write(SSL *s, int type)
{
int ret;
int curr_mtu;
+ int retry = 1;
unsigned int len, frag_off, mac_size, blocksize;
- /* AHA! Figure out the MTU, and stick to the right size */
- if (s->d1->mtu < dtls1_min_mtu() && !(SSL_get_options(s) & SSL_OP_NO_QUERY_MTU))
- {
- s->d1->mtu =
- BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_QUERY_MTU, 0, NULL);
-
- /* I've seen the kernel return bogus numbers when it doesn't know
- * (initial write), so just make sure we have a reasonable number */
- if (s->d1->mtu < dtls1_min_mtu())
- {
- s->d1->mtu = 0;
- s->d1->mtu = dtls1_guess_mtu(s->d1->mtu);
- BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_SET_MTU,
- s->d1->mtu, NULL);
- }
- }
+ if(!dtls1_query_mtu(s))
+ return -1;
#if 0
mtu = s->d1->mtu;
@@ -265,7 +283,7 @@ int dtls1_do_write(SSL *s, int type)
}
#endif
- OPENSSL_assert(s->d1->mtu >= dtls1_min_mtu()); /* should have something reasonable now */
+ OPENSSL_assert(s->d1->mtu >= dtls1_min_mtu(s)); /* should have something reasonable now */
if ( s->init_off == 0 && type == SSL3_RT_HANDSHAKE)
OPENSSL_assert(s->init_num ==
@@ -318,12 +336,18 @@ int dtls1_do_write(SSL *s, int type)
len += DTLS1_HM_HEADER_LENGTH;
}
+ if ( len < DTLS1_HM_HEADER_LENGTH )
+ {
+ /*
+ * len is so small that we really can't do anything sensible
+ * so fail
+ */
+ return -1;
+ }
dtls1_fix_message_header(s, frag_off,
len - DTLS1_HM_HEADER_LENGTH);
dtls1_write_message_header(s, (unsigned char *)&s->init_buf->data[s->init_off]);
-
- OPENSSL_assert(len >= DTLS1_HM_HEADER_LENGTH);
}
ret=dtls1_write_bytes(s,type,&s->init_buf->data[s->init_off],
@@ -336,12 +360,23 @@ int dtls1_do_write(SSL *s, int type)
* is fine and wait for an alert to handle the
* retransmit
*/
- if ( BIO_ctrl(SSL_get_wbio(s),
+ if ( retry && BIO_ctrl(SSL_get_wbio(s),
BIO_CTRL_DGRAM_MTU_EXCEEDED, 0, NULL) > 0 )
- s->d1->mtu = BIO_ctrl(SSL_get_wbio(s),
- BIO_CTRL_DGRAM_QUERY_MTU, 0, NULL);
+ {
+ if(!(SSL_get_options(s) & SSL_OP_NO_QUERY_MTU))
+ {
+ if(!dtls1_query_mtu(s))
+ return -1;
+ /* Have one more go */
+ retry = 0;
+ }
+ else
+ return -1;
+ }
else
+ {
return(-1);
+ }
}
else
{
@@ -1380,28 +1415,20 @@ dtls1_write_message_header(SSL *s, unsigned char *p)
return p;
}
-unsigned int
-dtls1_min_mtu(void)
+unsigned int
+dtls1_link_min_mtu(void)
{
return (g_probable_mtu[(sizeof(g_probable_mtu) /
sizeof(g_probable_mtu[0])) - 1]);
}
-static unsigned int
-dtls1_guess_mtu(unsigned int curr_mtu)
+unsigned int
+dtls1_min_mtu(SSL *s)
{
- unsigned int i;
-
- if ( curr_mtu == 0 )
- return g_probable_mtu[0] ;
-
- for ( i = 0; i < sizeof(g_probable_mtu)/sizeof(g_probable_mtu[0]); i++)
- if ( curr_mtu > g_probable_mtu[i])
- return g_probable_mtu[i];
-
- return curr_mtu;
+ return dtls1_link_min_mtu()-BIO_dgram_get_mtu_overhead(SSL_get_wbio(s));
}
+
void
dtls1_get_message_header(unsigned char *data, struct hm_header_st *msg_hdr)
{
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index f61f718..87c36ad 100644
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -113,6 +113,9 @@ int dtls1_new(SSL *s)
d1->cookie_len = sizeof(s->d1->cookie);
}
+ d1->link_mtu = 0;
+ d1->mtu = 0;
+
if( ! d1->unprocessed_rcds.q || ! d1->processed_rcds.q
|| ! d1->buffered_messages || ! d1->sent_messages || ! d1->buffered_app_data.q)
{
@@ -206,6 +209,7 @@ void dtls1_clear(SSL *s)
pqueue sent_messages;
pqueue buffered_app_data;
unsigned int mtu;
+ unsigned int link_mtu;
if (s->d1)
{
@@ -215,6 +219,7 @@ void dtls1_clear(SSL *s)
sent_messages = s->d1->sent_messages;
buffered_app_data = s->d1->buffered_app_data.q;
mtu = s->d1->mtu;
+ link_mtu = s->d1->link_mtu;
dtls1_clear_queues(s);
@@ -228,6 +233,7 @@ void dtls1_clear(SSL *s)
if (SSL_get_options(s) & SSL_OP_NO_QUERY_MTU)
{
s->d1->mtu = mtu;
+ s->d1->link_mtu = link_mtu;
}
s->d1->unprocessed_rcds.q = unprocessed_rcds;
@@ -401,12 +422,17 @@ void dtls1_stop_timer(SSL *s)
int dtls1_check_timeout_num(SSL *s)
{
+ unsigned int mtu;
+
s->d1->timeout.num_alerts++;
/* Reduce MTU after 2 unsuccessful retransmissions */
- if (s->d1->timeout.num_alerts > 2)
+ if (s->d1->timeout.num_alerts > 2
+ && !(SSL_get_options(s) & SSL_OP_NO_QUERY_MTU))
{
- s->d1->mtu = BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_GET_FALLBACK_MTU, 0, NULL);
+ mtu = BIO_ctrl(SSL_get_wbio(s), BIO_CTRL_DGRAM_GET_FALLBACK_MTU, 0, NULL);
+ if(mtu < s->d1->mtu)
+ s->d1->mtu = mtu;
}
if (s->d1->timeout.num_alerts > DTLS1_TMO_ALERT_COUNT)
diff --git a/ssl/dtls1.h b/ssl/dtls1.h
index e65d501..fd168e7 100644
--- a/ssl/dtls1.h
+++ b/ssl/dtls1.h
@@ -115,6 +115,9 @@ extern "C" {
#define DTLS1_SCTP_AUTH_LABEL "EXPORTER_DTLS_OVER_SCTP"
#endif
+/* Max MTU overhead we know about so far is 40 for IPv6 + 8 for UDP */
+#define DTLS1_MAX_MTU_OVERHEAD 48
+
typedef struct dtls1_bitmap_st
{
unsigned long map; /* track 32 packets on 32-bit systems
@@ -229,6 +232,7 @@ typedef struct dtls1_state_st
*/
record_pqueue buffered_app_data;
+ unsigned int link_mtu; /* max on-the-wire DTLS packet size */
unsigned int mtu; /* max DTLS packet size */
struct hm_header_st w_msg_hdr;
diff --git a/ssl/ssl.h b/ssl/ssl.h
index 593579e..377ceff 100644
--- a/ssl/ssl.h
+++ b/ssl/ssl.h
@@ -672,6 +672,12 @@ struct ssl_session_st
SSL_ctrl((ssl),SSL_CTRL_MODE,0,NULL)
#define SSL_set_mtu(ssl, mtu) \
SSL_ctrl((ssl),SSL_CTRL_SET_MTU,(mtu),NULL)
+#define DTLS_set_link_mtu(ssl, mtu) \
+ SSL_ctrl((ssl),DTLS_CTRL_SET_LINK_MTU,(mtu),NULL)
+#define DTLS_get_link_min_mtu(ssl) \
+ SSL_ctrl((ssl),DTLS_CTRL_GET_LINK_MIN_MTU,0,NULL)
+#define DTLS_CTRL_SET_LINK_MTU 120
+#define DTLS_CTRL_GET_LINK_MIN_MTU 121
#define SSL_get_secure_renegotiation_support(ssl) \
SSL_ctrl((ssl), SSL_CTRL_GET_RI_SUPPORT, 0, NULL)
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 14d143d..cd94f30 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1080,19 +1080,6 @@ long SSL_ctrl(SSL *s,int cmd,long larg,void *parg)
l=s->max_cert_list;
s->max_cert_list=larg;
return(l);
- case SSL_CTRL_SET_MTU:
-#ifndef OPENSSL_NO_DTLS1
- if (larg < (long)dtls1_min_mtu())
- return 0;
-#endif
-
- if (SSL_version(s) == DTLS1_VERSION ||
- SSL_version(s) == DTLS1_BAD_VER)
- {
- s->d1->mtu = larg;
- return larg;
- }
- return 0;
case SSL_CTRL_SET_MAX_SEND_FRAGMENT:
if (larg < 512 || larg > SSL3_RT_MAX_PLAIN_LENGTH)
return 0;
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 1b98947..7f4c375 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -983,7 +983,8 @@ void dtls1_stop_timer(SSL *s);
int dtls1_is_timer_expired(SSL *s);
void dtls1_double_timeout(SSL *s);
int dtls1_send_newsession_ticket(SSL *s);
-unsigned int dtls1_min_mtu(void);
+unsigned int dtls1_min_mtu(SSL *s);
+unsigned int dtls1_link_min_mtu(void);
/* some client-only functions */
int ssl3_client_hello(SSL *s);
--
2.1.0
-------------- next part --------------
--- a/ssl/d1_lib.c 2014-12-10 05:27:02.313246685 +0300
+++ b/ssl/d1_lib.c 2014-12-10 05:28:01.364020724 +0300
@@ -279,7 +279,22 @@
/* Just one protocol version is supported so far;
* fail closed if the version is not as expected. */
return s->version == DTLS_MAX_VERSION;
-
+ case DTLS_CTRL_SET_LINK_MTU:
+ if (larg < (long)dtls1_link_min_mtu())
+ return 0;
+ s->d1->link_mtu = larg;
+ return 1;
+ case DTLS_CTRL_GET_LINK_MIN_MTU:
+ return (long)dtls1_link_min_mtu();
+ case SSL_CTRL_SET_MTU:
+ /*
+ * We may not have a BIO set yet so can't call dtls1_min_mtu()
+ * We'll have to make do with dtls1_link_min_mtu() and max overhead
+ */
+ if (larg < (long)dtls1_link_min_mtu() - DTLS1_MAX_MTU_OVERHEAD)
+ return 0;
+ s->d1->mtu = larg;
+ return larg;
default:
ret = ssl3_ctrl(s, cmd, larg, parg);
break;
More information about the openssl-dev
mailing list