[PATCH 0/2] gnutls: Be more clever about DH key size
See 2/2 for explanation. Ideally, we wouldn't use gnutls_dh_params_generate2() at all, per [1]. But that would require bumping minimal required version to gnutls-3.6.0 and I'm not sure how available it is in OSes we support. Therefore, for now let's stick with patch 2/2. 1: https://www.gnutls.org/manual/html_node/Parameter-generation.html Michal Prívozník (2): virnettlscontext: Drop gnutls_dh_set_prime_bits() virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2() src/rpc/virnettlscontext.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) -- 2.32.0
According to the gnutls_dh_set_prime_bits() manpage: The function has no effect in server side. Therefore, don't call it when creating server side context. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnettlscontext.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 1a3dd92676..3b6687e7f4 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -1233,8 +1233,6 @@ virNetTLSSession *virNetTLSSessionNew(virNetTLSContext *ctxt, */ if (ctxt->isServer) { gnutls_certificate_server_set_request(sess->session, GNUTLS_CERT_REQUEST); - - gnutls_dh_set_prime_bits(sess->session, DH_BITS); } gnutls_transport_set_ptr(sess->session, sess); -- 2.32.0
On Tue, 21 Dec 2021, Michal Privoznik wrote:
According to the gnutls_dh_set_prime_bits() manpage:
The function has no effect in server side.
Therefore, don't call it when creating server side context.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
--- src/rpc/virnettlscontext.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 1a3dd92676..3b6687e7f4 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -1233,8 +1233,6 @@ virNetTLSSession *virNetTLSSessionNew(virNetTLSContext *ctxt, */ if (ctxt->isServer) { gnutls_certificate_server_set_request(sess->session, GNUTLS_CERT_REQUEST); - - gnutls_dh_set_prime_bits(sess->session, DH_BITS); }
gnutls_transport_set_ptr(sess->session, sess); -- 2.32.0
On a Tuesday in 2021, Michal Privoznik wrote:
According to the gnutls_dh_set_prime_bits() manpage:
The function has no effect in server side.
On top of that, it is deprecated as of 3.1.7 while our minimum version is 3.2.
Therefore, don't call it when creating server side context.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnettlscontext.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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"), -- 2.32.0
On Tue, 21 Dec 2021, 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@redhat.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
--- 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"), -- 2.32.0
On a Tuesday in 2021, 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
Is there a promise gnutls will increase those in the future?
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.
IMO this patch should use MEDIUM and the bump should be separate.
1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnettlscontext.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, Dec 22, 2021 at 02:12:37PM +0100, Ján Tomko wrote:
On a Tuesday in 2021, 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
Is there a promise gnutls will increase those in the future?
That is the whole point of the levels that they are not fixed and also that distributions can select theirs. And the fact that you can have one option for the whole system. Which makes me realise that we should actually try that and not pick a level ourselves. Having said that I should also add that I do not know how to do that. Maybe use "SYSTEM" or "DEFAULT"?
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.
IMO this patch should use MEDIUM and the bump should be separate.
1: https://www.gnutls.org/manual/gnutls.html#tab_003akey_002dsizes
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/rpc/virnettlscontext.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
On 12/22/21 14:12, Ján Tomko wrote:
On a Tuesday in 2021, 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
Is there a promise gnutls will increase those in the future?
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.
IMO this patch should use MEDIUM and the bump should be separate.
Good point, let me merge this with MEDIUM and post switch to HIGH in a separate patch. Michal
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@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 :|
On 1/4/22 12:59, Daniel P. Berrangé wrote:
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()
Fair enough, I don't know enough about gnutls, but let me see if I can cook a patch. Michal
On Tue, Dec 21, 2021 at 03:22:57PM +0100, Michal Privoznik wrote:
See 2/2 for explanation.
Ideally, we wouldn't use gnutls_dh_params_generate2() at all, per [1]. But that would require bumping minimal required version to gnutls-3.6.0 and I'm not sure how available it is in OSes we support. Therefore, for
As far as I can tell from repology.org all the major distros have 3.6.x in more than one version and definitely all those that we have in the CI, so I'd say bump that.
now let's stick with patch 2/2.
1: https://www.gnutls.org/manual/html_node/Parameter-generation.html
Michal Prívozník (2): virnettlscontext: Drop gnutls_dh_set_prime_bits() virnettlscontext: Don't pass static key length to gnutls_dh_params_generate2()
src/rpc/virnettlscontext.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
-- 2.32.0
On a Wednesday in 2021, Martin Kletzander wrote:
On Tue, Dec 21, 2021 at 03:22:57PM +0100, Michal Privoznik wrote:
See 2/2 for explanation.
Ideally, we wouldn't use gnutls_dh_params_generate2() at all, per [1]. But that would require bumping minimal required version to gnutls-3.6.0 and I'm not sure how available it is in OSes we support. Therefore, for
As far as I can tell from repology.org all the major distros have 3.6.x in more than one version and definitely all those that we have in the CI, so I'd say bump that.
There's Ubuntu 18.04 with 3.5.18. But we could #ifndef the old code out and use the pre-generated parameters on every other distro, as recommended. Jano
On Wed, Dec 22, 2021 at 02:14:59PM +0100, Ján Tomko wrote:
On a Wednesday in 2021, Martin Kletzander wrote:
On Tue, Dec 21, 2021 at 03:22:57PM +0100, Michal Privoznik wrote:
See 2/2 for explanation.
Ideally, we wouldn't use gnutls_dh_params_generate2() at all, per [1]. But that would require bumping minimal required version to gnutls-3.6.0 and I'm not sure how available it is in OSes we support. Therefore, for
As far as I can tell from repology.org all the major distros have 3.6.x in more than one version and definitely all those that we have in the CI, so I'd say bump that.
There's Ubuntu 18.04 with 3.5.18.
And we consider only LTS, so we can drop that in April when 20.04 is out for 2 years. I finally found the exact spelling in docs/platform.rst (available online at https://libvirt.org/platforms.html as well) which I always struggle to find.
But we could #ifndef the old code out and use the pre-generated parameters on every other distro, as recommended.
Since counting the bits is so discouraged I would also prefer this option with the hopes for us remembering to remove that. Actually, can we have like a commit hook that would check current date against some file in the repository and just let us know that there might be something to remove? O:-)
Jano
participants (6)
-
Ani Sinha -
Daniel P. Berrangé -
Ján Tomko -
Martin Kletzander -
Michal Privoznik -
Michal Prívozník