Skip to contentSkip to conversation

--tls-crypt fixes

PATCH In review Steffan Karger 2 participants 2 messages Nov 22, 2016

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 --tls-crypt fixes

* Check return value of buf_init() (found by coverity)

* Use the TLS frame to determine the buffer size, as is done for the reliability buffers used for tls-auth. (We previously incorrectly used the TLS *plaintext* buffer size, which is bigger for typical setups with tun-mtu <= 1500. Using the frame to calculate the size saves some bytes for typical setups, and doesn't break setups with big tun-mtu.)

* More carefully handle errors in tls_crypt_wrap() - just drop the packet instead of ASSERT()ing out (should not happen in the first place, but this is a bit more friendly if it happens somehow anyway).

---
 src/openvpn/ssl.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
src/openvpn/ssl.c +13 -6
index 4198741..1ae1547 100644
@@ -975,7 +975,7 @@ tls_session_init (struct tls_multi *multi, struct tls_session *session)
 
   /* Initialize control channel authentication parameters */
   session->tls_wrap = session->opt->tls_wrap;
-  session->tls_wrap.work = alloc_buf (TLS_CHANNEL_BUF_SIZE);
+  session->tls_wrap.work = alloc_buf (BUF_SIZE (&session->opt->frame));
 
   /* initialize packet ID replay window for --tls-auth */
   packet_id_init (&session->tls_wrap.opt.packet_id,
@@ -1322,13 +1322,20 @@ write_control_auth (struct tls_session *session,
     }
   else if (session->tls_wrap.mode == TLS_WRAP_CRYPT)
     {
-      buf_init (&session->tls_wrap.work, buf->offset);
+      ASSERT (buf_init (&session->tls_wrap.work, buf->offset));
       ASSERT (buf_write (&session->tls_wrap.work, &header, sizeof(header)));
       ASSERT (session_id_write (&session->session_id, &session->tls_wrap.work));
-      ASSERT (tls_crypt_wrap (buf, &session->tls_wrap.work, &session->tls_wrap.opt));
-      /* Don't change the original data in buf, it's used by the reliability
-       * layer to resend on failure. */
-      *buf = session->tls_wrap.work;
+      if (tls_crypt_wrap (buf, &session->tls_wrap.work, &session->tls_wrap.opt))
+	{
+	  /* Don't change the original data in buf, it's used by the reliability
+	   * layer to resend on failure. */
+	  *buf = session->tls_wrap.work;
+	}
+      else
+	{
+	  buf->len = 0;
+	  return;
+	}
     }
   *to_link_addr = &ks->remote_addr;
 }
Signature · 1 line
2.7.4
#1 lore raw

ACK, makes sense. And compiles and tests :)

Your patch has been applied to the master branch.

commit 418d2d98489dfe7afafcaf21828541d034afb7f4

Author: Steffan Karger
Date:   Tue Nov 22 21:41:26 2016 +0100
     --tls-crypt fixes

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13204.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering