Hi,
On 22/06/2020 16:02, Arne Schwabe wrote:
[CUT]
> @@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const
> char *profile)
> }
> }
>
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> + ASSERT(ctx);
> + struct gc_arena gc = gc_new();
> +
> + /* Get number of groups and allocate an array in ctx */
> + int groups_count = get_num_elements(groups, ':');
> + ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> + /* Parse allowed ciphers, getting IDs */
> + int i = 0;
> + char *tmp_groups = string_alloc(groups, &gc);
> +
> + const char *token;
> + while ((token = strsep(&tmp_groups, ":")))
> + {
> + const mbedtls_ecp_curve_info *ci =
> + mbedtls_ecp_curve_info_from_name(token);
> + if (!ci)
> + {
> + msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> + }
> + else
> + {
> + ctx->groups[i] = ci->grp_id;
> + i++;
> + }
> + token = strsep(&tmp_groups, ":");
The line above should be removed, otherwise end up doing strsep() twice
in a row.
> + }
> + ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
> +
> + gc_free(&gc);
> +}
> +
> +
> void
> tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
> {
> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
> mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config,
> ssl_ctx->allowed_ciphers);
> }
>
> + if (ssl_ctx->groups)
> + {
> + mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
> + }
> +
> /* Disable record splitting (for now). OpenVPN assumes records are sent
> * unfragmented, and changing that will require thorough review and
> * testing. Since OpenVPN is not susceptible to BEAST, we can just
> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
> index 92381f1a..0525134f 100644
> --- a/src/openvpn/ssl_mbedtls.h
> +++ b/src/openvpn/ssl_mbedtls.h
> @@ -105,6 +105,7 @@ struct tls_root_ctx {
> #endif
> struct external_context external_key; /**< External key context */
> int *allowed_ciphers; /**< List of allowed ciphers for this
> connection */
> + mbedtls_ecp_group_id *groups; /**< List of allowed groups for this
> connection */
> mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
> };
>
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index a489053b..da7d252a 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -565,6 +565,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const
> char *profile)
> #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
> }
>
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> + ASSERT(ctx);
> + struct gc_arena gc = gc_new();
> + /* This method could be as easy as
> + * SSL_CTX_set1_groups_list(ctx->ctx, groups)
> + * but OpenSSL does not like the name secp256r1 for prime256v1
> + * This is one of the important curves.
> + * To support the same name for OpenSSL and mbedTLS, we do
> + * this dance.
> + */
> +
> + int groups_count = get_num_elements(groups, ':');
> +
> + int *glist;
> + /* Allocate an array for them */
> + ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
> +
> + /* Parse allowed ciphers, getting IDs */
> + int glistlen = 0;
> + char *tmp_groups = string_alloc(groups, &gc);
> +
> + const char *token;
> + while ((token = strsep(&tmp_groups, ":")))
> + {
> + if (streq(token, "secp256r1"))
> + {
> + token = "prime256v1";
> + }
> + int nid = OBJ_sn2nid(token);
> +
> + if (nid == 0)
>From a style perspective, I think it looks better to add an empty line
*before* "int nid =..." and remove the one after.
This way we also follow the same pattern:
x = a();
if (x ..)
{
}
as other parts of the code.
The rest looks good to me.
Regards,
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
