[Openvpn-devel] [PATCH] block-dns using iservice: fix a potential double free
From: Selva Nair
- An item added to undo-list was not removed on error, causing
attempt to free again in Undo().
Also fix a memory leak possibility in the same context.
Github: fixes OpenVPN/openvpn#232
Signed-off-by: Selva Nair
---
src/openvpnserv/interactive.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 03361d66..636f32e9 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -780,7 +780,7 @@ static DWORD
HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists)
{
DWORD err = 0;
-block_dns_data_t *interface_data;
+block_dns_data_t *interface_data = NULL;
HANDLE engine = NULL;
LPCWSTR exe_path;
@@ -794,6 +794,7 @@ HandleBlockDNSMessage(const block_dns_message_t *msg,
undo_lists_t *lists)
interface_data = malloc(sizeof(block_dns_data_t));
if (!interface_data)
{
+delete_block_dns_filters(engine);
return ERROR_OUTOFMEMORY;
}
interface_data->engine = engine;
@@ -818,8 +819,17 @@ HandleBlockDNSMessage(const block_dns_message_t *msg,
undo_lists_t *lists)
BLOCK_DNS_IFACE_METRIC);
if (!err)
{
-set_interface_metric(msg->iface.index, AF_INET6,
- BLOCK_DNS_IFACE_METRIC);
+err = set_interface_metric(msg->iface.index, AF_INET6,
+ BLOCK_DNS_IFACE_METRIC);
+}
+if (err)
+{
+/* try to restore the interface metric values in case
changed */
+set_interface_metric(msg->iface.index, AF_INET,
+ interface_data->metric_v4);
+set_interface_metric(msg->iface.index, AF_INET,
+ interface_data->metric_v4);
+RemoveListItem(&(*lists)[block_dns], CmpAny, NULL);
}
}
}
@@ -853,6 +863,7 @@ HandleBlockDNSMessage(const block_dns_message_t *msg,
undo_lists_t *lists)
if (err && engine)
{
delete_block_dns_filters(engine);
+free(interface_data);
}
return err;
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Get rid of unused 'bool tuntap_buffer' arguments.
Hi, On Mon, Jan 30, 2023 at 04:17:30PM +, Gert Doering wrote: > overlapped_io_init() has a "bool tuntap_buffer" argument which is only > passed onwards to alloc_buf_sock_tun(), which does nothing with it. > > Remove from both functions. > > While at it, move alloc_buf_sock_tun() from mtu.c to win32.c and make > static. It's only ever called from win32.c / overlapped_io_init(). NAK this. There are more calls in socket.c, in the non-WIN32 code paths, so v2 breaks Linux compilation (not sure why I overlooked that before). Need to re-think how I want to clean this up... gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v15] Add DNS SRV remote host discovery support
Hi, On Wed, Jan 11, 2023 at 01:42:31AM +0500, Vladislav Grishenko wrote: > Hi, sure, will do. > Yes, I???ve noticed undesired code dup in v14 and have fixed everything found > in v15 rebase, same will be rechecked in v16 of course. Did you find time to have a look into this? 2.6.0 is out, but since there is demand for the SRV patch, we've agreed to do an exceptional feature merge for 2.6.1 for SRV and dynamic tls-crypt - but that means "we need to have a fully reviewed v16"... gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v7] Dynamic tls-crypt for secure soft_reset/session renegotiation
Currently we have only one slot for renegotiation of the session/keys. If a replayed/faked packet is inserted by a malicous attacker, the legimate peer cannot renegotiate anymore. This commit introduces dynamic tls-crypt. When both peer support this feature, both peer create a dynamic tls-crypt key using TLS EKM (export key material) and will enforce using that key and tls-crypt for all renegotiations. This also add an additional protection layer for renegotiations to be taken over by an illegimate client, binding the renegotiations tightly to the original session. Especially when 2FA, webauth or similar authentication is used, many third party setup ignore the need to secure renegotiation with an auth-token. Since one of tls-crypt/tls-crypt-v2 purposes is to provide poor man's post quantum crypto guarantees, we have to ensure that the dynamic key tls-crypt key that replace the original tls-crypt key is as strong as the orginal key to avoid problems if there is a weak RNG or TLS EKM produces weak keys. We ensure this but XORing the original key with the key from TLS EKM. If tls-crypt/tls-cryptv2 is not active, we use just the key generated by TLS EKM. We also do not use hashing or anything else on the original key before XOR to avoid any potential of a structure in the key or something else that might weaken post-quantum use cases. OpenVPN 2.x reserves the TM_ACTIVE session for renegotiations. When a SOFT_RESET_V1 packet is received, the active TLS session is moved from KS_PRIMARY to KS_SECONDARY. Here an attacker could theorectically send a faked/replayed SOFT_RESET_V1 and first packet containing the TLS client hello. If this happens, the session is blocked until the TLS renegotiation attempt times out, blocking the legimitate client. Using a dynamic tls-crypt key here blocks any SOFT_RESET_V1 (and following packets) as replay and fake packets will not have a matching authentication/encryption and will be discarded. HARD_RESET packets that are from a reconnecting peer are instead put in the TM_UNTRUSTED/KS_PRIMARY slot until they are sufficiently verified, so the dyanmic tls-crypt key is not used here. Replay/fake packets also do not block the legimitate client. This commit delays the purging of the original tls-crypt key data from directly after passing it to crypto library to tls_wrap_free. We do this to allow us mixing the new exported key with the original key. To be able to generate the dynamic tls-cryptn key, we need the original key, so deleting the key is not an option if we need it later again to generate another key. Even when the client does not support secure renegotiation, deleting the key is not an option since when the reconnecting client or (especially in p2p mode with float) another client does the reconnect, we might need to generate a dynamic tls-crypt key again. Delaying the deletion of the key has also little effect as the key is still present in the OpenSSL/mbed TLS structures in the tls_wrap structure, so only the number of times the keys is in memory would be reduced. Patch v2: fix spellings of reneg and renegotiations. Patch v3: expand comment to original_tlscrypt_keydata and commit message, add Changes.rst Patch v4: improve commit message, Changes.rst Patch v5: fix spelling/grammar mistakes. Add more comments. Patch v6: consistently calld this feature dynamic tls-crypt crypt. Note this changes the export label and makes it incompatible with previous patches. Patch v7: also xor tls-auth key data into the dynamic tls-crypt key like tls-crypt key data Signed-off-by: Arne Schwabe --- Changes.rst | 6 ++ src/openvpn/auth_token.h | 2 +- src/openvpn/crypto.c | 7 +- src/openvpn/crypto.h | 16 +++- src/openvpn/init.c| 22 +++--- src/openvpn/multi.c | 4 + src/openvpn/openvpn.h | 3 + src/openvpn/options.c | 4 + src/openvpn/push.c| 5 ++ src/openvpn/ssl.c | 25 -- src/openvpn/ssl.h | 5 ++ src/openvpn/ssl_backend.h | 1 + src/openvpn/ssl_common.h | 13 src/openvpn/ssl_ncp.c | 4 + src/openvpn/ssl_pkt.c | 2 +- src/openvpn/ssl_pkt.h | 26 +++ src/openvpn/tls_crypt.c | 94 --- src/openvpn/tls_crypt.h | 22 +- tests/unit_tests/openvpn/test_pkt.c | 17 +++- tests/unit_tests/openvpn/test_tls_crypt.c | 85 20 files changed, 326 insertions(+), 37 deletions(-) diff --git a/Changes.rst b/Changes.rst index f4c3587f4..1cd2c6ab6 100644 --- a/Changes.rst +++ b/Changes.rst @@ -122,6 +122,12 @@ Tun MTU can be pushed directive ``--tun-mtu-max`` has been introduced to increase the maximum pushable
[Openvpn-devel] [PATCH v2] dco-win: use proper calling convention on x86
From: Lev Stipakov
WinAPI uses __stdcall calling convention on x86. Wrong
calling convention causes UB, which in this case breaks
dco-win functionality.
Signed-off-by: Lev Stipakov
---
v2:
- use WINAPI instead of __stdcall
- replace another existing occurence of __stdcall with WINAPI
- uncrustify
src/openvpn/dco_win.c | 2 +-
src/openvpn/win32.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 7594024c..0931fb30 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -110,7 +110,7 @@ dco_connect_wait(HANDLE handle, OVERLAPPED *ov, int
timeout, struct signal_info
{
volatile int *signal_received = _info->signal_received;
/* GetOverlappedResultEx is available starting from Windows 8 */
-typedef BOOL (*get_overlapped_result_ex_t) (HANDLE, LPOVERLAPPED, LPDWORD,
DWORD, BOOL);
+typedef BOOL (WINAPI *get_overlapped_result_ex_t)(HANDLE, LPOVERLAPPED,
LPDWORD, DWORD, BOOL);
get_overlapped_result_ex_t get_overlapped_result_ex =
(get_overlapped_result_ex_t)GetProcAddress(GetModuleHandle("Kernel32.dll"),
"GetOverlappedResultEx");
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 44176936..6d482ef8 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1352,7 +1352,7 @@ win32_get_arch(arch_t *process_arch, arch_t *host_arch)
*process_arch = ARCH_UNKNOWN;
*host_arch = ARCH_NATIVE;
-typedef BOOL (__stdcall *is_wow64_process2_t)(HANDLE, USHORT *, USHORT *);
+typedef BOOL (WINAPI *is_wow64_process2_t)(HANDLE, USHORT *, USHORT *);
is_wow64_process2_t is_wow64_process2 = (is_wow64_process2_t)
GetProcAddress(GetModuleHandle("Kernel32.dll"), "IsWow64Process2");
--
2.38.1.windows.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco-win: use proper calling convention on x86
On 31/01/2023 13:25, Lev Stipakov wrote:
From: Lev Stipakov
WinAPI uses __stdcall calling convention on x86. Wrong
calling convention causes UB, which in this case breaks
dco-win functionality.
Signed-off-by: Lev Stipakov
---
src/openvpn/dco_win.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 7594024c..da1e1fbc 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -110,7 +110,7 @@ dco_connect_wait(HANDLE handle, OVERLAPPED *ov, int
timeout, struct signal_info
{
volatile int *signal_received = _info->signal_received;
/* GetOverlappedResultEx is available starting from Windows 8 */
-typedef BOOL (*get_overlapped_result_ex_t) (HANDLE, LPOVERLAPPED, LPDWORD,
DWORD, BOOL);
+typedef BOOL (__stdcall *get_overlapped_result_ex_t) (HANDLE,
LPOVERLAPPED, LPDWORD, DWORD, BOOL);
Wouldn't it be better to use the WINAPI Macro here, so it's the right
calling convention no matter the arch?
Not sure what header it's defined in, but a quick search suggests it
comes from minwindef.h.
get_overlapped_result_ex_t get_overlapped_result_ex =
(get_overlapped_result_ex_t)GetProcAddress(GetModuleHandle("Kernel32.dll"),
"GetOverlappedResultEx");
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] dco-win: use proper calling convention on x86
From: Lev Stipakov
WinAPI uses __stdcall calling convention on x86. Wrong
calling convention causes UB, which in this case breaks
dco-win functionality.
Signed-off-by: Lev Stipakov
---
src/openvpn/dco_win.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 7594024c..da1e1fbc 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -110,7 +110,7 @@ dco_connect_wait(HANDLE handle, OVERLAPPED *ov, int
timeout, struct signal_info
{
volatile int *signal_received = _info->signal_received;
/* GetOverlappedResultEx is available starting from Windows 8 */
-typedef BOOL (*get_overlapped_result_ex_t) (HANDLE, LPOVERLAPPED, LPDWORD,
DWORD, BOOL);
+typedef BOOL (__stdcall *get_overlapped_result_ex_t) (HANDLE,
LPOVERLAPPED, LPDWORD, DWORD, BOOL);
get_overlapped_result_ex_t get_overlapped_result_ex =
(get_overlapped_result_ex_t)GetProcAddress(GetModuleHandle("Kernel32.dll"),
"GetOverlappedResultEx");
--
2.38.1.windows.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/5] Fix unaligned access in macOS/Solaris hwaddr
On Mon, Jan 30, 2023 at 06:29:33PM +0100, Arne Schwabe wrote: > The undefined behaviour USAN clang checker found this. > > This fix is a bit messy but so are the original structures. > Acked-By: Frank Lichtenheld Well, it doesn't make it worse vOv Regards, -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 4/5] Add printing USAN stack trace on github actions
On Mon, Jan 30, 2023 at 06:29:35PM +0100, Arne Schwabe wrote:
> This allows identifying the source of undefined behaviour more easily
> from the github action logs.
>
> Signed-off-by: Arne Schwabe
> ---
> .github/workflows/build.yaml | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
> index 6adb69563..132624547 100644
> --- a/.github/workflows/build.yaml
> +++ b/.github/workflows/build.yaml
> @@ -242,6 +242,9 @@ jobs:
>
> name: "clang-asan - ${{matrix.os}} - ${{matrix.ssllib}}"
>
> +env:
> + UBSAN_OPTIONS: print_stacktrace=1
> +
> runs-on: ${{matrix.os}}
> steps:
>- name: Install dependencies
> @@ -291,6 +294,7 @@ jobs:
>LDFLAGS: ${{ matrix.ldflags }}
>OPENSSL_CFLAGS: "-I/usr/local/opt/${{matrix.libdir}}/include"
>OPENSSL_LIBS: "-L/usr/local/opt/${{matrix.libdir}}/lib -lcrypto -lssl"
> + UBSAN_OPTIONS: print_stacktrace=1
> steps:
>- name: Install dependencies
> run: brew install openssl@1.1 openssl@3 lzo lz4 man2html cmocka
> libtool automake autoconf libressl
> @@ -400,6 +404,7 @@ jobs:
>CFLAGS: ${{ matrix.cflags }}
>LDFLAGS: ${{ matrix.ldflags }}
>CC: ${{matrix.cc}}
> + UBSAN_OPTIONS: print_stacktrace=1
>
> steps:
>- name: Install dependencies
Acked-By: Frank Lichtenheld
Trivial enough.
Regards,
--
Frank Lichtenheld
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/5] Update LibreSSL to 3.7.0 in Github actions
On Mon, Jan 30, 2023 at 06:29:34PM +0100, Arne Schwabe wrote: > The version 3.5.3 triggers undefined behaviour with the usan sanatizer. > Updating LibreSSSL to 3.7.0 does unfortunately does not fix the issue but > at least we are now using a current version. > > Signed-off-by: Arne Schwabe > --- > .github/workflows/build.yaml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml > index 2db90bcde..6adb69563 100644 > --- a/.github/workflows/build.yaml > +++ b/.github/workflows/build.yaml > @@ -409,7 +409,7 @@ jobs: > with: >path: libressl >repository: libressl-portable/portable > - ref: v3.5.3 > + ref: v3.7.0 >- name: "libressl: autogen.sh" > run: ./autogen.sh > working-directory: libressl > @@ -417,7 +417,7 @@ jobs: > run: autoreconf -fvi > working-directory: libressl >- name: "libressl: configure" > -run: ./configure --enable-openvpn > +run: ./configure > working-directory: libressl >- name: "libressl: make all" Acked-By: Frank Lichtenheld Always good to update to latest. Regards, -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/5] Fix unaligned access in auth-token
On Mon, Jan 30, 2023 at 06:29:32PM +0100, Arne Schwabe wrote: > The undefined behaviour USAN clang checker found this. The optimiser > of clang/gcc will optimise the memcpy away in the auth_token case and output > excactly the same assembly on amd64/arm64 but it is still better to not rely > on undefined behaviour. > > Signed-off-by: Arne Schwabe > --- > src/openvpn/auth_token.c | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c > index 7b963a9c5..e4486eb08 100644 > --- a/src/openvpn/auth_token.c > +++ b/src/openvpn/auth_token.c > @@ -324,8 +324,14 @@ verify_auth_token(struct user_pass *up, struct tls_multi > *multi, > const uint8_t *tstamp_initial = sessid + AUTH_TOKEN_SESSION_ID_LEN; > const uint8_t *tstamp = tstamp_initial + sizeof(int64_t); > > -uint64_t timestamp = ntohll(*((uint64_t *) (tstamp))); > -uint64_t timestamp_initial = ntohll(*((uint64_t *) (tstamp_initial))); > +/* tstamp, tstamp_initial might not be aligned to an uint64, use memcpy > + * to avoid unaligned access */ > +uint64_t timestamp = 0, timestamp_initial = 0; > +memcpy(, tstamp, sizeof(uint64_t)); > +timestamp = ntohll(timestamp); > + > +memcpy(_initial, tstamp_initial, sizeof(uint64_t)); > +timestamp_initial = ntohll(timestamp_initial); > > hmac_ctx_t *ctx = multi->opt.auth_token_key.hmac; > if (check_hmac_token(ctx, b64decoded, up->username)) Acked-By: Frank Lichtenheld Trivial enough. Regards, -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/2] Improve signal handling using POSIX sigaction
On Sat, Jan 28, 2023 at 04:59:00PM -0500, selva.n...@gmail.com wrote: > From: Selva Nair > > Currently we use the old signal API which follows system-V or > BSD semantics depending on the platform and/or feature-set macros. > Further, signal has many weaknesses which makes proper masking > (deferring) of signals during update not possible. > > Improve this: > > - Use sigaction to properly mask signals when modifying. > Acked-By: Frank Lichtenheld Stared at code intensively. AFAICT this should not change the general behavior except to be more generally safe. Only compile+t_client tested. Regards, -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
