From 310115448188415e270bb0bef958c7c130939838 Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Tue, 6 Oct 2015 17:20:32 +0200 Subject: [PATCH] DTLS: remove unused cookie field Note that this commit constifies a user callback parameter and therefore will break compilation for applications using this callback. But unless they are abusing write access to the buffer, the fix is trivial. Reviewed-by: Andy Polyakov --- apps/s_apps.h | 2 +- apps/s_cb.c | 2 +- include/openssl/ssl.h | 2 +- ssl/d1_lib.c | 6 +++--- ssl/packet_locl.h | 13 +++++++++++++ ssl/s3_srvr.c | 39 +++++++-------------------------------- ssl/ssl_locl.h | 3 +-- ssl/ssl_sess.c | 2 +- test/packettest.c | 20 ++++++++++++++++++++ 9 files changed, 48 insertions(+), 41 deletions(-) diff --git a/apps/s_apps.h b/apps/s_apps.h index c8069a0f6e..55dc9f1ffc 100644 --- a/apps/s_apps.h +++ b/apps/s_apps.h @@ -195,7 +195,7 @@ void tlsext_cb(SSL *s, int client_server, int type, unsigned char *data, int generate_cookie_callback(SSL *ssl, unsigned char *cookie, unsigned int *cookie_len); -int verify_cookie_callback(SSL *ssl, unsigned char *cookie, +int verify_cookie_callback(SSL *ssl, const unsigned char *cookie, unsigned int cookie_len); typedef struct ssl_excert_st SSL_EXCERT; diff --git a/apps/s_cb.c b/apps/s_cb.c index 643d91a160..884b5e1cf3 100644 --- a/apps/s_cb.c +++ b/apps/s_cb.c @@ -806,7 +806,7 @@ int generate_cookie_callback(SSL *ssl, unsigned char *cookie, return 1; } -int verify_cookie_callback(SSL *ssl, unsigned char *cookie, +int verify_cookie_callback(SSL *ssl, const unsigned char *cookie, unsigned int cookie_len) { unsigned char *buffer, result[EVP_MAX_MD_SIZE]; diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 0727e7f78d..25ceca88f1 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -750,7 +750,7 @@ void SSL_CTX_set_cookie_generate_cb(SSL_CTX *ctx, *cookie_len)); void SSL_CTX_set_cookie_verify_cb(SSL_CTX *ctx, int (*app_verify_cookie_cb) (SSL *ssl, - unsigned char + const unsigned char *cookie, unsigned int cookie_len)); diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index 4bdf90a657..3a0a4cf443 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c @@ -723,9 +723,9 @@ int dtls1_listen(SSL *s, struct sockaddr *client) /* This is fatal */ return -1; } - if (PACKET_remaining(&cookiepkt) > sizeof(s->d1->rcvd_cookie) - || s->ctx->app_verify_cookie_cb(s, PACKET_data(&cookiepkt), - PACKET_remaining(&cookiepkt)) == 0) { + if (s->ctx->app_verify_cookie_cb(s, PACKET_data(&cookiepkt), + PACKET_remaining(&cookiepkt)) == + 0) { /* * We treat invalid cookies in the same was as no cookie as * per RFC6347 diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h index 9354e6c998..778ec774bc 100644 --- a/ssl/packet_locl.h +++ b/ssl/packet_locl.h @@ -62,6 +62,7 @@ # include # include # include +# include # include "e_os.h" # ifdef __cplusplus @@ -124,6 +125,18 @@ static inline void PACKET_null_init(PACKET *pkt) pkt->remaining = 0; } +/* + * Returns 1 if the packet has length |num| and its contents equal the |num| + * bytes read from |ptr|. Returns 0 otherwise (lengths or contents not equal). + * If lengths are equal, performs the comparison in constant time. + */ +__owur static inline int PACKET_equal(const PACKET *pkt, const void *ptr, + size_t num) { + if (PACKET_remaining(pkt) != num) + return 0; + return CRYPTO_memcmp(pkt->curr, ptr, num) == 0; +} + /* * Peek ahead and initialize |subpkt| with the next |len| bytes read from |pkt|. * Data is not copied: the |subpkt| packet will share its underlying buffer with diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 5f05b9f21f..ca11c6e8b3 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -1137,45 +1137,20 @@ int ssl3_get_client_hello(SSL *s) } if (SSL_IS_DTLS(s)) { - size_t cookie_len = PACKET_remaining(&cookie); - /* - * The ClientHello may contain a cookie even if the - * HelloVerify message has not been sent--make sure that it - * does not cause an overflow. - */ - if (cookie_len > sizeof(s->d1->rcvd_cookie)) { - /* too much data */ - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); - goto f_err; - } - - /* verify the cookie if appropriate option is set. */ - if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) && cookie_len > 0) { - /* Get cookie */ - /* - * TODO(openssl-team): rcvd_cookie appears unused outside this - * function. Remove the field? - */ - if (!PACKET_copy_bytes(&cookie, s->d1->rcvd_cookie, cookie_len)) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); - goto f_err; - } - + /* Empty cookie was already handled above by returning early. */ + if (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) { if (s->ctx->app_verify_cookie_cb != NULL) { - if (s->ctx->app_verify_cookie_cb(s, s->d1->rcvd_cookie, - cookie_len) == 0) { + if (s->ctx->app_verify_cookie_cb(s, PACKET_data(&cookie), + PACKET_remaining(&cookie)) == 0) { al = SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); goto f_err; + /* else cookie verification succeeded */ } - /* else cookie verification succeeded */ - } /* default verification */ - else if (memcmp(s->d1->rcvd_cookie, s->d1->cookie, - s->d1->cookie_len) != 0) { + } else if (!PACKET_equal(&cookie, s->d1->cookie, + s->d1->cookie_len)) { al = SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_COOKIE_MISMATCH); goto f_err; diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 7c57509fbc..ad6ae0ebca 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -798,7 +798,7 @@ struct ssl_ctx_st { unsigned int *cookie_len); /* verify cookie callback */ - int (*app_verify_cookie_cb) (SSL *ssl, unsigned char *cookie, + int (*app_verify_cookie_cb) (SSL *ssl, const unsigned char *cookie, unsigned int cookie_len); CRYPTO_EX_DATA ex_data; @@ -1421,7 +1421,6 @@ typedef struct hm_fragment_st { typedef struct dtls1_state_st { unsigned int send_cookie; unsigned char cookie[DTLS1_COOKIE_LENGTH]; - unsigned char rcvd_cookie[DTLS1_COOKIE_LENGTH]; unsigned int cookie_len; /* handshake message numbers */ diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 7660292196..6f46b9f37e 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -1217,7 +1217,7 @@ void SSL_CTX_set_cookie_generate_cb(SSL_CTX *ctx, } void SSL_CTX_set_cookie_verify_cb(SSL_CTX *ctx, - int (*cb) (SSL *ssl, unsigned char *cookie, + int (*cb) (SSL *ssl, const unsigned char *cookie, unsigned int cookie_len)) { ctx->app_verify_cookie_cb = cb; diff --git a/test/packettest.c b/test/packettest.c index edaa2824ba..ac360f59bd 100644 --- a/test/packettest.c +++ b/test/packettest.c @@ -360,6 +360,25 @@ static int test_PACKET_null_init() return 1; } +static int test_PACKET_equal(unsigned char buf[BUF_LEN]) +{ + PACKET pkt; + + if ( !PACKET_buf_init(&pkt, buf, 4) + || !PACKET_equal(&pkt, buf, 4) + || PACKET_equal(&pkt, buf + 1, 4) + || !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_equal(&pkt, buf, BUF_LEN) + || PACKET_equal(&pkt, buf, BUF_LEN - 1) + || PACKET_equal(&pkt, buf, BUF_LEN + 1) + || PACKET_equal(&pkt, buf, 0)) { + fprintf(stderr, "test_PACKET_equal() failed\n"); + return 0; + } + + return 1; +} + static int test_PACKET_get_length_prefixed_1() { unsigned char buf[BUF_LEN]; @@ -452,6 +471,7 @@ int main(int argc, char **argv) if ( !test_PACKET_buf_init() || !test_PACKET_null_init() || !test_PACKET_remaining(buf) + || !test_PACKET_equal(buf) || !test_PACKET_get_1(buf) || !test_PACKET_get_4(buf) || !test_PACKET_get_net_2(buf) -- 2.34.1