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 <appro@openssl.org>

diff --git a/apps/s_apps.h b/apps/s_apps.h
index c8069a0f6e66b701204dd6ea4735d9a4f593e4ad..55dc9f1ffcbc34ea09176b5d935ad510201e7c84 100644 (file)
--- 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 643d91a160bc6118c852972b14c981cc74e81cef..884b5e1cf3b59e13d7e930051681b43be0ef6c2b 100644 (file)
--- 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 0727e7f78d1665b8282b13c1893d04ff6d2e39bc..25ceca88f1b8e59817b9d08a9ebdf7b6eb29f7d8 100644 (file)
--- 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 4bdf90a6575ffa8c6d2ac455589288df22594fb6..3a0a4cf4435063fda68d0cac632a092d1742f33a 100644 (file)
--- 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 9354e6c99834ef87e3db9188d8daa590af274ed1..778ec774bc52ffd5d75298a22dee8b9aa13c331f 100644 (file)
--- a/ssl/packet_locl.h
+++ b/ssl/packet_locl.h
@@ -62,6 +62,7 @@
 # include <string.h>
 # include <openssl/bn.h>
 # include <openssl/buffer.h>
+# include <openssl/crypto.h>
 # 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 5f05b9f21fe39848236218589f125c99d6bee849..ca11c6e8b388d8606ff8f654d82ac5e556b5d48d 100644 (file)
--- 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 7c57509fbc37edd18da433b5caa89f74c9517ac1..ad6ae0ebca6c0a6185453e1e9936d91247172329 100644 (file)
--- 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 766029219680bc761292762d7bd1c5f6251e984d..6f46b9f37e7777a83cbe0e08711c064bcb57d237 100644 (file)
--- 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 edaa2824baf74fc40443e517d452ceadc9d270d4..ac360f59bdc6441b212b9ad89030513935746fc3 100644 (file)
--- 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)