[Openvpn-devel] [PATCH v2 3/3] ssl: remove unneeded if block
From: Antonio Quartulli
There is no need to check the result of a boolean function and then
assign a constant value to a variable based on that check.
Directly assign the return value of the function to the variable.
Signed-off-by: Antonio Quartulli
---
src/openvpn/ssl.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 9d18c6e5..d8662d00 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1592,7 +1592,6 @@ openvpn_PRF(const uint8_t *secret,
uint8_t *output,
int output_len)
{
-bool ret = true;
/* concatenate seed components */
struct buffer seed = alloc_buf(strlen(label)
@@ -1614,10 +1613,8 @@ openvpn_PRF(const uint8_t *secret,
}
/* compute PRF */
-if (!ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output,
output_len))
-{
-ret = false;
-}
+bool ret = ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len,
+output, output_len);
buf_clear();
free_buf();
--
2.26.3
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] ssl: remove unneeded if block
Hi,
On 05/04/2021 12:38, Gert Doering wrote:
> Hi,
>
> On Mon, Apr 05, 2021 at 10:00:07AM +0200, Antonio Quartulli wrote:
>> /* compute PRF */
>> -if (!ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output,
>> output_len))
>> -{
>> -ret = false;
>> -}
>> +ret = ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output,
>> + output_len);
>
> If touching this, I would move the "bool ret" declaration to here as
> well. Doesn't make sense to have a "bool ret = true" declaration 15
> lines further up if the first use is "assign it a new value", then.
>
> Thus,
>
> +bool ret = ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len,
> +output, output_len);
>
Agreed. v2 will land soon.
Regards,
--
Antonio Quartulli
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Remove conditionals compilation for P2MP, ENABLE_SHAPER and TIME_BACKTRACK_PROTECTION
Acked-by: Gert Doering (As the original mail already claimed :-) ) I have stared-at-diff and grepped for P2MP, SHAPER and BACKTRACK remains (no complaints). Client-side tested, server-side tested in all combinations I have (p2mp, p2p), no surprises. I did not test --shaper, but I am reasonably convinced that the bits removed only the "if ENABLE_SHAPER is not defined", as for "TIME_BACKTRACK_PROTECTION". Out it goes, lots of #ifdefs and especially "#else" code variants that are hard to test gone. Your patch has been applied to the master branch. commit 725dda00f809e5d9768a1aa33c8e73f2e38d9a7e Author: Arne Schwabe Date: Sun Apr 4 13:06:02 2021 +0200 Remove conditionals compilation for P2MP, ENABLE_SHAPER and TIME_BACKTRACK_PROTECTION Message-Id: <20210403184626.23067-1-a...@rfc2549.org> Signed-off-by: Arne Schwabe Acked-by: Gert Doering Message-Id: <20210404110602.20374-1-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22030.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: openssl: avoid NULL pointer dereference
Acked-by: Gert Doering According to OpenSSL documentation, this can indeed return NULL, so catch it... (Note: as for 1/3, this code is only in master, so no need to backport to release/2.5) Your patch has been applied to the master branch. commit f3c7698957483e0ea0f14e712502d34c826c53ca Author: Antonio Quartulli Date: Mon Apr 5 10:00:06 2021 +0200 openssl: avoid NULL pointer dereference Signed-off-by: Antonio Quartulli Acked-by: Gert Doering Message-Id: <20210405080007.1665-...@unstable.cc> URL: https://www.mail-archive.com/search?l=mid=20210405080007.1665-...@unstable.cc Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: openssl: fix EVP_PKEY_CTX memory leak
Acked-by: Gert Doering Looks reasonable, matches coding style & OpenSSL documentation, and passes my client-side tests with OpenSSL 1.1.1 Your patch has been applied to the master branch. commit 24e58164b845614c2176bc6b2a939856fd830c53 Author: Antonio Quartulli Date: Mon Apr 5 10:00:05 2021 +0200 openssl: fix EVP_PKEY_CTX memory leak Signed-off-by: Antonio Quartulli Acked-by: Gert Doering Message-Id: <20210405080007.1665-...@unstable.cc> URL: https://www.mail-archive.com/search?l=mid=20210405080007.1665-...@unstable.cc Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] ssl: remove unneeded if block
Hi,
On Mon, Apr 05, 2021 at 10:00:07AM +0200, Antonio Quartulli wrote:
> /* compute PRF */
> -if (!ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output,
> output_len))
> -{
> -ret = false;
> -}
> +ret = ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output,
> + output_len);
If touching this, I would move the "bool ret" declaration to here as
well. Doesn't make sense to have a "bool ret = true" declaration 15
lines further up if the first use is "assign it a new value", then.
Thus,
+bool ret = ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len,
+output, output_len);
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] Fix 'compress migrate' for 2.2 clients.
> Hi, > > On Mon, Apr 05, 2021 at 10:16:07AM +0200, Simon Matter wrote: >> Then I misunderstood what is written here? >> >> https://community.openvpn.net/openvpn/wiki/DeprecatedOptions#Option:--compress >> >> "Compression is not recommended and is a feature users should avoid >> using. >> To signal this clearly, --comp-lzo and --compress are discouraged and >> considered deprecated features. Beginning with 2.5, these options will >> no >> longer enable compression, just enable the compression framing to be >> able >> to receive compressed packets." >> >> That made me feel compression support in 2.5 is only on a compatibility >> level. > > This is half-correct, and half-incomplete. Look at > > --allow-compression yes > > (new in 2.5). > > Yes, 2.5 will no longer send compressed packets unless explicitly > permitted > by "allow-compression yes" - but the infrastructure is still there, and > still tested, but moved from "this is something everybody will want" to > "if you know what you are doing and why, you can still have it". > > Regarding your other mail where you doubted that "there will be a > discussion" > - well, I can assure you that there are discussions about every feature we > take out of OpenVPN (and usually, mails to openvpn-devel and openvpn-users > to figure out who might still be using it). Many of these discussions do > not happen on the list but on the #openvpn-devel and #openvpn-meeting > IRC channels - but Samuli is sending the meeting minutes to the -devel > list (and they are on the trac web page), so we try to make transparent > what we discuss and why. > > > As always, code is a balance between features, maintenance effort, bugs, > and security... > > gert Ok, thanks for explaining. Regards, Simon ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix 'compress migrate' for 2.2 clients.
Am 05.04.21 um 09:38 schrieb Simon Matter: >> Hi, >> >> On Sat, Apr 03, 2021 at 03:07:11PM +0200, Simon Matter wrote: >>> Apr 3 15:00:30 gw-X1 openvpn[1477]: pre-compress bytes,833300152 >>> Apr 3 15:00:30 gw-X1 openvpn[1477]: post-compress bytes,796650159 >>> Apr 3 15:00:30 gw-X1 openvpn[1477]: pre-decompress bytes,343572096 >>> Apr 3 15:00:30 gw-X1 openvpn[1477]: post-decompress bytes,510118472 >> >> This is indeed fairly significant - and if WAN circuits are full and/or >> cost money per Gbyte, compression is still a win. >> >> I'm more of a compression fan than not :-) - so this will be interesting >> discussions indeed. > > Hi Gert, > > Based on the answers I got on this list I'm afraid there won't be any > discussion and compression is gone. That's really sad as it is a useful > feature for many use cases as I was easily able to show. Currently compression is not planned to be removed for an upcoming release. However from our discussion it should be clear that compression has evolved from a once useful feature for VPN when the security impact wasn't known and HTTP still was 90% vs 10% HTTPS to a niche feature as enabling compression by default on a VPN is not reasonable any more. The future will tell how long compression will stay in OpenVPN. It depends on how big/small the niche is and how much work it is to continue supporting this feature. Arne ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix 'compress migrate' for 2.2 clients.
Hi, On Mon, Apr 05, 2021 at 10:16:07AM +0200, Simon Matter wrote: > Then I misunderstood what is written here? > > https://community.openvpn.net/openvpn/wiki/DeprecatedOptions#Option:--compress > > "Compression is not recommended and is a feature users should avoid using. > To signal this clearly, --comp-lzo and --compress are discouraged and > considered deprecated features. Beginning with 2.5, these options will no > longer enable compression, just enable the compression framing to be able > to receive compressed packets." > > That made me feel compression support in 2.5 is only on a compatibility > level. This is half-correct, and half-incomplete. Look at --allow-compression yes (new in 2.5). Yes, 2.5 will no longer send compressed packets unless explicitly permitted by "allow-compression yes" - but the infrastructure is still there, and still tested, but moved from "this is something everybody will want" to "if you know what you are doing and why, you can still have it". Regarding your other mail where you doubted that "there will be a discussion" - well, I can assure you that there are discussions about every feature we take out of OpenVPN (and usually, mails to openvpn-devel and openvpn-users to figure out who might still be using it). Many of these discussions do not happen on the list but on the #openvpn-devel and #openvpn-meeting IRC channels - but Samuli is sending the meeting minutes to the -devel list (and they are on the trac web page), so we try to make transparent what we discuss and why. As always, code is a balance between features, maintenance effort, bugs, and security... 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] Fix 'compress migrate' for 2.2 clients.
> Hi Simon > > On 05/04/2021 09:38, Simon Matter wrote: >>> Hi, >>> >>> On Sat, Apr 03, 2021 at 03:07:11PM +0200, Simon Matter wrote: Apr 3 15:00:30 gw-X1 openvpn[1477]: pre-compress bytes,833300152 Apr 3 15:00:30 gw-X1 openvpn[1477]: post-compress bytes,796650159 Apr 3 15:00:30 gw-X1 openvpn[1477]: pre-decompress bytes,343572096 Apr 3 15:00:30 gw-X1 openvpn[1477]: post-decompress bytes,510118472 >>> >>> This is indeed fairly significant - and if WAN circuits are full and/or >>> cost money per Gbyte, compression is still a win. >>> >>> I'm more of a compression fan than not :-) - so this will be >>> interesting >>> discussions indeed. >> >> Hi Gert, >> >> Based on the answers I got on this list I'm afraid there won't be any >> discussion and compression is gone. That's really sad as it is a useful >> feature for many use cases as I was easily able to show. >> > > > at the moment there is neither a patch nor a point on any meeting agenda > about removing compression from the OpenVPN codebase. > > Please don't hastily draw such conclusions. > > So far it was *you* opening this discussion about compression removal as > OT of the patch in the subject. We only expressed our personal opinions > about your statements. Then I misunderstood what is written here? https://community.openvpn.net/openvpn/wiki/DeprecatedOptions#Option:--compress "Compression is not recommended and is a feature users should avoid using. To signal this clearly, --comp-lzo and --compress are discouraged and considered deprecated features. Beginning with 2.5, these options will no longer enable compression, just enable the compression framing to be able to receive compressed packets." That made me feel compression support in 2.5 is only on a compatibility level. Regards, Simon ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/3] openssl: fix EVP_PKEY_CTX memory leak
From: Antonio Quartulli
A context allocated with EVP_PKEY_CTX_new_id() must be ultimately free'd
by Eng VP_PKEY_CTX_free(). Failing to do so will result in a memory leak.
This bug was discovered using GCC with "-fsanitize=address".
Signed-off-by: Antonio Quartulli
---
src/openvpn/crypto_openssl.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index f3e86863..d54ca6d2 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1125,37 +1125,41 @@ bool
ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret,
int secret_len, uint8_t *output, int output_len)
{
+bool ret = false;
EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_TLS1_PRF, NULL);
if (!EVP_PKEY_derive_init(pctx))
{
-return false;
+goto out;
}
if (!EVP_PKEY_CTX_set_tls1_prf_md(pctx, EVP_md5_sha1()))
{
-return false;
+goto out;
}
if (!EVP_PKEY_CTX_set1_tls1_prf_secret(pctx, secret, secret_len))
{
-return false;
+goto out;
}
if (!EVP_PKEY_CTX_add1_tls1_prf_seed(pctx, seed, seed_len))
{
-return false;
+goto out;
}
size_t out_len = output_len;
if (!EVP_PKEY_derive(pctx, output, _len))
{
-return false;
+goto out;
}
if (out_len != output_len)
{
-return false;
+goto out;
}
-return true;
+ret = true;
+out:
+EVP_PKEY_CTX_free(pctx);
+return ret;
}
#else /* if OPENSSL_VERSION_NUMBER >= 0x1010L */
/*
--
2.26.3
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 3/3] ssl: remove unneeded if block
From: Antonio Quartulli
There is no need to check the result of a boolean function and then
assign a constant value to a variable based on that check.
Directly assign the return value of the function to the variable.
Signed-off-by: Antonio Quartulli
---
src/openvpn/ssl.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 9d18c6e5..cb2a3e82 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1614,10 +1614,8 @@ openvpn_PRF(const uint8_t *secret,
}
/* compute PRF */
-if (!ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output,
output_len))
-{
-ret = false;
-}
+ret = ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output,
+ output_len);
buf_clear();
free_buf();
--
2.26.3
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 2/3] openssl: avoid NULL pointer dereference
From: Antonio Quartulli
EVP_PKEY_CTX_new_id() may return NULL and for this reason we must check
its return value and bail out in case of failure.
Failing to do so, may result in NULL pointer dereferece when we
pass the returned pointer (NULL) to other functions.
Signed-off-by: Antonio Quartulli
---
src/openvpn/crypto_openssl.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index d54ca6d2..dc6b0fa7 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1125,8 +1125,13 @@ bool
ssl_tls1_PRF(const uint8_t *seed, int seed_len, const uint8_t *secret,
int secret_len, uint8_t *output, int output_len)
{
-bool ret = false;
EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_TLS1_PRF, NULL);
+if (!pctx)
+{
+return false;
+}
+
+bool ret = false;
if (!EVP_PKEY_derive_init(pctx))
{
goto out;
--
2.26.3
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix 'compress migrate' for 2.2 clients.
Hi Simon On 05/04/2021 09:38, Simon Matter wrote: >> Hi, >> >> On Sat, Apr 03, 2021 at 03:07:11PM +0200, Simon Matter wrote: >>> Apr 3 15:00:30 gw-X1 openvpn[1477]: pre-compress bytes,833300152 >>> Apr 3 15:00:30 gw-X1 openvpn[1477]: post-compress bytes,796650159 >>> Apr 3 15:00:30 gw-X1 openvpn[1477]: pre-decompress bytes,343572096 >>> Apr 3 15:00:30 gw-X1 openvpn[1477]: post-decompress bytes,510118472 >> >> This is indeed fairly significant - and if WAN circuits are full and/or >> cost money per Gbyte, compression is still a win. >> >> I'm more of a compression fan than not :-) - so this will be interesting >> discussions indeed. > > Hi Gert, > > Based on the answers I got on this list I'm afraid there won't be any > discussion and compression is gone. That's really sad as it is a useful > feature for many use cases as I was easily able to show. > at the moment there is neither a patch nor a point on any meeting agenda about removing compression from the OpenVPN codebase. Please don't hastily draw such conclusions. So far it was *you* opening this discussion about compression removal as OT of the patch in the subject. We only expressed our personal opinions about your statements. [note that this patch is not about removing compression, but only about helping those people who want to disable it] *If* somebody will feel the need to remove compression at all, such topic will definitely be discussed in an open community meeting first (everybody is welcome to join). Only then a a bunch of patches (for deprecation first and removal later) that will appear and at that point there will be even more room for discussion. Still, this is the process we take for almost every feature deprecation/removal, but nobody has started it for compression. Regards, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix 'compress migrate' for 2.2 clients.
> Hi, > > On Sat, Apr 03, 2021 at 03:07:11PM +0200, Simon Matter wrote: >> Apr 3 15:00:30 gw-X1 openvpn[1477]: pre-compress bytes,833300152 >> Apr 3 15:00:30 gw-X1 openvpn[1477]: post-compress bytes,796650159 >> Apr 3 15:00:30 gw-X1 openvpn[1477]: pre-decompress bytes,343572096 >> Apr 3 15:00:30 gw-X1 openvpn[1477]: post-decompress bytes,510118472 > > This is indeed fairly significant - and if WAN circuits are full and/or > cost money per Gbyte, compression is still a win. > > I'm more of a compression fan than not :-) - so this will be interesting > discussions indeed. Hi Gert, Based on the answers I got on this list I'm afraid there won't be any discussion and compression is gone. That's really sad as it is a useful feature for many use cases as I was easily able to show. Regards, Simon ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
