Verify that extensions are used in the correct context

Perl changes reviewed by Richard Levitte. Non-perl changes reviewed by Rich
Salz

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 7e7c3b197157968a989897422d120cfc6bb8a42c..4c306ac8bf99636937b8c73dc1d34449b1fddc39 100644 (file)
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2324,6 +2324,7 @@ int ERR_load_SSL_strings(void);
 # define SSL_R_BAD_DIGEST_LENGTH                          111
 # define SSL_R_BAD_ECC_CERT                               304
 # define SSL_R_BAD_ECPOINT                                306
+# define SSL_R_BAD_EXTENSION                              110
 # define SSL_R_BAD_HANDSHAKE_LENGTH                       332
 # define SSL_R_BAD_HELLO_REQUEST                          105
 # define SSL_R_BAD_KEY_SHARE                              108

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 14a44a5278764f672d526a963e692799b2802aa3..68856eec48f4e04d7e2db75dbde329beb972ab06 100644 (file)
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -354,6 +354,7 @@ static ERR_STRING_DATA SSL_str_reasons[] = {
     {ERR_REASON(SSL_R_BAD_DIGEST_LENGTH), "bad digest length"},
     {ERR_REASON(SSL_R_BAD_ECC_CERT), "bad ecc cert"},
     {ERR_REASON(SSL_R_BAD_ECPOINT), "bad ecpoint"},
+    {ERR_REASON(SSL_R_BAD_EXTENSION), "bad extension"},
     {ERR_REASON(SSL_R_BAD_HANDSHAKE_LENGTH), "bad handshake length"},
     {ERR_REASON(SSL_R_BAD_HELLO_REQUEST), "bad hello request"},
     {ERR_REASON(SSL_R_BAD_KEY_SHARE), "bad key share"},

diff --git a/ssl/statem/statem_extensions.c b/ssl/statem/statem_extensions.c
index 78526bb6871b412f493addf93088f9728a00b972..21ce34ce71e7b1ef7b206dcfde0c9f579e19a056 100644 (file)
--- a/ssl/statem/statem_extensions.c
+++ b/ssl/statem/statem_extensions.c
@@ -11,6 +11,107 @@
 #include "../ssl_locl.h"
 #include "statem_locl.h"
 
+typedef struct {
+    /* The ID for the extension */
+    unsigned int type;
+    int (*server_parse)(SSL *s, PACKET *pkt);
+    int (*client_parse)(SSL *s, PACKET *pkt);
+    unsigned int context;
+} EXTENSION_DEFINITION;
+
+
+static const EXTENSION_DEFINITION ext_defs[] = {
+    {
+        TLSEXT_TYPE_renegotiate,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+    },
+    {
+        TLSEXT_TYPE_server_name,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+        | EXT_TLS1_3_ENCRYPTED_EXTENSIONS
+    },
+    {
+        TLSEXT_TYPE_srp,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+    },
+    {
+        TLSEXT_TYPE_ec_point_formats,
+        NULL, NULL,
+        EXT_CLIENT_HELLO
+    },
+    {
+        TLSEXT_TYPE_supported_groups,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_3_ENCRYPTED_EXTENSIONS
+    },
+    {
+        TLSEXT_TYPE_session_ticket,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+    },
+    {
+        TLSEXT_TYPE_signature_algorithms,
+        NULL, NULL,
+        EXT_CLIENT_HELLO
+    },
+    {
+        TLSEXT_TYPE_status_request,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_3_CERTIFICATE
+    },
+    {
+        TLSEXT_TYPE_next_proto_neg,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+    },
+    {
+        TLSEXT_TYPE_application_layer_protocol_negotiation,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+        | EXT_TLS1_3_ENCRYPTED_EXTENSIONS
+    },
+    {
+        TLSEXT_TYPE_use_srtp,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+        | EXT_TLS1_3_ENCRYPTED_EXTENSIONS | EXT_DTLS_ONLY
+    },
+    {
+        TLSEXT_TYPE_encrypt_then_mac,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+    },
+    {
+        TLSEXT_TYPE_signed_certificate_timestamp,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_3_CERTIFICATE
+    },
+    {
+        TLSEXT_TYPE_extended_master_secret,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO
+    },
+    {
+        TLSEXT_TYPE_supported_versions,
+        NULL, NULL,
+        EXT_CLIENT_HELLO
+    },
+    {
+        TLSEXT_TYPE_padding,
+        NULL, NULL,
+        EXT_CLIENT_HELLO
+    },
+    {
+        TLSEXT_TYPE_key_share,
+        NULL, NULL,
+        EXT_CLIENT_HELLO | EXT_TLS1_3_SERVER_HELLO
+        | EXT_TLS1_3_HELLO_RETRY_REQUEST
+    }
+};
+
 /*
  * Comparison function used in a call to qsort (see tls_collect_extensions()
  * below.)
@@ -35,8 +136,37 @@ static int compare_extensions(const void *p1, const void *p2)
 }
 
 /*
- * Gather a list of all the extensions. We don't actually process the content
- * of the extensions yet, except to check their types.
+ * Verify whether we are allowed to use the extension |type| in the current
+ * |context|. Returns 1 to indicate the extension is allowed or unknown or 0 to
+ * indicate the extension is not allowed.
+ */
+static int verify_extension(SSL *s, unsigned int context, unsigned int type)
+{
+    size_t i;
+
+    for (i = 0; i < OSSL_NELEM(ext_defs); i++) {
+        if (type == ext_defs[i].type) {
+            /* Check we're allowed to use this extension in this context */
+            if ((context & ext_defs[i].context) == 0)
+                return 0;
+            /* Make sure we don't use DTLS extensions in TLS */
+            if ((ext_defs[i].context & EXT_DTLS_ONLY) && !SSL_IS_DTLS(s))
+                return 0;
+
+            return 1;
+        }
+    }
+
+    /* Unknown extension. We allow it */
+    return 1;
+}
+
+/*
+ * Gather a list of all the extensions from the data in |packet]. |context|
+ * tells us which message this extension is for. The raw extension data is
+ * stored in |*res| with the number of found extensions in |*numfound|. In the
+ * event of an error the alert type to use is stored in |*ad|. We don't actually
+ * process the content of the extensions yet, except to check their types.
  *
  * Per http://tools.ietf.org/html/rfc5246#section-7.4.1.4, there may not be
  * more than one extension of the same type in a ClientHello or ServerHello.
@@ -48,8 +178,8 @@ static int compare_extensions(const void *p1, const void *p2)
  * TODO(TLS1.3): Refactor ServerHello extension parsing to use this and then
  * remove tls1_check_duplicate_extensions()
  */
-int tls_collect_extensions(PACKET *packet, RAW_EXTENSION **res,
-                             size_t *numfound, int *ad)
+int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context,
+                           RAW_EXTENSION **res, size_t *numfound, int *ad)
 {
     PACKET extensions = *packet;
     size_t num_extensions = 0, i = 0;
@@ -62,9 +192,16 @@ int tls_collect_extensions(PACKET *packet, RAW_EXTENSION **res,
 
         if (!PACKET_get_net_2(&extensions, &type) ||
             !PACKET_get_length_prefixed_2(&extensions, &extension)) {
+            SSLerr(SSL_F_TLS_COLLECT_EXTENSIONS, SSL_R_BAD_EXTENSION);
             *ad = SSL_AD_DECODE_ERROR;
             goto err;
         }
+        /* Verify this extension is allowed */
+        if (!verify_extension(s, context, type)) {
+            SSLerr(SSL_F_TLS_COLLECT_EXTENSIONS, SSL_R_BAD_EXTENSION);
+            *ad = SSL_AD_ILLEGAL_PARAMETER;
+            goto err;
+        }
         num_extensions++;
     }
 

diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h
index 32f55cca4399281240a563221852a7a1eb68909b..dd815c7174a633a99d14d47b844454d9cb47b0c0 100644 (file)
--- a/ssl/statem/statem_locl.h
+++ b/ssl/statem/statem_locl.h
@@ -26,6 +26,17 @@
 /* Max should actually be 36 but we are generous */
 #define FINISHED_MAX_LENGTH             64
 
+/* Extension context codes */
+#define EXT_DTLS_ONLY                       0x01
+#define EXT_CLIENT_HELLO                    0x02
+/* Really means TLS1.2 or below */
+#define EXT_TLS1_2_SERVER_HELLO             0x04
+#define EXT_TLS1_3_SERVER_HELLO             0x08
+#define EXT_TLS1_3_ENCRYPTED_EXTENSIONS     0x10
+#define EXT_TLS1_3_HELLO_RETRY_REQUEST      0x20
+#define EXT_TLS1_3_CERTIFICATE              0x40
+#define EXT_TLS1_3_NEW_SESSION_TICKET       0x80
+
 /* Message processing return codes */
 typedef enum {
     /* Something bad happened */
@@ -88,8 +99,8 @@ __owur int tls_construct_finished(SSL *s, WPACKET *pkt);
 __owur WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst);
 __owur WORK_STATE dtls_wait_for_dry(SSL *s);
 
-int tls_collect_extensions(PACKET *packet, RAW_EXTENSION **res,
-                             size_t *numfound, int *ad);
+int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context,
+                           RAW_EXTENSION **res, size_t *numfound, int *ad);
 
 /* some client-only functions */
 __owur int tls_construct_client_hello(SSL *s, WPACKET *pkt);

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 50caa42951e37b9c5bfea34b1d626366e07337da..d3491060a9a988f5d46a01c2b58d45e053dca68b 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1249,8 +1249,9 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
 
     /* Preserve the raw extensions PACKET for later use */
     extensions = clienthello.extensions;
-    if (!tls_collect_extensions(&extensions, &clienthello.pre_proc_exts,
-                                  &clienthello.num_extensions, &al)) {
+    if (!tls_collect_extensions(s, &extensions, EXT_CLIENT_HELLO,
+                                &clienthello.pre_proc_exts,
+                                &clienthello.num_extensions, &al)) {
         /* SSLerr already been called */
         goto f_err;
     }