[PATCH 2/2] virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()

[Daniel P. Berrangé]

On Tue, Dec 21, 2021 at 03:22:59PM +0100, Michal Privoznik wrote:
> As encryption norms get more strict it's easy to fall on the
> insecure side. For instance, so far we are generating 2048 bits
> long prime for Diffie-Hellman keys. Some systems consider this
> not long enough. While we may just keep increasing the value
> passed to the corresponding gnutls_* function, that is not well
> maintainable. Instead, we may do what's recommended in the
> gnutls_* manpage. From gnutls_dh_params_generate2(3):
> 
>   It is recommended not to set the number of bits directly, but
>   use gnutls_sec_param_to_pk_bits() instead.
> 
> Looking into the gnutls_sec_param_to_pk_bits() then [1], 2048
> bits corresponds to parameter MEDIUM. Therefore, we want to chose
> the next size (HIGH) to be future proof.
> 
> 1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/rpc/virnettlscontext.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index 3b6687e7f4..f0b1e8f9c1 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -38,8 +38,6 @@
>  #include "virthread.h"
>  #include "configmake.h"
>  
> -#define DH_BITS 2048
> -
>  #define LIBVIRT_PKI_DIR SYSCONFDIR "/pki"
>  #define LIBVIRT_CACERT LIBVIRT_PKI_DIR "/CA/cacert.pem"
>  #define LIBVIRT_CACRL LIBVIRT_PKI_DIR "/CA/cacrl.pem"
> @@ -718,6 +716,15 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
>       * security requirements.
>       */
>      if (isServer) {
> +        unsigned int bits = 0;
> +
> +        bits = gnutls_sec_param_to_pk_bits(GNUTLS_PK_DH, GNUTLS_SEC_PARAM_HIGH);
> +        if (bits == 0) {
> +            virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> +                           _("Unable to get key length for diffie-hellman parameters"));
> +            goto error;
> +        }
> +
>          err = gnutls_dh_params_init(&ctxt->dhParams);
>          if (err < 0) {
>              virReportError(VIR_ERR_SYSTEM_ERROR,
> @@ -725,7 +732,7 @@ static virNetTLSContext *virNetTLSContextNew(const char *cacert,
>                             gnutls_strerror(err));
>              goto error;
>          }
> -        err = gnutls_dh_params_generate2(ctxt->dhParams, DH_BITS);
> +        err = gnutls_dh_params_generate2(ctxt->dhParams, bits);
>          if (err < 0) {
>              virReportError(VIR_ERR_SYSTEM_ERROR,
>                             _("Unable to generate diffie-hellman parameters: %s"),

We shouldn't be introducing use of gnutls_sec_param_to_pk_bits at
all IMHO, rather we should be removing use of gnutls_dh_params_generate2
instead.

The recommendation is to use pre-generated DH parameters from the
the FFDHE set of RFC7919.

In gnutls >= 3.6.0 this happens automatically.

In gnutls >= 3.5.6 && < 3.6.0 we can replace thegnutls_dh_params_generate2 +
gnutls_certificate_set_dh_params pair of calls, with just
gnutls_certificate_set_known_dh_params()

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|