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