Re: [Openvpn-devel] [PATCH] Log serial number of revoked certificate

2020-08-05 Thread Lev Stipakov
Hi,

Compiled and tested on Ubuntu 20.04, looks good.

A few nit-picks:

> +verify_check_crl_dir(const char *crl_dir, int cert_depth, 
> openvpn_x509_cert_t *cert, char *subject)

The last parameter could benefit from const to indicate that function
is not going to modify it.


> -msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is 
> revoked", serial);
> +msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
> +cert_depth, subject, serial);

Since you are modifying this line, could you add a NULL check to
serial to and pass
something like "" in this case?


> +msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, 
> serial=%s: %s",
> +cert_depth, subject, serial ? serial : "", errstr);

I would use "" in NULL case, otherwise the error
message becomes funny.


> +msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, serial=%s",
>  X509_STORE_CTX_get_error_depth(ctx),
>  X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
> -subject);
> +subject, serial ? serial : "");

Same as above.


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Log serial number of revoked certificate

2020-08-05 Thread Vladislav Grishenko
Hi, Lev
Thanks for review, I'll make improvements in V2. 

--
Best Regards, Vladislav Grishenko

-Original Message-
From: Lev Stipakov  
Sent: Wednesday, August 5, 2020 1:29 PM
To: Vladislav Grishenko 
Cc: openvpn-devel 
Subject: Re: [Openvpn-devel] [PATCH] Log serial number of revoked certificate

Hi,

Compiled and tested on Ubuntu 20.04, looks good.

A few nit-picks:

> +verify_check_crl_dir(const char *crl_dir, int cert_depth, 
> +openvpn_x509_cert_t *cert, char *subject)

The last parameter could benefit from const to indicate that function is not 
going to modify it.


> -msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is 
> revoked", serial);
> +msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
> +cert_depth, subject, serial);

Since you are modifying this line, could you add a NULL check to serial to and 
pass something like "" in this case?


> +msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, 
> serial=%s: %s",
> +cert_depth, subject, serial ? serial : "", errstr);

I would use "" in NULL case, otherwise the error message becomes 
funny.


> +msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, 
> + serial=%s",
>  X509_STORE_CTX_get_error_depth(ctx),
>  X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
> -subject);
> +subject, serial ? serial : "");

Same as above.



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Skip existing interfaces on opening the first available utun on macOS

2020-08-05 Thread Lev Stipakov
Hi,

 +ASSERT(snprintf(ifname, sizeof(ifname), "utun%d", utunnum));

Not sure about ASSERT here, because according to
https://linux.die.net/man/3/snprintf

> If an output error is encountered, a negative value is returned.

which won't trigger assert.

Otherwise looks good.

Compiled and tested on macOS.

Maybe Gert could remove ASSERT wrapping before commiting.

Acked-by: Lev Stipakov 


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2] Log serial number of revoked certificate

2020-08-05 Thread Vladislav Grishenko
As it appears commit 767e4c56becbfeea525e4695a810593f373883cd "Log
serial number of revoked certificate" hasn't survive refactoring
of CRL handling.

In most of situations admin of OpenVPN server needs to know which
particular certificate is used by client.
In the case when certificate is valid, environment variable can be
used for that but once it is revoked, no user scripts are invoked
so there is no way to get serial number, only subject is logged.

Let's log certificate serial in case it is revoked and additionally
log certificate depth & subject in crl-verify "dir" mode for better
consistency with crl file (non-dir) mode.

v2: log if serial is not availble, require it in crl-verify dir mode

Signed-off-by: Vladislav Grishenko 
---
 src/openvpn/ssl_verify.c | 14 +++---
 src/openvpn/ssl_verify_mbedtls.c |  5 +++--
 src/openvpn/ssl_verify_openssl.c |  5 +++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 844bc57d..97ccb93b 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -599,7 +599,8 @@ cleanup:
  * check peer cert against CRL directory
  */
 static result_t
-verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert)
+verify_check_crl_dir(const char *crl_dir, openvpn_x509_cert_t *cert,
+ const char *subject, int cert_depth)
 {
 result_t ret = FAILURE;
 char fn[256];
@@ -607,6 +608,12 @@ verify_check_crl_dir(const char *crl_dir, 
openvpn_x509_cert_t *cert)
 struct gc_arena gc = gc_new();
 
 char *serial = backend_x509_get_serial(cert, &gc);
+if (!serial)
+{
+msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial number is not 
available",
+cert_depth, subject);
+goto cleanup;
+}
 
 if (!openvpn_snprintf(fn, sizeof(fn), "%s%c%s", crl_dir, 
OS_SPECIFIC_DIRSEP, serial))
 {
@@ -616,7 +623,8 @@ verify_check_crl_dir(const char *crl_dir, 
openvpn_x509_cert_t *cert)
 fd = platform_open(fn, O_RDONLY, 0);
 if (fd >= 0)
 {
-msg(D_HANDSHAKE, "VERIFY CRL: certificate serial number %s is 
revoked", serial);
+msg(D_HANDSHAKE, "VERIFY CRL: depth=%d, %s, serial=%s is revoked",
+cert_depth, subject, serial);
 goto cleanup;
 }
 
@@ -758,7 +766,7 @@ verify_cert(struct tls_session *session, 
openvpn_x509_cert_t *cert, int cert_dep
 {
 if (opt->ssl_flags & SSLF_CRL_VERIFY_DIR)
 {
-if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert))
+if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert, subject, 
cert_depth))
 {
 goto cleanup;
 }
diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index fd31bbbd..93891038 100644
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -68,6 +68,7 @@ verify_callback(void *session_obj, mbedtls_x509_crt *cert, 
int cert_depth,
 int ret = 0;
 char errstr[512] = { 0 };
 char *subject = x509_get_subject(cert, &gc);
+char *serial = backend_x509_get_serial(cert, &gc);
 
 ret = mbedtls_x509_crt_verify_info(errstr, sizeof(errstr)-1, "", 
*flags);
 if (ret <= 0 && !openvpn_snprintf(errstr, sizeof(errstr),
@@ -82,8 +83,8 @@ verify_callback(void *session_obj, mbedtls_x509_crt *cert, 
int cert_depth,
 
 if (subject)
 {
-msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s: %s",
-cert_depth, subject, errstr);
+msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, subject=%s, serial=%s: 
%s",
+cert_depth, subject, serial ? serial : "", 
errstr);
 }
 else
 {
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index ff14db23..454efeec 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -71,6 +71,7 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
 {
 /* get the X509 name */
 char *subject = x509_get_subject(current_cert, &gc);
+char *serial = backend_x509_get_serial(current_cert, &gc);
 
 if (!subject)
 {
@@ -89,10 +90,10 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
 }
 
 /* Remote site specified a certificate, but it's not correct */
-msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s",
+msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s, serial=%s",
 X509_STORE_CTX_get_error_depth(ctx),
 X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
-subject);
+subject, serial ? serial : "");
 
 ERR_clear_error();
 
-- 
2.17.1



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Log serial number of revoked certificate

2020-08-05 Thread Lev Stipakov
Code looks good. Compiled and tested (openssl and revoked cert) on Ubuntu 20.04.

Acked-by: Lev Stipakov 


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

2020-08-05 Thread Gert Doering
Hi,

On Wed, Aug 05, 2020 at 08:43:18AM +0200, Gert Doering wrote:
> Test run with "cipher bf-cbc" in all server configs next...

For completeness, this works nicely:

start client jobs...
22...
Test sets succeeded: 1 2 3 4 6 8.
Test sets failed: none.
23.small...
Test sets succeeded: 1 2 3 4.
Test sets failed: none.
23...
Test sets succeeded: 1 1a 1b 1d 2 2a 2b 2c 2d 3 4 5 6 8 8a 9.
Test sets failed: none.
24...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 4a 5 6 8 8a 9.
Test sets failed: none.
master...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 5 5a 5b 5c 5d 5v1 
5v2 5v3 5w1 5w2 5w3 5w4 5x1 5x2 5x3 5x4 6 7 7x 8 8a 9 2f 4b.
Test sets failed: none.


so, if we break someone's existing server setup, the answer is

 "put 'fallback-cipher BF-CBC' into your config!"

(or 'cipher BF-CBC' explicitly, so it's 2.4-compatible)


Not sure how to tackle the "ccd/ push cipher is broken now with 2.4-NCP
clients" part.  I think this is useful functionality, but the current
patch does not allow this "unless the client is already using the to-be-
pushed cipher, or it's one of the two NCP=2 AEAD ciphers".  Which makes
it slightly less than useful...

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 applied] Re: Log serial number of revoked certificate

2020-08-05 Thread Gert Doering
Your patch has been applied to the master branch.

I have not done much testing, just a test run "make check" on an
OpenSSL and mbedTLS build.

I have not looked into applying it to "release/2.4" - if you think 
it's needed, let me know (or if it needs more work because the code
has diverged too much, send a 2.4 patch) - thanks.

commit 992e9cec40539a155afa9eae10502aa62f617965
Author: Vladislav Grishenko
Date:   Wed Aug 5 15:23:33 2020 +0500

 Log serial number of revoked certificate

 Signed-off-by: Vladislav Grishenko 
 Acked-by: Lev Stipakov 
 Message-Id: <20200805102333.3109-1-themi...@yandex-team.ru>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20642.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


Re: [Openvpn-devel] [PATCH applied] Re: Log serial number of revoked certificate

2020-08-05 Thread Vladislav Grishenko
Hi, Gert

Thank you.
I'd appreciate if patch could be applied to release/2.4 too, no changes are
required - related code is the same, just hunks offset in ssl_verify.c and
ssl_verify_openssl.c
I've tested 2.4.9 [git:release/2.4/7c428ca19a8df6b9+] with patch on sample
certificates, please refer log is below:

OpenSSL, --crl-verify sample-keys/ca.crl
Wed Aug  5 17:18:49 2020 127.0.0.1:16001 VERIFY ERROR: depth=0,
error=certificate revoked: C=KG, ST=NA, O=OpenVPN-TEST, CN=client-revoked,
emailAddress=me@myhost.mydomain, serial=3
Wed Aug  5 17:18:49 2020 127.0.0.1:16001 OpenSSL: error:1417C086:SSL
routines:tls_process_client_certificate:certificate verify failed
Wed Aug  5 17:18:49 2020 127.0.0.1:16001 TLS_ERROR: BIO read
tls_read_plaintext error

mbedTLS, --crl-verify sample-keys/ca.crl
Wed Aug  5 17:25:28 2020 127.0.0.1:16001 VERIFY OK: depth=1, C=KG, ST=NA,
L=BISHKEK, O=OpenVPN-TEST, emailAddress=me@myhost.mydomain
Wed Aug  5 17:25:28 2020 127.0.0.1:16001 VERIFY ERROR: depth=0,
subject=C=KG, ST=NA, O=OpenVPN-TEST, CN=client-revoked,
emailAddress=me@myhost.mydomain, serial=3: The certificate has been revoked
(is on a CRL)
Wed Aug  5 17:25:28 2020 127.0.0.1:16001 TLS_ERROR: read tls_read_plaintext
error: X509 - Certificate verification failed, e.g. CRL, CA or signature
check failed

touch sample-keys/3, --crl-verify sample-keys/ dir
Wed Aug  5 17:18:12 2020 127.0.0.1:16001 VERIFY OK: depth=1, C=KG, ST=NA,
L=BISHKEK, O=OpenVPN-TEST, emailAddress=me@myhost.mydomain
Wed Aug  5 17:18:12 2020 127.0.0.1:16001 VERIFY CRL: depth=0, C=KG, ST=NA,
O=OpenVPN-TEST, CN=client-revoked, emailAddress=me@myhost.mydomain, serial=3
is revoked
Wed Aug  5 17:18:12 2020 127.0.0.1:16001 OpenSSL: error:1417C086:SSL
routines:tls_process_client_certificate:certificate verify failed
Wed Aug  5 17:18:12 2020 127.0.0.1:16001 TLS_ERROR: BIO read
tls_read_plaintext error

--
Best Regards, Vladislav Grishenko

-Original Message-
From: Gert Doering  
Sent: Wednesday, August 5, 2020 4:55 PM
To: Vladislav Grishenko 
Cc: openvpn-devel@lists.sourceforge.net
Subject: [PATCH applied] Re: Log serial number of revoked certificate

Your patch has been applied to the master branch.

I have not done much testing, just a test run "make check" on an OpenSSL and
mbedTLS build.

I have not looked into applying it to "release/2.4" - if you think it's
needed, let me know (or if it needs more work because the code has diverged
too much, send a 2.4 patch) - thanks.

commit 992e9cec40539a155afa9eae10502aa62f617965
Author: Vladislav Grishenko
Date:   Wed Aug 5 15:23:33 2020 +0500

 Log serial number of revoked certificate

 Signed-off-by: Vladislav Grishenko 
 Acked-by: Lev Stipakov 
 Message-Id: <20200805102333.3109-1-themi...@yandex-team.ru>
 URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20642.ht
ml
 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 applied] Re: Log serial number of revoked certificate

2020-08-05 Thread Gert Doering
Hi,

On Wed, Aug 05, 2020 at 05:27:45PM +0500, Vladislav Grishenko wrote:
> Thank you.
> I'd appreciate if patch could be applied to release/2.4 too, no changes are
> required - related code is the same, just hunks offset in ssl_verify.c and
> ssl_verify_openssl.c
> I've tested 2.4.9 [git:release/2.4/7c428ca19a8df6b9+] with patch on sample
> certificates, please refer log is below:

OK.  Done.  Thanks :-)

