Hi,
On 23/09/2019 15:32, Arne Schwabe wrote:
> We currently always announce IV_NCP=2 when we support these ciphers even
> when we do not accept them. This lead to a server pushing a AES-GCM-128
> cipher to clients and the client then rejecting it.
> ---
> src/openvpn/init.c | 1 +
> src/openvpn/openvpn.h | 1 +
> src/openvpn/options.c | 7 +++++++
> src/openvpn/ssl.c | 4 +++-
> src/openvpn/ssl_common.h | 1 +
> 5 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index b5a034dc..32f7bc7a 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2795,6 +2795,7 @@ do_init_crypto_tls(struct context *c, const unsigned
> int flags)
> to.tcp_mode = link_socket_proto_connection_oriented(options->ce.proto);
> to.config_ciphername = c->c1.ciphername;
> to.config_authname = c->c1.authname;
> + to.config_ncp_ciphers = c->c1.ncp_ciphers;
I can't find where config_ncp_ciphers is used and I can't find where
ncp_ciphers is set...something is missing?
> to.ncp_enabled = options->ncp_enabled;
> to.transition_window = options->transition_window;
> to.handshake_window = options->handshake_window;
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index 29d21f0a..1fdbfe3c 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -208,6 +208,7 @@ struct context_1
>
> const char *ciphername; /**< Data channel cipher from config file */
> const char *authname; /**< Data channel auth from config file */
> + const char *ncp_ciphers; /**< NCP Ciphers */
> int keysize; /**< Data channel keysize from config file */
> #endif
> };
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index c84b9d5e..cb25db5b 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -7687,6 +7687,13 @@ add_option(struct options *options,
> {
> VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
> options->ncp_ciphers = p[1];
> +
> + if (!(tls_item_in_cipher_list("AES-128-GCM", options->ncp_ciphers)
> + && tls_item_in_cipher_list("AES-256-GCM",
> options->ncp_ciphers)))
> + {
> + msg(M_INFO, "Not including AES-128-GCM and AES-256-GCM in
> ncp-ciphers "
> + "disables announcing NCP support with IV_NCP=2");
> + }
The condition and the text message are not agreeing with each other.
How about making the if condition easier to read by applying De Morgan's
law:
if (!tls_item_in_cipher_list("AES-128-GCM", options->ncp_ciphers)
|| !tls_item_in_cipher_list("AES-256-GCM", options->ncp_ciphers)))
This reads to me as: "Not enabling AES-128 or AES-256 disables NCP"
(Which I think is what you wanted to express in the debug message?)
What do you think?
> }
> else if (streq(p[0], "ncp-disable") && !p[1])
> {
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index d5833ac6..f1719303 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2327,7 +2327,9 @@ push_peer_info(struct buffer *buf, struct tls_session
> *session)
>
> /* support for Negotiable Crypto Parameters */
> if (session->opt->ncp_enabled
> - && (session->opt->mode == MODE_SERVER || session->opt->pull))
> + && (session->opt->mode == MODE_SERVER || session->opt->pull)
> + && tls_item_in_cipher_list("AES-128-GCM",
> session->opt->config_ncp_ciphers)
> + && tls_item_in_cipher_list("AES-256-GCM",
> session->opt->config_ncp_ciphers))
> {
> buf_printf(&out, "IV_NCP=2\n");
> }
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 0312c1f8..133c3c22 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -290,6 +290,7 @@ struct tls_options
>
> const char *config_ciphername;
> const char *config_authname;
> + const char *config_ncp_ciphers;
> bool ncp_enabled;
>
> bool tls_crypt_v2;
>
Cheers,
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
