[ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols

Flavio Leitner fbl at sysclose.org
Sat Sep 11 20:23:42 UTC 2021


On Fri, Sep 10, 2021 at 06:20:45PM +0200, Frode Nordahl wrote:
> On Thu, Sep 9, 2021 at 9:53 PM Flavio Leitner <fbl at sysclose.org> wrote:
> >
> >
> > Hi Frode,
> >
> > Thanks for your patch.
> > Please see my comments below.
> 
> Flavio, thank you for taking the time to review.
> 
> > 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.nordahl at 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?
> 
> Thank you for pointing out that, I did not realize we manipulated
> these options multiple places.
> 
> I do need to rephrase the commit message, but there is still a problem
> here. It became apparent when working on the next patch in the series,
> where functional tests behave unexpectedly when passing the
> ssl-protocols options. The reason being that when the protocol list
> matches what is already in the static char *ssl_protocols in
> lib/stream-ssl.c stream_ssl_set_protocols returns early without
> setting any option.

If that matches then it is because the default is set, so it
shouldn't make any difference to return early. Do you still
have the next patch without the default_ssl_protocols change
for me to take a look? That might help me to see the issue.

> So I guess the question then becomes, should we stop doing this
> multiple places or do you want me to update the call to
> SSL_CTX_set_options in do_ssl_init and not introduce this change?

Not sure yet because I didn't see the problem yet.

Thanks,
fbl

> 
> -- 
> Frode Nordahl
> 
> > 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
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > --
> > fbl

-- 
fbl


More information about the dev mailing list