Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1622?usp=email
to review the following change.
Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START)
with large passwords
......................................................................
Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
The current key exchange uses an intermediate buffer hardcoded
at 2048 bytes, that cannot handle anything longer, as it gets
silently truncated.
This breaks using JIT use-once tokens for authentications, that
are becoming common in enterprise devices to reduce attack
surfaces, as the tokens are typically long JWT encoded strings.
Add new helpers that, instead of doing a single 2048 bytes
read/write from/to the TLS layer, reads/writes all the available
data when doing the key method 2 exchange.
Other stages of the protocol are unaffected. Note that this is
an intermediary buffer, the TLS layer is already handling
fragmented/reassembled frames appropriately, so there is no
on-the-wire difference, aside from letting the client send more
data.
Older servers will simply continue to truncate passwords longer
than the existing limit, as they are doing currently, so no
incompatible changes, behaviour stays the same and current valid
passwords are still valid when the new client uses them.
When an old client talks to a new server, likewise there are no
changes, as the previous clients have the embedded limit, so all
existing passwords will continue to work.
Tested combinations:
old client <--> new server
new client <--> old server
new client <--> new server
With the last combination, a large password can be used successfully.
With either an old server or old client, existing limits apply and
a clear error is shown in case one tries a password longer than the
existing limits.
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, 116 insertions(+), 17 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/22/1622/1
diff --git a/src/openvpn/common.h b/src/openvpn/common.h
index aa7b721..b67a6f4 100644
--- a/src/openvpn/common.h
+++ b/src/openvpn/common.h
@@ -69,6 +69,16 @@
*/
#define TLS_CHANNEL_BUF_SIZE 2048
+/*
+ * Buffer size for key method 2 exchange data. This needs to be
+ * large enough to hold the key source material, options string,
+ * username, password and peer info. When USER_PASS_LEN is larger
+ * than the default TLS channel size (e.g. with ENABLE_PKCS11),
+ * we need a larger buffer. The key exchange write and read paths
+ * handle this by doing multiple TLS writes/reads as needed.
+ */
+#define KEY_METHOD_BUF_SIZE (TLS_CHANNEL_BUF_SIZE + 2 * USER_PASS_LEN)
+
/* 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..83bc46e 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(KEY_METHOD_BUF_SIZE);
+ ks->key_method_recv_buf = alloc_buf(KEY_METHOD_BUF_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);
@@ -915,6 +917,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);
@@ -2624,6 +2628,54 @@
return true;
}
+/**
+ * Read all currently available plaintext from the TLS object into buf,
+ * doing multiple reads if necessary. This is used for key method 2
+ * exchange data which may exceed a single TLS_CHANNEL_BUF_SIZE read
+ * when long passwords/tokens are used.
+ */
+static bool
+read_incoming_tls_plaintext_multi(struct key_state *ks, struct buffer *buf,
+ interval_t *wakeup, bool
*continue_tls_process)
+{
+ struct buffer tmp = alloc_buf(TLS_CHANNEL_BUF_SIZE);
+
+ while (buf_forward_capacity(buf) > 0)
+ {
+ buf_init(&tmp, 0);
+ int status = key_state_read_plaintext(&ks->ks_ssl, &tmp);
+
+ update_time();
+ if (status == -1)
+ {
+ msg(D_TLS_ERRORS,
+ "TLS Error: TLS object -> incoming plaintext read error");
+ free_buf(&tmp);
+ return false;
+ }
+ if (status == 1)
+ {
+ if (!buf_write(buf, BPTR(&tmp), BLEN(&tmp)))
+ {
+ msg(D_TLS_ERRORS,
+ "TLS Error: key exchange plaintext too large");
+ free_buf(&tmp);
+ return false;
+ }
+ *continue_tls_process = true;
+ *wakeup = 0;
+ dmsg(D_TLS_DEBUG, "TLS -> Incoming Plaintext (key exchange)");
+ }
+ else
+ {
+ break;
+ }
+ }
+
+ free_buf(&tmp);
+ return true;
+}
+
static bool
write_outgoing_tls_ciphertext(struct tls_session *session, bool
*continue_tls_process)
{
@@ -2836,22 +2888,40 @@
}
/* 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 all available plaintext into a buffer
+ * that is large enough for long passwords/tokens. Multiple reads
+ * are done to handle data exceeding TLS_CHANNEL_BUF_SIZE. */
+ if (!read_incoming_tls_plaintext_multi(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;
}
@@ -2861,13 +2931,12 @@
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 +2946,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..6f3da11 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -241,6 +241,8 @@
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
*/
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: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 1
Gerrit-Owner: Bluca <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
