Fix the Padding extension

In OpenSSL 1.1.0 the padding extension MUST be last because it calculates
the length of everything that has been written into the ClientHello to
determine whether it needs to be padded or not. With TLSv1.3 that isn't
possible because the specification requires that the PSK extension is last.
Therefore we need to fix the padding extension to take account of any PSK
extension that will be later added.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2968)

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index f11f5e03b970a3cafc4a590903b179acb53ad8b3..d62c5af3b6e04e12b4dcef3975a7f7704be75b44 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -289,7 +289,6 @@ static const EXTENSION_DEFINITION ext_defs[] = {
     },
     {
         /* Must be immediately before pre_shared_key */
-        /* TODO(TLS1.3): Fix me */
         TLSEXT_TYPE_padding,
         EXT_CLIENT_HELLO,
         NULL,

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index 84bfb3c265ed8e7e82f80e54d835fe82a6decbe0..400de3fb30a0f63967580fd16473f8c0ea0d2cc0 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -691,6 +691,20 @@ int tls_construct_ctos_early_data(SSL *s, WPACKET *pkt, unsigned int context,
 #define F5_WORKAROUND_MIN_MSG_LEN   0xff
 #define F5_WORKAROUND_MAX_MSG_LEN   0x200
 
+/*
+ * PSK pre binder overhead =
+ *  2 bytes for TLSEXT_TYPE_psk
+ *  2 bytes for extension length
+ *  2 bytes for identities list length
+ *  2 bytes for identity length
+ *  4 bytes for obfuscated_ticket_age
+ *  2 bytes for binder list length
+ *  1 byte for binder length
+ * The above excludes the number of bytes for the identity itself and the
+ * subsequent binder bytes
+ */
+#define PSK_PRE_BINDER_OVERHEAD (2 + 2 + 2 + 2 + 4 + 2 + 1)
+
 int tls_construct_ctos_padding(SSL *s, WPACKET *pkt, unsigned int context,
                                X509 *x, size_t chainidx, int *al)
 {
@@ -701,16 +715,35 @@ int tls_construct_ctos_padding(SSL *s, WPACKET *pkt, unsigned int context,
         return 1;
 
     /*
-     * Add padding to workaround bugs in F5 terminators. See
-     * https://tools.ietf.org/html/draft-agl-tls-padding-03 NB: because this
-     * code calculates the length of all existing extensions it MUST always
-     * appear last.
+     * Add padding to workaround bugs in F5 terminators. See RFC7685.
+     * This code calculates the length of all extensions added so far but
+     * excludes the PSK extension (because that MUST be written last). Therefore
+     * this extension MUST always appear second to last.
      */
     if (!WPACKET_get_total_written(pkt, &hlen)) {
         SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_PADDING, ERR_R_INTERNAL_ERROR);
         return 0;
     }
 
+    /*
+     * If we're going to send a PSK then that will be written out after this
+     * extension, so we need to calculate how long it is going to be.
+     */
+    if (s->session->ssl_version == TLS1_3_VERSION
+            && s->session->ext.ticklen != 0
+            && s->session->cipher != NULL) {
+        const EVP_MD *md = ssl_md(s->session->cipher->algorithm2);
+
+        if (md != NULL) {
+            /*
+             * Add the fixed PSK overhead, the identity length and the binder
+             * length.
+             */
+            hlen +=  PSK_PRE_BINDER_OVERHEAD + s->session->ext.ticklen
+                     + EVP_MD_size(md);
+        }
+    }
+
     if (hlen > F5_WORKAROUND_MIN_MSG_LEN && hlen < F5_WORKAROUND_MAX_MSG_LEN) {
         /* Calculate the amond of padding we need to add */
         hlen = F5_WORKAROUND_MAX_MSG_LEN - hlen;
@@ -750,6 +783,12 @@ int tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, X509 *x,
 
     s->session->ext.tick_identity = TLSEXT_PSK_BAD_IDENTITY;
 
+    /*
+     * Note: At this stage of the code we only support adding a single
+     * resumption PSK. If we add support for multiple PSKs then the length
+     * calculations in the padding extension will need to be adjusted.
+     */
+
     /*
      * If this is an incompatible or new session then we have nothing to resume
      * so don't add this extension.