diff mbox series

[1/3] crypto: Add qcrypto_tls_shutdown()

Message ID 20200327161936.2225989-2-eblake@redhat.com
State New
Headers show
Series nbd: Try for cleaner TLS shutdown | expand

Commit Message

Eric Blake March 27, 2020, 4:19 p.m. UTC
Gnutls documents that applications that want to distinguish between a
clean end-of-communication and a malicious client abruptly tearing the
underlying transport out of under our feet need to use gnutls_bye().
Our channel code is already set up to allow shutdown requests, but we
weren't forwarding those to gnutls.  To make that work, we first need
a new entry point that can isolate the rest of our code from the
gnutls interface.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/crypto.json            | 15 +++++++++++++++
 include/crypto/tlssession.h | 24 ++++++++++++++++++++++++
 crypto/tlssession.c         | 27 +++++++++++++++++++++++++++
 3 files changed, 66 insertions(+)

Comments

Markus Armbruster March 31, 2020, 8:30 a.m. UTC | #1
Eric Blake <eblake@redhat.com> writes:

> Gnutls documents that applications that want to distinguish between a
> clean end-of-communication and a malicious client abruptly tearing the
> underlying transport out of under our feet need to use gnutls_bye().
> Our channel code is already set up to allow shutdown requests, but we
> weren't forwarding those to gnutls.  To make that work, we first need
> a new entry point that can isolate the rest of our code from the
> gnutls interface.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/crypto.json            | 15 +++++++++++++++
>  include/crypto/tlssession.h | 24 ++++++++++++++++++++++++
>  crypto/tlssession.c         | 27 +++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+)
>
> diff --git a/qapi/crypto.json b/qapi/crypto.json
> index b2a4cff683ff..1df0f4502885 100644
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -119,6 +119,21 @@
>    'prefix': 'QCRYPTO_IVGEN_ALG',
>    'data': ['plain', 'plain64', 'essiv']}
>
> +##
> +# @QCryptoShutdownMode:
> +#
> +# The supported modes for requesting shutdown of a crypto
> +# communication channel.
> +#
> +# @shut-wr: No more writes will be sent, but the remote end can still send
> +#           data to be read.
> +# @shut-rdwr: No more reads or writes should occur.
> +# Since: 5.1
> +##
> +{ 'enum': 'QCryptoShutdownMode',
> +  'prefix': 'QCRYPTO',
> +  'data': ['shut-wr', 'shut-rdwr']}
> +
>  ##
>  # @QCryptoBlockFormat:
>  #
> diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> index 15b9cef086cc..10c670e3b6a2 100644
> --- a/include/crypto/tlssession.h
> +++ b/include/crypto/tlssession.h
> @@ -321,4 +321,28 @@ int qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess,
>   */
>  char *qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess);
>
> +/**
> + * qcrypto_tls_shutdown:
> + * @sess: the TLS session object
> + * @how: the desired shutdown mode
> + *
> + * Prepare to terminate the session.  If @how is QCRYPTO_SHUT_WR, this
> + * side will no longer write data, but should still process reads
> + * until EOF; if @how is QCRYPTO_SHUT_RDWR, then the entire session
> + * should shut down.  Use of this function is optional, since closing
> + * the session implies QCRYPTO_SHUT_RDWR.  However, using this
> + * function prior to terminating the underlying transport layer makes
> + * it possible for the remote endpoint to distinguish between a
> + * malicious party prematurely terminating the the connection and
> + * normal termination.
> + *
> + * This function should only be used after a successful
> + * qcrypto_tls_session_handshake().
> + *
> + * Returns: 0 for success, or -EAGAIN if more underlying I/O is
> + * required to finish proper session shutdown.
> + */
> +int qcrypto_tls_session_shutdown(QCryptoTLSSession *sess,
> +                                 QCryptoShutdownMode how);
> +
>  #endif /* QCRYPTO_TLSSESSION_H */
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index 33203e8ca711..903301189069 100644
> --- a/crypto/tlssession.c
> +++ b/crypto/tlssession.c
> @@ -521,6 +521,33 @@ qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session)
>  }
>
>
> +int qcrypto_tls_session_shutdown(QCryptoTLSSession *session,
> +                                 QCryptoShutdownMode how)
> +{
> +    gnutls_close_request_t mode;
> +    int ret;
> +
> +    assert(session->handshakeComplete);

Suggest a blank line here.

> +    switch (how) {
> +    case QCRYPTO_SHUT_WR:
> +        mode = GNUTLS_SHUT_WR;
> +        break;
> +    case QCRYPTO_SHUT_RDWR:
> +        mode = GNUTLS_SHUT_RDWR;
> +        break;
> +    default:
> +        abort();
> +    }
> +
> +    ret = gnutls_bye(session->handle, mode);
> +    if (ret == GNUTLS_E_INTERRUPTED ||
> +        ret == GNUTLS_E_AGAIN) {
> +        return -EAGAIN;
> +    }
> +    return 0;
> +}
> +
> +
>  int
>  qcrypto_tls_session_get_key_size(QCryptoTLSSession *session,
>                                   Error **errp)

