On Sep 3, 2009, at 10:50 AM, Robin Seggelmann via RT wrote: > > On Sep 2, 2009, at 3:02 PM, Stephen Henson via RT wrote: > >> There appear to be several problems with this patch, see inline: >> >>> [seggelm...@fh-muenster.de - Mon Aug 31 17:04:19 2009]: >>> >>> This patch fixes several issues with DTLS cookies. >>> >> [snip] >>> >> >> cookie_secret is defined: >> >>> +unsigned char cookie_secret[COOKIE_SECRET_LENGTH]; >>> +int cookie_initialized=0; >>> >> >> Then you call: >> >>> + if (!RAND_bytes((unsigned char*) &cookie_secret, >>> COOKIE_SECRET_LENGTH)) >> >> Shouldn't that (and several other places too) be cookie_secret and >> not >> &cookie_secret? > > Correct. > >>> --- crypto/bio/bio.h 24 Jul 2009 11:25:13 -0000 1.80 >>> +++ crypto/bio/bio.h 31 Aug 2009 13:24:35 -0000 >>> @@ -157,9 +157,10 @@ >>> * previous write >>> * operation */ >>> >>> -#define BIO_CTRL_DGRAM_SET_PEER 44 /* Destination for the >>> data */ >>> +#define BIO_CTRL_DGRAM_GET_PEER 44 >>> +#define BIO_CTRL_DGRAM_SET_PEER 45 /* Destination for the >>> data */ >>> >>> -#define BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT 45 /* Next DTLS handshake >>> timeout to >>> +#define BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT 46 /* Next DTLS handshake >>> timeout to >> >> The above changes the values of some ctrls which have appeared in a >> released version of OpenSSL i.e. 0.9.8k. That is a definite no-no as >> it >> breaks binary compatibility. > > Ok. I have attached an updated version, which addresses these issues > and fixes some other bugs. The order of the code in > ssl3_get_client_hello() was correct, the problem was in function > tls1_process_ticket() which didn't skip the cookie when trying to > process extensions. Additionally, no memory at all is allocated now if > cookies are required and the current ClientHello doesn't carry one. > Previously a new session was generated for every ClientHello which > could be used for denial of service attacks, which was the reason for > adding the HelloVerifyRequest in the first place.
Another update...forgot to change two pointers in
generate_cookie_callback() and removed the cookie secret
initialization in verify_cookie_callback()...
--- apps/s_apps.h 22 Dec 2008 14:05:42 -0000 1.21
+++ apps/s_apps.h 3 Sep 2009 11:31:32 -0000
@@ -171,3 +171,6 @@
unsigned char *data, int len,
void *arg);
#endif
+
+int MS_CALLBACK generate_cookie_callback(SSL *ssl, unsigned char
*cookie, unsigned int *cookie_len);
+int MS_CALLBACK verify_cookie_callback(SSL *ssl, unsigned char
*cookie, unsigned int cookie_len);
--- apps/s_cb.c 2 Sep 2009 12:45:19 -0000 1.27.2.2
+++ apps/s_cb.c 3 Sep 2009 11:31:32 -0000
@@ -121,9 +121,13 @@
#include <openssl/ssl.h>
#include "s_apps.h"
+#define COOKIE_SECRET_LENGTH 16
+
int verify_depth=0;
int verify_error=X509_V_OK;
int verify_return_error=0;
+unsigned char cookie_secret[COOKIE_SECRET_LENGTH];
+int cookie_initialized=0;
int MS_CALLBACK verify_callback(int ok, X509_STORE_CTX *ctx)
{
@@ -682,3 +686,86 @@
BIO_dump(bio, (char *)data, len);
(void)BIO_flush(bio);
}
+
+int MS_CALLBACK generate_cookie_callback(SSL *ssl, unsigned char
*cookie, unsigned int *cookie_len)
+ {
+ unsigned char *buffer, result[EVP_MAX_MD_SIZE];
+ unsigned int length, resultlength;
+ struct sockaddr_in peer;
+
+ /* Initialize a random secret */
+ if (!cookie_initialized)
+ {
+ if (!RAND_bytes((unsigned char*) cookie_secret,
COOKIE_SECRET_LENGTH))
+ {
+ BIO_printf(bio_err,"error setting random cookie
secret\n");
+ return 0;
+ }
+ cookie_initialized = 1;
+ }
+
+ /* Read peer information */
+ BIO_dgram_get_peer(SSL_get_rbio(ssl), &peer);
+
+ /* Create buffer with peer's address and port */
+ length = sizeof(peer.sin_addr);
+ length += sizeof(peer.sin_port);
+ buffer = OPENSSL_malloc(length);
+
+ if (buffer == NULL)
+ {
+ BIO_printf(bio_err,"out of memory\n");
+ return 0;
+ }
+
+ memcpy(buffer, &peer.sin_addr, sizeof(peer.sin_addr));
+ memcpy(buffer + sizeof(peer.sin_addr), &peer.sin_port,
sizeof(peer.sin_port));
+
+ /* Calculate HMAC of buffer using the secret */
+ HMAC(EVP_sha1(), cookie_secret, COOKIE_SECRET_LENGTH, buffer,
+ length, (unsigned char*) result, &resultlength);
+ OPENSSL_free(buffer);
+
+ memcpy(cookie, result, resultlength);
+ *cookie_len = resultlength;
+
+ return 1;
+ }
+
+int MS_CALLBACK verify_cookie_callback(SSL *ssl, unsigned char
*cookie, unsigned int cookie_len)
+ {
+ unsigned char *buffer, result[EVP_MAX_MD_SIZE];
+ unsigned int length, resultlength;
+ struct sockaddr_in peer;
+
+ /* If secret isn't initialized yet, the cookie can't be valid */
+ if (!cookie_initialized)
+ return 0;
+
+ /* Read peer information */
+ BIO_dgram_get_peer(SSL_get_rbio(ssl), &peer);
+
+ /* Create buffer with peer's address and port */
+ length = sizeof(peer.sin_addr);
+ length += sizeof(peer.sin_port);
+ buffer = OPENSSL_malloc(length);
+
+ if (buffer == NULL)
+ {
+ BIO_printf(bio_err,"out of memory\n");
+ return 0;
+ }
+
+ memcpy(buffer, &peer.sin_addr, sizeof(peer.sin_addr));
+ memcpy(buffer + sizeof(peer.sin_addr), &peer.sin_port,
sizeof(peer.sin_port));
+
+ /* Calculate HMAC of buffer using the secret */
+ HMAC(EVP_sha1(), cookie_secret, COOKIE_SECRET_LENGTH, buffer,
+ length, (unsigned char*) result, &resultlength);
+ OPENSSL_free(buffer);
+
+ if (cookie_len == resultlength && memcmp(&result, cookie,
resultlength) == 0)
+ return 1;
+
+ return 0;
+ }
--- apps/s_server.c 18 Aug 2009 11:14:12 -0000 1.136.2.7
+++ apps/s_server.c 3 Sep 2009 11:31:32 -0000
@@ -1654,6 +1654,10 @@
SSL_CTX_set_session_id_context(ctx,
(void*)&s_server_session_id_context,
sizeof s_server_session_id_context);
+ /* Set DTLS cookie generation and verification callbacks */
+ SSL_CTX_set_cookie_generate_cb(ctx, generate_cookie_callback);
+ SSL_CTX_set_cookie_verify_cb(ctx, verify_cookie_callback);
+
#ifndef OPENSSL_NO_TLSEXT
if (ctx2)
{
--- crypto/bio/bio.h 24 Jul 2009 11:24:45 -0000 1.76.2.4
+++ crypto/bio/bio.h 3 Sep 2009 11:31:32 -0000
@@ -157,6 +157,7 @@
* previous write
* operation */
+#define BIO_CTRL_DGRAM_GET_PEER 46
#define BIO_CTRL_DGRAM_SET_PEER 44 /* Destination for the
data */
#define BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT 45 /* Next DTLS handshake
timeout to
@@ -538,6 +539,8 @@
(int)BIO_ctrl(b, BIO_CTRL_DGRAM_GET_RECV_TIMER_EXP, 0, NULL)
#define BIO_dgram_send_timedout(b) \
(int)BIO_ctrl(b, BIO_CTRL_DGRAM_GET_SEND_TIMER_EXP, 0, NULL)
+#define BIO_dgram_get_peer(b,peer) \
+ (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)
--- crypto/bio/bss_dgram.c 26 Aug 2009 15:13:43 -0000 1.7.2.16
+++ crypto/bio/bss_dgram.c 3 Sep 2009 11:31:32 -0000
@@ -290,11 +290,11 @@
ret=recvfrom(b->num,out,outl,0,&peer,(void *)&peerlen);
dgram_reset_rcv_timeout(b);
- if ( ! data->connected && ret > 0)
- BIO_ctrl(b, BIO_CTRL_DGRAM_CONNECT, 0, &peer);
+ if ( ! data->connected && ret >= 0)
+ BIO_ctrl(b, BIO_CTRL_DGRAM_SET_PEER, 0, &peer);
BIO_clear_retry_flags(b);
- if (ret <= 0)
+ if (ret < 0)
{
if (BIO_dgram_should_retry(ret))
{
@@ -518,6 +518,12 @@
memset(&(data->peer), 0x00, sizeof(struct sockaddr));
}
break;
+ case BIO_CTRL_DGRAM_GET_PEER:
+ to = (struct sockaddr *) ptr;
+
+ memcpy(to, &(data->peer), sizeof(struct sockaddr));
+ ret = sizeof(struct sockaddr);
+ break;
case BIO_CTRL_DGRAM_SET_PEER:
to = (struct sockaddr *) ptr;
--- ssl/d1_srvr.c 5 Jun 2009 14:46:49 -0000 1.20.2.6
+++ ssl/d1_srvr.c 3 Sep 2009 11:31:33 -0000
@@ -238,11 +238,6 @@
s->state=SSL3_ST_SW_HELLO_REQ_A;
}
- if ( (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE))
- s->d1->send_cookie = 1;
- else
- s->d1->send_cookie = 0;
-
break;
case SSL3_ST_SW_HELLO_REQ_A:
@@ -273,7 +268,7 @@
dtls1_stop_timer(s);
s->new_session = 2;
- if (s->d1->send_cookie)
+ if (ret == 1 && (SSL_get_options(s) &
SSL_OP_COOKIE_EXCHANGE))
s->state = DTLS1_ST_SW_HELLO_VERIFY_REQUEST_A;
else
s->state = SSL3_ST_SW_SRVR_HELLO_A;
@@ -287,7 +282,6 @@
dtls1_start_timer(s);
ret = dtls1_send_hello_verify_request(s);
if ( ret <= 0) goto end;
- s->d1->send_cookie = 0;
s->state=SSL3_ST_SW_FLUSH;
s->s3->tmp.next_state=SSL3_ST_SR_CLNT_HELLO_A;
@@ -670,15 +664,13 @@
*(p++) = s->version >> 8;
*(p++) = s->version & 0xFF;
- if (s->ctx->app_gen_cookie_cb != NULL &&
- s->ctx->app_gen_cookie_cb(s, s->d1->cookie,
- &(s->d1->cookie_len)) == 0)
+ if (s->ctx->app_gen_cookie_cb == NULL ||
+ s->ctx->app_gen_cookie_cb(s, s->d1->cookie,
+ &(s->d1->cookie_len)) == 0)
{
SSLerr(SSL_F_DTLS1_SEND_HELLO_VERIFY_REQUEST,ERR_R_INTERNAL_ERROR);
return 0;
}
- /* else the cookie is assumed to have
- * been initialized by the application */
*(p++) = (unsigned char) s->d1->cookie_len;
memcpy(p, s->d1->cookie, s->d1->cookie_len);
--- ssl/dtls1.h 25 Aug 2009 07:10:09 -0000 1.12.2.13
+++ ssl/dtls1.h 3 Sep 2009 11:31:33 -0000
@@ -88,7 +88,7 @@
#endif
/* lengths of messages */
-#define DTLS1_COOKIE_LENGTH 32
+#define DTLS1_COOKIE_LENGTH 256
#define DTLS1_RT_HEADER_LENGTH 13
--- ssl/s3_srvr.c 26 Jun 2009 15:04:22 -0000 1.171.2.7
+++ ssl/s3_srvr.c 3 Sep 2009 11:31:33 -0000
@@ -816,6 +816,21 @@
goto f_err;
}
+ /* If we require cookies and this ClientHello doesn't
+ * contain one, just return since we do not want to
+ * allocate any memory yet. So check cookie length...
+ */
+ if (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE)
+ {
+ unsigned int session_length, cookie_length;
+
+ session_length = *(p + SSL3_RANDOM_SIZE);
+ cookie_length = *(p + SSL3_RANDOM_SIZE + session_length + 1);
+
+ if (cookie_len == 0)
+ return 1;
+ }
+
/* load the client random */
memcpy(s->s3->client_random,p,SSL3_RANDOM_SIZE);
p+=SSL3_RANDOM_SIZE;
@@ -855,23 +870,11 @@
p+=j;
- if (s->version == DTLS1_VERSION)
+ if (s->version == DTLS1_VERSION || s->version == DTLS1_BAD_VER)
{
/* cookie stuff */
cookie_len = *(p++);
- if ( (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) &&
- s->d1->send_cookie == 0)
- {
- /* HelloVerifyMessage has already been sent */
- if ( cookie_len != s->d1->cookie_len)
- {
- al = SSL_AD_HANDSHAKE_FAILURE;
- SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO,
SSL_R_COOKIE_MISMATCH);
- goto f_err;
- }
- }
-
/*
* The ClientHello may contain a cookie even if the
* HelloVerify message has not been sent--make sure that it
@@ -886,7 +889,7 @@
}
/* verify the cookie if appropriate option is set. */
- if ( (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) &&
+ if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) &&
cookie_len > 0)
{
memcpy(s->d1->rcvd_cookie, p, cookie_len);
@@ -911,6 +914,8 @@
SSL_R_COOKIE_MISMATCH);
goto f_err;
}
+
+ ret = 2;
}
p += cookie_len;
@@ -1185,7 +1190,7 @@
* s->tmp.new_cipher - the new cipher to use.
*/
- ret=1;
+ if (ret < 0) ret=1;
if (0)
{
f_err:
--- ssl/t1_lib.c 28 Apr 2009 22:01:53 -0000 1.64.2.1
+++ ssl/t1_lib.c 3 Sep 2009 11:31:33 -0000
@@ -1444,6 +1444,14 @@
return 1;
if (p >= limit)
return -1;
+ /* Skip past DTLS cookie */
+ if (s->version == DTLS1_VERSION || s->version == DTLS1_BAD_VER)
+ {
+ i = *(p++);
+ p+= i;
+ if (p >= limit)
+ return -1;
+ }
/* Skip past cipher list */
n2s(p, i);
p+= i;
dtls-cookie-management-bugs-0.9.8.patch
Description: Binary data
dtls-cookie-management-bugs-1.0.0.patch
Description: Binary data
