[PATCH 4/4] crypto: add tracing & warning about GNUTLS countermeasures

Daniel P. Berrangé posted 4 patches 3 months, 4 weeks ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
[PATCH 4/4] crypto: add tracing & warning about GNUTLS countermeasures
Posted by Daniel P. Berrangé 3 months, 4 weeks ago
We want some visibility on stderr when the GNUTLS thread
safety countermeasures are activated, to encourage people
to get the real fix deployed (once it exists). Some trace
points will also help if we see any further wierd crash
scenario we've not anticipated.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlssession.c | 10 ++++++++++
 crypto/trace-events |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 939f69bdb3..246cd6f7c0 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -615,10 +615,20 @@ qcrypto_tls_session_handshake(QCryptoTLSSession *session,
          * only have to protect against automatic rekeying
          * which doesn't trigger with CHACHA20
          */
+        trace_qcrypto_tls_session_parameters(
+            session,
+            session->requireThreadSafety,
+            gnutls_protocol_get_version(session->handle),
+            cipher);
+
         if (session->requireThreadSafety &&
             gnutls_protocol_get_version(session->handle) ==
             GNUTLS_TLS1_3 &&
             cipher != GNUTLS_CIPHER_CHACHA20_POLY1305) {
+            warn_report("WARNING: activating thread safety countermeasures "
+                        "for potentially broken GNUTLS with TLS1.3 cipher=%d",
+                        cipher);
+            trace_qcrypto_tls_session_bug1717_workaround(session);
             session->lockEnabled = true;
         }
 #endif
diff --git a/crypto/trace-events b/crypto/trace-events
index bccd0bbf29..d0e33427fa 100644
--- a/crypto/trace-events
+++ b/crypto/trace-events
@@ -21,6 +21,8 @@ qcrypto_tls_creds_x509_load_cert_list(void *creds, const char *file) "TLS creds
 # tlssession.c
 qcrypto_tls_session_new(void *session, void *creds, const char *hostname, const char *authzid, int endpoint) "TLS session new session=%p creds=%p hostname=%s authzid=%s endpoint=%d"
 qcrypto_tls_session_check_creds(void *session, const char *status) "TLS session check creds session=%p status=%s"
+qcrypto_tls_session_parameters(void *session, int threadSafety, int protocol, int cipher) "TLS session parameters session=%p threadSafety=%d protocol=%d cipher=%d"
+qcrypto_tls_session_bug1717_workaround(void *session) "TLS session bug1717 workaround session=%p"
 
 # tls-cipher-suites.c
 qcrypto_tls_cipher_suite_priority(const char *name) "priority: %s"
-- 
2.50.1


Re: [PATCH 4/4] crypto: add tracing & warning about GNUTLS countermeasures
Posted by Fabiano Rosas 3 months, 3 weeks ago
Daniel P. Berrangé <berrange@redhat.com> writes:

> We want some visibility on stderr when the GNUTLS thread
> safety countermeasures are activated, to encourage people
> to get the real fix deployed (once it exists). Some trace
> points will also help if we see any further wierd crash
> scenario we've not anticipated.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  crypto/tlssession.c | 10 ++++++++++
>  crypto/trace-events |  2 ++
>  2 files changed, 12 insertions(+)
>
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index 939f69bdb3..246cd6f7c0 100644
> --- a/crypto/tlssession.c
> +++ b/crypto/tlssession.c
> @@ -615,10 +615,20 @@ qcrypto_tls_session_handshake(QCryptoTLSSession *session,
>           * only have to protect against automatic rekeying
>           * which doesn't trigger with CHACHA20
>           */
> +        trace_qcrypto_tls_session_parameters(
> +            session,
> +            session->requireThreadSafety,
> +            gnutls_protocol_get_version(session->handle),
> +            cipher);
> +
>          if (session->requireThreadSafety &&
>              gnutls_protocol_get_version(session->handle) ==
>              GNUTLS_TLS1_3 &&
>              cipher != GNUTLS_CIPHER_CHACHA20_POLY1305) {
> +            warn_report("WARNING: activating thread safety countermeasures "

And this hit the missing error-report.h weirdness.

> +                        "for potentially broken GNUTLS with TLS1.3 cipher=%d",
> +                        cipher);
> +            trace_qcrypto_tls_session_bug1717_workaround(session);
>              session->lockEnabled = true;
>          }
>  #endif
> diff --git a/crypto/trace-events b/crypto/trace-events
> index bccd0bbf29..d0e33427fa 100644
> --- a/crypto/trace-events
> +++ b/crypto/trace-events
> @@ -21,6 +21,8 @@ qcrypto_tls_creds_x509_load_cert_list(void *creds, const char *file) "TLS creds
>  # tlssession.c
>  qcrypto_tls_session_new(void *session, void *creds, const char *hostname, const char *authzid, int endpoint) "TLS session new session=%p creds=%p hostname=%s authzid=%s endpoint=%d"
>  qcrypto_tls_session_check_creds(void *session, const char *status) "TLS session check creds session=%p status=%s"
> +qcrypto_tls_session_parameters(void *session, int threadSafety, int protocol, int cipher) "TLS session parameters session=%p threadSafety=%d protocol=%d cipher=%d"
> +qcrypto_tls_session_bug1717_workaround(void *session) "TLS session bug1717 workaround session=%p"
>  
>  # tls-cipher-suites.c
>  qcrypto_tls_cipher_suite_priority(const char *name) "priority: %s"
Re: [PATCH 4/4] crypto: add tracing & warning about GNUTLS countermeasures
Posted by Fabiano Rosas 3 months, 3 weeks ago
Daniel P. Berrangé <berrange@redhat.com> writes:

> We want some visibility on stderr when the GNUTLS thread
> safety countermeasures are activated, to encourage people
> to get the real fix deployed (once it exists). Some trace
> points will also help if we see any further wierd crash
> scenario we've not anticipated.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>