commit 4ee2c1cd877b2e99b41fd248bf853329af825188 (HEAD -> release/2.4)

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 applied] Re: Fix compilation with --disable-lzo and --disable-lz4

2020-08-05 Thread Gert Doering
Acked-by: Gert Doering 

Thanks.  Tested that it indeed fixes things, and that it is 
not necessary for 2.4 (compile-tested only).

Added "trac #1308" to the commit message.

Your patch has been applied to the master branch.

commit dab34fdd0639c6de8c5ca759cca00b7e60da32f1
Author: Lev Stipakov
Date:   Wed Aug 5 06:25:48 2020 +

 Fix compilation with --disable-lzo and --disable-lz4

 Signed-off-by: Lev Stipakov 
 Acked-by: Gert Doering 
 Message-Id: <20200805062548.38082-1-lstipa...@gmail.com>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20637.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


Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

2020-08-05 Thread Steffan Karger
Hi,

No full review yet, but this version does seem to address my previous
comments. Some minor nits I noticed on my first run through v2:

On 29-07-2020 13:38, Arne Schwabe wrote:
> This reworks the NCP logic to be more strict about what is
> considered an acceptable result of an NCP negotiation. It also
> us to finally drop BF-CBC support by default.
> 
> All new behaviour is currently limited to server/client
> mode with pull enabled. P2p mode without pull does not change.
> 
> New Server behaviour:
> - when a client announces its supported ciphers through either
>   OCC or IV_CIPHER/IV_NCP we reject the client with a
>   AUTH_FAILED message if we have no common cipher.
> 
> - When a client does not announce any cipher in either
>   OCC or NCP we by reject it unless fallback-cipher is
>   specified in either ccd or config.
> 
> New client behaviour:
> - When no cipher is pushed (or a cipher we refused to support)
>   and we also cannot support the server's server announced in
>   OCC we fail the connection and log why
> 
> - If fallback-cipher is specified we will in the case that
>   cipher is missing from occ use the fallback cipher instead
>   of failing the connection
> 
> Both client and server behaviour:
> - We only announce --cipher xyz in occ if we are willing
>   to support that cipher.
> 
>   It means that we only announce the fallback-cipher if
>   it is also contained in --data-ciphers
> 
> Compatiblity behaviour:
> 
> In 2.5 both client and server will automatically set
> fallback-cipher xyz if --cipher xyz is in the config and
> also add append the cipher to the end of data-ciphers.
> 
> We log a warn user about this and point to --data-ciphers and
> --fallback-cipher. This also happens if the configuration
> contains an explicit --cipher BF-CBC.
> 
> If --cipher is not set, we only warn that previous versions
> allowed BF-CBC and point how to reenable BF-CBC. This might
> break very few config where someone connects a very old
> client to a 2.5 server but at some point we need to drop
> the BF-CBC and those affected use will already have a the
> scary SWEET32 warning in their logs.
> 
> In short: If --cipher is explicitly set 2.6 will work the same as
> 2.4 did. When --cipher is not set, BF-CBC support is dropped and
> we warn about it.
> 
> Examples how breaking the default BF-CBC will be logged:
> 
> Client side:
>  - Client connecting to server that does not push cipher but
>has --cipher in OCC
> 
> OPTIONS ERROR: failed to negotiate cipher with server.  Add the server's 
> cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if 
> you want to connect to this server.
> 
>  - Client connecting to a server that does not support OCC:
> 
>OPTIONS ERROR: failed to negotioate cipher with server. Configure 
> --fallback-cipher if you want connect to this server.
> 
> Server Side:
> 
> - Server has a client only supporting BF-CBC connecting:
> 
>   styx/IP PUSH: No common cipher between server and client. Server 
> data-ciphers: 
> 'CHACHA20-POLY1305:AES-128-GCM:AES-256-GCM:AES-256-CBC:AES-128-CBC', client 
> supports cipher 'BF-CBC'.
> 
>  - Client without OCC:
> 
>styx/IP PUSH:No NCP or OCC cipher data received from peer.
>styx/IP Use --fallback-cipher with the cipher the client is using if you 
> want to allow the client to connect
> 
> In all reject cases on the client:
> 
>AUTH: Received control message: AUTH_FAILED,Data channel cipher 
> negotiation failed (no shared cipher)
> 
> Signed-off-by: Arne Schwabe 
> 
> Patch V2: rename fallback-cipher to data-ciphers-fallback
>   add all corrections from Steffan
>   Ignore occ cipher for clients sending IV_CIPHERS
>   move client side ncp in its own function
>   do not print INSECURE cipher warning if BF-CBC is not allowed
> 
> Signed-off-by: Arne Schwabe 
> ---
>  doc/man-sections/protocol-options.rst |  22 -
>  src/openvpn/crypto.c  |   4 +-
>  src/openvpn/init.c|  18 ++--
>  src/openvpn/multi.c   | 135 --
>  src/openvpn/options.c | 117 +-
>  src/openvpn/options.h |   2 +
>  src/openvpn/ssl.c |  17 ++--
>  src/openvpn/ssl_ncp.c |  82 +---
>  src/openvpn/ssl_ncp.h |  18 ++--
>  tests/unit_tests/openvpn/test_ncp.c   |  26 +++--
>  10 files changed, 311 insertions(+), 130 deletions(-)
> 
> diff --git a/doc/man-sections/protocol-options.rst 
> b/doc/man-sections/protocol-options.rst
> index 051f1d32..69d3bc67 100644
> --- a/doc/man-sections/protocol-options.rst
> +++ b/doc/man-sections/protocol-options.rst
> @@ -57,6 +57,9 @@ configured in a compatible way between both the local and 
> remote side.
>http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
>  
>  --cipher alg
> +  This option is deprecated for server-client mode and ``--data-ciphers``
> +  or rarely `--data-ciphe

Re: [Openvpn-devel] [PATCH v2] Rework NCP compability logic and drop BF-CBC support by default

2020-08-05 Thread tincanteksup




On 05/08/2020 21:25, Steffan Karger wrote:

Hi,

No full review yet, but this version does seem to address my previous
comments. Some minor nits I noticed on my first run through v2:

On 29-07-2020 13:38, Arne Schwabe wrote:

This reworks the NCP logic to be more strict about what is
considered an acceptable result of an NCP negotiation. It also
us to finally drop BF-CBC support by default.

All new behaviour is currently limited to server/client
mode with pull enabled. P2p mode without pull does not change.

New Server behaviour:
- when a client announces its supported ciphers through either
   OCC or IV_CIPHER/IV_NCP we reject the client with a
   AUTH_FAILED message if we have no common cipher.

- When a client does not announce any cipher in either
   OCC or NCP we by reject it unless fallback-cipher is
   specified in either ccd or config.

New client behaviour:
- When no cipher is pushed (or a cipher we refused to support)
   and we also cannot support the server's server announced in
   OCC we fail the connection and log why

- If fallback-cipher is specified we will in the case that
   cipher is missing from occ use the fallback cipher instead
   of failing the connection

Both client and server behaviour:
- We only announce --cipher xyz in occ if we are willing
   to support that cipher.

   It means that we only announce the fallback-cipher if
   it is also contained in --data-ciphers

Compatiblity behaviour:


Compatiblity -> Compatibility




In 2.5 both client and server will automatically set
fallback-cipher xyz if --cipher xyz is in the config and
also add append the cipher to the end of data-ciphers.

We log a warn user about this and point to --data-ciphers and
--fallback-cipher. This also happens if the configuration
contains an explicit --cipher BF-CBC.

If --cipher is not set, we only warn that previous versions
allowed BF-CBC and point how to reenable BF-CBC. This might
break very few config where someone connects a very old
client to a 2.5 server but at some point we need to drop
the BF-CBC and those affected use will already have a the
scary SWEET32 warning in their logs.

In short: If --cipher is explicitly set 2.6 will work the same as
2.4 did. When --cipher is not set, BF-CBC support is dropped and
we warn about it.

Examples how breaking the default BF-CBC will be logged:

Client side:
  - Client connecting to server that does not push cipher but
has --cipher in OCC

 OPTIONS ERROR: failed to negotiate cipher with server.  Add the server's 
cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if 
you want to connect to this server.

  - Client connecting to a server that does not support OCC:

OPTIONS ERROR: failed to negotioate cipher with server. Configure 
--fallback-cipher if you want connect to this server.


negotioate -> *-)





Server Side:

- Server has a client only supporting BF-CBC connecting:

   styx/IP PUSH: No common cipher between server and client. Server 
data-ciphers: 
'CHACHA20-POLY1305:AES-128-GCM:AES-256-GCM:AES-256-CBC:AES-128-CBC', client 
supports cipher 'BF-CBC'.

  - Client without OCC:

styx/IP PUSH:No NCP or OCC cipher data received from peer.
styx/IP Use --fallback-cipher with the cipher the client is using if you 
want to allow the client to connect

In all reject cases on the client:

AUTH: Received control message: AUTH_FAILED,Data channel cipher negotiation 
failed (no shared cipher)

Signed-off-by: Arne Schwabe 

Patch V2: rename fallback-cipher to data-ciphers-fallback
   add all corrections from Steffan
   Ignore occ cipher for clients sending IV_CIPHERS
   move client side ncp in its own function
   do not print INSECURE cipher warning if BF-CBC is not allowed

Signed-off-by: Arne Schwabe 
---
  doc/man-sections/protocol-options.rst |  22 -
  src/openvpn/crypto.c  |   4 +-
  src/openvpn/init.c|  18 ++--
  src/openvpn/multi.c   | 135 --
  src/openvpn/options.c | 117 +-
  src/openvpn/options.h |   2 +
  src/openvpn/ssl.c |  17 ++--
  src/openvpn/ssl_ncp.c |  82 +---
  src/openvpn/ssl_ncp.h |  18 ++--
  tests/unit_tests/openvpn/test_ncp.c   |  26 +++--
  10 files changed, 311 insertions(+), 130 deletions(-)



No other spelling or grammar to worry about.















diff --git a/doc/man-sections/protocol-options.rst 
b/doc/man-sections/protocol-options.rst
index 051f1d32..69d3bc67 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -57,6 +57,9 @@ configured in a compatible way between both the local and 
remote side.
http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
  
  --cipher alg

+  This option is deprecated for server-client mode and ``--data-ciphers``
+  or rarely `--data-ciphers-fallback`` should be used inste