[M] Change in openvpn[master]: ssl: use TLS record-sized buffers for key method 2 exchange

Attention is currently required from: Bluca, plaisthos, selvanair.

Hello plaisthos, selvanair, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1622?usp=email

to look at the new patch set (#3).

The following approvals got outdated and were removed:
Code-Review-1 by plaisthos


Change subject: ssl: use TLS record-sized buffers for key method 2 exchange
......................................................................

ssl: use TLS record-sized buffers for key method 2 exchange

The current key exchange uses plaintext_read_buf/plaintext_write_buf
(sized at TLS_CHANNEL_BUF_SIZE, 2048 bytes) as intermediary buffers
for the key method 2 exchange. Passwords or tokens longer than
~1900 bytes (after accounting for key material, options and peer
info overhead) get silently truncated.

This breaks using JIT use-once tokens for authentication, which are
becoming common in enterprise setups. These tokens are typically
long JWT-encoded strings that exceed the 2048-byte buffer.

Instead of increasing TLS_CHANNEL_BUF_SIZE (which would change the
control channel framing and require both endpoints to be updated),
introduce separate key_method_send_buf and key_method_recv_buf
buffers in key_state, sized at TLS_RECORD_MAX_SIZE (16384 bytes,
the maximum plaintext payload of a single TLS record).

On the write side, key_method_2_write() assembles the full payload
into the 16 KB buffer, which is then passed to the TLS library via
a single key_state_write_plaintext() call. The TLS library creates
one TLS record, and the existing write_outgoing_tls_ciphertext()
already splits the resulting ciphertext into properly-sized reliable
transport packets.

On the read side, a single key_state_read_plaintext() call reads
into the 16 KB buffer. Since SSL_read/BIO_read returns one TLS
record worth of data per call, this preserves the one-read-per-
message framing assumption used throughout OpenVPN.

All other control channel messages (push, CR_RESPONSE, etc.)
continue to use the original 2048-byte plaintext_read/write_buf.

When the key exchange payload exceeds TLS_CHANNEL_BUF_SIZE and the
connection subsequently fails (state never reaches S_ACTIVE), a
diagnostic message is printed at key_state teardown to help the
user identify that the server may not support large payloads.

Protocol compatibility:
- old client <-> new server: no change, old limits apply
- new client <-> old server: large passwords will cause the old
  server to truncate/fail; a clear error message is shown
- new client <-> new server: large passwords work

Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Signed-off-by: Luca Boccassi <[email protected]>
---
M src/openvpn/common.h
M src/openvpn/ssl.c
M src/openvpn/ssl_common.h
3 files changed, 88 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/22/1622/3

diff --git a/src/openvpn/common.h b/src/openvpn/common.h
index aa7b721..b1a8516 100644
--- a/src/openvpn/common.h
+++ b/src/openvpn/common.h
@@ -69,6 +69,17 @@
  */
 #define TLS_CHANNEL_BUF_SIZE 2048
 
+/*
+ * Buffer size for key method 2 exchange data (send and receive).
+ * A single TLS record can carry up to 2^14 (16384) bytes of
+ * plaintext. We size the key exchange buffers to a full TLS
+ * record so that payloads larger than TLS_CHANNEL_BUF_SIZE
+ * (e.g. with long PKCS#11 passwords/tokens) can be sent and
+ * received in a single TLS read/write without breaking the
+ * one-read-per-message assumption used elsewhere.
+ */
+#define TLS_RECORD_MAX_SIZE (1 << 14)
+
 /* TLS control buffer minimum size
  *
  * A control frame might have IPv6 header (40 byte),
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 576157d..c8bf35c 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -860,6 +860,8 @@
     ks->plaintext_read_buf = alloc_buf(TLS_CHANNEL_BUF_SIZE);
     ks->plaintext_write_buf = alloc_buf(TLS_CHANNEL_BUF_SIZE);
     ks->ack_write_buf = alloc_buf(BUF_SIZE(&session->opt->frame));
+    ks->key_method_send_buf = alloc_buf(TLS_RECORD_MAX_SIZE);
+    ks->key_method_recv_buf = alloc_buf(TLS_RECORD_MAX_SIZE);
     reliable_init(ks->send_reliable, BUF_SIZE(&session->opt->frame),
                   session->opt->frame.buf.headroom, TLS_RELIABLE_N_SEND_BUFFERS,
                   ks->key_id ? false : session->opt->xmit_hold);
@@ -906,6 +908,17 @@
 static void
 key_state_free(struct key_state *ks, bool clear)
 {
+    if (ks->key_method_large_payload && ks->state >= S_SENT_KEY
+        && ks->state < S_ACTIVE)
+    {
+        /* Servers before a7f80d402f do not send back a useful error so print one */
+        msg(M_WARN, "Connection failed after sending a key exchange "
+            "payload larger than %d bytes (due to a long password). "
+            "This may happen when the server does not support large "
+            "key exchange payloads, try with a smaller password.",
+            TLS_CHANNEL_BUF_SIZE);
+    }
+
     ks->state = S_UNDEF;
 
     key_state_ssl_free(&ks->ks_ssl);
@@ -915,6 +928,8 @@
     free_buf(&ks->plaintext_read_buf);
     free_buf(&ks->plaintext_write_buf);
     free_buf(&ks->ack_write_buf);