This is a thin wrapper around gnutls_bye().  I understand this is an
abstraction layer backed by GnuTLS.  Not sure abstracting from just one
concrete thing is a good idea, but that's way out of scope here.

In scope: why do you need QCryptoShutdownMode be a QAPI type?
Eric Blake March 31, 2020, 3:17 p.m. UTC | #2
On 3/31/20 3:30 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Gnutls documents that applications that want to distinguish between a
>> clean end-of-communication and a malicious client abruptly tearing the
>> underlying transport out of under our feet need to use gnutls_bye().
>> Our channel code is already set up to allow shutdown requests, but we
>> weren't forwarding those to gnutls.  To make that work, we first need
>> a new entry point that can isolate the rest of our code from the
>> gnutls interface.
>>

>> +int qcrypto_tls_session_shutdown(QCryptoTLSSession *session,
>> +                                 QCryptoShutdownMode how)

> 
> This is a thin wrapper around gnutls_bye().  I understand this is an
> abstraction layer backed by GnuTLS.  Not sure abstracting from just one
> concrete thing is a good idea, but that's way out of scope here.

If we ever add an alternative TLS implementation to gnutls, then the 
abstraction is useful.  But I'm not sure how likely that is, so maybe 
Dan has more insight why he chose this design originally.

> 
> In scope: why do you need QCryptoShutdownMode be a QAPI type?

I don't, other than the fact that other TLS parameters were also QAPI 
types (such as QCryptoTLSCredsEndpoint).

But that may be moot, as Dan argued that this series adds more 
complexity than it is worth (I originally wrote it while trying to debug 
an nbdkit bug; but in the meantime, I have fixed the nbdkit bug without 
any change to qemu behavior).  So at this point, I will probably not be 
posting a v2 of this series.
Daniel P. Berrangé March 31, 2020, 3:33 p.m. UTC | #3
On Tue, Mar 31, 2020 at 10:17:49AM -0500, Eric Blake wrote:
> On 3/31/20 3:30 AM, Markus Armbruster wrote:
> > Eric Blake <eblake@redhat.com> writes:
> > 
> > > Gnutls documents that applications that want to distinguish between a
> > > clean end-of-communication and a malicious client abruptly tearing the
> > > underlying transport out of under our feet need to use gnutls_bye().
> > > Our channel code is already set up to allow shutdown requests, but we
> > > weren't forwarding those to gnutls.  To make that work, we first need
> > > a new entry point that can isolate the rest of our code from the
> > > gnutls interface.
> > > 
> 
> > > +int qcrypto_tls_session_shutdown(QCryptoTLSSession *session,
> > > +                                 QCryptoShutdownMode how)
> 
> > 
> > This is a thin wrapper around gnutls_bye().  I understand this is an
> > abstraction layer backed by GnuTLS.  Not sure abstracting from just one
> > concrete thing is a good idea, but that's way out of scope here.
> 
> If we ever add an alternative TLS implementation to gnutls, then the
> abstraction is useful.  But I'm not sure how likely that is, so maybe Dan
> has more insight why he chose this design originally.

