Hi Frode,
Thanks for your patch.
Please see my comments below.
On Wed, Aug 25, 2021 at 01:05:13PM +0200, Frode Nordahl wrote:
> Contrary to what is stated in the documentation, when SSL
> ciphers or protocols options are omitted the default values
> will not be set. The SSL library default settings will be used
> instead.
>
> Fix handling of default ciphers and protocols so that we actually
> enforce what is listed as defaults.
>
> Fixes: e18a1d086133 ("Add support for specifying SSL connection parameters to
> ovsdb")
> Signed-off-by: Frode Nordahl <frode.nord...@canonical.com>
> ---
> lib/stream-ssl.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> index 0ea3f2c08..6b4cf6970 100644
> --- a/lib/stream-ssl.c
> +++ b/lib/stream-ssl.c
> @@ -162,8 +162,10 @@ struct ssl_config_file {
> static struct ssl_config_file private_key;
> static struct ssl_config_file certificate;
> static struct ssl_config_file ca_cert;
> -static char *ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> -static char *ssl_ciphers = "HIGH:!aNULL:!MD5";
> +static char *default_ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> +static char *default_ssl_ciphers = "HIGH:!aNULL:!MD5";
> +static char *ssl_protocols = NULL;
> +static char *ssl_ciphers = NULL;
>
> /* Ordinarily, the SSL client and server verify each other's certificates
> using
> * a CA certificate. Setting this to false disables this behavior. (This
> is a
> @@ -1225,14 +1227,19 @@ stream_ssl_set_key_and_cert(const char
> *private_key_file,
> void
> stream_ssl_set_ciphers(const char *arg)
> {
> - if (ssl_init() || !arg || !strcmp(ssl_ciphers, arg)) {
The ssl_init() calls at least one time do_ssl_init() which then
calls SSL_CTX_set_cipher_list(ctx, "HIGH:!aNULL:!MD5").
Those are the defaults in the man-page and not from the library.
The do_ssl_init() also does:
method = CONST_CAST(SSL_METHOD *, SSLv23_method());
That should return SSLv3, TLSv1, TLSv1.1 and TLS1.2.
ctx = SSL_CTX_new(method);
SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
And there it excludes those SSL v2 and v3.
Therefore, the default would be "TLSv1,TLSv1.1,TLSv1.2" which is
the same in the man-page.
Did I miss something?
Thanks
fbl
> + const char *input = arg ? arg : default_ssl_ciphers;
> +
> + if (ssl_init() || !input || (ssl_ciphers && !strcmp(ssl_ciphers,
> input))) {
> return;
> }
> - if (SSL_CTX_set_cipher_list(ctx,arg) == 0) {
> + if (SSL_CTX_set_cipher_list(ctx, input) == 0) {
> VLOG_ERR("SSL_CTX_set_cipher_list: %s",
> ERR_error_string(ERR_get_error(), NULL));
> }
> - ssl_ciphers = xstrdup(arg);
> + if (ssl_ciphers) {
> + free(ssl_ciphers);
> + }
> + ssl_ciphers = xstrdup(input);
> }
>
> /* Set SSL protocols based on the string input. Aborts with an error message
> @@ -1240,7 +1247,11 @@ stream_ssl_set_ciphers(const char *arg)
> void
> stream_ssl_set_protocols(const char *arg)
> {
> - if (ssl_init() || !arg || !strcmp(arg, ssl_protocols)){
> + const char *input = arg ? arg : default_ssl_protocols;
> +
> + if (ssl_init() || !input
> + || (ssl_protocols && !strcmp(input, ssl_protocols)))
> + {
> return;
> }
>
> @@ -1253,7 +1264,7 @@ stream_ssl_set_protocols(const char *arg)
> #endif
> long protocol_flags = SSL_OP_NO_SSL_MASK;
>
> - char *s = xstrdup(arg);
> + char *s = xstrdup(input);
> char *save_ptr = NULL;
> char *word = strtok_r(s, " ,\t", &save_ptr);
> if (word == NULL) {
> @@ -1281,7 +1292,10 @@ stream_ssl_set_protocols(const char *arg)
> /* Set the actual options. */
> SSL_CTX_set_options(ctx, protocol_flags);
>
> - ssl_protocols = xstrdup(arg);
> + if (ssl_protocols) {
> + free(ssl_protocols);
> + }
> + ssl_protocols = xstrdup(input);
>
> exit:
> free(s);
> --
> 2.32.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
