Reuse extension_is_relevant() in should_add_extension()

At the core of things is the concept that each extension is only
defined in certain context(s) -- the ClientHello, EncryptedExtensions,
etc., and sometimes only for a specific protocol or protocol range;
we want to enforce that we only parse or generate extensions in the
context(s) for which they are defined.  There is some subtlety here,
in that the protocol version in use is not known when generating the
ClientHello (but it is known when the ClientHello extensions are
being parsed!), so the SSL_IS_TLS13() macro must be used with caution.
Nonetheless, by making assertions about whether we are acting in a
server role and whether the current context is (not) a ClientHello,
we can consolidate almost all of the logic for determining whether
an extension is permitted in a given protocol message, whether we
are generating or parsing that message.

The only logic that remains separate relates to generating the ClientHello,
as it depends on an external factor (the maximum permitted TLS version) that
is not defined in the parsing context.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2945)

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 8a8e524899cba28d0974e968ccbe823d8f3afbff..0641a253d3a831c7c7bb759ae77ab9904f0b05db 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -516,11 +516,20 @@ int extension_is_relevant(SSL *s, unsigned int extctx, unsigned int thisctx)
                 && (extctx & SSL_EXT_TLS_IMPLEMENTATION_ONLY) != 0)
             || (s->version == SSL3_VERSION
                     && (extctx & SSL_EXT_SSL3_ALLOWED) == 0)
+            /*
+             * Note that SSL_IS_TLS13() means "TLS 1.3 has been negotiated",
+             * which is never true when generating the ClientHello.
+             * However, version negotiation *has* occurred by the time the
+             * ClientHello extensions are being parsed.
+             * Be careful to allow TLS 1.3-only extensions when generating
+             * the ClientHello.
+             */
             || (is_tls13 && (extctx & SSL_EXT_TLS1_2_AND_BELOW_ONLY) != 0)
-            || (!is_tls13 && (extctx & SSL_EXT_TLS1_3_ONLY) != 0)
+            || (!is_tls13 && (extctx & SSL_EXT_TLS1_3_ONLY) != 0
+                && (thisctx & SSL_EXT_CLIENT_HELLO) == 0)
+            || (s->server && !is_tls13 && (extctx & SSL_EXT_TLS1_3_ONLY) != 0)
             || (s->hit && (extctx & SSL_EXT_IGNORE_ON_RESUMPTION) != 0))
         return 0;
-
     return 1;
 }
 
@@ -762,14 +771,7 @@ int should_add_extension(SSL *s, unsigned int extctx, unsigned int thisctx,
         return 0;
 
     /* Check if this extension is defined for our protocol. If not, skip */
-    if ((SSL_IS_DTLS(s) && (extctx & SSL_EXT_TLS_IMPLEMENTATION_ONLY) != 0)
-            || (s->version == SSL3_VERSION
-                    && (extctx & SSL_EXT_SSL3_ALLOWED) == 0)
-            || (SSL_IS_TLS13(s)
-                && (extctx & SSL_EXT_TLS1_2_AND_BELOW_ONLY) != 0)
-            || (!SSL_IS_TLS13(s)
-                && (extctx & SSL_EXT_TLS1_3_ONLY) != 0
-                && (thisctx & SSL_EXT_CLIENT_HELLO) == 0)
+    if (!extension_is_relevant(s, extctx, thisctx)
             || ((extctx & SSL_EXT_TLS1_3_ONLY) != 0
                 && (thisctx & SSL_EXT_CLIENT_HELLO) != 0
                 && (SSL_IS_DTLS(s) || max_version < TLS1_3_VERSION)))