The abstraction serves several purposes.

First, it means that we don't need #ifdefs wrt GNUTLS in every piece of
QEMU code that involves TLS. They are isolated in the crypto/ code only.

Related to that, it means that anything that touches GNUTLS APIs directly
gets funnelled via the crypto maintainer for review.

It is easy to mis-use many of the GNUTLS APIs, and so the abstraction
serves to apply/enforce a more desirable usage policy on the rest of
the QEMU code, making it harder to screw up TLS.

Much of this is based on learning from libvirt code where the usage of
GNUTLS was not nearly so well encapsulated and increased burden.

Regards,
Daniel
diff mbox series

Patch

diff --git a/qapi/crypto.json b/qapi/crypto.json
index b2a4cff683ff..1df0f4502885 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -119,6 +119,21 @@ 
   'prefix': 'QCRYPTO_IVGEN_ALG',
   'data': ['plain', 'plain64', 'essiv']}

+##
+# @QCryptoShutdownMode:
+#
+# The supported modes for requesting shutdown of a crypto
+# communication channel.
+#
+# @shut-wr: No more writes will be sent, but the remote end can still send
+#           data to be read.
+# @shut-rdwr: No more reads or writes should occur.
+# Since: 5.1
+##
+{ 'enum': 'QCryptoShutdownMode',
+  'prefix': 'QCRYPTO',
+  'data': ['shut-wr', 'shut-rdwr']}
+
 ##
 # @QCryptoBlockFormat:
 #
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 15b9cef086cc..10c670e3b6a2 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -321,4 +321,28 @@  int qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess,
  */
 char *qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess);

+/**
+ * qcrypto_tls_shutdown:
+ * @sess: the TLS session object
+ * @how: the desired shutdown mode
+ *
+ * Prepare to terminate the session.  If @how is QCRYPTO_SHUT_WR, this
+ * side will no longer write data, but should still process reads
+ * until EOF; if @how is QCRYPTO_SHUT_RDWR, then the entire session
+ * should shut down.  Use of this function is optional, since closing
+ * the session implies QCRYPTO_SHUT_RDWR.  However, using this
+ * function prior to terminating the underlying transport layer makes
+ * it possible for the remote endpoint to distinguish between a
+ * malicious party prematurely terminating the the connection and
+ * normal termination.
+ *
+ * This function should only be used after a successful
+ * qcrypto_tls_session_handshake().
+ *
+ * Returns: 0 for success, or -EAGAIN if more underlying I/O is
+ * required to finish proper session shutdown.
+ */
+int qcrypto_tls_session_shutdown(QCryptoTLSSession *sess,
+                                 QCryptoShutdownMode how);
+
 #endif /* QCRYPTO_TLSSESSION_H */
diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 33203e8ca711..903301189069 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -521,6 +521,33 @@  qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session)
 }


+int qcrypto_tls_session_shutdown(QCryptoTLSSession *session,
+                                 QCryptoShutdownMode how)
+{
+    gnutls_close_request_t mode;
+    int ret;
+
+    assert(session->handshakeComplete);
+    switch (how) {
+    case QCRYPTO_SHUT_WR:
+        mode = GNUTLS_SHUT_WR;
+        break;
+    case QCRYPTO_SHUT_RDWR:
+        mode = GNUTLS_SHUT_RDWR;
+        break;
+    default:
+        abort();
+    }
+
+    ret = gnutls_bye(session->handle, mode);
+    if (ret == GNUTLS_E_INTERRUPTED ||
+        ret == GNUTLS_E_AGAIN) {
+        return -EAGAIN;
+    }
+    return 0;
+}
+
+
 int
 qcrypto_tls_session_get_key_size(QCryptoTLSSession *session,
                                  Error **errp)