+    free_buf(&ks->key_method_send_buf);
+    free_buf(&ks->key_method_recv_buf);
     buffer_list_free(ks->paybuf);
 
     reliable_free(ks->send_reliable);
@@ -2836,38 +2851,61 @@
     }
 
     /* Read incoming plaintext from TLS object */
-    struct buffer *buf = &ks->plaintext_read_buf;
-    if (!buf->len)
+    bool in_key_exchange_read =
+        (ks->state == S_SENT_KEY && !session->opt->server)
+        || (ks->state == S_START && session->opt->server);
+
+    if (in_key_exchange_read)
     {
-        if (!read_incoming_tls_plaintext(ks, buf, wakeup, &continue_tls_process))
+        /* For key exchange, read into a buffer large enough for a full
+         * TLS record (16 KB) so that long passwords/tokens that exceed
+         * TLS_CHANNEL_BUF_SIZE are received in a single read. */
+        if (!BLEN(&ks->key_method_recv_buf))
         {
-            goto error;
+            if (!read_incoming_tls_plaintext(ks, &ks->key_method_recv_buf,
+                                             wakeup, &continue_tls_process))
+            {
+                goto error;
+            }
+        }
+    }
+    else
+    {
+        struct buffer *buf = &ks->plaintext_read_buf;
+        if (!buf->len)
+        {
+            if (!read_incoming_tls_plaintext(ks, buf, wakeup, &continue_tls_process))
+            {
+                goto error;
+            }
         }
     }
 
-    /* Send Key */
-    buf = &ks->plaintext_write_buf;
-    if (!buf->len
+    /* Send Key -- use larger key_method_send_buf for assembly to support
+     * long passwords/tokens that exceed TLS_CHANNEL_BUF_SIZE */
+    if (!ks->key_method_send_buf.len
         && ((ks->state == S_START && !session->opt->server)
             || (ks->state == S_GOT_KEY && session->opt->server)))
     {
-        if (!key_method_2_write(buf, multi, session))
+        if (!key_method_2_write(&ks->key_method_send_buf, multi, session))
         {
             goto error;
         }
 
+        ks->key_method_large_payload =
+            BLEN(&ks->key_method_send_buf) > TLS_CHANNEL_BUF_SIZE;
+
         continue_tls_process = true;
         dmsg(D_TLS_DEBUG_MED, "STATE S_SENT_KEY");
         ks->state = S_SENT_KEY;
     }
 
-    /* Receive Key */
-    buf = &ks->plaintext_read_buf;
-    if (buf->len
+    /* Receive Key -- use the larger key_method_recv_buf */
+    if (BLEN(&ks->key_method_recv_buf)
         && ((ks->state == S_SENT_KEY && !session->opt->server)
             || (ks->state == S_START && session->opt->server)))
     {
-        if (!key_method_2_read(buf, multi, session))
+        if (!key_method_2_read(&ks->key_method_recv_buf, multi, session))
         {
             goto error;
         }
@@ -2877,20 +2915,38 @@
         ks->state = S_GOT_KEY;
     }
 
-    /* Write outgoing plaintext to TLS object */
-    buf = &ks->plaintext_write_buf;
-    if (buf->len)
+    /* Write key exchange data to TLS object */
+    if (ks->key_method_send_buf.len)
     {
-        int status = key_state_write_plaintext(&ks->ks_ssl, buf);
+        int status = key_state_write_plaintext(&ks->ks_ssl, &ks->key_method_send_buf);
         if (status == -1)
         {
-            msg(D_TLS_ERRORS, "TLS ERROR: Outgoing Plaintext -> TLS object write error");
+            msg(D_TLS_ERRORS, "TLS ERROR: Key Method -> TLS object write error");
             goto error;
         }
         if (status == 1)
         {
             continue_tls_process = true;
-            dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
+            dmsg(D_TLS_DEBUG, "Key Method -> TLS");
+        }
+    }
+
+    /* Write outgoing plaintext to TLS object */
+    {
+        struct buffer *buf = &ks->plaintext_write_buf;
+        if (buf->len)
+        {
+            int status = key_state_write_plaintext(&ks->ks_ssl, buf);
+            if (status == -1)
+            {
+                msg(D_TLS_ERRORS, "TLS ERROR: Outgoing Plaintext -> TLS object write error");
+                goto error;
+            }
+            if (status == 1)
+            {
+                continue_tls_process = true;
+                dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
+            }
         }
     }
     if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 6f310a5..913a0c2 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -241,6 +241,9 @@
     struct buffer plaintext_read_buf;
     struct buffer plaintext_write_buf;
     struct buffer ack_write_buf;
+    struct buffer key_method_send_buf; /* larger buffer for key method 2 write */
+    struct buffer key_method_recv_buf; /* larger buffer for key method 2 read */
+    bool key_method_large_payload;     /* key exchange exceeded TLS_CHANNEL_BUF_SIZE */
 
     struct reliable *send_reliable; /* holds a copy of outgoing packets until ACK received */
     struct reliable *rec_reliable;  /* order incoming ciphertext packets before we pass to TLS */

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1622?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 3
Gerrit-Owner: Bluca <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: selvanair <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: Bluca <[email protected]>
Gerrit-Attention: selvanair <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel