Disallow lameduck's float to an address taken by another client
PATCH 1/1 Disallow lameduck's float to an address taken by another client
Existing check didn't take into account the case when floated client is lame duck (CN for lame duck is NULL), which allowed lame duck to float to an address taken by another client.
As a fix we use cert_hash_compare function which, besides fixing mentioned case, also allows lame duck to float to an address already taken by the same client.
--- src/openvpn/multi.c | 13 ++++++++----- src/openvpn/ssl_verify.c | 2 +- src/openvpn/ssl_verify.h | 8 ++++++++ 3 files changed, 17 insertions(+), 6 deletions(-)
src/openvpn/multi.c +8 -5
@@ -2127,17 +2127,20 @@ void multi_process_float (struct multi_context* m, struct multi_instance* mi)const uint32_t hv =hash_value (hash, &real);struct hash_bucket *bucket =hash_bucket (hash, hv); +/* make sure that we don't float to an address taken by another client */ struct hash_element *he =hash_lookup_fast (hash, bucket, &real, hv);if (he) {struct multi_instance *ex_mi = (struct multi_instance *) he->value; -const char *cn =tls_common_name (mi->context.c2.tls_multi, true); -const char *ex_cn =tls_common_name (ex_mi->context.c2.tls_multi, true); -if (cn && ex_cn &&strcmp (cn, ex_cn)) +struct tls_multi *m1 = mi->context.c2.tls_multi; +struct tls_multi *m2 = ex_mi->context.c2.tls_multi; + +/* do not float if target address is taken by client with another cert */ +if (!cert_hash_compare (m1->locked_cert_hash_set, m2->locked_cert_hash_set)) { -msg (D_MULTI_MEDIUM ,"prevent float to %s" , -multi_instance_string (ex_mi, false, &gc)); +msg (D_MULTI_MEDIUM ,"Disallow float to an address taken by another client %s" , +multi_instance_string (ex_mi, false, &gc)); mi->context.c2.buf.len =0 ;
src/openvpn/ssl_verify.c +1 -1
@@ -238,7 +238,7 @@ cert_hash_free (struct cert_hash_set *chs) } } -static bool +bool cert_hash_compare (const struct cert_hash_set *chs1,const struct cert_hash_set *chs2) {if (chs1 && chs2)
src/openvpn/ssl_verify.h +8 -0
@@ -137,6 +137,14 @@ const char *tls_common_name (const struct tls_multi* multi, const bool null);*/ const char *tls_username (const struct tls_multi *multi,const bool null); +/** +* Compares certificates hashes, returns true if hashes are equal. +* +* @param chs1 cert 1 hash set +* @param chs2 cert 2 hash set +*/ +bool cert_hash_compare (const struct cert_hash_set *chs1,const struct cert_hash_set *chs2); +#ifdef ENABLE_PF /**
Signature · 1 line
1.9.1
ACK
An elegant fix.
-Steffan
Quoted 76 lines · Lev Stipakov
Existing check didn't take into account the case when floated client is lame duck (CN for lame duck is NULL), which allowed lame duck to float to an address taken by another client.
As a fix we use
cert_hash_comparefunction which, besides fixing mentioned case, also allows lame duck to float to an address already taken by the same client.--- src/openvpn/multi.c | 13 ++++++++----- src/openvpn/ssl_verify.c | 2 +- src/openvpn/ssl_verify.h | 8 ++++++++ 3 files changed, 17 insertions(+), 6 deletions(-)src/openvpn/multi.c +8
-5@@ -2127,17 +2127,20 @@ void multi_process_float (struct multi_context* m, struct multi_instance* mi)const uint32_t hv =hash_value (hash, &real);struct hash_bucket *bucket =hash_bucket (hash, hv); +/* make sure that we don't float to an address taken by another client */ struct hash_element *he =hash_lookup_fast (hash, bucket, &real, hv);if (he) {struct multi_instance *ex_mi = (struct multi_instance *) he->value; -const char *cn =tls_common_name (mi->context.c2.tls_multi, true); -const char *ex_cn =tls_common_name (ex_mi->context.c2.tls_multi, true); -if (cn && ex_cn &&strcmp (cn, ex_cn)) +struct tls_multi *m1 = mi->context.c2.tls_multi; +struct tls_multi *m2 = ex_mi->context.c2.tls_multi; + +/* do not float if target address is taken by client with another cert */ +if (!cert_hash_compare (m1->locked_cert_hash_set, m2->locked_cert_hash_set)) { -msg (D_MULTI_MEDIUM ,"prevent float to %s" , -multi_instance_string (ex_mi, false, &gc)); +msg (D_MULTI_MEDIUM ,"Disallow float to an address taken by another client %s" , +multi_instance_string (ex_mi, false, &gc)); mi->context.c2.buf.len =0 ;src/openvpn/ssl_verify.c +1
-1@@ -238,7 +238,7 @@ cert_hash_free (struct cert_hash_set *chs) } } -static bool +bool cert_hash_compare (const struct cert_hash_set *chs1,const struct cert_hash_set *chs2) {if (chs1 && chs2)src/openvpn/ssl_verify.h +8
-0@@ -137,6 +137,14 @@ const char *tls_common_name (const struct tls_multi* multi, const bool null);*/ const char *tls_username (const struct tls_multi *multi,const bool null); +/** +* Compares certificates hashes, returns true if hashes are equal. +* +* @param chs1 cert 1 hash set +* @param chs2 cert 2 hash set +*/ +bool cert_hash_compare (const struct cert_hash_set *chs1,const struct cert_hash_set *chs2); +#ifdef ENABLE_PF /**
Your patch has been applied to the master branch.
commit 0c0c178a3d3bc541ccf076f99c54d5aa6908cbcb
Author: Lev Stipakov Date: Wed Jan 7 21:26:38 2015 +0200
Disallow lameduck's float to an address taken by another client
Acked-by: Steffan Karger <[email protected]...>
Message-Id: <[email protected]...>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9386
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering