[Openvpn-devel] [PATCH] Fix for bug #49 for openvpn 2.2.2
Hi,
our setup needs openvpn UDP/TLS tunnels with dynamic client IP addresses, so I
implemented a fix for the bug #49 that has been open for over two years.
The patch is for version 2.2.2 as I had trouble compiling the 2.3.x tarball
from openvpn.net. As the solution is rather simple (just two small utility
functions in mudp.c) I'd guess it could be comfortambly migrated to 2.3.x.
Basically what the fix does is the following: incoming data channel UDP packets
from an unknown IP are matched against existing UDP/TLS sessions, and if the
packet passes the HMAC authentication against an existing TLS context we know
the client IP has changed and the session state will be instantly updated
accordingly.
I have tested this fix to some extent, and the IP handover works impressively
smoothly in my test setup where I randomly switch between two routes from
client to server.
Dynamic client IP's are enabled/disabled with --float in the server side.
Please feel free to contact me for any questions etc.
Best regards,
Mikko Vainikainen
From c00381f1b53c5169dba1b2e4bf22bb6e50e4eaea Mon Sep 17 00:00:00 2001
From: Mikko Vainikainen
Date: Fri, 8 Mar 2013 12:00:52 +0200
Subject: [PATCH] Fixes bug #49, support for dynamic client IP over UDP/TLS.
---
mudp.c | 166
1 file changed, 166 insertions(+)
diff --git a/mudp.c b/mudp.c
index a478b29..ef5bb3c 100644
--- a/mudp.c
+++ b/mudp.c
@@ -30,6 +30,161 @@
#include "forward-inline.h"
#include "memdbg.h"
+#include "socket.h"
+
+
+/*
+ * Helper function for authenticating packets against existing TLS context.
+ */
+
+int authenticate_tls_packet(struct multi_context* m,
+struct context* ctx,
+struct buffer* buf,
+struct tls_multi* multi)
+{
+ int i;
+ int op;
+ int key_id;
+ int status;
+ struct link_socket_actual* from = &m->top.c2.from;
+
+ if (buf-> len <= 0) return 0;
+
+ {
+uint8_t c = *BPTR(buf);
+op = c >> P_OPCODE_SHIFT;
+key_id = c & P_KEY_ID_MASK;
+ }
+
+ /*
+ * we are only interested in data channel packets
+ */
+
+ if (op != P_DATA_V1) return 0;
+
+ for (i = 0; i < KEY_SCAN_SIZE; ++i)
+{
+
+ struct key_state* ks = multi->key_scan[i];
+ struct key_ctx* kc = &ks->key.decrypt;
+ int hmac_len;
+ int in_hmac_len;
+ uint8_t local_hmac[MAX_HMAC_KEY_LENGTH];
+
+ if (!DECRYPT_KEY_ENABLED(multi, ks) ||
+ key_id != ks->key_id ||
+ !ks->authenticated)
+{
+ return 0;
+}
+
+#ifdef ENABLE_DEF_AUTH
+
+ if (ks->auth_deferred) return 0;
+
+#endif
+
+ if (!kc->hmac) return 0;
+
+ /*
+ * authenticate the packet with HMAC
+ */
+
+ HMAC_Init_ex(kc->hmac, NULL, 0, NULL, NULL);
+ hmac_len = HMAC_size(kc->hmac);
+ if (buf->len - 1 < hmac_len) return 0;
+ HMAC_Update(kc->hmac, BPTR(buf) + hmac_len + 1, BLEN(buf) - hmac_len - 1);
+ HMAC_Final(kc->hmac, local_hmac, (unsigned int*)&in_hmac_len);
+ if (hmac_len != in_hmac_len) return 0;
+ if (memcmp(local_hmac, BPTR(buf) + 1, hmac_len)) return 0;
+
+ /*
+ * HMAC authentication successful, change the IP at
+ * TLS layer
+ */
+
+ {
+struct gc_arena gc = gc_new();
+msg(D_TLS_DEBUG_LOW, "%s/%s IP changed: %s",
+tls_common_name(multi, 0),
+print_link_socket_actual(&ks->remote_addr, &gc),
+print_link_socket_actual(from, &gc));
+memcpy(&ks->remote_addr, from, sizeof(*from));
+gc_free(&gc);
+ }
+ return 1;
+
+}
+
+ return 0;
+}
+
+
+/*
+ * Utility function for finding an existing TLS session
+ * with different IP
+ */
+
+struct hash_element* detect_ip_change(struct multi_context* m,
+ struct hash* hash,
+ struct hash_bucket* bucket,
+ uint32_t hv,
+ struct mroute_addr* real)
+{
+ struct hash_iterator it;
+ struct hash_element* pe;
+ struct hash_element* retval = NULL;
+ struct buffer* buf = &m->top.c2.buf;
+
+ /*
+ * match the incoming packet against all existing sessions
+ */
+
+ hash_iterator_init(hash, &it);
+ while ((pe = hash_iterator_next(&it)) != NULL)
+{
+
+ /*
+ * get TLS context for current item
+ */
+
+ struct multi_instance* mi = pe->value;
+ struct context* ctx = &mi->context;
+ struct tls_multi* multi = ctx->c2.tls_multi;
+
+ /*
+ * we are only interested in TUN devices
+ */
+
+ if (ctx->c1.tuntap->type != DEV_TYPE_TU
Re: [Openvpn-devel] [PATCH] Fix for bug #49 for openvpn 2.2.2
Hi, it is absolutely true that it is necessary to do HMAC hash per every existing session, as far as I know there's no other way to tell reliably if the packet really belongs to an existing session... I haven't measured the HMAC performance myself, but I'd guess it would take a bit more than a few hundred packets per second to exhaust a modern CPU. The for-loop part was originally (shamelessly) adapted from tls_pre_decrypt() @ ssl.c, it could have been more clear I have to admit... I think the memcmp() is already constant-time as the HMAC hash has fixed size per hash algorithm (as far as I can remember, please correct me if I'm wrong). This was also shamelessly adopted from openvpn_decrypt() @ crypto.c. I don't think there are any timing related attack opportunities as accepting the incoming packet does not trigger any immediate replies being sent back to anyone; the accepted packet is just processed as if it was received from the original address. As the data channel packet is being forwarded away from the original sender to the trusted side of the tunnel it should be quite hard for an attacker to make any timing calculations.. I wasn't aware the --float option could be specified per client... Best regards, and nice weekend! Mikko From: Joachim Schipper [joachim.schip...@fox-it.com] Sent: Friday, March 08, 2013 2:58 PM To: openvpn-devel@lists.sourceforge.net Cc: Mikko Vainikainen; Steffan Karger Subject: RE: [PATCH] Fix for bug #49 for openvpn 2.2.2 > Hi, > > our setup needs openvpn UDP/TLS tunnels with dynamic client IP addresses, so > I implemented a fix for the bug #49 that has been open for over two years. > > The patch is for version 2.2.2 as I had trouble compiling the 2.3.x tarball > from openvpn.net. As the solution is rather simple (just two small utility > functions in mudp.c) I'd guess it could be comfortambly migrated to 2.3.x. > > Basically what the fix does is the following: incoming data channel UDP > packets from an unknown IP are matched against existing UDP/TLS sessions, and > if the packet passes the HMAC authentication against an existing TLS context > we know the client IP has changed and the session state will be instantly > updated accordingly. > > I have tested this fix to some extent, and the IP handover works impressively > smoothly in my test setup where I randomly switch between two routes from > client to server. > > Dynamic client IP's are enabled/disabled with --float in the server side. > > Please feel free to contact me for any questions etc. This is not a full review of the patch, just a few quick remarks. This patch seems to do (number of tunnels) HMAC's for any packet received from an unknown IP. If this is correct, couldn't a server with a couple thousand tunnels be brought just by sending it a few hundred packets a second? authenticate_tls_packet() contains a for() loop, but I see no code path that actually loops - everything seems to return immediately? memcmp() should be replaced by a constant-time function to prevent timing attacks (probably only realistic on a server with a single tunnel); even then, this patch allows determining the (approximate) number of tunnels in use by looking at processing time. I haven't looked at how this interacts with other features; it's at least noteworthy that the floating behavior can't be specified on a per-connection basis. Joachim
Re: [Openvpn-devel] [PATCH] Fix for bug #49 for openvpn 2.2.2
Hi, I did some quick HMAC benchmarking on my linux box with one core of my Intel i7: + 300k HMAC ops/s using 1200 byte packets + 700k HMAC ops/s using 100 byte packets + 840k HMAC ops/s using 10 byte packets This is clearly fast enough for use cases like ours where a few tens of customer sites with fault-tolerant primary/backup internet connections are connected to our backend VPN service. If openvpn can't deal with client IP changes in (near) real-time it makes network design much harder as problems with the remote site's primary internet connection will quickly affect end-users. I can't antipate any major risks in integrating this patch (assuming the implementation is correct and secure): 1) currently the --float option is broken for TLS servers anyway, so hardly anyone should be using the option at the moment. 2) sysadmins can use their own judgement when assessing whether to enable the patched --float on their servers: limited scalability in terms of number of clients vs. improved network reliability and quality for site-to-site live systems. 3) I wouldn't put too much emphasis on the number of client sessions this patch can handle, as the problem seems to be partially caused by openvpn's current inability to run multi-threaded (that will hopefully be fixed in the future?). If a server setup needs max scalability the --float can always be turned off. This is why to me this patch looks like a win-win case. Any thoughts? Best regards, Mikko ________ From: Mikko Vainikainen [mikko.vainikai...@konecranes.com] Sent: Friday, March 08, 2013 4:41 PM To: Joachim Schipper; openvpn-devel@lists.sourceforge.net Subject: Re: [Openvpn-devel] [PATCH] Fix for bug #49 for openvpn 2.2.2 Hi, it is absolutely true that it is necessary to do HMAC hash per every existing session, as far as I know there's no other way to tell reliably if the packet really belongs to an existing session... I haven't measured the HMAC performance myself, but I'd guess it would take a bit more than a few hundred packets per second to exhaust a modern CPU. The for-loop part was originally (shamelessly) adapted from tls_pre_decrypt() @ ssl.c, it could have been more clear I have to admit... I think the memcmp() is already constant-time as the HMAC hash has fixed size per hash algorithm (as far as I can remember, please correct me if I'm wrong). This was also shamelessly adopted from openvpn_decrypt() @ crypto.c. I don't think there are any timing related attack opportunities as accepting the incoming packet does not trigger any immediate replies being sent back to anyone; the accepted packet is just processed as if it was received from the original address. As the data channel packet is being forwarded away from the original sender to the trusted side of the tunnel it should be quite hard for an attacker to make any timing calculations.. I wasn't aware the --float option could be specified per client... Best regards, and nice weekend! Mikko From: Joachim Schipper [joachim.schip...@fox-it.com] Sent: Friday, March 08, 2013 2:58 PM To: openvpn-devel@lists.sourceforge.net Cc: Mikko Vainikainen; Steffan Karger Subject: RE: [PATCH] Fix for bug #49 for openvpn 2.2.2 > Hi, > > our setup needs openvpn UDP/TLS tunnels with dynamic client IP addresses, so > I implemented a fix for the bug #49 that has been open for over two years. > > The patch is for version 2.2.2 as I had trouble compiling the 2.3.x tarball > from openvpn.net. As the solution is rather simple (just two small utility > functions in mudp.c) I'd guess it could be comfortambly migrated to 2.3.x. > > Basically what the fix does is the following: incoming data channel UDP > packets from an unknown IP are matched against existing UDP/TLS sessions, and > if the packet passes the HMAC authentication against an existing TLS context > we know the client IP has changed and the session state will be instantly > updated accordingly. > > I have tested this fix to some extent, and the IP handover works impressively > smoothly in my test setup where I randomly switch between two routes from > client to server. > > Dynamic client IP's are enabled/disabled with --float in the server side. > > Please feel free to contact me for any questions etc. This is not a full review of the patch, just a few quick remarks. This patch seems to do (number of tunnels) HMAC's for any packet received from an unknown IP. If this is correct, couldn't a server with a couple thousand tunnels be brought just by sending it a few hundred packets a second? authenticate_tls_packet() contains a for() loop, but I see no code path that actually loops - everything seems to return immediately? memcmp() should be replaced by a constant-time function to prevent t
