You can subscribe to this list here.
2002 |
Jan
|
Feb
|
Mar
|
Apr
(24) |
May
(14) |
Jun
(29) |
Jul
(33) |
Aug
(3) |
Sep
(8) |
Oct
(18) |
Nov
(1) |
Dec
(10) |
---|---|---|---|---|---|---|---|---|---|---|---|---|
2003 |
Jan
(3) |
Feb
(33) |
Mar
(7) |
Apr
(28) |
May
(30) |
Jun
(5) |
Jul
(10) |
Aug
(7) |
Sep
(32) |
Oct
(41) |
Nov
(20) |
Dec
(10) |
2004 |
Jan
(24) |
Feb
(18) |
Mar
(57) |
Apr
(40) |
May
(55) |
Jun
(48) |
Jul
(77) |
Aug
(15) |
Sep
(56) |
Oct
(80) |
Nov
(74) |
Dec
(52) |
2005 |
Jan
(38) |
Feb
(42) |
Mar
(39) |
Apr
(56) |
May
(79) |
Jun
(73) |
Jul
(16) |
Aug
(23) |
Sep
(68) |
Oct
(77) |
Nov
(52) |
Dec
(27) |
2006 |
Jan
(27) |
Feb
(18) |
Mar
(51) |
Apr
(62) |
May
(28) |
Jun
(50) |
Jul
(36) |
Aug
(33) |
Sep
(47) |
Oct
(50) |
Nov
(77) |
Dec
(13) |
2007 |
Jan
(15) |
Feb
(8) |
Mar
(14) |
Apr
(18) |
May
(25) |
Jun
(16) |
Jul
(16) |
Aug
(19) |
Sep
(32) |
Oct
(17) |
Nov
(5) |
Dec
(5) |
2008 |
Jan
(64) |
Feb
(25) |
Mar
(25) |
Apr
(6) |
May
(28) |
Jun
(20) |
Jul
(10) |
Aug
(27) |
Sep
(28) |
Oct
(59) |
Nov
(37) |
Dec
(43) |
2009 |
Jan
(40) |
Feb
(25) |
Mar
(12) |
Apr
(57) |
May
(46) |
Jun
(29) |
Jul
(39) |
Aug
(10) |
Sep
(20) |
Oct
(42) |
Nov
(50) |
Dec
(57) |
2010 |
Jan
(82) |
Feb
(165) |
Mar
(256) |
Apr
(260) |
May
(36) |
Jun
(87) |
Jul
(53) |
Aug
(89) |
Sep
(107) |
Oct
(51) |
Nov
(88) |
Dec
(117) |
2011 |
Jan
(69) |
Feb
(60) |
Mar
(113) |
Apr
(71) |
May
(67) |
Jun
(90) |
Jul
(88) |
Aug
(90) |
Sep
(48) |
Oct
(64) |
Nov
(69) |
Dec
(118) |
2012 |
Jan
(49) |
Feb
(528) |
Mar
(351) |
Apr
(190) |
May
(238) |
Jun
(193) |
Jul
(104) |
Aug
(100) |
Sep
(57) |
Oct
(41) |
Nov
(47) |
Dec
(51) |
2013 |
Jan
(94) |
Feb
(57) |
Mar
(96) |
Apr
(105) |
May
(77) |
Jun
(102) |
Jul
(27) |
Aug
(81) |
Sep
(32) |
Oct
(53) |
Nov
(127) |
Dec
(65) |
2014 |
Jan
(113) |
Feb
(59) |
Mar
(104) |
Apr
(259) |
May
(70) |
Jun
(70) |
Jul
(146) |
Aug
(45) |
Sep
(58) |
Oct
(149) |
Nov
(77) |
Dec
(83) |
2015 |
Jan
(53) |
Feb
(66) |
Mar
(86) |
Apr
(50) |
May
(135) |
Jun
(76) |
Jul
(151) |
Aug
(83) |
Sep
(97) |
Oct
(262) |
Nov
(245) |
Dec
(231) |
2016 |
Jan
(131) |
Feb
(233) |
Mar
(97) |
Apr
(138) |
May
(221) |
Jun
(254) |
Jul
(92) |
Aug
(248) |
Sep
(168) |
Oct
(275) |
Nov
(477) |
Dec
(445) |
2017 |
Jan
(218) |
Feb
(217) |
Mar
(146) |
Apr
(172) |
May
(216) |
Jun
(252) |
Jul
(164) |
Aug
(192) |
Sep
(190) |
Oct
(143) |
Nov
(255) |
Dec
(182) |
2018 |
Jan
(295) |
Feb
(164) |
Mar
(113) |
Apr
(147) |
May
(64) |
Jun
(262) |
Jul
(184) |
Aug
(90) |
Sep
(69) |
Oct
(364) |
Nov
(102) |
Dec
(101) |
2019 |
Jan
(119) |
Feb
(64) |
Mar
(64) |
Apr
(102) |
May
(57) |
Jun
(154) |
Jul
(84) |
Aug
(81) |
Sep
(76) |
Oct
(102) |
Nov
(233) |
Dec
(89) |
2020 |
Jan
(38) |
Feb
(170) |
Mar
(155) |
Apr
(172) |
May
(120) |
Jun
(223) |
Jul
(461) |
Aug
(227) |
Sep
(268) |
Oct
(113) |
Nov
(56) |
Dec
(124) |
2021 |
Jan
(121) |
Feb
(48) |
Mar
(334) |
Apr
(345) |
May
(207) |
Jun
(136) |
Jul
(71) |
Aug
(112) |
Sep
(122) |
Oct
(173) |
Nov
(184) |
Dec
(223) |
2022 |
Jan
(197) |
Feb
(206) |
Mar
(156) |
Apr
(212) |
May
(192) |
Jun
(170) |
Jul
(143) |
Aug
(380) |
Sep
(182) |
Oct
(148) |
Nov
(128) |
Dec
(269) |
2023 |
Jan
(248) |
Feb
(196) |
Mar
(264) |
Apr
(36) |
May
(123) |
Jun
(66) |
Jul
(120) |
Aug
(48) |
Sep
(157) |
Oct
(198) |
Nov
(300) |
Dec
(273) |
2024 |
Jan
(271) |
Feb
(147) |
Mar
(207) |
Apr
(78) |
May
(107) |
Jun
(168) |
Jul
(151) |
Aug
(51) |
Sep
(298) |
Oct
|
Nov
|
Dec
|
From: MaxF (C. Review) <ge...@op...> - 2024-09-21 14:27:02
|
Attention is currently required from: flichtenheld, plaisthos. MaxF has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/30?usp=email ) Change subject: Implement initial packet reflection protection using bloom filter ...................................................................... Patch Set 9: Code-Review-1 (10 comments) Patchset: PS9: I havne't reviewed the whole thing yet, but I found some stuff to complain about File src/openvpn/Makefile.am: http://gerrit.openvpn.net/c/openvpn/+/30/comment/5feab99c_f33c2318 : PS8, Line 46: b tabs vs spaces File src/openvpn/bloom.h: http://gerrit.openvpn.net/c/openvpn/+/30/comment/88db0086_921e0752 : PS8, Line 8: 2022 Shouldn't this be 2024? http://gerrit.openvpn.net/c/openvpn/+/30/comment/c0e46973_ad5f7ee1 : PS8, Line 32: BLOOM_FILTER_BIT_COUNT BITS File src/openvpn/bloom.c: http://gerrit.openvpn.net/c/openvpn/+/30/comment/ca1b18dc_b9ffbcbe : PS8, Line 101: /** : * Calculates the number of bytes we need for storing a bloom filter of size : * size. We add + 1 to avoid rounding problems and too small allocation */ : static inline : size_t : bloom_get_filter_byte_count(size_t size) : { : static_assert(sizeof(bloom_counter_t) * 8 % BLOOM_FILTER_BITS_COUNT == 0, : "bloom_counter_t must be a multiple of BLOOM_FILTER_BIT_COUNT"); : : return size * sizeof(bloom_counter_t)/BLOOM_FILTER_BITS_COUNT + 1; : } I'm really confused by what size means here. This is *not* the size field in the bloom filter struct, right? It's the total number of bits in the bloom filter, right? http://gerrit.openvpn.net/c/openvpn/+/30/comment/88af9dc3_bf01868e : PS8, Line 115: static inline : size_t : bloom_get_filter_bit_offset(size_t bucket) : { : return (bucket * BLOOM_FILTER_BITS_COUNT) % sizeof(bloom_counter_t); : } If this is a bit offset, don't we need 8 * sizeof(bloom_counter_t) here? http://gerrit.openvpn.net/c/openvpn/+/30/comment/f0ce5e7f_d277c415 : PS8, Line 122: static inline : size_t : bloom_get_filter_array_index(size_t bucket) : { : return (bucket * BLOOM_FILTER_BITS_COUNT) / sizeof(bloom_counter_t); : } This seems wrong to me. Every bucket is 2 bits wide. The bloom_counter_t is 32 bits wide, or 4 bytes, so sizeof(bloom_counter_t) == 4. Bucket 3 is at a 6 bit offset, so it's in the first bloom_counter_t (array index 0). But this function gives 3 * 2 / 4 == 1. http://gerrit.openvpn.net/c/openvpn/+/30/comment/688a1a4a_2c63f6ac : PS8, Line 172: whitespace File src/openvpn/reflect_filter.c: http://gerrit.openvpn.net/c/openvpn/+/30/comment/e570d1bc_a93710c4 : PS8, Line 43: static bool : reflect_filter_rate_l Remove empty line http://gerrit.openvpn.net/c/openvpn/+/30/comment/71c672e1_c93872de : PS8, Line 116: /* we keep the count in the key instead of in the bloom filter table as : * can then keep the counter in the bloom filter itself small (2 bits) ...as we can then... -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/30?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0a9274cab7fefce3b13c05052fb9a072e0bfa6b9 Gerrit-Change-Number: 30 Gerrit-PatchSet: 9 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: MaxF <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Sat, 21 Sep 2024 14:26:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: plaisthos (C. Review) <ge...@op...> - 2024-09-21 14:16:47
|
Attention is currently required from: flichtenheld. Hello flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/31?usp=email to review the following change. Change subject: Prefer OpenSSL's SIPHASH implementation when available ...................................................................... Prefer OpenSSL's SIPHASH implementation when available OpenSSL library is significantly faster than the reference implementation (almost 2x). Prefer using this when available. The API for using the SIPHASH MAC is different enough from using normal HMAC or Digest that we already implement that combining them into one API does not make sense. SIPHASH is only available on OpenSSL 3.1 and later. We still check for support on 3.0 and later as the whole API to allow using the SIPHASH alrady exists in OpenSSL 3.0. Some of the later OpenSSL 3.0.x might get support for it. Theoretically, a provider can be loaded in OpenSSL 3.0 that implements SIPHASH. Change-Id: I09aa27caa1a3aab0d1be6118b26d54a1c1bf7aa0 Signed-off-by: Arne Schwabe <ar...@rf...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/bloom.c M src/openvpn/bloom.h M src/openvpn/reflect_filter.c M src/openvpn/siphash.h A src/openvpn/siphash_openssl.c M src/openvpn/siphash_reference.c M tests/unit_tests/openvpn/Makefile.am M tests/unit_tests/openvpn/test_reflect.c 10 files changed, 253 insertions(+), 7 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/31/31/9 diff --git a/CMakeLists.txt b/CMakeLists.txt index 5aba2d4..0faf8da 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -513,6 +513,7 @@ src/openvpn/sig.c src/openvpn/sig.h src/openvpn/siphash.h + src/openvpn/siphash_openssl.c src/openvpn/siphash_reference.c src/openvpn/socket.c src/openvpn/socket.h @@ -780,6 +781,7 @@ src/openvpn/reflect_filter.h src/openvpn/siphash.h src/openvpn/siphash_reference.c + src/openvpn/siphash_openssl.c src/openvpn/otime.c src/openvpn/crypto_mbedtls.c src/openvpn/crypto_openssl.c diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index ca77718..f357764 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -126,6 +126,7 @@ shaper.c shaper.h \ sig.c sig.h \ siphash_reference.c siphash.h \ + siphash_openssl.c \ socket.c socket.h \ socks.c socks.h \ ssl.c ssl.h ssl_backend.h \ diff --git a/src/openvpn/bloom.c b/src/openvpn/bloom.c index 729429e..6225e03 100644 --- a/src/openvpn/bloom.c +++ b/src/openvpn/bloom.c @@ -170,10 +170,19 @@ ALLOC_ARRAY_GC(bf->siphash_keys, struct siphash_key, bf->num_siphash, gc); + bf->siphash_ctx = siphash_cryptolib_init(); + bloom_clear(bf); return bf; } +void +bloom_free(struct bloom_filter *bf) +{ + siphash_cryptolib_uninit(bf->siphash_ctx); +} + + /** * Clear the bloom filter, making it empty again as if it were freshly created * @param bf the bloom structure to clear @@ -209,7 +218,8 @@ if (idx == 0) { /* We have no longer unused bytes in result, generate the next hash */ - siphash(item, len, bf->siphash_keys[j++].key, result, SIPHASH_HASH_SIZE); + siphash(bf->siphash_ctx, item, len, bf->siphash_keys[j++].key, + result, SIPHASH_HASH_SIZE); } bucket = bucket << 8; diff --git a/src/openvpn/bloom.h b/src/openvpn/bloom.h index e180261..b35d784 100644 --- a/src/openvpn/bloom.h +++ b/src/openvpn/bloom.h @@ -75,6 +75,9 @@ /** keys for the siphash functions */ struct siphash_key *siphash_keys; + /** (opaque) context for the siphash implementation */ + void *siphash_ctx; + /** the actual buckets that hold the data */ bloom_counter_t buckets[]; }; @@ -83,6 +86,9 @@ struct bloom_filter * bloom_create(size_t size, size_t num_hashes, struct gc_arena *gc); +void +bloom_free(struct bloom_filter *bf); + bloom_counter_t bloom_test(struct bloom_filter *bf, const uint8_t *item, size_t len); diff --git a/src/openvpn/reflect_filter.c b/src/openvpn/reflect_filter.c index 75445c0..f665e4c 100644 --- a/src/openvpn/reflect_filter.c +++ b/src/openvpn/reflect_filter.c @@ -448,6 +448,7 @@ void initial_rate_limit_free(struct initial_packet_rate_limit *irl) { + bloom_free(irl->bf); gc_free(&irl->gc); free(irl); irl = NULL; diff --git a/src/openvpn/siphash.h b/src/openvpn/siphash.h index d26ee36..14414d5 100644 --- a/src/openvpn/siphash.h +++ b/src/openvpn/siphash.h @@ -20,12 +20,52 @@ #include <inttypes.h> #include <string.h> - -int siphash(const void *in, size_t inlen, const void *k, uint8_t *out, - size_t outlen); +#include <stdbool.h> /* siphash always uses 128-bit keys */ #define SIPHASH_KEY_SIZE 16 #define SIPHASH_HASH_SIZE 16 -#endif + +/* Prototypes for an implementation of SIPHASH in a crypto library */ + +/** + * Calculates SIPHASH using the crypto library function. + */ +int +siphash_cryptolib(void *sip_context, const void *in, size_t inlen, + const void *k, uint8_t *out, size_t outlen); + +/** + * Calculates SIPHASH using the reference implementation + */ +int +siphash_reference(const void *in, size_t inlen, const void *k, + uint8_t *out, size_t outlen); + +void * +siphash_cryptolib_init(void); + +void +siphash_cryptolib_uninit(void *sip_context); + +bool +siphash_cryptolib_available(void *sip_context); + +static inline +int +siphash(void *ctx, const void *in, size_t inlen, const void *k, + uint8_t *out, size_t outlen) +{ + if (siphash_cryptolib_available(ctx) && false) + { + return siphash_cryptolib(ctx, in, inlen, k, out, outlen); + } + else + { + return siphash_reference(in, inlen, k, out, outlen); + } + +} + +#endif /* ifndef SIPHASH_H */ diff --git a/src/openvpn/siphash_openssl.c b/src/openvpn/siphash_openssl.c new file mode 100644 index 0000000..67a77f1 --- /dev/null +++ b/src/openvpn/siphash_openssl.c @@ -0,0 +1,149 @@ +/* + * OpenVPN -- An application to securely tunnel IP networks + * over a single TCP/UDP port, with support for SSL/TLS-based + * session authentication and key exchange, + * packet encryption, packet authentication, and + * packet compression. + * + * Copyright (C) 2023 OpenVPN Inc <sa...@op...> + * Copyright (C) 2023 Arne Schwabe <ar...@rf...> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#ifdef HAVE_CONFIG_H +#include "config.h" +#elif defined(_MSC_VER) +#include "config-msvc.h" +#endif + +#include "syshead.h" + +#include "siphash.h" +#include "buffer.h" + + +#ifdef ENABLE_CRYPTO_OPENSSL +#include <openssl/opensslv.h> +#endif + +#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x30000000L + +#include <openssl/evp.h> +#include "crypto_openssl.h" + +struct siphash_context +{ + EVP_MAC *mac; + EVP_MAC_CTX *ctx; + size_t size; + OSSL_PARAM params[3]; +}; + +/* + * Computes a SipHash value + * in: pointer to input data (read-only) + * inlen: input data length in bytes (any size_t value) + * k: pointer to the key data (read-only), must be 16 bytes + * out: pointer to output data (write-only), outlen bytes must be allocated + * outlen: length of the output in bytes, must be 8 or 16 + */ +int +siphash_cryptolib(void *sip_context, const void *in, const size_t inlen, + const void *k, uint8_t *out, const size_t outlen) +{ + struct siphash_context *sip = sip_context; + + + sip->params[1] = OSSL_PARAM_construct_octet_string("key", (void *)k, + SIPHASH_KEY_SIZE); + if (!EVP_MAC_init(sip->ctx, NULL, 0, sip->params)) + { + crypto_msg(M_FATAL, "EVP_MAC_init failed"); + } + EVP_MAC_update(sip->ctx, in, inlen); + + size_t outl = 0; + EVP_MAC_final(sip->ctx, out, &outl, outlen); + return 0; +} + +void * +siphash_cryptolib_init(void) +{ + struct siphash_context *sip; + ALLOC_OBJ(sip, struct siphash_context); + + sip->mac = EVP_MAC_fetch(NULL, "SIPHASH", NULL); + if (!sip->mac) + { + /* Our OpenSSL library does not support SIPHASH */ + return sip; + } + sip->ctx = EVP_MAC_CTX_new(sip->mac); + + /* OpenSSL will truly hold a pointer to an int in that parameter */ + sip->size = SIPHASH_HASH_SIZE; + sip->params[0] = OSSL_PARAM_construct_size_t("size", &sip->size); + /* params[1] will hold the key that changes which each invocation */ + sip->params[2] = OSSL_PARAM_construct_end(); + return sip; +} + +bool +siphash_cryptolib_available(void *sip_context) +{ + struct siphash_context *sip = sip_context; + + return (bool)(sip->mac); +} + +void +siphash_cryptolib_uninit(void *sip_context) +{ + struct siphash_context *sip = sip_context; + EVP_MAC_CTX_free(sip->ctx); + EVP_MAC_free(sip->mac); + free(sip_context); +} + +#else /* if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x30000000L */ +/* for now, we only have one implementation of SIPHASH in a libray, so put the + * dummy functions also here */ +int +siphash_cryptolib(void *sip_context, const void *in, const size_t inlen, + const void *k, uint8_t *out, const size_t outlen) +{ + return -1; +} + +bool +siphash_cryptolib_available(void *sip_context) +{ + return false; +} + +void * +siphash_cryptolib_init(void) +{ + return NULL; +} + +void +siphash_cryptolib_uninit(void *sip_context) +{ +} + + +#endif /* if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x30000000L */ diff --git a/src/openvpn/siphash_reference.c b/src/openvpn/siphash_reference.c index 35af707..2a83b5e 100644 --- a/src/openvpn/siphash_reference.c +++ b/src/openvpn/siphash_reference.c @@ -98,8 +98,8 @@ * outlen: length of the output in bytes, must be 8 or 16 */ int -siphash(const void *in, const size_t inlen, const void *k, uint8_t *out, - const size_t outlen) +siphash_reference(const void *in, const size_t inlen, const void *k, + uint8_t *out, const size_t outlen) { const unsigned char *ni = (const unsigned char *)in; diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index cd1c378..9370360 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -158,6 +158,7 @@ $(top_srcdir)/src/openvpn/packet_id.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/siphash_reference.c \ + $(top_srcdir)/src/openvpn/siphash_openssl.c \ $(top_srcdir)/src/openvpn/win32-util.c if !WIN32 diff --git a/tests/unit_tests/openvpn/test_reflect.c b/tests/unit_tests/openvpn/test_reflect.c index 5158631..9238fe8 100644 --- a/tests/unit_tests/openvpn/test_reflect.c +++ b/tests/unit_tests/openvpn/test_reflect.c @@ -41,7 +41,42 @@ #include <stdio.h> +static void +test_siphash(void **state) +{ + const char *message = "Look behind you, a Three-Headed Monkey!"; + uint8_t out[SIPHASH_HASH_SIZE]; + const uint8_t key[SIPHASH_KEY_SIZE] = { 0x11, 0x22, 0x33, 0x44, 0x55, 0x66 }; + + siphash_reference(message, strlen(message), key, out, SIPHASH_HASH_SIZE); + + const uint8_t expected_out[SIPHASH_HASH_SIZE] = + { 0x3e, 0xea, 0x95, 0xb2, 0x6d, 0x5c, 0x4e, 0xfa, + 0x20, 0x47, 0x65, 0x7e, 0xdd, 0xcd, 0x62, 0x51}; + assert_memory_equal(out, expected_out, SIPHASH_HASH_SIZE); + + struct gc_arena gc = gc_new(); + void *sipctx = siphash_cryptolib_init(); + + + uint8_t out2[SIPHASH_HASH_SIZE]; + + if (siphash_cryptolib_available(sipctx)) + { + siphash_cryptolib(sipctx, message, strlen(message), key, out2, + SIPHASH_HASH_SIZE); + assert_memory_equal(out, out2, SIPHASH_HASH_SIZE); + + /* check that calling the function twice is safe */ + siphash_cryptolib(sipctx, message, strlen(message), key, out2, + SIPHASH_HASH_SIZE); + assert_memory_equal(out, out2, SIPHASH_HASH_SIZE); + } + + siphash_cryptolib_uninit(sipctx); + gc_free(&gc); +} static void test_bloom(void **state) @@ -311,6 +346,7 @@ { openvpn_unit_test_setup(); const struct CMUnitTest tests[] = { + cmocka_unit_test(test_siphash), cmocka_unit_test(test_bloom_access_functions), cmocka_unit_test(test_bloom), cmocka_unit_test(test_bloom_minimal), -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/31?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I09aa27caa1a3aab0d1be6118b26d54a1c1bf7aa0 Gerrit-Change-Number: 31 Gerrit-PatchSet: 9 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: plaisthos (C. Review) <ge...@op...> - 2024-09-21 14:16:46
|
Attention is currently required from: flichtenheld. Hello flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/30?usp=email to review the following change. Change subject: Implement initial packet reflection protection using bloom filter ...................................................................... Implement initial packet reflection protection using bloom filter When an OpenVPN server is used/tried to be usedc in a reflection attack the protection with the simple --connect-freq-initial also block legimitate client from other networks that are not attacked by a reflection attack. To allow a server to still reply to these clients, we need to make the counts rather more detailed and count per subnet or IP address. On the other hand when we keep all this state, we eliminate the advantage of having a stateless cookie based initial packet handshake. As compromoise we use a bloom filter to store the information. This data structure is probabilistic and can have false positive and more packets being dropped but since it is a constant size and the size of this map is small enough for non-embedded systems (tests were done with a 2MB bloom filter), it is a good compromise. The code is split into the bloom filter implementation and the actual logic implementing tracking the subnets, so the bloom filter should be relatively easily be exchangable by another data structure. As hash funtion SIPHASH has been chosen since it was designed for this kind of application. Change-Id: I0a9274cab7fefce3b13c05052fb9a072e0bfa6b9 Signed-off-by: Arne Schwabe <ar...@rf...> --- M .github/workflows/build.yaml M CMakeLists.txt M Changes.rst M doc/man-sections/server-options.rst M src/openvpn/Makefile.am A src/openvpn/bloom.c A src/openvpn/bloom.h M src/openvpn/mudp.c M src/openvpn/multi.c M src/openvpn/options.c M src/openvpn/options.h M src/openvpn/reflect_filter.c M src/openvpn/reflect_filter.h M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/test_reflect.c 15 files changed, 1,302 insertions(+), 15 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/30/30/9 diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 361d457..93c67d1 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -102,6 +102,9 @@ env: srcdir: "${{ github.workspace }}/tests/unit_tests/openvpn" + - name: Run reflect unit test + run: ./unittests/test_reflect.exe + ubuntu: strategy: fail-fast: false diff --git a/CMakeLists.txt b/CMakeLists.txt index ad620fa..5aba2d4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -369,6 +369,7 @@ src/openvpn/base64.c src/openvpn/base64.h src/openvpn/basic.h + src/openvpn/bloom.c src/openvpn/buffer.c src/openvpn/buffer.h src/openvpn/circ_list.h @@ -511,6 +512,8 @@ src/openvpn/shaper.h src/openvpn/sig.c src/openvpn/sig.h + src/openvpn/siphash.h + src/openvpn/siphash_reference.c src/openvpn/socket.c src/openvpn/socket.h src/openvpn/socks.c @@ -605,6 +608,7 @@ "test_packet_id" "test_pkt" "test_provider" + "test_reflect" "test_ssl" "test_user_pass" ) @@ -770,6 +774,19 @@ src/openvpn/session_id.c ) + target_sources(test_reflect PRIVATE + src/openvpn/bloom.c + src/openvpn/reflect_filter.c + src/openvpn/reflect_filter.h + src/openvpn/siphash.h + src/openvpn/siphash_reference.c + src/openvpn/otime.c + src/openvpn/crypto_mbedtls.c + src/openvpn/crypto_openssl.c + src/openvpn/crypto.c + src/openvpn/packet_id.c + ) + target_sources(test_pkt PRIVATE tests/unit_tests/openvpn/mock_win32_execve.c src/openvpn/argv.c diff --git a/Changes.rst b/Changes.rst index 439352a..af72678 100644 --- a/Changes.rst +++ b/Changes.rst @@ -9,6 +9,14 @@ the user experience as the client shows an error instead of running into a timeout when the server just stops responding completely. +Bloom filter based reflection protection + To avoid the limitation of ``--connect-freq-initial`` to block legimitate + clients when an OpenVPN server is tried to be used in a reflection attack, + the new ``--connect-freq-initial-bloom-limit`` can impose limit on a + per-subnet basis. See the manual for more details. Note: this option + requires 2MB of extra memory in the default configuration. + + Deprecated features ------------------- ``secret`` support has been removed by default. diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst index 0632e31..f102df2 100644 --- a/doc/man-sections/server-options.rst +++ b/doc/man-sections/server-options.rst @@ -208,6 +208,77 @@ will not be counted against the limit. The default is to allow 100 initial connection per 10s. + +--connect-freq-initial-bloom-size args + + Valid syntax: + :: + + connect-freq-initial-bloom-size size [numhashes] + + Configures the size of the bloom filter used for the initial connection + packet responses. See `--connect-freq-initial-bloom-limit` for a full + description of the feature. + + Bloom filters are probabilistic by their nature and the larger the size + of the bloom filter is, the more accurate they are. This option configures + the number of buckets in the bloom filter. Each bucket takes up 4 bits. + E.g. the default of 8336608 (8 * 2^20) when the option is not set + will require 2MB of memory. The numhashes will control the number of hash + functions that are used for the bloom filter. The default is 7. + +--connect-freq-initial-bloom-limit arg + + Valid syntax: + :: + + connect-freq-initial-bloom-limit inet netmask limit + connect-freq-initial-bloom-limit inet6 netmask limit + + This option implements a rate limiting function for initial packet like + ``--connect-freq-initial`` but takes the source of the initial packet into + account. This function shares the the reset after a time period and + enabling this function will not disable the overall limit of + ``--conect-freq-initial``. + + To avoid resource exhaustion on the OpenVPN server side from reflection + attacks and also trying to avoid blocking legitimate clients when an + attacker tries to use an OpenVPN server, the limits are not applied globally + but rather a per netmask granularity. Each line of this command configures + a separate granularity. As example, the following configuration, + + :: + + connect-freq-initial 1000 60 + connect-freq-initial-bloom-limit inet 24 50 + connect-freq-initial-bloom-limit inet 8 100 + connect-freq-initial-bloom-limit inet6 56 50 + connect-freq-initial-bloom-limit inet6 32 100 + + will limit the number of (unanswered) replies per /24 network to 50 + packets. After 50 packets from the same /24, the server will + drop any further packets. Similarly, if a /16 network receives more than + 100 connection requests, the server will drop further packets. + If the count of all packets exceeds 1000, further packets will be + completely dropped regardless of their origin. + + The implementation uses a counting bloom filter to store information of how + many packets the server has seen per subnet. A bloom filter is a + probabilistic data structure can yield false positives. In this case, the + server will drop packets even though the limit + for that particular subnet is not yet reached. Since we are using a + counting bloom filter, a reply from a client that triggers a removal of an + entry can also lead to a false negative. However, this is limited in the + sense a reply can at most trigger one false negative. + + Every limit configured with this option will put more more entries into + the bloomfilter since per limit (per family), one entry is put into the + bloom filter. So a large number of entries requires a larger bloom filter + to avoid too many false positives. + + IPv6 mapped IPv4 addresses (::ffff:0:0/96) are treated as IPv4 addresses + for these limits. + --duplicate-cn Allow multiple clients with the same common name to concurrently connect. In the absence of this option, OpenVPN will disconnect a client diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index 3784a98..ca77718 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -44,6 +44,7 @@ auth_token.c auth_token.h \ base64.c base64.h \ basic.h \ + bloom.c bloom.h \ buffer.c buffer.h \ circ_list.h \ clinat.c clinat.h \ @@ -124,6 +125,7 @@ session_id.c session_id.h \ shaper.c shaper.h \ sig.c sig.h \ + siphash_reference.c siphash.h \ socket.c socket.h \ socks.c socks.h \ ssl.c ssl.h ssl_backend.h \ diff --git a/src/openvpn/bloom.c b/src/openvpn/bloom.c new file mode 100644 index 0000000..729429e --- /dev/null +++ b/src/openvpn/bloom.c @@ -0,0 +1,253 @@ +/* + * OpenVPN -- An application to securely tunnel IP networks + * over a single TCP/UDP port, with support for SSL/TLS-based + * session authentication and key exchange, + * packet encryption, packet authentication, and + * packet compression. + * + * Copyright (C) 2022 OpenVPN Inc <sa...@op...> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +/* This file implements a specialised counting bloom filter implementation + * used in OpenVPN to mitigate it being used in reflection attacks. The bloom + * implementation should be general enough to be used in other contexts + * however */ + +#ifdef HAVE_CONFIG_H +#include "config.h" +#elif defined(_MSC_VER) +#include "config-msvc.h" +#endif + +#include "syshead.h" + + +#include <stdint.h> +#include <stddef.h> +#include <stdlib.h> +#include <string.h> +#include <stdbool.h> +#include "siphash.h" +#include "crypto.h" + +#include "bloom.h" + + +static size_t +calc_ceil_log2(size_t num) +{ + /* Figure out how many bits we have in our hash by doing + * ceil(log2(bf->size) */ + + size_t numbits = 0; + while (num >>= 1) + { + numbits++; + } + return numbits; +} + +/** + * Calculate number of bytes needed for each hash function in the bloom filter + * + */ +static inline size_t +calculate_num_bytes_hashfun(size_t size) +{ + size_t hash_bits = calc_ceil_log2(size); + /* round up to the nearest byte size */ + size_t hash_bytes = (hash_bits + 7) /8; + return hash_bytes; +} + +/** + * A bloom filter uses a number of hashes to perform its functionality. + * + * Instead of using the same number of siphash function we split the bytes of + * the siphash to the different hashes. + * + * We could also optimise this further by splitting at the bit level but that + * crates a lot of extra code for bit shifting, etc. so we waste some bits + * + * @param bf + * @param num_hashes + * @return + */ +size_t +calculate_num_sip_hash_hashes(struct bloom_filter *bf) +{ + size_t hash_bytes = calculate_num_bytes_hashfun(bf->size); + size_t total_bytes = bf->num_hashes * hash_bytes; + + /* Round up to next SIPHASH_HASH_SIZE */ + size_t num_siphash = (total_bytes + SIPHASH_HASH_SIZE -1) / SIPHASH_HASH_SIZE; + + return num_siphash; +} + +/** + * Calculates the number of bytes we need for storing a bloom filter of size + * size. We add + 1 to avoid rounding problems and too small allocation */ +static inline +size_t +bloom_get_filter_byte_count(size_t size) +{ + static_assert(sizeof(bloom_counter_t) * 8 % BLOOM_FILTER_BITS_COUNT == 0, + "bloom_counter_t must be a multiple of BLOOM_FILTER_BIT_COUNT"); + + return size * sizeof(bloom_counter_t)/BLOOM_FILTER_BITS_COUNT + 1; +} + + +static inline +size_t +bloom_get_filter_bit_offset(size_t bucket) +{ + return (bucket * BLOOM_FILTER_BITS_COUNT) % sizeof(bloom_counter_t); +} + +static inline +size_t +bloom_get_filter_array_index(size_t bucket) +{ + return (bucket * BLOOM_FILTER_BITS_COUNT) / sizeof(bloom_counter_t); +} + +static inline bloom_counter_t +bloom_get_filter_get_counter(struct bloom_filter *bf, size_t bucket) +{ + static_assert((BLOOM_FILTER_BITS_MASK + 1) == (1 << BLOOM_FILTER_BITS_COUNT), + "BLOOM_FILTER_BITMASK and BLOOM_FILTER_BIT_COUNT are inconsistent"); + + bloom_counter_t counter = bf->buckets[bloom_get_filter_array_index(bucket)]; + size_t bitoffset = bloom_get_filter_bit_offset(bucket); + + return (counter >> bitoffset) & BLOOM_FILTER_BITS_MASK; +} + +static inline void +bloom_set_filter_counter(struct bloom_filter *bf, size_t bucket, bloom_counter_t value) +{ + bloom_counter_t data = bf->buckets[bloom_get_filter_array_index(bucket)]; + size_t bitoffset = bloom_get_filter_bit_offset(bucket); + + data = data & ~(BLOOM_FILTER_BITS_MASK << bitoffset); + + data = data | ((value & BLOOM_FILTER_BITS_MASK) << bitoffset); + + bf->buckets[bloom_get_filter_array_index(bucket)] = data; +} + +/** + * Creates a new bloom filter structure + * @param size the number of buckets. + * @return the newly created bloom filter structure + */ +struct bloom_filter * +bloom_create(size_t size, size_t num_hashes, struct gc_arena *gc) +{ + size_t bloomfilter_bytes = bloom_get_filter_byte_count(size); + struct bloom_filter *bf = gc_malloc(sizeof(struct bloom_filter) + bloomfilter_bytes, + false, gc); + bf->size = size; + bf->num_hashes = num_hashes; + + bf->hash_bytes = calculate_num_bytes_hashfun(size); + bf->num_siphash = calculate_num_sip_hash_hashes(bf); + + ALLOC_ARRAY_GC(bf->siphash_keys, struct siphash_key, bf->num_siphash, gc); + + bloom_clear(bf); + return bf; +} + +/** + * Clear the bloom filter, making it empty again as if it were freshly created + * @param bf the bloom structure to clear + */ +void +bloom_clear(struct bloom_filter *bf) +{ + memset(bf->buckets, 0, bloom_get_filter_byte_count(bf->size)); + + /* We randomise the bloom filter keys on every clear of the bloom filter + * to avoid scenarios where an attacker might learn specific pattern + * that could exploit false positives in the bloom filter */ + for (size_t i = 0; i < bf->num_siphash; i++) + { + prng_bytes(bf->siphash_keys[i].key, SIPHASH_KEY_SIZE); + } +} + + +static bloom_counter_t +bloom_add_test(struct bloom_filter *bf, const uint8_t *item, size_t len, bloom_counter_t inc) +{ + uint8_t result[SIPHASH_HASH_SIZE]; + size_t j = 0; + size_t idx = 0; + bloom_counter_t ret = bloom_counter_max; + + for (size_t i = 0; i < bf->num_hashes; i++) + { + size_t bucket = 0; + for (int k = 0; k < bf->hash_bytes; k++) + { + if (idx == 0) + { + /* We have no longer unused bytes in result, generate the next hash */ + siphash(item, len, bf->siphash_keys[j++].key, result, SIPHASH_HASH_SIZE); + } + + bucket = bucket << 8; + bucket |= result[idx]; + + idx = (idx + 1) % SIPHASH_HASH_SIZE; + } + + bucket = bucket % bf->size; + bloom_counter_t value = bloom_get_filter_get_counter(bf, bucket); + + ret = min_bloom_counter(ret, value); + + if (inc) + { + value = min_bloom_counter(bloom_counter_max, value + 1); + bloom_set_filter_counter(bf, bucket, value); + } + } + return ret; +} + + +bloom_counter_t +bloom_add(struct bloom_filter *bf, const uint8_t *item, size_t len) +{ + return bloom_add_test(bf, item, len, 1); +} + +bloom_counter_t +bloom_remove(struct bloom_filter *bf, const uint8_t *item, size_t len) +{ + return bloom_add_test(bf, item, len, -1); +} + + +bloom_counter_t +bloom_test(struct bloom_filter *bf, const uint8_t *item, size_t len) +{ + return bloom_add_test(bf, item, len, 0); +} diff --git a/src/openvpn/bloom.h b/src/openvpn/bloom.h new file mode 100644 index 0000000..e180261 --- /dev/null +++ b/src/openvpn/bloom.h @@ -0,0 +1,97 @@ +/* + * OpenVPN -- An application to securely tunnel IP networks + * over a single TCP/UDP port, with support for SSL/TLS-based + * session authentication and key exchange, + * packet encryption, packet authentication, and + * packet compression. + * + * Copyright (C) 2022 OpenVPN Inc <sa...@op...> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#ifndef BLOOM_H +#define BLOOM_H + +#include <stdint.h> +#include "siphash.h" +#include "buffer.h" + +/* This is the type we use for the buckets. This is split into small buckets + * with BLOOM_FILTER_BIT_COUNT size */ +typedef uint32_t bloom_counter_t; +#define BLOOM_FILTER_BITS_COUNT 2 +#define BLOOM_FILTER_BITS_MASK 0x03 +#define bloom_counter_max 0x03 + + +static inline bloom_counter_t +min_bloom_counter(bloom_counter_t x, bloom_counter_t y) +{ + if (x < y) + { + return x; + } + else + { + return y; + } +} + +struct siphash_key { + uint8_t key[SIPHASH_KEY_SIZE]; +}; + +struct bloom_filter { + /** Size of the bloom filter in entries, ie total bits/bits per counter */ + size_t size; + + /** Number of bytes used by each hash function: + * + * log2(size * 8) bits rounded up to the next byte + * + * This is a cached value since log2 is surprisingly slow + * (5% of total time of if we do not cache it) */ + size_t hash_bytes; + + /** number of hashes we use to determine the bit positions */ + size_t num_hashes; + /** number of siphash function needed to calculate. This can be + * calculated from the other members of the struct but we store it + * in the struct for fast access */ + size_t num_siphash; + + /** keys for the siphash functions */ + struct siphash_key *siphash_keys; + + /** the actual buckets that hold the data */ + bloom_counter_t buckets[]; +}; + + +struct bloom_filter * +bloom_create(size_t size, size_t num_hashes, struct gc_arena *gc); + +bloom_counter_t +bloom_test(struct bloom_filter *bf, const uint8_t *item, size_t len); + +bloom_counter_t +bloom_add(struct bloom_filter *bf, const uint8_t *item, size_t len); + +bloom_counter_t +bloom_remove(struct bloom_filter *bf, const uint8_t *item, size_t len); + +void +bloom_clear(struct bloom_filter *bf); +#endif /* ifndef BLOOM_H */ diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c index 268b430..9fbffac 100644 --- a/src/openvpn/mudp.c +++ b/src/openvpn/mudp.c @@ -84,7 +84,7 @@ { /* Check if we are still below our limit for sending out * responses */ - if (!reflect_filter_rate_limit_check(m->initial_rate_limiter)) + if (!reflect_filter_check(m->initial_rate_limiter, from)) { return false; } @@ -254,7 +254,8 @@ { /* a successful three-way handshake only counts against * connect-freq but not against connect-freq-initial */ - reflect_filter_rate_limit_decrease(m->initial_rate_limiter); + reflect_filter_rate_limit_decrease(m->initial_rate_limiter, + &m->top.c2.from.dest); mi = multi_create_instance(m, &real); if (mi) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 0509911..b9de040 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -368,7 +368,8 @@ m->new_connection_limiter = frequency_limit_init(t->options.cf_max, t->options.cf_per); m->initial_rate_limiter = initial_rate_limit_init(t->options.cf_initial_max, - t->options.cf_initial_per); + t->options.cf_initial_per, + &t->options.initial_cf_bloom_config); /* * Allocate broadcast/multicast buffer list diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 649f48b..20c953a 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -3461,6 +3461,22 @@ } static void +check_filter_tier_under_global_limit(struct filter_tier *ft, size_t limit, + const char *family) +{ + while (ft) + { + if (ft->limit < limit) + { + msg(M_WARN, "Note: --connect-freq-initial-bloom-limit %s %d %d " + "has a larger limit than connect-freq-limit-initial " + "limit (%zu).", family, ft->netmask, ft->limit, limit); + } + ft = ft->next; + } +} + +static void options_postprocess_verify(const struct options *o) { if (o->connection_list) @@ -3484,6 +3500,18 @@ "channel offload: packets are always sent to the VPN " "interface and then routed based on the system routing table"); } + + if ((bool)(o->initial_cf_bloom_config.inet_tiers) + +(bool)(o->initial_cf_bloom_config.inet6_tiers) == 1) + { + msg(M_FATAL, "connect-freq-initial-bloom-limit must be provided for " + "both inet and inet6"); + } + check_filter_tier_under_global_limit(o->initial_cf_bloom_config.inet_tiers, + o->cf_initial_max, "inet"); + check_filter_tier_under_global_limit(o->initial_cf_bloom_config.inet6_tiers, + o->cf_initial_max, "inet6"); + } @@ -7562,6 +7590,71 @@ options->cf_initial_max = cf_max; options->cf_initial_per = cf_per; } + else if (streq(p[0], "connect-freq-initial-bloom-size") && p[1] && !p[3]) + { + VERIFY_PERMISSION(OPT_P_GENERAL); + + int cf_bloom_size = atoi(p[1]); + + if (cf_bloom_size <= 0) + { + msg(msglevel, "--connect-freq-initial-bloom-size size must be > 0"); + goto err; + } + options->initial_cf_bloom_config.size = cf_bloom_size; + if (p[2]) + { + int num_hash = atoi(p[1]); + if (num_hash <= 0) + { + msg(msglevel, "--connect-freq-initial-bloom-size number of hash " + "functions must be > 0"); + goto err; + } + options->initial_cf_bloom_config.num_hashes = num_hash; + } + } + else if (streq(p[0], "connect-freq-initial-bloom-limit") && p[1] && p[2] + && p[3] && !p[4]) + { + int netmask = atoi(p[2]); + int limit = atoi(p[3]); + + if (limit <= 0) + { + msg(msglevel, "Limit parameter to %s must be > 0", p[0]); + goto err; + } + + if (streq(p[1], "inet")) + { + if (netmask < 0 || netmask > 32) + { + msg(msglevel, "Netmask parameter (%s) for IPv4 for %s must be " + "between 0 and 32", p[2], p[0]); + goto err; + } + reflect_add_filter_tier(&options->initial_cf_bloom_config, + &options->gc, false, netmask, limit); + } + else if (streq(p[1], "inet6")) + { + if (netmask < 0 || netmask > 128) + { + msg(msglevel, "Netmask parameter (%s) for IPv6 for %s must be " + "between 0 and 128", p[2], p[0]); + goto err; + } + reflect_add_filter_tier(&options->initial_cf_bloom_config, + &options->gc, true, netmask, limit); + } + else + { + msg(msglevel, "Unknown parameter %s for %s. Must be inet or inet6", + p[1], p[0]); + goto err; + } + } else if (streq(p[0], "max-clients") && p[1] && !p[2]) { int max_clients; diff --git a/src/openvpn/options.h b/src/openvpn/options.h index f608cb8..8448071 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -43,6 +43,7 @@ #include "clinat.h" #include "crypto_backend.h" #include "dns.h" +#include "reflect_filter.h" /* @@ -515,6 +516,8 @@ int cf_initial_max; int cf_initial_per; + struct bloom_filter_conf initial_cf_bloom_config; + int max_clients; int max_routes_per_client; int stale_routes_check_interval; diff --git a/src/openvpn/reflect_filter.c b/src/openvpn/reflect_filter.c index 538bbbc..75445c0 100644 --- a/src/openvpn/reflect_filter.c +++ b/src/openvpn/reflect_filter.c @@ -38,8 +38,7 @@ #include "crypto.h" #include "reflect_filter.h" - -bool +static bool reflect_filter_rate_limit_check(struct initial_packet_rate_limit *irl) { if (now > irl->last_period_reset + irl->period_length) @@ -72,34 +71,384 @@ return !over_limit; } -void -reflect_filter_rate_limit_decrease(struct initial_packet_rate_limit *irl) +static void +reset_filter_tier(struct initial_packet_rate_limit *irl, + struct filter_tier *tier, const char *prefix) { + for (; tier != NULL; tier = tier->next) + { + if (tier->dropped > 0) + { + msg(D_TLS_DEBUG_LOW, "Dropped %zu initial handshake packets due to " + "--connect-freq-initial-bloom-limit %s %d %d", + tier->dropped, prefix, tier->netmask, tier->limit); + tier->dropped = 0; + } + } +} + +static void +bloom_filter_check_reset(struct initial_packet_rate_limit *irl) +{ + if (now > irl->last_period_reset + irl->period_length) + { + reset_filter_tier(irl, irl->bloom_conf.inet_tiers, "inet"); + reset_filter_tier(irl, irl->bloom_conf.inet6_tiers, "inet6"); + + bloom_clear(irl->bf); + } +} + + +/** + * structure used as lookup key for the bloom structure. We used the + * netmask as part of the structure to avoid the look for the first + * IP of a subnet and the subnet to be same key. + */ +struct bloom_filter_key { + union { + struct in_addr in; + struct in6_addr in6; + }; + int netmask; + /* we keep the count in the key instead of in the bloom filter table as + * can then keep the counter in the bloom filter itself small (2 bits) + * and bloom filter usage is the same for 20000 request from the same IP + * (20k entries with different count but same IP) and from 20000 random ips + * (20k entries with count 1 but different IP) */ + int count; +}; + +static inline struct bloom_filter_key +filter_mask_inet6(struct openvpn_sockaddr *addr, int netmask) +{ + struct bloom_filter_key ret = { 0 }; + ret.in6 = addr->addr.in6.sin6_addr; + + int bits_to_clear = 128 - netmask; + int bytes_to_clear = bits_to_clear /8; + bits_to_clear = bits_to_clear % 8; + + memset(&ret.in6.s6_addr[15 - bytes_to_clear], 0x00, bytes_to_clear); + + ret.in6.s6_addr[15 - bytes_to_clear - 1] &= (0xff << bits_to_clear); + + ret.netmask = netmask; + + return ret; +} + +static inline struct bloom_filter_key +filter_mask_inet(struct openvpn_sockaddr *addr, int netmask) +{ + struct bloom_filter_key ret = { 0 }; + ret.in = addr->addr.in4.sin_addr; + ret.in.s_addr &= htonl(0xffffffff << (32 - netmask)); + ret.netmask = netmask; + return ret; +} + +/* We use one function and an action argument to avoid repeating + * the code to iterate to the tiers and the creating the lookup + * keys */ +enum bloom_filter_action +{ + REFLECT_CHECK, + REFLECT_INCREASE, + REFLECT_DECREASE, +}; + +static int +reflect_lookup_bf_key(struct bloom_filter *bf, struct bloom_filter_key *key, int limit) +{ + /* we do a lookup for 1,2,3, and 4 and the limit first + * and after that do regular binary search. This is meant to optimise + * the common case where just a small number of requests are coming from + * each IP */ + key->count = limit; + if (bloom_test(bf, (const uint8_t *) key, sizeof(struct bloom_filter_key)) > 0) + { + return limit; + } + + key->count = 4; + if (bloom_test(bf, (const uint8_t *) key, sizeof(struct bloom_filter_key)) == 0) + { + /* The value for 4 has not been found, so the real value might be 1, 2, or 3. */ + for (int i = 3; i > 0; i--) + { + key->count = i; + if (bloom_test(bf, (const uint8_t *) key, sizeof(struct bloom_filter_key)) > 0) + { + return i; + } + } + + /* 4 was no in the map and 1-3 are also not there, so assume the key is not in the map */ + return 0; + } + + int low = 3; + int high = limit; + + while (low < high) + { + + key->count = (high + low + 1)/2; + bloom_counter_t count = bloom_test(bf, (const uint8_t *) key, sizeof(struct bloom_filter_key)); + if (count > 0) + { + low = key->count; + } + else + { + high = key->count - 1; + } + } + + if (low == 4) + { + /* we reached the lower end of our binary search have not found the + * key and we know that 4 is in the map */ + return 4; + } + else + { + return low; + } +} + +/** + * Convert a mapped IPv6 mapped IPv4 address (::ffff:0:0/96) to an + * equivalent IPv4 adress. + * + * @note: This function only converts the IP itself and ignores other + * parts of the \c from structure like port or protocol. + */ +static struct openvpn_sockaddr +convert_mapped_inet_sockaddr(struct openvpn_sockaddr *from) +{ + struct openvpn_sockaddr from_mapped = { 0 }; + /* we ignore the fields like port that the key in the bloom filter + * ignores too. This makes this function non-generic */ + + from_mapped.addr.in4.sin_family = AF_INET; + + memcpy(&from_mapped.addr.in4.sin_addr.s_addr, + &from->addr.in6.sin6_addr.s6_addr[12], + sizeof(from_mapped.addr.in4.sin_addr.s_addr)); + + return from_mapped; +} + +static bool +bloom_filter_action(struct initial_packet_rate_limit *irl, + struct openvpn_sockaddr *from, + enum bloom_filter_action action) +{ + bool found = false; + struct filter_tier *tier = NULL; + + struct openvpn_sockaddr from_mapped; + + if (from->addr.sa.sa_family == AF_INET6 + && IN6_IS_ADDR_V4MAPPED(&from->addr.in6.sin6_addr)) + { + from_mapped = convert_mapped_inet_sockaddr(from); + from = &from_mapped; + } + + if (from->addr.sa.sa_family == AF_INET) + { + tier = irl->bloom_conf.inet_tiers; + + } + else if (from->addr.sa.sa_family == AF_INET6) + { + tier = irl->bloom_conf.inet6_tiers; + } + + while (tier) + { + struct filter_tier *next_tier = tier->next; + struct bloom_filter_key key; + + if (from->addr.sa.sa_family == AF_INET6) + { + key = filter_mask_inet6(from, tier->netmask); + } + else + { + key = filter_mask_inet(from, tier->netmask); + } + + /* fetch the current count of the key in the bloom filter */ + int result = reflect_lookup_bf_key(irl->bf, &key, tier->limit); + struct gc_arena gc = gc_new(); + gc_free(&gc); + + switch (action) + { + + case REFLECT_CHECK: + if (result >= tier->limit) + { + found = true; + tier->dropped++; + if (tier->dropped == 1) + { + msg(M_WARN, "Note: --connect-freq-initial-bloom-limit " + "limit for netmask /%d exceeded. Expect additional " + "initial packet drops for the next %d seconds", + tier->netmask, + (int)(irl->last_period_reset + irl->period_length - now)); + } + } + break; + + case REFLECT_INCREASE: + ASSERT(result < tier->limit); + key.count = result + 1; + bloom_add(irl->bf, (const uint8_t *) &key, sizeof(key)); + break; + + case REFLECT_DECREASE: + key.count = result - 1; + bloom_remove(irl->bf, (const uint8_t *) &key, sizeof(key)); + break; + } + + tier = next_tier; + + } + if (!found && action == REFLECT_CHECK) + { + /* We only want to increase the counters if the IP is not already + * in the set. */ + bloom_filter_action(irl, from, REFLECT_INCREASE); + } + return found; +} + +static bool +bloom_filter_check(struct initial_packet_rate_limit *irl, + struct openvpn_sockaddr *from) +{ + if (now > irl->last_period_reset + irl->period_length) + { + + bloom_filter_check_reset(irl); + bloom_clear(irl->bf); + } + + return bloom_filter_action(irl, from, REFLECT_CHECK); +} + + +bool +reflect_filter_check(struct initial_packet_rate_limit *irl, + struct openvpn_sockaddr *from) +{ + /* We are doing the bloom filter check first so packets that are already + * rejected by the bloom filter do not count against the limit of the + * simple rate limiter */ + if (irl->bf && bloom_filter_check(irl, from)) + { + return false; + } + + if (!reflect_filter_rate_limit_check(irl)) + { + return false; + } + + return true; +} + + +void +reflect_filter_rate_limit_decrease(struct initial_packet_rate_limit *irl, struct openvpn_sockaddr *from) +{ + if (irl->bf && bloom_filter_action(irl, from, REFLECT_CHECK)) + { + /* Only remove if it is actually present. This might be a packet + * coming from an early period or be relayed */ + bloom_filter_action(irl, from, REFLECT_DECREASE); + } + if (irl->curr_period_counter > 0) { irl->curr_period_counter--; } } +void +reflect_add_filter_tier(struct bloom_filter_conf *bfconf, struct gc_arena *gc, + bool ipv6, int netmask, int limit) +{ + struct filter_tier *ftnew = gc_malloc(sizeof(struct filter_tier), true, gc); + + ftnew->netmask = netmask; + ftnew->limit = limit; + + if (ipv6) + { + ftnew->next = bfconf->inet6_tiers; + bfconf->inet6_tiers = ftnew; + } + else + { + ftnew->next = bfconf->inet_tiers; + bfconf->inet_tiers = ftnew; + } +} + +void +init_bloom_filter(struct initial_packet_rate_limit *irl) +{ + if (!irl->bloom_conf.size) + { + /* the default allocates 2MB for bloom filter entries */ + irl->bloom_conf.size = 1024ul * 1024 * 8; + } + if (!irl->bloom_conf.num_hashes) + { + irl->bloom_conf.num_hashes = 7; + } + + irl->bf = bloom_create(irl->bloom_conf.size, irl->bloom_conf.num_hashes, + &irl->gc); + bloom_clear(irl->bf); +} struct initial_packet_rate_limit * -initial_rate_limit_init(int max_per_period, int period_length) +initial_rate_limit_init(int max_per_period, int period_length, + struct bloom_filter_conf *bconf) { - struct initial_packet_rate_limit *irl; + struct initial_packet_rate_limit *irl = NULL; + ALLOC_OBJ_CLEAR(irl, struct initial_packet_rate_limit); - ALLOC_OBJ(irl, struct initial_packet_rate_limit); + irl->gc = gc_new(); irl->max_per_period = max_per_period; irl->period_length = period_length; irl->curr_period_counter = 0; irl->last_period_reset = 0; + if (bconf) + { + irl->bloom_conf = *bconf; + init_bloom_filter(irl); + } + return irl; } void initial_rate_limit_free(struct initial_packet_rate_limit *irl) { + gc_free(&irl->gc); free(irl); + irl = NULL; } diff --git a/src/openvpn/reflect_filter.h b/src/openvpn/reflect_filter.h index 12eb0a1..c639888 100644 --- a/src/openvpn/reflect_filter.h +++ b/src/openvpn/reflect_filter.h @@ -24,6 +24,28 @@ #define REFLECT_FILTER_H #include <limits.h> +#include "socket.h" +#include "bloom.h" + +struct filter_tier { + struct filter_tier *next; + + int limit; + int netmask; + + /** The number of packets we dropped since we went over this limit */ + size_t dropped; +}; + +struct bloom_filter_conf { + struct filter_tier *inet_tiers; + struct filter_tier *inet6_tiers; + + /* Configuration of the bloom filter */ + size_t num_hashes; + size_t size; +}; + /** struct that handles all the rate limiting logic for initial * responses */ @@ -35,7 +57,7 @@ int period_length; /** Number of packets in the current period. We use int64_t here - * to avoid any potiential issues with overflow */ + * to avoid any potential issues with overflow */ int64_t curr_period_counter; /* Last time we reset our timer */ @@ -44,15 +66,28 @@ /* we want to warn once per period that packets are being started to * be dropped */ bool warning_displayed; + + struct bloom_filter_conf bloom_conf; + struct bloom_filter *bf; + + /* gc_arena used for the various allocations by this struct */ + struct gc_arena gc; }; +/** + * Adds a bloom filter tier to the bloom filter config. + */ +void +reflect_add_filter_tier(struct bloom_filter_conf *bfconf, struct gc_arena *gc, + bool ipv6, int netmask, int limit); /** * checks if the connection is still allowed to connect under the rate * limit. This also increases the internal counter at the same time */ bool -reflect_filter_rate_limit_check(struct initial_packet_rate_limit *irl); +reflect_filter_check(struct initial_packet_rate_limit *irl, + struct openvpn_sockaddr *from); /** * decreases the counter of initial packets seen, so connections that @@ -60,16 +95,27 @@ * the counter of initial connection attempts */ void -reflect_filter_rate_limit_decrease(struct initial_packet_rate_limit *irl); +reflect_filter_rate_limit_decrease(struct initial_packet_rate_limit *irl, + struct openvpn_sockaddr *from); /** * allocate and initialize the initial-packet rate limiter structure + * + * Note: this function does not copy bconf's contents. */ struct initial_packet_rate_limit * -initial_rate_limit_init(int max_per_period, int period_length); +initial_rate_limit_init(int max_per_period, int period_length, + struct bloom_filter_conf *bconf); /** * free the initial-packet rate limiter structure */ void initial_rate_limit_free(struct initial_packet_rate_limit *irl); + +/** + * Initialises the bloom filter with the configuration values of + * irl->bloom_conf + */ +void +init_bloom_filter(struct initial_packet_rate_limit *irl); #endif /* ifndef REFLECT_FILTER_H */ diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index a4e6235..cd1c378 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -11,7 +11,7 @@ endif test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver \ - pkt_testdriver ssl_testdriver user_pass_testdriver + pkt_testdriver ssl_testdriver user_pass_testdriver reflect_testdriver if HAVE_LD_WRAP_SUPPORT if !WIN32 @@ -144,6 +144,22 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/tls_crypt.c +reflect_testdriver_CFLAGS = @TEST_CFLAGS@ \ + -I$(top_srcdir)/include -I$(top_srcdir)/src/compat -I$(top_srcdir)/src/openvpn +reflect_testdriver_LDFLAGS = @TEST_LDFLAGS@ +reflect_testdriver_SOURCES = test_reflect.c mock_msg.c mock_msg.h \ + $(top_srcdir)/src/openvpn/reflect_filter.c \ + $(top_srcdir)/src/openvpn/bloom.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/crypto.c \ + $(top_srcdir)/src/openvpn/crypto_mbedtls.c \ + $(top_srcdir)/src/openvpn/crypto_openssl.c \ + $(top_srcdir)/src/openvpn/otime.c \ + $(top_srcdir)/src/openvpn/packet_id.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/siphash_reference.c \ + $(top_srcdir)/src/openvpn/win32-util.c + if !WIN32 tls_crypt_testdriver_CFLAGS = \ -I$(top_srcdir)/include -I$(top_srcdir)/src/compat -I$(top_srcdir)/src/openvpn \ diff --git a/tests/unit_tests/openvpn/test_reflect.c b/tests/unit_tests/openvpn/test_reflect.c new file mode 100644 index 0000000..5158631 --- /dev/null +++ b/tests/unit_tests/openvpn/test_reflect.c @@ -0,0 +1,327 @@ +/* + * OpenVPN -- An application to securely tunnel IP networks + * over a single UDP port, with support for SSL/TLS-based + * session authentication and key exchange, + * packet encryption, packet authentication, and + * packet compression. + * + * Copyright (C) 2016-2021 Fox Crypto B.V. <op...@fo...> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program (see the file COPYING included with this + * distribution); if not, write to the Free Software Foundation, Inc., + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#ifdef HAVE_CONFIG_H +#include "config.h" +#elif defined(_MSC_VER) +#include "config-msvc.h" +#endif + +#include "syshead.h" + +#include "bloom.h" +#include "buffer.h" +#include "reflect_filter.h" +#include "test_common.h" + +#include <setjmp.h> +#include <stdarg.h> +#include <cmocka.h> + +#include <stdio.h> + + + +static void +test_bloom(void **state) +{ + static const int present_mod = 77; + + struct gc_arena gc = gc_new(); + /* Use a bloom filter with 1M entries (256kB) for the unit test */ + struct bloom_filter *bf = bloom_create(1024ul*1024, 8, &gc); + + for (int32_t i = 0; i < 20000; i += present_mod) + { + bloom_add(bf, (const uint8_t *) &i, sizeof(i)); + } + + /* all these should be positive, for the small unit test we do not expect + * false positive */ + for (int32_t i = 0; i < 20000; i++) + { + int present = (i % present_mod == 0 ) ? 1 : 0; + /* cast to bool to only get 1 and 0 for present/not present and not */ + assert_int_equal((bool) bloom_test(bf, (const uint8_t *) &i, sizeof(i)), present); + } + + bloom_free(bf); + gc_free(&gc); +} + +static void +test_reflect_ddos(void **state) +{ + /* This tests if the bloom filter implementation does actually work with + * the goal of dropping packets to a reflected /24 while still allowing + * other clients */ + + /* Disable the normal fallback that puts a hard cap on the reflection filter */ + struct initial_packet_rate_limit *irl = initial_rate_limit_init(INT_MAX, 300, NULL); + + reflect_add_filter_tier(&irl->bloom_conf, &irl->gc, false, 24, 5); + reflect_add_filter_tier(&irl->bloom_conf, &irl->gc, false, 8, 20); + init_bloom_filter(irl); + + int num_legimate_reject = 0; + int num_legimate_accepted = 0; + + int num_ddos_rejected = 0; + int num_ddos_accepted = 0; + + + /* /24 net addresses in host byte order */ + in_addr_t net_attack[4]; + for (int i = 0; i < 3; i++) + { + net_attack[i] = random() % 0xff; + } + + /* The 4th network is close enough to the 3rd to fall in the same /16 */ + /* XOR the 3rd byte to achieve this */ + net_attack[3] = net_attack[2] ^ 00002300; + + /* Assume 200000 packets, including roughly 200 legitimate packets, + * the unit test works also with 20 millions packet but takes too long*/ + static const int total_packets = 200 * 1000; + + for (int i = 0; i < total_packets; i++) + { + struct openvpn_sockaddr from = { 0 }; + from.addr.in4.sin_family = AF_INET; + from.addr.in4.sin_addr.s_addr = random(); + from.addr.in4.sin_port = random(); + + if (i % 1023 == 0) + { + /* roughly 200 legitimate clients with random addresses */ + from.addr.in4.sin_addr.s_addr = random(); + + bool allowed = reflect_filter_check(irl, &from); + if (allowed) + { + num_legimate_accepted++; + } + else + { + num_legimate_reject++; + } + + } + else + { + /* We attack the 4 networks at random */ + from.addr.in4.sin_addr.s_addr = net_attack[random() % 4] + (random() % 256); + + bool allowed = reflect_filter_check(irl, &from); + if (allowed) + { + num_ddos_accepted++; + } + else + { + num_ddos_rejected++; + } + } + } + + assert_int_equal(num_legimate_reject + num_legimate_accepted + num_ddos_accepted + num_ddos_rejected, total_packets); + + /* We assume that most legitimate made it through but a few were unfortunate to be in an attacked network */ + assert_in_range(num_legimate_reject, 0, 10); + + /* We disabled total number of packets, so we expect all /8 to have their + * 20 packets, which is 5120. */ + assert_in_range(num_ddos_accepted, 0, 256 * 20); + + initial_rate_limit_free(irl); +} + + +static void +test_bloom_minimal(void **state) +{ + struct gc_arena gc = gc_new(); + struct bloom_filter *bf = bloom_create(2048, 3, &gc); + + int item = 0xbabe; + + bloom_add(bf, (const uint8_t *) &item, sizeof(item)); + assert_int_equal(bloom_test(bf, (const uint8_t *) &item, sizeof(item)), 1); + + item = 0xf00f; + assert_int_equal(bloom_test(bf, (const uint8_t *) &item, sizeof(item)), 0); + + bloom_free(bf); + gc_free(&gc); +} + +static void +test_reflect_reflect_bloom_simple(void **state) +{ + struct initial_packet_rate_limit *irl = initial_rate_limit_init(INT_MAX, 300, NULL); + + reflect_add_filter_tier(&irl->bloom_conf, &irl->gc, true, 32, 50); + reflect_add_filter_tier(&irl->bloom_conf, &irl->gc, true, 56, 200); + init_bloom_filter(irl); + + struct openvpn_sockaddr from = { 0 }; + from.addr.in6.sin6_family = AF_INET6; + from.addr.in6.sin6_port = random(); + from.addr.in6.sin6_addr.s6_addr[15] = 1; /* ::1 */ + + /* There are 50 attempts that should work until one fails */ + for (int i = 0; i < 50; i++) + { + assert_true(reflect_filter_check(irl, &from)); + } + + /* 2002::1 */ + struct openvpn_sockaddr from2 = from; + from2.addr.in6.sin6_addr.s6_addr[0] = 0x7; + from2.addr.in6.sin6_addr.s6_addr[1] = 0x7; + + assert_true(reflect_filter_check(irl, &from2)); + + /* Any more attempts from ::1 should fail */ + assert_false(reflect_filter_check(irl, &from)); + + initial_rate_limit_free(irl); +} + +static void +test_reflect_bloom_netmask_masking(void **state) +{ + struct initial_packet_rate_limit *irl = initial_rate_limit_init(INT_MAX, 300, NULL); + + reflect_add_filter_tier(&irl->bloom_conf, &irl->gc, true, 45, 10); + init_bloom_filter(irl); + + struct openvpn_sockaddr from = { 0 }; + from.addr.in6.sin6_family = AF_INET6; + from.addr.in6.sin6_port = random(); + for (int i = 0; i < 15; i++) + { + from.addr.in6.sin6_addr.s6_addr[i] = random(); + } + + for (int i = 0; i < 10; i++) + { + /* /45 means that if we leave the leave first 8 bytes and 5 bits + * untouched, it is still the same subnet */ + for (int j = 8; j<15; j++) + { + from.addr.in6.sin6_addr.s6_addr[j] = random(); + from.addr.in6.sin6_addr.s6_addr[7] ^= random() & 0x7; + } + assert_true(reflect_filter_check(irl, &from)); + + } + + /* testing the last IP again should give us a negative result */ + assert_false(reflect_filter_check(irl, &from)); + + initial_rate_limit_free(irl); +} + + +static void +test_reflect_reflect_bloom_mapped(void **state) +{ + struct initial_packet_rate_limit *irl = initial_rate_limit_init(INT_MAX, 300, NULL); + + reflect_add_filter_tier(&irl->bloom_conf, &irl->gc, false, 24, 50); + reflect_add_filter_tier(&irl->bloom_conf, &irl->gc, false, 8, 80); + init_bloom_filter(irl); + + /* Our OpenVPN server will not receive IPv4 as well as IPv4 mapped + * addresses in the same process but for the unit test it is convient + * to see if they actually mapped to the same entries */ + + struct openvpn_sockaddr mapped_v4 = { 0 }; + mapped_v4.addr.in6.sin6_family = AF_INET6; + mapped_v4.addr.in6.sin6_port = random(); + /* ::ffff:192.168.0.99 */ + mapped_v4.addr.in6.sin6_addr.s6_addr[10] = 0xff; + mapped_v4.addr.in6.sin6_addr.s6_addr[11] = 0xff; + mapped_v4.addr.in6.sin6_addr.s6_addr[12] = 192; + mapped_v4.addr.in6.sin6_addr.s6_addr[13] = 168; + mapped_v4.addr.in6.sin6_addr.s6_addr[14] = 0; + mapped_v4.addr.in6.sin6_addr.s6_addr[15] = 99; + + assert_true(IN6_IS_ADDR_V4MAPPED(&mapped_v4.addr.in6.sin6_addr)); + + /* Not the same address but in the same /8 */ + struct openvpn_sockaddr v4addr = {0 }; + v4addr.addr.in4.sin_family = AF_INET; + v4addr.addr.in4.sin_port = random(); + /* 192.168.123.244 */ + v4addr.addr.in4.sin_addr.s_addr = htonl(0xc0a87bf4); + + /* check that we run into the 50 limit with our mapped address */ + for (int i = 0; i < 50; i++) + { + assert_true(reflect_filter_check(irl, &mapped_v4)); + } + assert_false(reflect_filter_check(irl, &mapped_v4)); + + + /* Check that the non-mapped IPv4 address uses the same /8 subnet limit */ + for (int i = 0; i < 30; i++) + { + assert_true(reflect_filter_check(irl, &v4addr)); + } + assert_false(reflect_filter_check(irl, &v4addr)); + + initial_rate_limit_free(irl); +} + + +static void +test_bloom_access_functions(void **state) +{ + static_assert(BLOOM_FILTER_BITS_COUNT == 2, "unit test not in sync"); + static_assert(BLOOM_FILTER_BITS_MASK == 0x3, "unit test not in sync"); +} + + +int +main(void) +{ + openvpn_unit_test_setup(); + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_bloom_access_functions), + cmocka_unit_test(test_bloom), + cmocka_unit_test(test_bloom_minimal), + cmocka_unit_test(test_reflect_reflect_bloom_simple), + cmocka_unit_test(test_reflect_reflect_bloom_mapped), + cmocka_unit_test(test_reflect_bloom_netmask_masking), + cmocka_unit_test(test_reflect_ddos), + }; + + + int ret = cmocka_run_group_tests_name("crypto tests", tests, NULL, NULL); + + return ret; +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/30?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0a9274cab7fefce3b13c05052fb9a072e0bfa6b9 Gerrit-Change-Number: 30 Gerrit-PatchSet: 9 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: plaisthos (C. Review) <ge...@op...> - 2024-09-21 14:16:39
|
Attention is currently required from: flichtenheld. plaisthos has uploaded this change for review. ( http://gerrit.openvpn.net/c/openvpn/+/29?usp=email ) Change subject: Add siphash reference implementation ...................................................................... Add siphash reference implementation OpenSSL only supports SIPHASH with OpenSSL 3.1 and newer. The source code of siphash is quite small and has very liberal CC0 license, so include it instead of pulling an extra library for it. Change-Id: I1292894fe7f537049a97bee97af4419e5e854a00 Signed-off-by: Arne Schwabe <ar...@rf...> --- A src/openvpn/siphash.h A src/openvpn/siphash_reference.c 2 files changed, 254 insertions(+), 0 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/29/29/6 diff --git a/src/openvpn/siphash.h b/src/openvpn/siphash.h new file mode 100644 index 0000000..d26ee36 --- /dev/null +++ b/src/openvpn/siphash.h @@ -0,0 +1,31 @@ +/* + * SipHash reference C implementation + * + * Copyright (c) 2012-2021 Jean-Philippe Aumasson + * <jea...@gm...> + * Copyright (c) 2012-2014 Daniel J. Bernstein <dj...@cr...> + * + * To the extent possible under law, the author(s) have dedicated all copyright + * and related and neighboring rights to this software to the public domain + * worldwide. This software is distributed without any warranty. + * + * You should have received a copy of the CC0 Public Domain Dedication along + * with + * this software. If not, see + * <http://creativecommons.org/publicdomain/zero/1.0/>. + */ + +#ifndef SIPHASH_H +#define SIPHASH_H + +#include <inttypes.h> +#include <string.h> + +int siphash(const void *in, size_t inlen, const void *k, uint8_t *out, + size_t outlen); + +/* siphash always uses 128-bit keys */ +#define SIPHASH_KEY_SIZE 16 +#define SIPHASH_HASH_SIZE 16 + +#endif diff --git a/src/openvpn/siphash_reference.c b/src/openvpn/siphash_reference.c new file mode 100644 index 0000000..35af707 --- /dev/null +++ b/src/openvpn/siphash_reference.c @@ -0,0 +1,223 @@ +/* + * SipHash reference C implementation + * + * Copyright 2012-2024 JP Aumasson + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ + +/* Note: the reference implementation is also available under CC0 license + * (dual licensed) we included the MIT license here since it is shorter */ + + #include "siphash.h" +#include <assert.h> +#include <stddef.h> +#include <stdint.h> + +/* default: SipHash-2-4 */ +#ifndef cROUNDS +#define cROUNDS 2 +#endif +#ifndef dROUNDS +#define dROUNDS 4 +#endif + +#define ROTL(x, b) (uint64_t)(((x) << (b)) | ((x) >> (64 - (b)))) + +#define U32TO8_LE(p, v) \ + (p)[0] = (uint8_t)((v)); \ + (p)[1] = (uint8_t)((v) >> 8); \ + (p)[2] = (uint8_t)((v) >> 16); \ + (p)[3] = (uint8_t)((v) >> 24); + +#define U64TO8_LE(p, v) \ + U32TO8_LE((p), (uint32_t)((v))); \ + U32TO8_LE((p) + 4, (uint32_t)((v) >> 32)); + +#define U8TO64_LE(p) \ + (((uint64_t)((p)[0])) | ((uint64_t)((p)[1]) << 8) \ + |((uint64_t)((p)[2]) << 16) | ((uint64_t)((p)[3]) << 24) \ + |((uint64_t)((p)[4]) << 32) | ((uint64_t)((p)[5]) << 40) \ + |((uint64_t)((p)[6]) << 48) | ((uint64_t)((p)[7]) << 56)) + +#define SIPROUND \ + do { \ + v0 += v1; \ + v1 = ROTL(v1, 13); \ + v1 ^= v0; \ + v0 = ROTL(v0, 32); \ + v2 += v3; \ + v3 = ROTL(v3, 16); \ + v3 ^= v2; \ + v0 += v3; \ + v3 = ROTL(v3, 21); \ + v3 ^= v0; \ + v2 += v1; \ + v1 = ROTL(v1, 17); \ + v1 ^= v2; \ + v2 = ROTL(v2, 32); \ + } while (0) + +#ifdef DEBUG_SIPHASH +#include <stdio.h> + +#define TRACE \ + do { \ + printf("(%3zu) v0 %016" PRIx64 "\n", inlen, v0); \ + printf("(%3zu) v1 %016" PRIx64 "\n", inlen, v1); \ + printf("(%3zu) v2 %016" PRIx64 "\n", inlen, v2); \ + printf("(%3zu) v3 %016" PRIx64 "\n", inlen, v3); \ + } while (0) +#else /* ifdef DEBUG_SIPHASH */ +#define TRACE +#endif + +/* + * Computes a SipHash value + * in: pointer to input data (read-only) + * inlen: input data length in bytes (any size_t value) + * k: pointer to the key data (read-only), must be 16 bytes + * out: pointer to output data (write-only), outlen bytes must be allocated + * outlen: length of the output in bytes, must be 8 or 16 + */ +int +siphash(const void *in, const size_t inlen, const void *k, uint8_t *out, + const size_t outlen) +{ + + const unsigned char *ni = (const unsigned char *)in; + const unsigned char *kk = (const unsigned char *)k; + + assert((outlen == 8) || (outlen == 16)); + uint64_t v0 = UINT64_C(0x736f6d6570736575); + uint64_t v1 = UINT64_C(0x646f72616e646f6d); + uint64_t v2 = UINT64_C(0x6c7967656e657261); + uint64_t v3 = UINT64_C(0x7465646279746573); + uint64_t k0 = U8TO64_LE(kk); + uint64_t k1 = U8TO64_LE(kk + 8); + uint64_t m; + int i; + const unsigned char *end = ni + inlen - (inlen % sizeof(uint64_t)); + const int left = inlen & 7; + uint64_t b = ((uint64_t)inlen) << 56; + v3 ^= k1; + v2 ^= k0; + v1 ^= k1; + v0 ^= k0; + + if (outlen == 16) + { + v1 ^= 0xee; + } + + for (; ni != end; ni += 8) + { + m = U8TO64_LE(ni); + v3 ^= m; + + TRACE; + for (i = 0; i < cROUNDS; ++i) + { + SIPROUND; + } + + v0 ^= m; + } + + switch (left) + { + case 7: + b |= ((uint64_t)ni[6]) << 48; + + /* FALLTHRU */ + case 6: + b |= ((uint64_t)ni[5]) << 40; + + /* FALLTHRU */ + case 5: + b |= ((uint64_t)ni[4]) << 32; + + /* FALLTHRU */ + case 4: + b |= ((uint64_t)ni[3]) << 24; + + /* FALLTHRU */ + case 3: + b |= ((uint64_t)ni[2]) << 16; + + /* FALLTHRU */ + case 2: + b |= ((uint64_t)ni[1]) << 8; + + /* FALLTHRU */ + case 1: + b |= ((uint64_t)ni[0]); + break; + + case 0: + break; + } + + v3 ^= b; + + TRACE; + for (i = 0; i < cROUNDS; ++i) + { + SIPROUND; + } + + v0 ^= b; + + if (outlen == 16) + { + v2 ^= 0xee; + } + else + { + v2 ^= 0xff; + } + + TRACE; + for (i = 0; i < dROUNDS; ++i) + { + SIPROUND; + } + + b = v0 ^ v1 ^ v2 ^ v3; + U64TO8_LE(out, b); + + if (outlen == 8) + { + return 0; + } + + v1 ^= 0xdd; + + TRACE; + for (i = 0; i < dROUNDS; ++i) + { + SIPROUND; + } + + b = v0 ^ v1 ^ v2 ^ v3; + U64TO8_LE(out + 8, b); + + return 0; +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/29?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I1292894fe7f537049a97bee97af4419e5e854a00 Gerrit-Change-Number: 29 Gerrit-PatchSet: 6 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: cron2 (C. Review) <ge...@op...> - 2024-09-20 15:46:11
|
Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/746?usp=email ) Change subject: Move cipher/data-ciphers warning to D_LOW (verb 4) ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: So as discussed today, this only scratches the top of the itch... what we want - make "--data-ciphers" accept something like "default plus AES-128-CBC", so future changes in "default" (adding a new supercipher, or deprecating something) will have an effect, and not get negated by having `--data-ciphers AES-128-GCM:AES-128-CBC` stuck in config files (again) - adjust the warning messages(2) to print the full syntax for the `--data-cipher` command to be added -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/746?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ie2797a82ad769cb640440d1ba7dfeb416e7b932d Gerrit-Change-Number: 746 Gerrit-PatchSet: 3 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Fri, 20 Sep 2024 15:45:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment |
From: plaisthos (C. Review) <ge...@op...> - 2024-09-20 12:02:49
|
Attention is currently required from: cron2, flichtenheld. plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/748?usp=email ) Change subject: Change dev null to be a driver type instead of a special mode of tun/tap ...................................................................... Patch Set 10: (1 comment) File src/openvpn/dco.c: http://gerrit.openvpn.net/c/openvpn/+/748/comment/7fcffed5_5e97825a : PS9, Line 312: "offload"); > the wrapping has lost the space ("data channeloffload") and I wouldn't actuall break the string here […] Acknowledged -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/748?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I5987ebb7c38ab176eed7efc004ea54f606a77a12 Gerrit-Change-Number: 748 Gerrit-PatchSet: 10 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Fri, 20 Sep 2024 12:02:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-09-19 15:32:17
|
Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/721?usp=email to look at the new patch set (#9). Change subject: route: extended logic to omit gateway when unnecessary ...................................................................... route: extended logic to omit gateway when unnecessary Extracted and extended the logic behind 'gateway_needed' both in add_route() and add_route_ipv6(). Other than checking the dev-type, special routes and if the gateway is on-link, - set gateway_needed to true if the vpn instance is a multipoint server and DCO is enabled. - set gateway_needed to false if the gateway is in the vpn subnet. Additionally, extended support for these checks and conditions to DARWIN and BSD-based operating systems. These changes ensure that the gateway is only included when necessary, optimizing route configuration and potentially reducing redundant route entries. Change-Id: I87777e74b1fd34781e1d72c9f994eb84f39d800c Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/route.c M src/openvpn/route.h M src/openvpn/tun.c M src/openvpn/tun.h 7 files changed, 418 insertions(+), 253 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/21/721/9 diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 374ba47..e46eaec 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -421,7 +421,7 @@ check_add_routes_action(struct context *c, const bool errors) { bool route_status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx, c->mode == CM_TOP); int flags = (errors ? ISC_ERRORS : 0); flags |= (!route_status ? ISC_ROUTE_ERRORS : 0); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index dd56961..446537a 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1134,7 +1134,7 @@ tuncfg(options->dev, options->dev_type, options->dev_node, options->persist_mode, options->username, options->groupname, &options->tuntap_options, - ctx); + ctx, options->mode == MODE_SERVER); if (options->persist_mode && options->lladdr) { set_lladdr(ctx, options->dev, options->lladdr, NULL); @@ -1689,13 +1689,14 @@ const struct tuntap *tt, const struct plugin_list *plugins, struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { bool ret = true; if (!options->route_noexec && ( route_list || route_ipv6_list ) ) { ret = add_routes(route_list, route_ipv6_list, tt, ROUTE_OPTION_FLAGS(options), - es, ctx); + es, ctx, is_multipoint); setenv_int(es, "redirect_gateway", route_did_redirect_default_gateway(route_list)); } #ifdef ENABLE_MANAGEMENT @@ -1898,7 +1899,7 @@ c->options.dev_node, &gc); do_ifconfig(c->c1.tuntap, guess, c->c2.frame.tun_mtu, c->c2.es, - &c->net_ctx); + &c->net_ctx, c->mode == CM_TOP); } /* possibly add routes */ @@ -1906,7 +1907,7 @@ { /* Ignore route_delay, would cause ROUTE_BEFORE_TUN to be ignored */ bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx, c->mode == CM_TOP); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } #ifdef TARGET_ANDROID @@ -1934,7 +1935,7 @@ && ifconfig_order() == IFCONFIG_AFTER_TUN_OPEN) { do_ifconfig(c->c1.tuntap, c->c1.tuntap->actual_name, - c->c2.frame.tun_mtu, c->c2.es, &c->net_ctx); + c->c2.frame.tun_mtu, c->c2.es, &c->net_ctx, c->mode == CM_TOP); } /* run the up script */ @@ -1960,7 +1961,7 @@ if ((route_order() == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx, c->mode == CM_TOP); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } @@ -2017,7 +2018,7 @@ { undo_ifconfig(c->c1.tuntap, &c->net_ctx); } - close_tun(c->c1.tuntap, &c->net_ctx); + close_tun(c->c1.tuntap, &c->net_ctx, c->mode == CM_TOP); c->c1.tuntap = NULL; } c->c1.tuntap_owned = false; @@ -2086,7 +2087,7 @@ delete_routes(c->c1.route_list, c->c1.route_ipv6_list, c->c1.tuntap, ROUTE_OPTION_FLAGS(&c->options), - c->c2.es, &c->net_ctx); + c->c2.es, &c->net_ctx, c->mode == CM_TOP); } /* actually close tun/tap device based on --down-pre flag */ diff --git a/src/openvpn/init.h b/src/openvpn/init.h index ea7eb30..82eec7e 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -74,7 +74,7 @@ bool do_route(const struct options *options, struct route_list *route_list, struct route_ipv6_list *route_ipv6_list, const struct tuntap *tt, const struct plugin_list *plugins, struct env_set *es, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, const bool is_multipoint); void close_instance(struct context *c); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index ae7707c..31a634a 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -77,7 +77,7 @@ static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, const bool is_multipoint); static void get_bypass_addresses(struct route_bypass *rb, const unsigned int flags); @@ -927,7 +927,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { struct route_ipv4 r; CLEAR(r); @@ -935,7 +936,7 @@ r.network = network; r.netmask = netmask; r.gateway = gateway; - return add_route(&r, tt, flags, rgi, es, ctx); + return add_route(&r, tt, flags, rgi, es, ctx, is_multipoint); } static void @@ -946,7 +947,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { struct route_ipv4 r; CLEAR(r); @@ -954,7 +956,7 @@ r.network = network; r.netmask = netmask; r.gateway = gateway; - delete_route(&r, tt, flags, rgi, es, ctx); + delete_route(&r, tt, flags, rgi, es, ctx, is_multipoint); } static bool @@ -964,7 +966,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { int ret = true; for (int i = 0; i < rb->n_bypass; ++i) @@ -972,7 +975,7 @@ if (rb->bypass[i]) { ret = add_route3(rb->bypass[i], IPV4_NETMASK_HOST, gateway, tt, - flags | ROUTE_REF_GW, rgi, es, ctx) && ret; + flags | ROUTE_REF_GW, rgi, es, ctx, is_multipoint) && ret; } } return ret; @@ -985,7 +988,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { int i; for (i = 0; i < rb->n_bypass; ++i) @@ -999,7 +1003,8 @@ flags | ROUTE_REF_GW, rgi, es, - ctx); + ctx, + is_multipoint); } } } @@ -1007,7 +1012,7 @@ static bool redirect_default_route_to_vpn(struct route_list *rl, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, const bool is_multipoint) { const char err[] = "NOTE: unable to redirect IPv4 default gateway --"; bool ret = true; @@ -1059,7 +1064,7 @@ { ret = add_route3(rl->spec.remote_host, IPV4_NETMASK_HOST, rl->rgi.gateway.addr, tt, flags | ROUTE_REF_GW, - &rl->rgi, es, ctx); + &rl->rgi, es, ctx, is_multipoint); rl->iflags |= RL_DID_LOCAL; } else @@ -1071,7 +1076,7 @@ /* route DHCP/DNS server traffic through original default gateway */ ret = add_bypass_routes(&rl->spec.bypass, rl->rgi.gateway.addr, tt, flags, - &rl->rgi, es, ctx) && ret; + &rl->rgi, es, ctx, is_multipoint) && ret; if (rl->flags & RG_REROUTE_GW) { @@ -1079,11 +1084,11 @@ { /* add new default route (1st component) */ ret = add_route3(0x00000000, 0x80000000, rl->spec.remote_endpoint, - tt, flags, &rl->rgi, es, ctx) && ret; + tt, flags, &rl->rgi, es, ctx, is_multipoint) && ret; /* add new default route (2nd component) */ ret = add_route3(0x80000000, 0x80000000, rl->spec.remote_endpoint, - tt, flags, &rl->rgi, es, ctx) && ret; + tt, flags, &rl->rgi, es, ctx, is_multipoint) && ret; } else { @@ -1092,12 +1097,12 @@ { /* delete default route */ del_route3(0, 0, rl->rgi.gateway.addr, tt, - flags | ROUTE_REF_GW, &rl->rgi, es, ctx); + flags | ROUTE_REF_GW, &rl->rgi, es, ctx, is_multipoint); } /* add new default route */ ret = add_route3(0, 0, rl->spec.remote_endpoint, tt, - flags, &rl->rgi, es, ctx) && ret; + flags, &rl->rgi, es, ctx, is_multipoint) && ret; } } @@ -1112,7 +1117,8 @@ undo_redirect_default_route_to_vpn(struct route_list *rl, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { if (rl && rl->iflags & RL_DID_REDIRECT_DEFAULT_GATEWAY) { @@ -1126,13 +1132,14 @@ flags | ROUTE_REF_GW, &rl->rgi, es, - ctx); + ctx, + is_multipoint); rl->iflags &= ~RL_DID_LOCAL; } /* delete special DHCP/DNS bypass route */ del_bypass_routes(&rl->spec.bypass, rl->rgi.gateway.addr, tt, flags, - &rl->rgi, es, ctx); + &rl->rgi, es, ctx, is_multipoint); if (rl->flags & RG_REROUTE_GW) { @@ -1146,7 +1153,8 @@ flags, &rl->rgi, es, - ctx); + ctx, + is_multipoint); /* delete default route (2nd component) */ del_route3(0x80000000, @@ -1156,7 +1164,8 @@ flags, &rl->rgi, es, - ctx); + ctx, + is_multipoint); } else { @@ -1168,12 +1177,13 @@ flags, &rl->rgi, es, - ctx); + ctx, + is_multipoint); /* restore original default route if there was any */ if (rl->rgi.flags & RGI_ADDR_DEFINED) { add_route3(0, 0, rl->rgi.gateway.addr, tt, - flags | ROUTE_REF_GW, &rl->rgi, es, ctx); + flags | ROUTE_REF_GW, &rl->rgi, es, ctx, is_multipoint); } } } @@ -1185,9 +1195,10 @@ bool add_routes(struct route_list *rl, struct route_ipv6_list *rl6, const struct tuntap *tt, unsigned int flags, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, + const bool is_multipoint) { - bool ret = redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + bool ret = redirect_default_route_to_vpn(rl, tt, flags, es, ctx, is_multipoint); if (rl && !(rl->iflags & RL_ROUTES_ADDED) ) { struct route_ipv4 *r; @@ -1218,9 +1229,9 @@ check_subnet_conflict(r->network, r->netmask, "route"); if (flags & ROUTE_DELETE_FIRST) { - delete_route(r, tt, flags, &rl->rgi, es, ctx); + delete_route(r, tt, flags, &rl->rgi, es, ctx, is_multipoint); } - ret = add_route(r, tt, flags, &rl->rgi, es, ctx) && ret; + ret = add_route(r, tt, flags, &rl->rgi, es, ctx, is_multipoint) && ret; } rl->iflags |= RL_ROUTES_ADDED; } @@ -1240,9 +1251,9 @@ { if (flags & ROUTE_DELETE_FIRST) { - delete_route_ipv6(r, tt, flags, es, ctx); + delete_route_ipv6(r, tt, flags, es, ctx, is_multipoint); } - ret = add_route_ipv6(r, tt, flags, es, ctx) && ret; + ret = add_route_ipv6(r, tt, flags, es, ctx, is_multipoint) && ret; } rl6->iflags |= RL_ROUTES_ADDED; } @@ -1253,19 +1264,20 @@ void delete_routes(struct route_list *rl, struct route_ipv6_list *rl6, const struct tuntap *tt, unsigned int flags, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, + const bool is_multipoint) { if (rl && rl->iflags & RL_ROUTES_ADDED) { struct route_ipv4 *r; for (r = rl->routes; r; r = r->next) { - delete_route(r, tt, flags, &rl->rgi, es, ctx); + delete_route(r, tt, flags, &rl->rgi, es, ctx, is_multipoint); } rl->iflags &= ~RL_ROUTES_ADDED; } - undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx, is_multipoint); if (rl) { @@ -1277,7 +1289,7 @@ struct route_ipv6 *r6; for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) { - delete_route_ipv6(r6, tt, flags, es, ctx); + delete_route_ipv6(r6, tt, flags, es, ctx, is_multipoint); } rl6->iflags &= ~RL_ROUTES_ADDED; } @@ -1561,13 +1573,47 @@ } #endif +static bool +is_gateway_needed_ipv4(const struct route_ipv4 *r4, + const struct route_gateway_info *rgi, + const struct tuntap *tt, + const bool is_multipoint) +{ + +#ifndef _WIN32 + + if (rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ + { + if (rgi->flags & RGI_ADDR_DEFINED && r4->gateway != 0) + { + return true; + } + } + +#endif + + if (tt->type == DEV_TYPE_TAP + && !( (r4->flags & RT_METRIC_DEFINED) && r4->metric == 0 ) ) + { + return true; + } + + if (is_multipoint && !tt->options.disable_dco) + { + return true; + } + + return false; +} + bool add_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, /* may be NULL */ const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { int status = 0; int is_local_route; @@ -1595,21 +1641,7 @@ goto done; } -#ifndef _WIN32 - if (rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ - { - if (rgi->flags & RGI_ADDR_DEFINED && r->gateway != 0) - { - gateway_needed = true; - } - } -#endif - - if (tt->type == DEV_TYPE_TAP - && !( (r->flags & RT_METRIC_DEFINED) && r->metric == 0 ) ) - { - gateway_needed = true; - } + gateway_needed = is_gateway_needed_ipv4(r, rgi, tt, is_multipoint); if (gateway_needed && r->gateway == 0) { @@ -1621,18 +1653,21 @@ goto done; } -#if defined(TARGET_LINUX) - const char *iface = tt->actual_name; +#if !defined(_WIN32) && !defined(TARGET_SOLARIS) \ + && !defined(TARGET_AIX) + const char *device = tt->actual_name; if (!gateway_needed && rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ { - iface = rgi->iface; + device = rgi->iface; } +#endif +#if defined(TARGET_LINUX) int metric = -1; if (is_on_link(is_local_route, flags, rgi)) { - iface = rgi->iface; + device = rgi->iface; } if (r->flags & RT_METRIC_DEFINED) @@ -1643,7 +1678,7 @@ status = RTA_SUCCESS; int ret = net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask), gateway_needed ? &r->gateway : NULL, - iface, 0, metric); + device, 0, metric); if (ret == -EEXIST) { msg(D_ROUTE, "NOTE: Linux route add command failed because route exists"); @@ -1660,7 +1695,7 @@ if (rgi) { - snprintf(out, sizeof(out), "%s %s %s dev %s", network, netmask, gateway, rgi->iface); + snprintf(out, sizeof(out), "%s %s %s dev %s", network, netmask, gateway, device); } else { @@ -1764,9 +1799,9 @@ "ERROR: Solaris route add command failed"); status = ret ? RTA_SUCCESS : RTA_ERROR; -#elif defined(TARGET_FREEBSD) +#elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) - argv_printf(&argv, "%s add", + argv_printf(&argv, "%s add ", ROUTE_PATH); #if 0 @@ -1776,45 +1811,28 @@ } #endif - argv_printf_cat(&argv, "-net %s %s %s", - network, - gateway, - netmask); - - /* FIXME -- add on-link support for FreeBSD */ - - argv_msg(D_ROUTE, &argv); - bool ret = openvpn_execve_check(&argv, es, 0, - "ERROR: FreeBSD route add command failed"); - status = ret ? RTA_SUCCESS : RTA_ERROR; - -#elif defined(TARGET_DRAGONFLY) - - argv_printf(&argv, "%s add", - ROUTE_PATH); - -#if 0 - if (r->flags & RT_METRIC_DEFINED) + if (gateway_needed) { - argv_printf_cat(&argv, "-rtt %d", r->metric); + argv_printf_cat(&argv, "-net %s %s %s", + network, + gateway, + netmask); } -#endif - - argv_printf_cat(&argv, "-net %s %s %s", - network, - gateway, - netmask); - - /* FIXME -- add on-link support for Dragonfly */ + else + { + argv_printf_cat(&argv, "-net %s -iface %s", + network, + device); + } argv_msg(D_ROUTE, &argv); bool ret = openvpn_execve_check(&argv, es, 0, - "ERROR: DragonFly route add command failed"); + "ERROR: BSD route add command failed"); status = ret ? RTA_SUCCESS : RTA_ERROR; #elif defined(TARGET_DARWIN) - argv_printf(&argv, "%s add", + argv_printf(&argv, "%s add ", ROUTE_PATH); #if 0 @@ -1835,10 +1853,19 @@ } else { - argv_printf_cat(&argv, "-net %s %s %s", - network, - gateway, - netmask); + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -interface %s", + network, + device); + } } argv_msg(D_ROUTE, &argv); @@ -1848,7 +1875,7 @@ #elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) - argv_printf(&argv, "%s add", + argv_printf(&argv, "%s add ", ROUTE_PATH); #if 0 @@ -1858,12 +1885,20 @@ } #endif - argv_printf_cat(&argv, "-net %s %s -netmask %s", - network, - gateway, - netmask); - - /* FIXME -- add on-link support for OpenBSD/NetBSD */ + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s -netmask %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -netmask %s -link -iface %s", + network, + netmask, + device); + } argv_msg(D_ROUTE, &argv); bool ret = openvpn_execve_check(&argv, es, 0, @@ -1928,10 +1963,98 @@ } } +static inline struct in6_addr +netbits_to_netmask_ipv6(const int netbits) +{ + struct in6_addr netmask = {{{0}}}; + + if (netbits > 0 && netbits <= 128) + { + const int full_bytes = netbits / 8; + int i; + for (i = 0; i < full_bytes; i++) + { + netmask.s6_addr[i] = 0xFF; + } + if (netbits % 8) + { + netmask.s6_addr[i] = (0xFF << (8 - (netbits % 8))); + } + } + + return netmask; +} + +static bool +is_gateway_in_vpn_subnet_ipv6(const struct in6_addr *gateway, + const struct in6_addr *vpn_subnet, + int netbits) +{ + struct in6_addr netmask = netbits_to_netmask_ipv6(netbits); + + for (int i = 0; i < 16; i++) + { + if ((gateway->s6_addr[i] & netmask.s6_addr[i]) != (vpn_subnet->s6_addr[i] & netmask.s6_addr[i])) + { + return false; + } + } + return true; +} + +static bool +is_gateway_needed_ipv6(const struct route_ipv6 *r6, + const struct tuntap *tt, + const char *network, + const bool is_multipoint) +{ + +#ifndef _WIN32 + if (r6->iface != NULL) /* VPN server special route */ + { + if (!IN6_IS_ADDR_UNSPECIFIED(&r6->gateway)) + { + return true; + } + } +#endif + + /* + * Filter out routes which are essentially no-ops + * (not currently done for IPv6) + */ + + /* On "tun" interface, we never set a gateway if the operating system + * can do "route to interface" - it does not add value, as the target + * dev already fully qualifies the route destination on point-to-point + * interfaces. OTOH, on "tap" interface, we must always set the + * gateway unless the route is to be an on-link network + */ + if (tt->type == DEV_TYPE_TAP + && !( (r6->flags & RT_METRIC_DEFINED) && r6->metric == 0 ) ) + { + return true; + } + + /* if is server and dco enabled, the gateway is needed*/ + if (is_multipoint && !tt->options.disable_dco) + { + return true; + } + + if (is_gateway_in_vpn_subnet_ipv6(&r6->gateway, &tt->local_ipv6, tt->netbits_ipv6)) + { + msg(D_ROUTE, "Ignoring gateway in VPN subnet for route %s/%d", network, r6->netbits); + return false; + } + + return false; +} + bool add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, const bool is_multipoint) { int status = 0; bool gateway_needed = false; @@ -1944,22 +2067,12 @@ struct argv argv = argv_new(); struct gc_arena gc = gc_new(); -#ifndef _WIN32 - const char *device = tt->actual_name; - if (r6->iface != NULL) /* vpn server special route */ - { - device = r6->iface; - if (!IN6_IS_ADDR_UNSPECIFIED(&r6->gateway) ) - { - gateway_needed = true; - } - } -#endif - route_ipv6_clear_host_bits(r6); const char *network = print_in6_addr( r6->network, 0, &gc); const char *gateway = print_in6_addr( r6->gateway, 0, &gc); + gateway_needed = is_gateway_needed_ipv6(r6, tt, network, is_multipoint); + #if defined(TARGET_DARWIN) \ || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \ || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) @@ -1978,6 +2091,15 @@ } #endif +#if !defined(_WIN32) + + const char *device = tt->actual_name; + if (r6->iface != NULL) + { + device = r6->iface; + } +#endif + #ifndef _WIN32 msg(D_ROUTE, "add_route_ipv6(%s/%d -> %s metric %d) dev %s", network, r6->netbits, gateway, r6->metric, device ); @@ -1987,23 +2109,6 @@ r6->adapter_index ? r6->adapter_index : tt->adapter_index); #endif - /* - * Filter out routes which are essentially no-ops - * (not currently done for IPv6) - */ - - /* On "tun" interface, we never set a gateway if the operating system - * can do "route to interface" - it does not add value, as the target - * dev already fully qualifies the route destination on point-to-point - * interfaces. OTOH, on "tap" interface, we must always set the - * gateway unless the route is to be an on-link network - */ - if (tt->type == DEV_TYPE_TAP - && !( (r6->flags & RT_METRIC_DEFINED) && r6->metric == 0 ) ) - { - gateway_needed = true; - } - if (gateway_needed && IN6_IS_ADDR_UNSPECIFIED(&r6->gateway)) { msg(M_WARN, "ROUTE6 WARNING: " PACKAGE_NAME " needs a gateway " @@ -2016,6 +2121,7 @@ #if defined(TARGET_LINUX) int metric = -1; + if ((r6->flags & RT_METRIC_DEFINED) && (r6->metric > 0)) { metric = r6->metric; @@ -2085,7 +2191,7 @@ #elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) - argv_printf(&argv, "%s add -inet6 %s/%d", + argv_printf(&argv, "%s add -inet6 %s/%d ", ROUTE_PATH, network, r6->netbits); @@ -2106,7 +2212,7 @@ #elif defined(TARGET_DARWIN) - argv_printf(&argv, "%s add -inet6 %s -prefixlen %d", + argv_printf(&argv, "%s add -inet6 %s -prefixlen %d ", ROUTE_PATH, network, r6->netbits ); @@ -2124,26 +2230,24 @@ "ERROR: MacOS X route add -inet6 command failed"); status = ret ? RTA_SUCCESS : RTA_ERROR; -#elif defined(TARGET_OPENBSD) +#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) - argv_printf(&argv, "%s add -inet6 %s -prefixlen %d %s", + argv_printf(&argv, "%s add -inet6 %s -prefixlen %d ", ROUTE_PATH, - network, r6->netbits, gateway ); + network, r6->netbits); + + if (gateway_needed) + { + argv_printf_cat(&argv, "%s", gateway); + } + else + { + argv_printf_cat(&argv, "-link -iface %s", device); + } argv_msg(D_ROUTE, &argv); bool ret = openvpn_execve_check(&argv, es, 0, - "ERROR: OpenBSD route add -inet6 command failed"); - status = ret ? RTA_SUCCESS : RTA_ERROR; - -#elif defined(TARGET_NETBSD) - - argv_printf(&argv, "%s add -inet6 %s/%d %s", - ROUTE_PATH, - network, r6->netbits, gateway ); - - argv_msg(D_ROUTE, &argv); - bool ret = openvpn_execve_check(&argv, es, 0, - "ERROR: NetBSD route add -inet6 command failed"); + "ERROR: OpenBSD/NetBSD route add -inet6 command failed"); status = ret ? RTA_SUCCESS : RTA_ERROR; #elif defined(TARGET_AIX) @@ -2183,7 +2287,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { #if !defined(TARGET_LINUX) const char *network; @@ -2198,6 +2303,21 @@ #endif int is_local_route; +#if defined(TARGET_DARWIN) || defined(TARGET_LINUX) \ + || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \ + || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) + + bool gateway_needed = is_gateway_needed_ipv4(r, rgi, tt, is_multipoint); + +#if !defined(TARGET_OPENBSD) && !defined(TARGET_NETBSD) + const char *device = tt->actual_name; + if (!gateway_needed && rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ + { + device = rgi->iface; + } +#endif +#endif + if ((r->flags & (RT_DEFINED|RT_ADDED)) != (RT_DEFINED|RT_ADDED)) { return; @@ -2224,13 +2344,20 @@ #if defined(TARGET_LINUX) metric = -1; + + if (is_on_link(is_local_route, flags, rgi)) + { + device = rgi->iface; + } + if (r->flags & RT_METRIC_DEFINED) { metric = r->metric; } if (net_route_v4_del(ctx, &r->network, netmask_to_netbits2(r->netmask), - &r->gateway, NULL, 0, metric) < 0) + gateway_needed ? &r->gateway : NULL, + device, 0, metric) < 0) { msg(M_WARN, "ERROR: Linux route delete command failed"); } @@ -2289,27 +2416,26 @@ argv_msg(D_ROUTE, &argv); openvpn_execve_check(&argv, es, 0, "ERROR: Solaris route delete command failed"); -#elif defined(TARGET_FREEBSD) +#elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) - argv_printf(&argv, "%s delete -net %s %s %s", - ROUTE_PATH, - network, - gateway, - netmask); + argv_printf(&argv, "%s delete", ROUTE_PATH); + + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -iface %s", + network, + device); + } argv_msg(D_ROUTE, &argv); - openvpn_execve_check(&argv, es, 0, "ERROR: FreeBSD route delete command failed"); - -#elif defined(TARGET_DRAGONFLY) - - argv_printf(&argv, "%s delete -net %s %s %s", - ROUTE_PATH, - network, - gateway, - netmask); - - argv_msg(D_ROUTE, &argv); - openvpn_execve_check(&argv, es, 0, "ERROR: DragonFly route delete command failed"); + openvpn_execve_check(&argv, es, 0, "ERROR: BSD route delete command failed"); #elif defined(TARGET_DARWIN) @@ -2323,11 +2449,22 @@ } else { - argv_printf(&argv, "%s delete -net %s %s %s", - ROUTE_PATH, - network, - gateway, - netmask); + argv_printf(&argv, "%s delete ", + ROUTE_PATH); + + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -interface %s", + network, + device); + } } argv_msg(D_ROUTE, &argv); @@ -2335,11 +2472,22 @@ #elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) - argv_printf(&argv, "%s delete -net %s %s -netmask %s", - ROUTE_PATH, - network, - gateway, - netmask); + argv_printf(&argv, "%s delete ", + ROUTE_PATH); + + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s -netmask %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -netmask %s", + network, + netmask); + } argv_msg(D_ROUTE, &argv); openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD/NetBSD route delete command failed"); @@ -2374,7 +2522,7 @@ void delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, const bool is_multipoint) { const char *network; @@ -2387,23 +2535,15 @@ #if !defined(TARGET_LINUX) const char *gateway; #endif -#if !defined(TARGET_SOLARIS) - bool gateway_needed = false; +#if defined(TARGET_DARWIN) || defined(TARGET_LINUX) \ + || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) + const char *device = tt->actual_name; if (r6->iface != NULL) /* vpn server special route */ { device = r6->iface; - gateway_needed = true; } - /* if we used a gateway on "add route", we also need to specify it on - * delete, otherwise some OSes will refuse to delete the route - */ - if (tt->type == DEV_TYPE_TAP - && !( (r6->flags & RT_METRIC_DEFINED) && r6->metric == 0 ) ) - { - gateway_needed = true; - } #endif #endif @@ -2415,6 +2555,16 @@ gateway = print_in6_addr( r6->gateway, 0, &gc); #endif +#if defined(TARGET_DARWIN) || defined(TARGET_LINUX) \ + || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \ + || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) + /* if we used a gateway on "add route", we also need to specify it on + * delete, otherwise some OSes will refuse to delete the route + */ + bool gateway_needed = false; + gateway_needed = is_gateway_needed_ipv6(r6, tt, network, is_multipoint); +#endif + #if defined(TARGET_DARWIN) \ || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \ || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) @@ -2509,23 +2659,19 @@ argv_msg(D_ROUTE, &argv); openvpn_execve_check(&argv, es, 0, "ERROR: MacOS X route delete -inet6 command failed"); -#elif defined(TARGET_OPENBSD) +#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) - argv_printf(&argv, "%s delete -inet6 %s -prefixlen %d %s", + argv_printf(&argv, "%s delete -inet6 %s -prefixlen %d", ROUTE_PATH, - network, r6->netbits, gateway ); + network, r6->netbits); + + if (gateway_needed) + { + argv_printf_cat(&argv, "%s", gateway); + } argv_msg(D_ROUTE, &argv); - openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD route delete -inet6 command failed"); - -#elif defined(TARGET_NETBSD) - - argv_printf(&argv, "%s delete -inet6 %s/%d %s", - ROUTE_PATH, - network, r6->netbits, gateway ); - - argv_msg(D_ROUTE, &argv); - openvpn_execve_check(&argv, es, 0, "ERROR: NetBSD route delete -inet6 command failed"); + openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD/NetBSD route delete -inet6 command failed"); #elif defined(TARGET_AIX) diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 421e7d2..4c3f317 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -271,13 +271,13 @@ void route_ipv6_clear_host_bits( struct route_ipv6 *r6 ); -bool add_route_ipv6(struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx); +bool add_route_ipv6(struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint); -void delete_route_ipv6(const struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx); +void delete_route_ipv6(const struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint); bool add_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, const bool is_multipoint); void add_route_to_option_list(struct route_option_list *l, const char *network, @@ -312,14 +312,16 @@ bool add_routes(struct route_list *rl, struct route_ipv6_list *rl6, const struct tuntap *tt, unsigned int flags, - const struct env_set *es, openvpn_net_ctx_t *ctx); + const struct env_set *es, openvpn_net_ctx_t *ctx, + const bool is_multipoint); void delete_routes(struct route_list *rl, struct route_ipv6_list *rl6, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, + const bool is_multipoint); void setenv_routes(struct env_set *es, const struct route_list *rl); diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 82c5c00..c3c321c 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -1017,7 +1017,7 @@ */ static void add_route_connected_v6_net(struct tuntap *tt, - const struct env_set *es) + const struct env_set *es, const bool is_multipoint) { struct route_ipv6 r6; @@ -1027,11 +1027,11 @@ r6.gateway = tt->local_ipv6; r6.metric = 0; /* connected route */ r6.flags = RT_DEFINED | RT_METRIC_DEFINED; - add_route_ipv6(&r6, tt, 0, es, NULL); + add_route_ipv6(&r6, tt, 0, es, NULL, is_multipoint); } void -delete_route_connected_v6_net(const struct tuntap *tt) +delete_route_connected_v6_net(const struct tuntap *tt, const bool is_multipoint) { struct route_ipv6 r6; @@ -1042,7 +1042,7 @@ r6.metric = 0; /* connected route */ r6.flags = RT_DEFINED | RT_ADDED | RT_METRIC_DEFINED; route_ipv6_clear_host_bits(&r6); - delete_route_ipv6(&r6, tt, 0, NULL, NULL); + delete_route_ipv6(&r6, tt, 0, NULL, NULL, is_multipoint); } #endif /* if defined(_WIN32) || defined(TARGET_DARWIN) || defined(TARGET_NETBSD) || defined(TARGET_OPENBSD) */ @@ -1084,7 +1084,8 @@ */ static void do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, + const bool is_multipoint) { #if !defined(TARGET_LINUX) struct argv argv = argv_new(); @@ -1223,7 +1224,7 @@ do_address_service(true, AF_INET6, tt); if (tt->type == DEV_TYPE_TUN) { - add_route_connected_v6_net(tt, es); + add_route_connected_v6_net(tt, es, is_multipoint); } do_dns_service(true, AF_INET6, tt); do_set_mtu_service(tt, AF_INET6, tun_mtu); @@ -1251,7 +1252,7 @@ netsh_command(&argv, 4, M_FATAL); if (tt->type == DEV_TYPE_TUN) { - add_route_connected_v6_net(tt, es); + add_route_connected_v6_net(tt, es, is_multipoint); } /* set ipv6 dns servers if any are specified */ netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, tt->adapter_index); @@ -1283,7 +1284,7 @@ */ static void do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint) { #if !defined(_WIN32) && !defined(TARGET_ANDROID) /* @@ -1388,7 +1389,7 @@ r.netmask = tt->remote_netmask; r.gateway = tt->local; r.metric = 0; - add_route(&r, tt, 0, NULL, es, NULL); + add_route(&r, tt, 0, NULL, es, NULL, is_multipoint); } #elif defined(TARGET_OPENBSD) @@ -1435,7 +1436,7 @@ r.network = tt->local & tt->remote_netmask; r.netmask = tt->remote_netmask; r.gateway = remote_end; - add_route(&r, tt, 0, NULL, es, NULL); + add_route(&r, tt, 0, NULL, es, NULL, is_multipoint); } #elif defined(TARGET_NETBSD) @@ -1477,7 +1478,7 @@ r.network = tt->local & tt->remote_netmask; r.netmask = tt->remote_netmask; r.gateway = remote_end; - add_route(&r, tt, 0, NULL, es, NULL); + add_route(&r, tt, 0, NULL, es, NULL, is_multipoint); } #elif defined(TARGET_DARWIN) @@ -1524,7 +1525,7 @@ r.network = tt->local & tt->remote_netmask; r.netmask = tt->remote_netmask; r.gateway = tt->local; - add_route(&r, tt, 0, NULL, es, NULL); + add_route(&r, tt, 0, NULL, es, NULL, is_multipoint); } #elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) @@ -1618,7 +1619,7 @@ /* execute the ifconfig command through the shell */ void do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint) { msg(D_LOW, "do_ifconfig, ipv4=%d, ipv6=%d", tt->did_ifconfig_setup, tt->did_ifconfig_ipv6_setup); @@ -1638,12 +1639,12 @@ if (tt->did_ifconfig_setup) { - do_ifconfig_ipv4(tt, ifname, tun_mtu, es, ctx); + do_ifconfig_ipv4(tt, ifname, tun_mtu, es, ctx, is_multipoint); } if (tt->did_ifconfig_ipv6_setup) { - do_ifconfig_ipv6(tt, ifname, tun_mtu, es, ctx); + do_ifconfig_ipv6(tt, ifname, tun_mtu, es, ctx, is_multipoint); } /* release resources potentially allocated during interface setup */ @@ -2108,12 +2109,13 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); close_tun_generic(tt); free(tt); + (void)is_multipoint; } int @@ -2277,7 +2279,8 @@ void tuncfg(const char *dev, const char *dev_type, const char *dev_node, int persist_mode, const char *username, const char *groupname, - const struct tuntap_options *options, openvpn_net_ctx_t *ctx) + const struct tuntap_options *options, openvpn_net_ctx_t *ctx, + const bool is_multipoint) { struct tuntap *tt; @@ -2317,14 +2320,14 @@ msg(M_ERR, "Cannot ioctl TUNSETGROUP(%s) %s", groupname, dev); } } - close_tun(tt, ctx); + close_tun(tt, ctx, is_multipoint); msg(M_INFO, "Persist state set to: %s", (persist_mode ? "ON" : "OFF")); } #endif /* ENABLE_FEATURE_TUN_PERSIST */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -2336,6 +2339,7 @@ #endif close_tun_generic(tt); free(tt); + (void)is_multipoint; } int @@ -2647,7 +2651,7 @@ * Close TUN device. */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -2657,6 +2661,7 @@ clear_tuntap(tt); free(tt); + (void)is_multipoint; } static void @@ -2680,7 +2685,7 @@ argv_msg(M_INFO, &argv); openvpn_execve_check(&argv, es, 0, "Solaris ifconfig unplumb failed"); - close_tun(tt, NULL); + close_tun(tt, NULL, false); msg(M_FATAL, "Solaris ifconfig failed"); argv_free(&argv); } @@ -2744,10 +2749,12 @@ */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); + (void)is_multipoint; + /* only *TAP* devices need destroying, tun devices auto-self-destruct */ if (tt->type == DEV_TYPE_TUN || tt->persistent_if) @@ -2858,10 +2865,12 @@ * need to be explicitly destroyed */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); + (void)is_multipoint; + /* only tun devices need destroying, tap devices auto-self-destruct */ if (tt->type != DEV_TYPE_TUN || tt->persistent_if) @@ -3014,10 +3023,12 @@ * we need to call "ifconfig ... destroy" for cleanup */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); + (void)is_multipoint; + if (tt->persistent_if) /* keep pre-existing if around */ { close_tun_generic(tt); @@ -3131,12 +3142,13 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); close_tun_generic(tt); free(tt); + (void)is_multipoint; } int @@ -3398,7 +3410,7 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -3420,6 +3432,7 @@ free(tt); argv_free(&argv); gc_free(&gc); + (void)is_multipoint; } int @@ -3547,7 +3560,7 @@ /* tap devices need to be manually destroyed on AIX */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -3575,6 +3588,7 @@ free(tt); env_set_destroy(es); argv_free(&argv); + (void)is_multipoint; } int @@ -6862,7 +6876,7 @@ } static void -netsh_delete_address_dns(const struct tuntap *tt, bool ipv6, struct gc_arena *gc) +netsh_delete_address_dns(const struct tuntap *tt, bool ipv6, struct gc_arena *gc, const bool is_multipoint) { const char *ifconfig_ip_local; struct argv argv = argv_new(); @@ -6892,7 +6906,7 @@ if (ipv6 && tt->type == DEV_TYPE_TUN) { - delete_route_connected_v6_net(tt); + delete_route_connected_v6_net(tt, is_multipoint); } /* "store=active" is needed in Windows 8(.1) to delete the @@ -6962,7 +6976,7 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -6982,7 +6996,7 @@ do_dns_domain_service(false, tt); } do_dns_service(false, AF_INET6, tt); - delete_route_connected_v6_net(tt); + delete_route_connected_v6_net(tt, is_multipoint); do_address_service(false, AF_INET6, tt); } else @@ -6992,7 +7006,7 @@ do_dns_domain_wmic(false, tt); } - netsh_delete_address_dns(tt, true, &gc); + netsh_delete_address_dns(tt, true, &gc, is_multipoint); } } @@ -7019,7 +7033,7 @@ if (tt->options.ip_win32_type == IPW32_SET_NETSH) { - netsh_delete_address_dns(tt, false, &gc); + netsh_delete_address_dns(tt, false, &gc, is_multipoint); } } } @@ -7140,12 +7154,13 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); close_tun_generic(tt); free(tt); + (void)is_multipoint; } int diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index 33b9552..9716d19 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -266,7 +266,7 @@ void open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tuntap *tt, openvpn_net_ctx_t *ctx); -void close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx); +void close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint); void tun_open_device(struct tuntap *tt, const char *dev_node, const char **device_guid, struct gc_arena *gc); @@ -280,7 +280,7 @@ void tuncfg(const char *dev, const char *dev_type, const char *dev_node, int persist_mode, const char *username, const char *groupname, const struct tuntap_options *options, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, const bool is_multipoint); const char *guess_tuntap_dev(const char *dev, const char *dev_type, @@ -319,7 +319,7 @@ * @param ctx the networking API opaque context */ void do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, - const struct env_set *es, openvpn_net_ctx_t *ctx); + const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint); /** * undo_ifconfig - undo configuration of the tunnel interface @@ -754,6 +754,7 @@ } const char *tun_stat(const struct tuntap *tt, unsigned int rwflags, struct gc_arena *gc); + bool tun_name_is_fixed(const char *dev); static inline bool -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/721?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I87777e74b1fd34781e1d72c9f994eb84f39d800c Gerrit-Change-Number: 721 Gerrit-PatchSet: 9 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset |
From: plaisthos (C. Review) <ge...@op...> - 2024-09-19 14:07:07
|
Attention is currently required from: flichtenheld. Hello flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/754?usp=email to review the following change. Change subject: Ensure that the AF_UNIX socket pair has at least 65k of buffer space ...................................................................... Ensure that the AF_UNIX socket pair has at least 65k of buffer space Without this change, pinging a lwipovpn client with something like a 3000 byte payload on macOS often fails as the default buffer sizes on macOS are 2048 for send and 4096 for receive. Change-Id: Ice015df81543c01094479929f0cb3075ca4f3813 Signed-off-by: Arne Schwabe <ar...@rf...> --- M src/openvpn/socket.c M src/openvpn/socket.h M src/openvpn/tun_afunix.c 3 files changed, 29 insertions(+), 6 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/54/754/1 diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 6c790a0..7b1e603 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -890,20 +890,23 @@ #endif } -static void -socket_set_buffers(socket_descriptor_t fd, const struct socket_buffer_size *sbs) +void +socket_set_buffers(socket_descriptor_t fd, const struct socket_buffer_size *sbs, + bool reduce_size) { if (sbs) { const int sndbuf_old = socket_get_sndbuf(fd); const int rcvbuf_old = socket_get_rcvbuf(fd); - if (sbs->sndbuf) + if (sbs->sndbuf + && (reduce_size || sndbuf_old < sbs->sndbuf)) { socket_set_sndbuf(fd, sbs->sndbuf); } - if (sbs->rcvbuf) + if (sbs->rcvbuf + && (reduce_size || rcvbuf_old < sbs->rcvbuf)) { socket_set_rcvbuf(fd, sbs->rcvbuf); } @@ -986,7 +989,7 @@ { ls->socket_buffer_sizes.sndbuf = sndbuf; ls->socket_buffer_sizes.rcvbuf = rcvbuf; - socket_set_buffers(ls->sd, &ls->socket_buffer_sizes); + socket_set_buffers(ls->sd, &ls->socket_buffer_sizes, true); } } @@ -1136,7 +1139,7 @@ sock->info.af = addr->ai_family; /* set socket buffers based on --sndbuf and --rcvbuf options */ - socket_set_buffers(sock->sd, &sock->socket_buffer_sizes); + socket_set_buffers(sock->sd, &sock->socket_buffer_sizes, true); /* set socket to --mark packets with given value */ socket_set_mark(sock->sd, sock->mark); diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index bbdabfb..2e583af 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -157,6 +157,18 @@ int sndbuf; }; +/** + * Sets the receive and send buffer sizes of a socket descriptor. + * + * @param fd The socket to modify + * @param sbs new sizes. + * @param reduce_size apply the new size even if smaller than current one + */ +void +socket_set_buffers(socket_descriptor_t fd, + const struct socket_buffer_size *sbs, + bool reduce_size); + /* * This is the main socket structure used by OpenVPN. The SOCKET_ * defines try to abstract away our implementation differences between diff --git a/src/openvpn/tun_afunix.c b/src/openvpn/tun_afunix.c index 27cdb01..d41c05a 100644 --- a/src/openvpn/tun_afunix.c +++ b/src/openvpn/tun_afunix.c @@ -35,6 +35,7 @@ #include "wfp_block.h" #include "argv.h" #include "options.h" +#include "socket.h" #if defined(AF_UNIX) && !defined(WIN32) /* Windows does implement some AF_UNIX functionality but key features @@ -80,6 +81,13 @@ return; } + + /* Ensure that the buffer sizes are decently sized. Otherwise macOS will + * just have 2048 */ + struct socket_buffer_size newsizes = {65536, 65536 }; + socket_set_buffers(fds[0], &newsizes, false); + socket_set_buffers(fds[1], &newsizes, false); + /* Use the first file descriptor for our side and avoid passing it * to the child */ tt->fd = fds[1]; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/754?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ice015df81543c01094479929f0cb3075ca4f3813 Gerrit-Change-Number: 754 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: mrbff (C. Review) <ge...@op...> - 2024-09-19 12:59:08
|
Attention is currently required from: cron2, flichtenheld, plaisthos. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/677?usp=email ) Change subject: route: copied 'gateway_needed' logic from add_route_ipv6 to add_route ...................................................................... Patch Set 10: (5 comments) File src/openvpn/route.c: http://gerrit.openvpn.net/c/openvpn/+/677/comment/6cceaa5b_47265977 : PS5, Line 1598: #ifndef _WIN32 > preprocessor directives should not be indented Done File src/openvpn/route.c: http://gerrit.openvpn.net/c/openvpn/+/677/comment/71e62482_873a6e01 : PS9, Line 1585: #if !defined(TARGET_LINUX) > why is that `#ifndef` moved`? It's not used by the new Linux code either, or am I misreading it (so […] "network" is now used in the new waning http://gerrit.openvpn.net/c/openvpn/+/677/comment/39ad8e78_7d4d9eb3 : PS9, Line 1599: > this blank line (and the one at the end of the if()) feel superfluous Done http://gerrit.openvpn.net/c/openvpn/+/677/comment/aa38e9c7_74e2196e : PS9, Line 1616: if (gateway_needed && r->gateway == 0) > if we add this here, we might consider getting rid of this warning in `init_route()` then... […] Acknowledged http://gerrit.openvpn.net/c/openvpn/+/677/comment/423f7057_a24ac63a : PS9, Line 1634: #if defined(TARGET_LINUX) > this needs to go - two `#if defined(TARGET_LINUX)` next to each other, with nothing else in between. […] no, you are right, my bad -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/677?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Id1ec0c6e4c391604ec5dbb8b7122f2e47ad186d1 Gerrit-Change-Number: 677 Gerrit-PatchSet: 10 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: cron2 <ge...@gr...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 19 Sep 2024 12:58:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Comment-In-Reply-To: flichtenheld <fr...@li...> Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-09-19 12:55:12
|
Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/721?usp=email to look at the new patch set (#8). Change subject: route: extended logic to omit gateway when unnecessary ...................................................................... route: extended logic to omit gateway when unnecessary Extracted and extended the logic behind 'gateway_needed' both in add_route() and add_route_ipv6(). Other than checking the dev-type, special routes and if the gateway is on-link, - set gateway_needed to true if the vpn instance is a multipoint server and DCO is enabled. - set gateway_needed to false if the gateway is in the vpn subnet. Additionally, extended support for these checks and conditions to DARWIN and BSD-based operating systems. These changes ensure that the gateway is only included when necessary, optimizing route configuration and potentially reducing redundant route entries. Change-Id: I87777e74b1fd34781e1d72c9f994eb84f39d800c Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/init.h M src/openvpn/route.c M src/openvpn/route.h M src/openvpn/tun.c M src/openvpn/tun.h 7 files changed, 416 insertions(+), 253 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/21/721/8 diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 374ba47..e46eaec 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -421,7 +421,7 @@ check_add_routes_action(struct context *c, const bool errors) { bool route_status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx, c->mode == CM_TOP); int flags = (errors ? ISC_ERRORS : 0); flags |= (!route_status ? ISC_ROUTE_ERRORS : 0); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index dd56961..446537a 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1134,7 +1134,7 @@ tuncfg(options->dev, options->dev_type, options->dev_node, options->persist_mode, options->username, options->groupname, &options->tuntap_options, - ctx); + ctx, options->mode == MODE_SERVER); if (options->persist_mode && options->lladdr) { set_lladdr(ctx, options->dev, options->lladdr, NULL); @@ -1689,13 +1689,14 @@ const struct tuntap *tt, const struct plugin_list *plugins, struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { bool ret = true; if (!options->route_noexec && ( route_list || route_ipv6_list ) ) { ret = add_routes(route_list, route_ipv6_list, tt, ROUTE_OPTION_FLAGS(options), - es, ctx); + es, ctx, is_multipoint); setenv_int(es, "redirect_gateway", route_did_redirect_default_gateway(route_list)); } #ifdef ENABLE_MANAGEMENT @@ -1898,7 +1899,7 @@ c->options.dev_node, &gc); do_ifconfig(c->c1.tuntap, guess, c->c2.frame.tun_mtu, c->c2.es, - &c->net_ctx); + &c->net_ctx, c->mode == CM_TOP); } /* possibly add routes */ @@ -1906,7 +1907,7 @@ { /* Ignore route_delay, would cause ROUTE_BEFORE_TUN to be ignored */ bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx, c->mode == CM_TOP); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } #ifdef TARGET_ANDROID @@ -1934,7 +1935,7 @@ && ifconfig_order() == IFCONFIG_AFTER_TUN_OPEN) { do_ifconfig(c->c1.tuntap, c->c1.tuntap->actual_name, - c->c2.frame.tun_mtu, c->c2.es, &c->net_ctx); + c->c2.frame.tun_mtu, c->c2.es, &c->net_ctx, c->mode == CM_TOP); } /* run the up script */ @@ -1960,7 +1961,7 @@ if ((route_order() == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx, c->mode == CM_TOP); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } @@ -2017,7 +2018,7 @@ { undo_ifconfig(c->c1.tuntap, &c->net_ctx); } - close_tun(c->c1.tuntap, &c->net_ctx); + close_tun(c->c1.tuntap, &c->net_ctx, c->mode == CM_TOP); c->c1.tuntap = NULL; } c->c1.tuntap_owned = false; @@ -2086,7 +2087,7 @@ delete_routes(c->c1.route_list, c->c1.route_ipv6_list, c->c1.tuntap, ROUTE_OPTION_FLAGS(&c->options), - c->c2.es, &c->net_ctx); + c->c2.es, &c->net_ctx, c->mode == CM_TOP); } /* actually close tun/tap device based on --down-pre flag */ diff --git a/src/openvpn/init.h b/src/openvpn/init.h index ea7eb30..82eec7e 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -74,7 +74,7 @@ bool do_route(const struct options *options, struct route_list *route_list, struct route_ipv6_list *route_ipv6_list, const struct tuntap *tt, const struct plugin_list *plugins, struct env_set *es, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, const bool is_multipoint); void close_instance(struct context *c); diff --git a/src/openvpn/route.c b/src/openvpn/route.c index ae7707c..3ae423a 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -77,7 +77,7 @@ static void delete_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, const bool is_multipoint); static void get_bypass_addresses(struct route_bypass *rb, const unsigned int flags); @@ -927,7 +927,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { struct route_ipv4 r; CLEAR(r); @@ -935,7 +936,7 @@ r.network = network; r.netmask = netmask; r.gateway = gateway; - return add_route(&r, tt, flags, rgi, es, ctx); + return add_route(&r, tt, flags, rgi, es, ctx, is_multipoint); } static void @@ -946,7 +947,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { struct route_ipv4 r; CLEAR(r); @@ -954,7 +956,7 @@ r.network = network; r.netmask = netmask; r.gateway = gateway; - delete_route(&r, tt, flags, rgi, es, ctx); + delete_route(&r, tt, flags, rgi, es, ctx, is_multipoint); } static bool @@ -964,7 +966,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { int ret = true; for (int i = 0; i < rb->n_bypass; ++i) @@ -972,7 +975,7 @@ if (rb->bypass[i]) { ret = add_route3(rb->bypass[i], IPV4_NETMASK_HOST, gateway, tt, - flags | ROUTE_REF_GW, rgi, es, ctx) && ret; + flags | ROUTE_REF_GW, rgi, es, ctx, is_multipoint) && ret; } } return ret; @@ -985,7 +988,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { int i; for (i = 0; i < rb->n_bypass; ++i) @@ -999,7 +1003,8 @@ flags | ROUTE_REF_GW, rgi, es, - ctx); + ctx, + is_multipoint); } } } @@ -1007,7 +1012,7 @@ static bool redirect_default_route_to_vpn(struct route_list *rl, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, const bool is_multipoint) { const char err[] = "NOTE: unable to redirect IPv4 default gateway --"; bool ret = true; @@ -1059,7 +1064,7 @@ { ret = add_route3(rl->spec.remote_host, IPV4_NETMASK_HOST, rl->rgi.gateway.addr, tt, flags | ROUTE_REF_GW, - &rl->rgi, es, ctx); + &rl->rgi, es, ctx, is_multipoint); rl->iflags |= RL_DID_LOCAL; } else @@ -1071,7 +1076,7 @@ /* route DHCP/DNS server traffic through original default gateway */ ret = add_bypass_routes(&rl->spec.bypass, rl->rgi.gateway.addr, tt, flags, - &rl->rgi, es, ctx) && ret; + &rl->rgi, es, ctx, is_multipoint) && ret; if (rl->flags & RG_REROUTE_GW) { @@ -1079,11 +1084,11 @@ { /* add new default route (1st component) */ ret = add_route3(0x00000000, 0x80000000, rl->spec.remote_endpoint, - tt, flags, &rl->rgi, es, ctx) && ret; + tt, flags, &rl->rgi, es, ctx, is_multipoint) && ret; /* add new default route (2nd component) */ ret = add_route3(0x80000000, 0x80000000, rl->spec.remote_endpoint, - tt, flags, &rl->rgi, es, ctx) && ret; + tt, flags, &rl->rgi, es, ctx, is_multipoint) && ret; } else { @@ -1092,12 +1097,12 @@ { /* delete default route */ del_route3(0, 0, rl->rgi.gateway.addr, tt, - flags | ROUTE_REF_GW, &rl->rgi, es, ctx); + flags | ROUTE_REF_GW, &rl->rgi, es, ctx, is_multipoint); } /* add new default route */ ret = add_route3(0, 0, rl->spec.remote_endpoint, tt, - flags, &rl->rgi, es, ctx) && ret; + flags, &rl->rgi, es, ctx, is_multipoint) && ret; } } @@ -1112,7 +1117,8 @@ undo_redirect_default_route_to_vpn(struct route_list *rl, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { if (rl && rl->iflags & RL_DID_REDIRECT_DEFAULT_GATEWAY) { @@ -1126,13 +1132,14 @@ flags | ROUTE_REF_GW, &rl->rgi, es, - ctx); + ctx, + is_multipoint); rl->iflags &= ~RL_DID_LOCAL; } /* delete special DHCP/DNS bypass route */ del_bypass_routes(&rl->spec.bypass, rl->rgi.gateway.addr, tt, flags, - &rl->rgi, es, ctx); + &rl->rgi, es, ctx, is_multipoint); if (rl->flags & RG_REROUTE_GW) { @@ -1146,7 +1153,8 @@ flags, &rl->rgi, es, - ctx); + ctx, + is_multipoint); /* delete default route (2nd component) */ del_route3(0x80000000, @@ -1156,7 +1164,8 @@ flags, &rl->rgi, es, - ctx); + ctx, + is_multipoint); } else { @@ -1168,12 +1177,13 @@ flags, &rl->rgi, es, - ctx); + ctx, + is_multipoint); /* restore original default route if there was any */ if (rl->rgi.flags & RGI_ADDR_DEFINED) { add_route3(0, 0, rl->rgi.gateway.addr, tt, - flags | ROUTE_REF_GW, &rl->rgi, es, ctx); + flags | ROUTE_REF_GW, &rl->rgi, es, ctx, is_multipoint); } } } @@ -1185,9 +1195,10 @@ bool add_routes(struct route_list *rl, struct route_ipv6_list *rl6, const struct tuntap *tt, unsigned int flags, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, + const bool is_multipoint) { - bool ret = redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + bool ret = redirect_default_route_to_vpn(rl, tt, flags, es, ctx, is_multipoint); if (rl && !(rl->iflags & RL_ROUTES_ADDED) ) { struct route_ipv4 *r; @@ -1218,9 +1229,9 @@ check_subnet_conflict(r->network, r->netmask, "route"); if (flags & ROUTE_DELETE_FIRST) { - delete_route(r, tt, flags, &rl->rgi, es, ctx); + delete_route(r, tt, flags, &rl->rgi, es, ctx, is_multipoint); } - ret = add_route(r, tt, flags, &rl->rgi, es, ctx) && ret; + ret = add_route(r, tt, flags, &rl->rgi, es, ctx, is_multipoint) && ret; } rl->iflags |= RL_ROUTES_ADDED; } @@ -1240,9 +1251,9 @@ { if (flags & ROUTE_DELETE_FIRST) { - delete_route_ipv6(r, tt, flags, es, ctx); + delete_route_ipv6(r, tt, flags, es, ctx, is_multipoint); } - ret = add_route_ipv6(r, tt, flags, es, ctx) && ret; + ret = add_route_ipv6(r, tt, flags, es, ctx, is_multipoint) && ret; } rl6->iflags |= RL_ROUTES_ADDED; } @@ -1253,19 +1264,20 @@ void delete_routes(struct route_list *rl, struct route_ipv6_list *rl6, const struct tuntap *tt, unsigned int flags, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, + const bool is_multipoint) { if (rl && rl->iflags & RL_ROUTES_ADDED) { struct route_ipv4 *r; for (r = rl->routes; r; r = r->next) { - delete_route(r, tt, flags, &rl->rgi, es, ctx); + delete_route(r, tt, flags, &rl->rgi, es, ctx, is_multipoint); } rl->iflags &= ~RL_ROUTES_ADDED; } - undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx); + undo_redirect_default_route_to_vpn(rl, tt, flags, es, ctx, is_multipoint); if (rl) { @@ -1277,7 +1289,7 @@ struct route_ipv6 *r6; for (r6 = rl6->routes_ipv6; r6; r6 = r6->next) { - delete_route_ipv6(r6, tt, flags, es, ctx); + delete_route_ipv6(r6, tt, flags, es, ctx, is_multipoint); } rl6->iflags &= ~RL_ROUTES_ADDED; } @@ -1561,13 +1573,47 @@ } #endif +static bool +is_gateway_needed_ipv4(const struct route_ipv4 *r4, + const struct route_gateway_info *rgi, + const struct tuntap *tt, + const bool is_multipoint) +{ + +#ifndef _WIN32 + + if (rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ + { + if (rgi->flags & RGI_ADDR_DEFINED && r4->gateway != 0) + { + return true; + } + } + +#endif + + if (tt->type == DEV_TYPE_TAP + && !( (r4->flags & RT_METRIC_DEFINED) && r4->metric == 0 ) ) + { + return true; + } + + if (is_multipoint && !tt->options.disable_dco) + { + return true; + } + + return false; +} + bool add_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, /* may be NULL */ const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { int status = 0; int is_local_route; @@ -1595,21 +1641,7 @@ goto done; } -#ifndef _WIN32 - if (rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ - { - if (rgi->flags & RGI_ADDR_DEFINED && r->gateway != 0) - { - gateway_needed = true; - } - } -#endif - - if (tt->type == DEV_TYPE_TAP - && !( (r->flags & RT_METRIC_DEFINED) && r->metric == 0 ) ) - { - gateway_needed = true; - } + gateway_needed = is_gateway_needed_ipv4(r, rgi, tt, is_multipoint); if (gateway_needed && r->gateway == 0) { @@ -1621,18 +1653,19 @@ goto done; } -#if defined(TARGET_LINUX) - const char *iface = tt->actual_name; +#if !defined(_WIN32) && !defined(TARGET_SOLARIS) \ + && !defined(TARGET_AIX) + const char *device = tt->actual_name; if (!gateway_needed && rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ { - iface = rgi->iface; + device = rgi->iface; } int metric = -1; if (is_on_link(is_local_route, flags, rgi)) { - iface = rgi->iface; + device = rgi->iface; } if (r->flags & RT_METRIC_DEFINED) @@ -1643,7 +1676,7 @@ status = RTA_SUCCESS; int ret = net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask), gateway_needed ? &r->gateway : NULL, - iface, 0, metric); + device, 0, metric); if (ret == -EEXIST) { msg(D_ROUTE, "NOTE: Linux route add command failed because route exists"); @@ -1660,7 +1693,7 @@ if (rgi) { - snprintf(out, sizeof(out), "%s %s %s dev %s", network, netmask, gateway, rgi->iface); + snprintf(out, sizeof(out), "%s %s %s dev %s", network, netmask, gateway, device); } else { @@ -1764,9 +1797,9 @@ "ERROR: Solaris route add command failed"); status = ret ? RTA_SUCCESS : RTA_ERROR; -#elif defined(TARGET_FREEBSD) +#elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) - argv_printf(&argv, "%s add", + argv_printf(&argv, "%s add ", ROUTE_PATH); #if 0 @@ -1776,45 +1809,28 @@ } #endif - argv_printf_cat(&argv, "-net %s %s %s", - network, - gateway, - netmask); - - /* FIXME -- add on-link support for FreeBSD */ - - argv_msg(D_ROUTE, &argv); - bool ret = openvpn_execve_check(&argv, es, 0, - "ERROR: FreeBSD route add command failed"); - status = ret ? RTA_SUCCESS : RTA_ERROR; - -#elif defined(TARGET_DRAGONFLY) - - argv_printf(&argv, "%s add", - ROUTE_PATH); - -#if 0 - if (r->flags & RT_METRIC_DEFINED) + if (gateway_needed) { - argv_printf_cat(&argv, "-rtt %d", r->metric); + argv_printf_cat(&argv, "-net %s %s %s", + network, + gateway, + netmask); } -#endif - - argv_printf_cat(&argv, "-net %s %s %s", - network, - gateway, - netmask); - - /* FIXME -- add on-link support for Dragonfly */ + else + { + argv_printf_cat(&argv, "-net %s -iface %s", + network, + device); + } argv_msg(D_ROUTE, &argv); bool ret = openvpn_execve_check(&argv, es, 0, - "ERROR: DragonFly route add command failed"); + "ERROR: BSD route add command failed"); status = ret ? RTA_SUCCESS : RTA_ERROR; #elif defined(TARGET_DARWIN) - argv_printf(&argv, "%s add", + argv_printf(&argv, "%s add ", ROUTE_PATH); #if 0 @@ -1835,10 +1851,19 @@ } else { - argv_printf_cat(&argv, "-net %s %s %s", - network, - gateway, - netmask); + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -interface %s", + network, + device); + } } argv_msg(D_ROUTE, &argv); @@ -1848,7 +1873,7 @@ #elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) - argv_printf(&argv, "%s add", + argv_printf(&argv, "%s add ", ROUTE_PATH); #if 0 @@ -1858,12 +1883,20 @@ } #endif - argv_printf_cat(&argv, "-net %s %s -netmask %s", - network, - gateway, - netmask); - - /* FIXME -- add on-link support for OpenBSD/NetBSD */ + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s -netmask %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -netmask %s -link -iface %s", + network, + netmask, + device); + } argv_msg(D_ROUTE, &argv); bool ret = openvpn_execve_check(&argv, es, 0, @@ -1928,10 +1961,98 @@ } } +static inline struct in6_addr +netbits_to_netmask_ipv6(const int netbits) +{ + struct in6_addr netmask = {{{0}}}; + + if (netbits > 0 && netbits <= 128) + { + const int full_bytes = netbits / 8; + int i; + for (i = 0; i < full_bytes; i++) + { + netmask.s6_addr[i] = 0xFF; + } + if (netbits % 8) + { + netmask.s6_addr[i] = (0xFF << (8 - (netbits % 8))); + } + } + + return netmask; +} + +static bool +is_gateway_in_vpn_subnet_ipv6(const struct in6_addr *gateway, + const struct in6_addr *vpn_subnet, + int netbits) +{ + struct in6_addr netmask = netbits_to_netmask_ipv6(netbits); + + for (int i = 0; i < 16; i++) + { + if ((gateway->s6_addr[i] & netmask.s6_addr[i]) != (vpn_subnet->s6_addr[i] & netmask.s6_addr[i])) + { + return false; + } + } + return true; +} + +static bool +is_gateway_needed_ipv6(const struct route_ipv6 *r6, + const struct tuntap *tt, + const char *network, + const bool is_multipoint) +{ + +#ifndef _WIN32 + if (r6->iface != NULL) /* VPN server special route */ + { + if (!IN6_IS_ADDR_UNSPECIFIED(&r6->gateway)) + { + return true; + } + } +#endif + + /* + * Filter out routes which are essentially no-ops + * (not currently done for IPv6) + */ + + /* On "tun" interface, we never set a gateway if the operating system + * can do "route to interface" - it does not add value, as the target + * dev already fully qualifies the route destination on point-to-point + * interfaces. OTOH, on "tap" interface, we must always set the + * gateway unless the route is to be an on-link network + */ + if (tt->type == DEV_TYPE_TAP + && !( (r6->flags & RT_METRIC_DEFINED) && r6->metric == 0 ) ) + { + return true; + } + + /* if is server and dco enabled, the gateway is needed*/ + if (is_multipoint && !tt->options.disable_dco) + { + return true; + } + + if (is_gateway_in_vpn_subnet_ipv6(&r6->gateway, &tt->local_ipv6, tt->netbits_ipv6)) + { + msg(D_ROUTE, "Ignoring gateway in VPN subnet for route %s/%d", network, r6->netbits); + return false; + } + + return false; +} + bool add_route_ipv6(struct route_ipv6 *r6, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, const bool is_multipoint) { int status = 0; bool gateway_needed = false; @@ -1944,22 +2065,12 @@ struct argv argv = argv_new(); struct gc_arena gc = gc_new(); -#ifndef _WIN32 - const char *device = tt->actual_name; - if (r6->iface != NULL) /* vpn server special route */ - { - device = r6->iface; - if (!IN6_IS_ADDR_UNSPECIFIED(&r6->gateway) ) - { - gateway_needed = true; - } - } -#endif - route_ipv6_clear_host_bits(r6); const char *network = print_in6_addr( r6->network, 0, &gc); const char *gateway = print_in6_addr( r6->gateway, 0, &gc); + gateway_needed = is_gateway_needed_ipv6(r6, tt, network, is_multipoint); + #if defined(TARGET_DARWIN) \ || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \ || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) @@ -1978,6 +2089,15 @@ } #endif +#if !defined(_WIN32) + + const char *device = tt->actual_name; + if (r6->iface != NULL) + { + device = r6->iface; + } +#endif + #ifndef _WIN32 msg(D_ROUTE, "add_route_ipv6(%s/%d -> %s metric %d) dev %s", network, r6->netbits, gateway, r6->metric, device ); @@ -1987,23 +2107,6 @@ r6->adapter_index ? r6->adapter_index : tt->adapter_index); #endif - /* - * Filter out routes which are essentially no-ops - * (not currently done for IPv6) - */ - - /* On "tun" interface, we never set a gateway if the operating system - * can do "route to interface" - it does not add value, as the target - * dev already fully qualifies the route destination on point-to-point - * interfaces. OTOH, on "tap" interface, we must always set the - * gateway unless the route is to be an on-link network - */ - if (tt->type == DEV_TYPE_TAP - && !( (r6->flags & RT_METRIC_DEFINED) && r6->metric == 0 ) ) - { - gateway_needed = true; - } - if (gateway_needed && IN6_IS_ADDR_UNSPECIFIED(&r6->gateway)) { msg(M_WARN, "ROUTE6 WARNING: " PACKAGE_NAME " needs a gateway " @@ -2016,6 +2119,7 @@ #if defined(TARGET_LINUX) int metric = -1; + if ((r6->flags & RT_METRIC_DEFINED) && (r6->metric > 0)) { metric = r6->metric; @@ -2085,7 +2189,7 @@ #elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) - argv_printf(&argv, "%s add -inet6 %s/%d", + argv_printf(&argv, "%s add -inet6 %s/%d ", ROUTE_PATH, network, r6->netbits); @@ -2106,7 +2210,7 @@ #elif defined(TARGET_DARWIN) - argv_printf(&argv, "%s add -inet6 %s -prefixlen %d", + argv_printf(&argv, "%s add -inet6 %s -prefixlen %d ", ROUTE_PATH, network, r6->netbits ); @@ -2124,26 +2228,24 @@ "ERROR: MacOS X route add -inet6 command failed"); status = ret ? RTA_SUCCESS : RTA_ERROR; -#elif defined(TARGET_OPENBSD) +#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) - argv_printf(&argv, "%s add -inet6 %s -prefixlen %d %s", + argv_printf(&argv, "%s add -inet6 %s -prefixlen %d ", ROUTE_PATH, - network, r6->netbits, gateway ); + network, r6->netbits); + + if (gateway_needed) + { + argv_printf_cat(&argv, "%s", gateway); + } + else + { + argv_printf_cat(&argv, "-link -iface %s", device); + } argv_msg(D_ROUTE, &argv); bool ret = openvpn_execve_check(&argv, es, 0, - "ERROR: OpenBSD route add -inet6 command failed"); - status = ret ? RTA_SUCCESS : RTA_ERROR; - -#elif defined(TARGET_NETBSD) - - argv_printf(&argv, "%s add -inet6 %s/%d %s", - ROUTE_PATH, - network, r6->netbits, gateway ); - - argv_msg(D_ROUTE, &argv); - bool ret = openvpn_execve_check(&argv, es, 0, - "ERROR: NetBSD route add -inet6 command failed"); + "ERROR: OpenBSD/NetBSD route add -inet6 command failed"); status = ret ? RTA_SUCCESS : RTA_ERROR; #elif defined(TARGET_AIX) @@ -2183,7 +2285,8 @@ unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, + const bool is_multipoint) { #if !defined(TARGET_LINUX) const char *network; @@ -2198,6 +2301,21 @@ #endif int is_local_route; +#if defined(TARGET_DARWIN) || defined(TARGET_LINUX) \ + || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \ + || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) + + bool gateway_needed = is_gateway_needed_ipv4(r, rgi, tt, is_multipoint); + +#if !defined(TARGET_OPENBSD) && !defined(TARGET_NETBSD) + const char *device = tt->actual_name; + if (!gateway_needed && rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ + { + device = rgi->iface; + } +#endif +#endif + if ((r->flags & (RT_DEFINED|RT_ADDED)) != (RT_DEFINED|RT_ADDED)) { return; @@ -2224,13 +2342,20 @@ #if defined(TARGET_LINUX) metric = -1; + + if (is_on_link(is_local_route, flags, rgi)) + { + device = rgi->iface; + } + if (r->flags & RT_METRIC_DEFINED) { metric = r->metric; } if (net_route_v4_del(ctx, &r->network, netmask_to_netbits2(r->netmask), - &r->gateway, NULL, 0, metric) < 0) + gateway_needed ? &r->gateway : NULL, + device, 0, metric) < 0) { msg(M_WARN, "ERROR: Linux route delete command failed"); } @@ -2289,27 +2414,26 @@ argv_msg(D_ROUTE, &argv); openvpn_execve_check(&argv, es, 0, "ERROR: Solaris route delete command failed"); -#elif defined(TARGET_FREEBSD) +#elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) - argv_printf(&argv, "%s delete -net %s %s %s", - ROUTE_PATH, - network, - gateway, - netmask); + argv_printf(&argv, "%s delete", ROUTE_PATH); + + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -iface %s", + network, + device); + } argv_msg(D_ROUTE, &argv); - openvpn_execve_check(&argv, es, 0, "ERROR: FreeBSD route delete command failed"); - -#elif defined(TARGET_DRAGONFLY) - - argv_printf(&argv, "%s delete -net %s %s %s", - ROUTE_PATH, - network, - gateway, - netmask); - - argv_msg(D_ROUTE, &argv); - openvpn_execve_check(&argv, es, 0, "ERROR: DragonFly route delete command failed"); + openvpn_execve_check(&argv, es, 0, "ERROR: BSD route delete command failed"); #elif defined(TARGET_DARWIN) @@ -2323,11 +2447,22 @@ } else { - argv_printf(&argv, "%s delete -net %s %s %s", - ROUTE_PATH, - network, - gateway, - netmask); + argv_printf(&argv, "%s delete ", + ROUTE_PATH); + + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -interface %s", + network, + device); + } } argv_msg(D_ROUTE, &argv); @@ -2335,11 +2470,22 @@ #elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) - argv_printf(&argv, "%s delete -net %s %s -netmask %s", - ROUTE_PATH, - network, - gateway, - netmask); + argv_printf(&argv, "%s delete ", + ROUTE_PATH); + + if (gateway_needed) + { + argv_printf_cat(&argv, "-net %s %s -netmask %s", + network, + gateway, + netmask); + } + else + { + argv_printf_cat(&argv, "-net %s -netmask %s", + network, + netmask); + } argv_msg(D_ROUTE, &argv); openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD/NetBSD route delete command failed"); @@ -2374,7 +2520,7 @@ void delete_route_ipv6(const struct route_ipv6 *r6, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx) + openvpn_net_ctx_t *ctx, const bool is_multipoint) { const char *network; @@ -2387,23 +2533,15 @@ #if !defined(TARGET_LINUX) const char *gateway; #endif -#if !defined(TARGET_SOLARIS) - bool gateway_needed = false; +#if defined(TARGET_DARWIN) || defined(TARGET_LINUX) \ + || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) + const char *device = tt->actual_name; if (r6->iface != NULL) /* vpn server special route */ { device = r6->iface; - gateway_needed = true; } - /* if we used a gateway on "add route", we also need to specify it on - * delete, otherwise some OSes will refuse to delete the route - */ - if (tt->type == DEV_TYPE_TAP - && !( (r6->flags & RT_METRIC_DEFINED) && r6->metric == 0 ) ) - { - gateway_needed = true; - } #endif #endif @@ -2415,6 +2553,16 @@ gateway = print_in6_addr( r6->gateway, 0, &gc); #endif +#if defined(TARGET_DARWIN) || defined(TARGET_LINUX) \ + || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \ + || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) + /* if we used a gateway on "add route", we also need to specify it on + * delete, otherwise some OSes will refuse to delete the route + */ + bool gateway_needed = false; + gateway_needed = is_gateway_needed_ipv6(r6, tt, network, is_multipoint); +#endif + #if defined(TARGET_DARWIN) \ || defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) \ || defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) @@ -2509,23 +2657,19 @@ argv_msg(D_ROUTE, &argv); openvpn_execve_check(&argv, es, 0, "ERROR: MacOS X route delete -inet6 command failed"); -#elif defined(TARGET_OPENBSD) +#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) - argv_printf(&argv, "%s delete -inet6 %s -prefixlen %d %s", + argv_printf(&argv, "%s delete -inet6 %s -prefixlen %d", ROUTE_PATH, - network, r6->netbits, gateway ); + network, r6->netbits); + + if (gateway_needed) + { + argv_printf_cat(&argv, "%s", gateway); + } argv_msg(D_ROUTE, &argv); - openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD route delete -inet6 command failed"); - -#elif defined(TARGET_NETBSD) - - argv_printf(&argv, "%s delete -inet6 %s/%d %s", - ROUTE_PATH, - network, r6->netbits, gateway ); - - argv_msg(D_ROUTE, &argv); - openvpn_execve_check(&argv, es, 0, "ERROR: NetBSD route delete -inet6 command failed"); + openvpn_execve_check(&argv, es, 0, "ERROR: OpenBSD/NetBSD route delete -inet6 command failed"); #elif defined(TARGET_AIX) diff --git a/src/openvpn/route.h b/src/openvpn/route.h index 421e7d2..4c3f317 100644 --- a/src/openvpn/route.h +++ b/src/openvpn/route.h @@ -271,13 +271,13 @@ void route_ipv6_clear_host_bits( struct route_ipv6 *r6 ); -bool add_route_ipv6(struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx); +bool add_route_ipv6(struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint); -void delete_route_ipv6(const struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx); +void delete_route_ipv6(const struct route_ipv6 *r, const struct tuntap *tt, unsigned int flags, const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint); bool add_route(struct route_ipv4 *r, const struct tuntap *tt, unsigned int flags, const struct route_gateway_info *rgi, const struct env_set *es, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, const bool is_multipoint); void add_route_to_option_list(struct route_option_list *l, const char *network, @@ -312,14 +312,16 @@ bool add_routes(struct route_list *rl, struct route_ipv6_list *rl6, const struct tuntap *tt, unsigned int flags, - const struct env_set *es, openvpn_net_ctx_t *ctx); + const struct env_set *es, openvpn_net_ctx_t *ctx, + const bool is_multipoint); void delete_routes(struct route_list *rl, struct route_ipv6_list *rl6, const struct tuntap *tt, unsigned int flags, const struct env_set *es, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, + const bool is_multipoint); void setenv_routes(struct env_set *es, const struct route_list *rl); diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 82c5c00..c3c321c 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -1017,7 +1017,7 @@ */ static void add_route_connected_v6_net(struct tuntap *tt, - const struct env_set *es) + const struct env_set *es, const bool is_multipoint) { struct route_ipv6 r6; @@ -1027,11 +1027,11 @@ r6.gateway = tt->local_ipv6; r6.metric = 0; /* connected route */ r6.flags = RT_DEFINED | RT_METRIC_DEFINED; - add_route_ipv6(&r6, tt, 0, es, NULL); + add_route_ipv6(&r6, tt, 0, es, NULL, is_multipoint); } void -delete_route_connected_v6_net(const struct tuntap *tt) +delete_route_connected_v6_net(const struct tuntap *tt, const bool is_multipoint) { struct route_ipv6 r6; @@ -1042,7 +1042,7 @@ r6.metric = 0; /* connected route */ r6.flags = RT_DEFINED | RT_ADDED | RT_METRIC_DEFINED; route_ipv6_clear_host_bits(&r6); - delete_route_ipv6(&r6, tt, 0, NULL, NULL); + delete_route_ipv6(&r6, tt, 0, NULL, NULL, is_multipoint); } #endif /* if defined(_WIN32) || defined(TARGET_DARWIN) || defined(TARGET_NETBSD) || defined(TARGET_OPENBSD) */ @@ -1084,7 +1084,8 @@ */ static void do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, + const bool is_multipoint) { #if !defined(TARGET_LINUX) struct argv argv = argv_new(); @@ -1223,7 +1224,7 @@ do_address_service(true, AF_INET6, tt); if (tt->type == DEV_TYPE_TUN) { - add_route_connected_v6_net(tt, es); + add_route_connected_v6_net(tt, es, is_multipoint); } do_dns_service(true, AF_INET6, tt); do_set_mtu_service(tt, AF_INET6, tun_mtu); @@ -1251,7 +1252,7 @@ netsh_command(&argv, 4, M_FATAL); if (tt->type == DEV_TYPE_TUN) { - add_route_connected_v6_net(tt, es); + add_route_connected_v6_net(tt, es, is_multipoint); } /* set ipv6 dns servers if any are specified */ netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, tt->adapter_index); @@ -1283,7 +1284,7 @@ */ static void do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint) { #if !defined(_WIN32) && !defined(TARGET_ANDROID) /* @@ -1388,7 +1389,7 @@ r.netmask = tt->remote_netmask; r.gateway = tt->local; r.metric = 0; - add_route(&r, tt, 0, NULL, es, NULL); + add_route(&r, tt, 0, NULL, es, NULL, is_multipoint); } #elif defined(TARGET_OPENBSD) @@ -1435,7 +1436,7 @@ r.network = tt->local & tt->remote_netmask; r.netmask = tt->remote_netmask; r.gateway = remote_end; - add_route(&r, tt, 0, NULL, es, NULL); + add_route(&r, tt, 0, NULL, es, NULL, is_multipoint); } #elif defined(TARGET_NETBSD) @@ -1477,7 +1478,7 @@ r.network = tt->local & tt->remote_netmask; r.netmask = tt->remote_netmask; r.gateway = remote_end; - add_route(&r, tt, 0, NULL, es, NULL); + add_route(&r, tt, 0, NULL, es, NULL, is_multipoint); } #elif defined(TARGET_DARWIN) @@ -1524,7 +1525,7 @@ r.network = tt->local & tt->remote_netmask; r.netmask = tt->remote_netmask; r.gateway = tt->local; - add_route(&r, tt, 0, NULL, es, NULL); + add_route(&r, tt, 0, NULL, es, NULL, is_multipoint); } #elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY) @@ -1618,7 +1619,7 @@ /* execute the ifconfig command through the shell */ void do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, - const struct env_set *es, openvpn_net_ctx_t *ctx) + const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint) { msg(D_LOW, "do_ifconfig, ipv4=%d, ipv6=%d", tt->did_ifconfig_setup, tt->did_ifconfig_ipv6_setup); @@ -1638,12 +1639,12 @@ if (tt->did_ifconfig_setup) { - do_ifconfig_ipv4(tt, ifname, tun_mtu, es, ctx); + do_ifconfig_ipv4(tt, ifname, tun_mtu, es, ctx, is_multipoint); } if (tt->did_ifconfig_ipv6_setup) { - do_ifconfig_ipv6(tt, ifname, tun_mtu, es, ctx); + do_ifconfig_ipv6(tt, ifname, tun_mtu, es, ctx, is_multipoint); } /* release resources potentially allocated during interface setup */ @@ -2108,12 +2109,13 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); close_tun_generic(tt); free(tt); + (void)is_multipoint; } int @@ -2277,7 +2279,8 @@ void tuncfg(const char *dev, const char *dev_type, const char *dev_node, int persist_mode, const char *username, const char *groupname, - const struct tuntap_options *options, openvpn_net_ctx_t *ctx) + const struct tuntap_options *options, openvpn_net_ctx_t *ctx, + const bool is_multipoint) { struct tuntap *tt; @@ -2317,14 +2320,14 @@ msg(M_ERR, "Cannot ioctl TUNSETGROUP(%s) %s", groupname, dev); } } - close_tun(tt, ctx); + close_tun(tt, ctx, is_multipoint); msg(M_INFO, "Persist state set to: %s", (persist_mode ? "ON" : "OFF")); } #endif /* ENABLE_FEATURE_TUN_PERSIST */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -2336,6 +2339,7 @@ #endif close_tun_generic(tt); free(tt); + (void)is_multipoint; } int @@ -2647,7 +2651,7 @@ * Close TUN device. */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -2657,6 +2661,7 @@ clear_tuntap(tt); free(tt); + (void)is_multipoint; } static void @@ -2680,7 +2685,7 @@ argv_msg(M_INFO, &argv); openvpn_execve_check(&argv, es, 0, "Solaris ifconfig unplumb failed"); - close_tun(tt, NULL); + close_tun(tt, NULL, false); msg(M_FATAL, "Solaris ifconfig failed"); argv_free(&argv); } @@ -2744,10 +2749,12 @@ */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); + (void)is_multipoint; + /* only *TAP* devices need destroying, tun devices auto-self-destruct */ if (tt->type == DEV_TYPE_TUN || tt->persistent_if) @@ -2858,10 +2865,12 @@ * need to be explicitly destroyed */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); + (void)is_multipoint; + /* only tun devices need destroying, tap devices auto-self-destruct */ if (tt->type != DEV_TYPE_TUN || tt->persistent_if) @@ -3014,10 +3023,12 @@ * we need to call "ifconfig ... destroy" for cleanup */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); + (void)is_multipoint; + if (tt->persistent_if) /* keep pre-existing if around */ { close_tun_generic(tt); @@ -3131,12 +3142,13 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); close_tun_generic(tt); free(tt); + (void)is_multipoint; } int @@ -3398,7 +3410,7 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -3420,6 +3432,7 @@ free(tt); argv_free(&argv); gc_free(&gc); + (void)is_multipoint; } int @@ -3547,7 +3560,7 @@ /* tap devices need to be manually destroyed on AIX */ void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -3575,6 +3588,7 @@ free(tt); env_set_destroy(es); argv_free(&argv); + (void)is_multipoint; } int @@ -6862,7 +6876,7 @@ } static void -netsh_delete_address_dns(const struct tuntap *tt, bool ipv6, struct gc_arena *gc) +netsh_delete_address_dns(const struct tuntap *tt, bool ipv6, struct gc_arena *gc, const bool is_multipoint) { const char *ifconfig_ip_local; struct argv argv = argv_new(); @@ -6892,7 +6906,7 @@ if (ipv6 && tt->type == DEV_TYPE_TUN) { - delete_route_connected_v6_net(tt); + delete_route_connected_v6_net(tt, is_multipoint); } /* "store=active" is needed in Windows 8(.1) to delete the @@ -6962,7 +6976,7 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); @@ -6982,7 +6996,7 @@ do_dns_domain_service(false, tt); } do_dns_service(false, AF_INET6, tt); - delete_route_connected_v6_net(tt); + delete_route_connected_v6_net(tt, is_multipoint); do_address_service(false, AF_INET6, tt); } else @@ -6992,7 +7006,7 @@ do_dns_domain_wmic(false, tt); } - netsh_delete_address_dns(tt, true, &gc); + netsh_delete_address_dns(tt, true, &gc, is_multipoint); } } @@ -7019,7 +7033,7 @@ if (tt->options.ip_win32_type == IPW32_SET_NETSH) { - netsh_delete_address_dns(tt, false, &gc); + netsh_delete_address_dns(tt, false, &gc, is_multipoint); } } } @@ -7140,12 +7154,13 @@ } void -close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) +close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint) { ASSERT(tt); close_tun_generic(tt); free(tt); + (void)is_multipoint; } int diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index 33b9552..9716d19 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -266,7 +266,7 @@ void open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tuntap *tt, openvpn_net_ctx_t *ctx); -void close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx); +void close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx, const bool is_multipoint); void tun_open_device(struct tuntap *tt, const char *dev_node, const char **device_guid, struct gc_arena *gc); @@ -280,7 +280,7 @@ void tuncfg(const char *dev, const char *dev_type, const char *dev_node, int persist_mode, const char *username, const char *groupname, const struct tuntap_options *options, - openvpn_net_ctx_t *ctx); + openvpn_net_ctx_t *ctx, const bool is_multipoint); const char *guess_tuntap_dev(const char *dev, const char *dev_type, @@ -319,7 +319,7 @@ * @param ctx the networking API opaque context */ void do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, - const struct env_set *es, openvpn_net_ctx_t *ctx); + const struct env_set *es, openvpn_net_ctx_t *ctx, const bool is_multipoint); /** * undo_ifconfig - undo configuration of the tunnel interface @@ -754,6 +754,7 @@ } const char *tun_stat(const struct tuntap *tt, unsigned int rwflags, struct gc_arena *gc); + bool tun_name_is_fixed(const char *dev); static inline bool -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/721?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I87777e74b1fd34781e1d72c9f994eb84f39d800c Gerrit-Change-Number: 721 Gerrit-PatchSet: 8 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2024-09-19 12:55:10
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/677?usp=email to look at the new patch set (#10). Change subject: route: copied 'gateway_needed' logic from add_route_ipv6 to add_route ...................................................................... route: copied 'gateway_needed' logic from add_route_ipv6 to add_route Under certain circumstances it may not be necessary to pass the gateway when adding a new route via net_route_v4_add() API function. add_route_ipv6() already accounts for some of these cases and therefore this patch copies the same logic to add_route(). Change-Id: Id1ec0c6e4c391604ec5dbb8b7122f2e47ad186d1 Signed-off-by: Marco Baffo <ma...@ma...> --- M src/openvpn/route.c 1 file changed, 36 insertions(+), 3 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/77/677/10 diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 91f2032..ae7707c 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1571,6 +1571,7 @@ { int status = 0; int is_local_route; + bool gateway_needed = false; if (!(r->flags & RT_DEFINED)) { @@ -1580,8 +1581,8 @@ struct argv argv = argv_new(); struct gc_arena gc = gc_new(); -#if !defined(TARGET_LINUX) const char *network = print_in_addr_t(r->network, 0, &gc); +#if !defined(TARGET_LINUX) #if !defined(TARGET_AIX) const char *netmask = print_in_addr_t(r->netmask, 0, &gc); #endif @@ -1594,8 +1595,39 @@ goto done; } +#ifndef _WIN32 + if (rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ + { + if (rgi->flags & RGI_ADDR_DEFINED && r->gateway != 0) + { + gateway_needed = true; + } + } +#endif + + if (tt->type == DEV_TYPE_TAP + && !( (r->flags & RT_METRIC_DEFINED) && r->metric == 0 ) ) + { + gateway_needed = true; + } + + if (gateway_needed && r->gateway == 0) + { + msg(M_WARN, "ROUTE WARNING: " PACKAGE_NAME " needs a gateway " + "parameter for a --route option and no default was set via " + "--route-gateway or --ifconfig option. Not installing " + "IPv4 route to %s/%d.", network, netmask_to_netbits2(r->netmask)); + status = 0; + goto done; + } + #if defined(TARGET_LINUX) - const char *iface = NULL; + const char *iface = tt->actual_name; + if (!gateway_needed && rgi && (rgi->flags & RGI_IFACE_DEFINED) && rgi->iface[0] != 0) /* vpn server special route */ + { + iface = rgi->iface; + } + int metric = -1; if (is_on_link(is_local_route, flags, rgi)) @@ -1610,7 +1642,8 @@ status = RTA_SUCCESS; int ret = net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask), - &r->gateway, iface, 0, metric); + gateway_needed ? &r->gateway : NULL, + iface, 0, metric); if (ret == -EEXIST) { msg(D_ROUTE, "NOTE: Linux route add command failed because route exists"); -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/677?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Id1ec0c6e4c391604ec5dbb8b7122f2e47ad186d1 Gerrit-Change-Number: 677 Gerrit-PatchSet: 10 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: cron2 <ge...@gr...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: cron2 (C. Review) <ge...@op...> - 2024-09-19 11:10:56
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/677?usp=email ) Change subject: route: copied 'gateway_needed' logic from add_route_ipv6 to add_route ...................................................................... Patch Set 9: (5 comments) Patchset: PS9: Getting there... File src/openvpn/route.c: http://gerrit.openvpn.net/c/openvpn/+/677/comment/cc58944a_810b6a73 : PS9, Line 1585: #if !defined(TARGET_LINUX) why is that `#ifndef` moved`? It's not used by the new Linux code either, or am I misreading it (so, no -Werror safe)? http://gerrit.openvpn.net/c/openvpn/+/677/comment/fc88af46_52bd13d6 : PS9, Line 1599: this blank line (and the one at the end of the if()) feel superfluous http://gerrit.openvpn.net/c/openvpn/+/677/comment/6c4000d3_493952ff : PS9, Line 1616: if (gateway_needed && r->gateway == 0) if we add this here, we might consider getting rid of this warning in `init_route()` then... but this is a bit complex, as for most platforms `gateway_needed = true` is still valid, due to limits in route table handling http://gerrit.openvpn.net/c/openvpn/+/677/comment/1ea919ca_7a56279c : PS9, Line 1634: #if defined(TARGET_LINUX) this needs to go - two `#if defined(TARGET_LINUX)` next to each other, with nothing else in between. Or am I misreading the diff? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/677?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Id1ec0c6e4c391604ec5dbb8b7122f2e47ad186d1 Gerrit-Change-Number: 677 Gerrit-PatchSet: 9 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: cron2 <ge...@gr...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Thu, 19 Sep 2024 11:10:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment |
From: plaisthos (C. Review) <ge...@op...> - 2024-09-19 10:52:56
|
Attention is currently required from: cron2, flichtenheld. plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/753?usp=email ) Change subject: Refactor methods to use the actual state of tun rather the options struct ...................................................................... Patch Set 1: -Code-Review This change is ready for review. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/753?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ib8ba2b63ea9fd1944ff07f69c545a880e464680c Gerrit-Change-Number: 753 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 19 Sep 2024 10:52:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: cron2 (C. Review) <ge...@op...> - 2024-09-19 10:40:16
|
Attention is currently required from: its_Giaan, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/524?usp=email ) Change subject: Route: add support for user defined routing table ...................................................................... Patch Set 6: Code-Review-1 (1 comment) Patchset: PS6: Still not there - I think the code in `route.c` is much more complex than it needs to be. So with the new patch to `options.c`, we pass the to-be-used routing table ID to `add_route_to_option_list()`, so every route option always(!) has a routing-table to be used (or 0). This should be sufficient. But then, we have a default table_id added in `init_route()`, a number of extra flags (`RT_TABLE_DEFINED` for `r->flags`) etc., which is all not used in the end because even without the flag, `table_id` would become 0 in `add_route()` (by means of `r->table_id` being 0)...? So.. can we do this with half the code? Getting rid of all new DEFINEs, all new arguments and flags to `route_list` things, just storing the table id in `struct route_ipv4` and `struct route_ipv6`)= -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/524?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I3e4ebef484d2a04a383a65ede5617ee98bf218a7 Gerrit-Change-Number: 524 Gerrit-PatchSet: 6 Gerrit-Owner: its_Giaan <gia...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: its_Giaan <gia...@ma...> Gerrit-Comment-Date: Thu, 19 Sep 2024 10:40:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: Gert D. <ge...@gr...> - 2024-09-19 10:02:21
|
Thanks for the review. Indent fixed, that was a <tab> that sneaked in. Patch has been applied to the master branch. commit b322690394b75a9d4987d4b27107ccb01bbcd90e Author: Gert Doering Date: Wed Sep 18 18:29:17 2024 +0200 make t_server_null 'server alive?' check more robust Acked-by: Frank Lichtenheld <fr...@li...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29314.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering |
From: Frank L. <fr...@li...> - 2024-09-19 09:52:06
|
On Wed, Sep 18, 2024 at 06:29:17PM +0200, Gert Doering wrote: > - use "$RUN_SUDO kill -0 $pid" to test if a given process is running, not > "ps -p $pid" - the latter will not work if security.bsd.see_other_uids=0 > is set > > - produce proper error messages if pid files can not be found or are > empty at server shutdown time > --- > tests/t_server_null_client.sh | 5 ++++- > tests/t_server_null_server.sh | 5 +++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/tests/t_server_null_client.sh b/tests/t_server_null_client.sh > index e7dd3324..c1a25dfc 100755 > --- a/tests/t_server_null_client.sh > +++ b/tests/t_server_null_client.sh > @@ -87,7 +87,10 @@ while [ $count -lt $server_max_wait ]; do > # the active server count as the processes won't be running. > for i in `set|grep 'SERVER_NAME_'|cut -d "=" -f 2|tr -d "[\']"`; do > server_pid=$(cat $i.pid 2> /dev/null) > - if ps -p $server_pid > /dev/null 2>&1; then > + if [ -z "$server_pid" ] ; then > + continue > + fi > + if $RUN_SUDO kill -0 $server_pid > /dev/null 2>&1; then > servers_up=$(( $servers_up + 1 )) > fi > done > diff --git a/tests/t_server_null_server.sh b/tests/t_server_null_server.sh > index e5906eec..32f0362d 100755 > --- a/tests/t_server_null_server.sh > +++ b/tests/t_server_null_server.sh > @@ -82,6 +82,11 @@ for PID_FILE in $server_pid_files > do > SERVER_PID=$(cat "${PID_FILE}") > > + if [ -z "$SERVER_PID" ] ; then > + echo "WARNING: could not kill server ${PID_FILE}!" > + continue Indentation looks slightly wrong. Otherwise looks good to me, so Acked-by: Frank Lichtenheld <fr...@li...> Regards, -- Frank Lichtenheld |
From: cron2 (C. Review) <ge...@op...> - 2024-09-19 07:03:47
|
Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/748?usp=email ) Change subject: Change dev null to be a driver type instead of a special mode of tun/tap ...................................................................... Patch Set 9: Code-Review-1 (2 comments) Patchset: PS9: close :-) File src/openvpn/dco.c: http://gerrit.openvpn.net/c/openvpn/+/748/comment/ba0f33a0_74f63ab7 : PS9, Line 312: "offload"); the wrapping has lost the space ("data channeloffload") and I wouldn't actuall break the string here... this is more ugly than a 87 character long line (and I think our styleguide permits this) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/748?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I5987ebb7c38ab176eed7efc004ea54f606a77a12 Gerrit-Change-Number: 748 Gerrit-PatchSet: 9 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 19 Sep 2024 07:03:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: cron2 (C. Review) <ge...@op...> - 2024-09-19 06:15:56
|
cron2 has uploaded a new patch set (#2) to the change originally created by flichtenheld. ( http://gerrit.openvpn.net/c/openvpn/+/740?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: socket: Change return types of link_socket_write* to ssize_t ...................................................................... socket: Change return types of link_socket_write* to ssize_t This is the actual return value of send/sendto/sendmsg. We will leave it to the single caller of link_socket_write() to decide how to map that to the int buffer world. For now just cast it explicitly. Change-Id: I7852c06d331326c1dbab7b642254c0c00d4cebb8 Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29323.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/forward.c M src/openvpn/socket.c M src/openvpn/socket.h 3 files changed, 13 insertions(+), 13 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/40/740/2 diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 374ba47..8c02407 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1792,7 +1792,7 @@ socks_preprocess_outgoing_link(c, &to_addr, &size_delta); /* Send packet */ - size = link_socket_write(c->c2.link_socket, &c->c2.to_link, to_addr); + size = (int)link_socket_write(c->c2.link_socket, &c->c2.to_link, to_addr); /* Undo effect of prepend */ link_socket_write_post_size_adjust(&size, size_delta, &c->c2.to_link); diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 17c5e76..6c790a0 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -3399,7 +3399,7 @@ * Socket Write Routines */ -int +ssize_t link_socket_write_tcp(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -3418,7 +3418,7 @@ #if ENABLE_IP_PKTINFO -size_t +ssize_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index 47083ad..bbdabfb 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -1099,9 +1099,9 @@ * Socket Write routines */ -int link_socket_write_tcp(struct link_socket *sock, - struct buffer *buf, - struct link_socket_actual *to); +ssize_t link_socket_write_tcp(struct link_socket *sock, + struct buffer *buf, + struct link_socket_actual *to); #ifdef _WIN32 @@ -1135,12 +1135,12 @@ #else /* ifdef _WIN32 */ -size_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, - struct buffer *buf, - struct link_socket_actual *to); +ssize_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, + struct buffer *buf, + struct link_socket_actual *to); -static inline size_t +static inline ssize_t link_socket_write_udp_posix(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1158,7 +1158,7 @@ (socklen_t) af_addr_size(to->dest.addr.sa.sa_family)); } -static inline size_t +static inline ssize_t link_socket_write_tcp_posix(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1168,7 +1168,7 @@ #endif /* ifdef _WIN32 */ -static inline size_t +static inline ssize_t link_socket_write_udp(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1181,7 +1181,7 @@ } /* write a TCP or UDP packet to link */ -static inline int +static inline ssize_t link_socket_write(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/740?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I7852c06d331326c1dbab7b642254c0c00d4cebb8 Gerrit-Change-Number: 740 Gerrit-PatchSet: 2 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset |
From: cron2 (C. Review) <ge...@op...> - 2024-09-19 06:15:51
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/740?usp=email ) Change subject: socket: Change return types of link_socket_write* to ssize_t ...................................................................... socket: Change return types of link_socket_write* to ssize_t This is the actual return value of send/sendto/sendmsg. We will leave it to the single caller of link_socket_write() to decide how to map that to the int buffer world. For now just cast it explicitly. Change-Id: I7852c06d331326c1dbab7b642254c0c00d4cebb8 Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29323.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/forward.c M src/openvpn/socket.c M src/openvpn/socket.h 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 374ba47..8c02407 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1792,7 +1792,7 @@ socks_preprocess_outgoing_link(c, &to_addr, &size_delta); /* Send packet */ - size = link_socket_write(c->c2.link_socket, &c->c2.to_link, to_addr); + size = (int)link_socket_write(c->c2.link_socket, &c->c2.to_link, to_addr); /* Undo effect of prepend */ link_socket_write_post_size_adjust(&size, size_delta, &c->c2.to_link); diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 17c5e76..6c790a0 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -3399,7 +3399,7 @@ * Socket Write Routines */ -int +ssize_t link_socket_write_tcp(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -3418,7 +3418,7 @@ #if ENABLE_IP_PKTINFO -size_t +ssize_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index 47083ad..bbdabfb 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -1099,9 +1099,9 @@ * Socket Write routines */ -int link_socket_write_tcp(struct link_socket *sock, - struct buffer *buf, - struct link_socket_actual *to); +ssize_t link_socket_write_tcp(struct link_socket *sock, + struct buffer *buf, + struct link_socket_actual *to); #ifdef _WIN32 @@ -1135,12 +1135,12 @@ #else /* ifdef _WIN32 */ -size_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, - struct buffer *buf, - struct link_socket_actual *to); +ssize_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, + struct buffer *buf, + struct link_socket_actual *to); -static inline size_t +static inline ssize_t link_socket_write_udp_posix(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1158,7 +1158,7 @@ (socklen_t) af_addr_size(to->dest.addr.sa.sa_family)); } -static inline size_t +static inline ssize_t link_socket_write_tcp_posix(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1168,7 +1168,7 @@ #endif /* ifdef _WIN32 */ -static inline size_t +static inline ssize_t link_socket_write_udp(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1181,7 +1181,7 @@ } /* write a TCP or UDP packet to link */ -static inline int +static inline ssize_t link_socket_write(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/740?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I7852c06d331326c1dbab7b642254c0c00d4cebb8 Gerrit-Change-Number: 740 Gerrit-PatchSet: 2 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged |
From: Gert D. <ge...@gr...> - 2024-09-19 06:15:31
|
This is fallout from the -Wconversion patch set (commit 53449cb61f) where it turned out that the whole link_socket* call chain used inconsistent return types - so now it's consistent with sendmsg() etc., all using ssize_t. Stared-at-code and asked GHA :-) Your patch has been applied to the master branch. commit 2cc77debf0221fa0cef3ea470c1328d25397d021 (master) Author: Frank Lichtenheld Date: Wed Sep 18 22:48:44 2024 +0200 socket: Change return types of link_socket_write* to ssize_t Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29323.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering |
From: plaisthos (C. Review) <ge...@op...> - 2024-09-18 21:38:57
|
Attention is currently required from: cron2, flichtenheld. plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/753?usp=email ) Change subject: Refactor methods to use the actual state of tun rather the options struct ...................................................................... Patch Set 1: (1 comment) File src/openvpn/dco.c: http://gerrit.openvpn.net/c/openvpn/+/753/comment/0e3f0b75_606dc005 : PS1, Line 634: if (tun_dco_enabled(m->top.c1.tuntap)) > this does not look correct - it goes from "if(!dco_enabled)" to "if(tun_dco_enabled)" without `!`. Yeah that looks like a mistake but this patch is also accidentially pushed and not really ready yet. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/753?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ib8ba2b63ea9fd1944ff07f69c545a880e464680c Gerrit-Change-Number: 753 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Wed, 18 Sep 2024 21:38:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment |
From: cron2 (C. Review) <ge...@op...> - 2024-09-18 21:01:26
|
cron2 has uploaded a new patch set (#4) to the change originally created by flichtenheld. ( http://gerrit.openvpn.net/c/openvpn/+/674?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: configure: Review use of standard AC macros ...................................................................... configure: Review use of standard AC macros - Increase required version to 2.60 since that is documented minimum version for AC_USE_SYSTEM_EXTENSIONS - Remove obsolete macros AC_C_CONST and AC_C_VOLATILE. They are noops on every compiler we support. - Add explicit call to AC_PROG_CC. We get this implictly as dependency of other macros, but it is nicer to have it explicitely as well. - A few typo and whitespace fixes. Change-Id: I7927a572611b7c1dc0b522fd6cdf05fd222a852d Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29321.html Signed-off-by: Gert Doering <ge...@gr...> --- M configure.ac 1 file changed, 6 insertions(+), 8 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/74/674/4 diff --git a/configure.ac b/configure.ac index 5d7d0fc..0876a6a 100644 --- a/configure.ac +++ b/configure.ac @@ -23,7 +23,7 @@ dnl Process this file with autoconf to produce a configure script. -AC_PREREQ(2.59) +AC_PREREQ(2.60) m4_include(version.m4) AC_INIT([PRODUCT_NAME], [PRODUCT_VERSION], [PRODUCT_BUGREPORT], [PRODUCT_TARNAME]) @@ -387,6 +387,7 @@ pkg_config_found="(${PKG_CONFIG})" fi +AC_PROG_CC AC_PROG_CPP AC_PROG_INSTALL AC_PROG_LN_S @@ -443,9 +444,7 @@ ] ) -AC_C_CONST AC_C_INLINE -AC_C_VOLATILE AC_TYPE_OFF_T AC_TYPE_PID_T AC_TYPE_SIZE_T @@ -958,7 +957,7 @@ [AC_MSG_ERROR([OpenSSL check for AES-256-GCM support failed])] ) - # All supported OpenSSL version (>= 1.1.0) + # All supported OpenSSL versions (>= 1.1.0) # have this feature have_export_keying_material="yes" @@ -1065,9 +1064,8 @@ elif test "${with_crypto_library}" = "wolfssl"; then AC_ARG_VAR([WOLFSSL_CFLAGS], [C compiler flags for wolfssl. The include directory should - contain the regular wolfSSL header files but also the - wolfSSL OpenSSL header files. Ex: -I/usr/local/include - -I/usr/local/include/wolfssl]) + contain the regular wolfSSL header files but also the wolfSSL OpenSSL header files. + Ex: -I/usr/local/include -I/usr/local/include/wolfssl]) AC_ARG_VAR([WOLFSSL_LIBS], [linker flags for wolfssl]) saved_CFLAGS="${CFLAGS}" @@ -1258,7 +1256,7 @@ [PKG_CHECK_MODULES([libsystemd], [libsystemd-daemon])] ) - PKG_CHECK_EXISTS( [libsystemd > 216], + PKG_CHECK_EXISTS([libsystemd > 216], [AC_DEFINE([SYSTEMD_NEWER_THAN_216], [1], [systemd is newer than v216])] ) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/674?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I7927a572611b7c1dc0b522fd6cdf05fd222a852d Gerrit-Change-Number: 674 Gerrit-PatchSet: 4 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset |
From: cron2 (C. Review) <ge...@op...> - 2024-09-18 21:01:26
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/674?usp=email ) Change subject: configure: Review use of standard AC macros ...................................................................... configure: Review use of standard AC macros - Increase required version to 2.60 since that is documented minimum version for AC_USE_SYSTEM_EXTENSIONS - Remove obsolete macros AC_C_CONST and AC_C_VOLATILE. They are noops on every compiler we support. - Add explicit call to AC_PROG_CC. We get this implictly as dependency of other macros, but it is nicer to have it explicitely as well. - A few typo and whitespace fixes. Change-Id: I7927a572611b7c1dc0b522fd6cdf05fd222a852d Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29321.html Signed-off-by: Gert Doering <ge...@gr...> --- M configure.ac 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/configure.ac b/configure.ac index 5d7d0fc..0876a6a 100644 --- a/configure.ac +++ b/configure.ac @@ -23,7 +23,7 @@ dnl Process this file with autoconf to produce a configure script. -AC_PREREQ(2.59) +AC_PREREQ(2.60) m4_include(version.m4) AC_INIT([PRODUCT_NAME], [PRODUCT_VERSION], [PRODUCT_BUGREPORT], [PRODUCT_TARNAME]) @@ -387,6 +387,7 @@ pkg_config_found="(${PKG_CONFIG})" fi +AC_PROG_CC AC_PROG_CPP AC_PROG_INSTALL AC_PROG_LN_S @@ -443,9 +444,7 @@ ] ) -AC_C_CONST AC_C_INLINE -AC_C_VOLATILE AC_TYPE_OFF_T AC_TYPE_PID_T AC_TYPE_SIZE_T @@ -958,7 +957,7 @@ [AC_MSG_ERROR([OpenSSL check for AES-256-GCM support failed])] ) - # All supported OpenSSL version (>= 1.1.0) + # All supported OpenSSL versions (>= 1.1.0) # have this feature have_export_keying_material="yes" @@ -1065,9 +1064,8 @@ elif test "${with_crypto_library}" = "wolfssl"; then AC_ARG_VAR([WOLFSSL_CFLAGS], [C compiler flags for wolfssl. The include directory should - contain the regular wolfSSL header files but also the - wolfSSL OpenSSL header files. Ex: -I/usr/local/include - -I/usr/local/include/wolfssl]) + contain the regular wolfSSL header files but also the wolfSSL OpenSSL header files. + Ex: -I/usr/local/include -I/usr/local/include/wolfssl]) AC_ARG_VAR([WOLFSSL_LIBS], [linker flags for wolfssl]) saved_CFLAGS="${CFLAGS}" @@ -1258,7 +1256,7 @@ [PKG_CHECK_MODULES([libsystemd], [libsystemd-daemon])] ) - PKG_CHECK_EXISTS( [libsystemd > 216], + PKG_CHECK_EXISTS([libsystemd > 216], [AC_DEFINE([SYSTEMD_NEWER_THAN_216], [1], [systemd is newer than v216])] ) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/674?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I7927a572611b7c1dc0b522fd6cdf05fd222a852d Gerrit-Change-Number: 674 Gerrit-PatchSet: 4 Gerrit-Owner: flichtenheld <fr...@li...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged |
From: Gert D. <ge...@gr...> - 2024-09-18 21:00:47
|
I admit that I have no deep understanding of autoconf, but I did check that "about everything" has more recent autoconf versions than 2.60 ("the gold standard" has been 2.69 for many years) and that all our buildbots and GHA are building fine with these changes. Half of them are typo fixes or whitespace cleanups anyway :-) Your patch has been applied to the master branch. commit b59620f24f5bc853e2aa92943f14abf74aa81511 Author: Frank Lichtenheld Date: Wed Sep 18 22:45:50 2024 +0200 configure: Review use of standard AC macros Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg29321.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering |
From: Gert D. <ge...@gr...> - 2024-09-18 20:48:57
|
From: Frank Lichtenheld <fr...@li...> This is the actual return value of send/sendto/sendmsg. We will leave it to the single caller of link_socket_write() to decide how to map that to the int buffer world. For now just cast it explicitly. Change-Id: I7852c06d331326c1dbab7b642254c0c00d4cebb8 Signed-off-by: Frank Lichtenheld <fr...@li...> Acked-by: Gert Doering <ge...@gr...> --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/740 This mail reflects revision 1 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 40b7cc4..84c23b3 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1790,7 +1790,7 @@ socks_preprocess_outgoing_link(c, &to_addr, &size_delta); /* Send packet */ - size = link_socket_write(c->c2.link_socket, &c->c2.to_link, to_addr); + size = (int)link_socket_write(c->c2.link_socket, &c->c2.to_link, to_addr); /* Undo effect of prepend */ link_socket_write_post_size_adjust(&size, size_delta, &c->c2.to_link); diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 17c5e76..6c790a0 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -3399,7 +3399,7 @@ * Socket Write Routines */ -int +ssize_t link_socket_write_tcp(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -3418,7 +3418,7 @@ #if ENABLE_IP_PKTINFO -size_t +ssize_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index 47083ad..bbdabfb 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -1099,9 +1099,9 @@ * Socket Write routines */ -int link_socket_write_tcp(struct link_socket *sock, - struct buffer *buf, - struct link_socket_actual *to); +ssize_t link_socket_write_tcp(struct link_socket *sock, + struct buffer *buf, + struct link_socket_actual *to); #ifdef _WIN32 @@ -1135,12 +1135,12 @@ #else /* ifdef _WIN32 */ -size_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, - struct buffer *buf, - struct link_socket_actual *to); +ssize_t link_socket_write_udp_posix_sendmsg(struct link_socket *sock, + struct buffer *buf, + struct link_socket_actual *to); -static inline size_t +static inline ssize_t link_socket_write_udp_posix(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1158,7 +1158,7 @@ (socklen_t) af_addr_size(to->dest.addr.sa.sa_family)); } -static inline size_t +static inline ssize_t link_socket_write_tcp_posix(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1168,7 +1168,7 @@ #endif /* ifdef _WIN32 */ -static inline size_t +static inline ssize_t link_socket_write_udp(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) @@ -1181,7 +1181,7 @@ } /* write a TCP or UDP packet to link */ -static inline int +static inline ssize_t link_socket_write(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to) |