From c6a9a9f57503a2736c08711a0387c3e7718353ba Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 15 Mar 2017 11:53:22 +0000 Subject: [PATCH 1/4] Default to GSSAPI (Kerberos) instead of DIGEST-MD5 for SASL RFC 6331 documents a number of serious security weaknesses in the SASL DIGEST-MD5 mechanism. As such, QEMU should not be using or recommending it as a default mechanism for VNC auth with SASL. GSSAPI (Kerberos) is the only other viable SASL mechanism that can provide secure session encryption so enable that by defalt as the replacement. If users have TLS enabled for VNC, they can optionally decide to use SCRAM-SHA-1 instead of GSSAPI, allowing plain username and password auth. Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrange --- qemu-doc.texi | 54 ++++++++++++++++++++++++++++--------------------- qemu.sasl | 56 +++++++++++++++++++++++++++++---------------------- 2 files changed, 63 insertions(+), 47 deletions(-) diff --git a/qemu-doc.texi b/qemu-doc.texi index 794ab4a080..a4be7143b9 100644 --- a/qemu-doc.texi +++ b/qemu-doc.texi @@ -1732,37 +1732,45 @@ SASL service config /etc/sasl2/qemu.conf. If running QEMU as an unprivileged user, an environment variable SASL_CONF_PATH can be used to make it search alternate locations for the service config. -The default configuration might contain +If the TLS option is enabled for VNC, then it will provide session encryption, +otherwise the SASL mechanism will have to provide encryption. In the latter +case the list of possible plugins that can be used is drastically reduced. In +fact only the GSSAPI SASL mechanism provides an acceptable level of security +by modern standards. Previous versions of QEMU referred to the DIGEST-MD5 +mechanism, however, it has multiple serious flaws described in detail in +RFC 6331 and thus should never be used any more. The SCRAM-SHA-1 mechanism +provides a simple username/password auth facility similar to DIGEST-MD5, but +does not support session encryption, so can only be used in combination with +TLS. -@example -mech_list: digest-md5 -sasldb_path: /etc/qemu/passwd.db -@end example - -This says to use the 'Digest MD5' mechanism, which is similar to the HTTP -Digest-MD5 mechanism. The list of valid usernames & passwords is maintained -in the /etc/qemu/passwd.db file, and can be updated using the saslpasswd2 -command. While this mechanism is easy to configure and use, it is not -considered secure by modern standards, so only suitable for developers / -ad-hoc testing. - -A more serious deployment might use Kerberos, which is done with the 'gssapi' -mechanism +When not using TLS the recommended configuration is @example mech_list: gssapi keytab: /etc/qemu/krb5.tab @end example -For this to work the administrator of your KDC must generate a Kerberos -principal for the server, with a name of 'qemu/somehost.example.com@@EXAMPLE.COM' -replacing 'somehost.example.com' with the fully qualified host name of the -machine running QEMU, and 'EXAMPLE.COM' with the Kerberos Realm. +This says to use the 'GSSAPI' mechanism with the Kerberos v5 protocol, with +the server principal stored in /etc/qemu/krb5.tab. For this to work the +administrator of your KDC must generate a Kerberos principal for the server, +with a name of 'qemu/somehost.example.com@@EXAMPLE.COM' replacing +'somehost.example.com' with the fully qualified host name of the machine +running QEMU, and 'EXAMPLE.COM' with the Kerberos Realm. -Other configurations will be left as an exercise for the reader. It should -be noted that only Digest-MD5 and GSSAPI provides a SSF layer for data -encryption. For all other mechanisms, VNC should always be configured to -use TLS and x509 certificates to protect security credentials from snooping. +When using TLS, if username+password authentication is desired, then a +reasonable configuration is + +@example +mech_list: scram-sha-1 +sasldb_path: /etc/qemu/passwd.db +@end example + +The saslpasswd2 program can be used to populate the passwd.db file with +accounts. + +Other SASL configurations will be left as an exercise for the reader. Note that +all mechanisms except GSSAPI, should be combined with use of TLS to ensure a +secure data channel. @node gdb_usage @section GDB usage diff --git a/qemu.sasl b/qemu.sasl index 64fdef3d5b..fb8a92ba58 100644 --- a/qemu.sasl +++ b/qemu.sasl @@ -1,36 +1,44 @@ -# If you want to use the non-TLS socket, then you *must* include -# the GSSAPI or DIGEST-MD5 mechanisms, because they are the only -# ones that can offer session encryption as well as authentication. +# If you want to use VNC remotely without TLS, then you *must* +# pick a mechanism which provides session encryption as well +# as authentication. # -# If you're only using TLS, then you can turn on any mechanisms +# If you are only using TLS, then you can turn on any mechanisms # you like for authentication, because TLS provides the encryption # -# Default to a simple username+password mechanism -# NB digest-md5 is no longer considered secure by current standards -mech_list: digest-md5 - -# Before you can use GSSAPI, you need a service principle on the -# KDC server for libvirt, and that to be exported to the keytab -# file listed below -#mech_list: gssapi +# If you are only using UNIX sockets then encryption is not +# required at all. # -# You can also list many mechanisms at once, then the user can choose -# by adding '?auth=sasl.gssapi' to their libvirt URI, eg -# qemu+tcp://hostname/system?auth=sasl.gssapi -#mech_list: digest-md5 gssapi +# NB, previously DIGEST-MD5 was set as the default mechanism for +# QEMU VNC. Per RFC 6331 this is vulnerable to many serious security +# flaws as should no longer be used. Thus GSSAPI is now the default. +# +# To use GSSAPI requires that a QEMU service principal is +# added to the Kerberos server for each host running QEMU. +# This principal needs to be exported to the keytab file listed below +mech_list: gssapi + +# If using TLS with VNC, or a UNIX socket only, it is possible to +# enable plugins which don't provide session encryption. The +# 'scram-sha-1' plugin allows plain username/password authentication +# to be performed +# +#mech_list: scram-sha-1 + +# You can also list many mechanisms at once, and the VNC server will +# negotiate which to use by considering the list enabled on the VNC +# client. +#mech_list: scram-sha-1 gssapi # Some older builds of MIT kerberos on Linux ignore this option & # instead need KRB5_KTNAME env var. # For modern Linux, and other OS, this should be sufficient # -# There is no default value here, uncomment if you need this -#keytab: /etc/qemu/krb5.tab +# This file needs to be populated with the service principal that +# was created on the Kerberos v5 server. If switching to a non-gssapi +# mechanism this can be commented out. +keytab: /etc/qemu/krb5.tab -# If using digest-md5 for username/passwds, then this is the file +# If using scram-sha-1 for username/passwds, then this is the file # containing the passwds. Use 'saslpasswd2 -a qemu [username]' # to add entries, and 'sasldblistusers2 -f [sasldb_path]' to browse it -sasldb_path: /etc/qemu/passwd.db - - -auxprop_plugin: sasldb - +#sasldb_path: /etc/qemu/passwd.db From 899833cd65cafbdbae4b5b9da3b1c3726e13e299 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Thu, 16 Mar 2017 11:18:46 +0000 Subject: [PATCH 2/4] List SASL config file under the cryptography maintainer's realm No one is listed as maintainer for qemu.sasl. It is used by the VNC server for SASL auth, but since it is cryptography related, list it under the crytography maintainer's realm, rather than under the UI maintainer. Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrange --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index efdec47319..b4d877b92b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1484,6 +1484,7 @@ S: Maintained F: crypto/ F: include/crypto/ F: tests/test-crypto-* +F: qemu.sasl Coroutines M: Stefan Hajnoczi From e4a3507e86a1ef1453d603031bca27d5ac4cff3c Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 24 Apr 2017 16:33:15 +0100 Subject: [PATCH 3/4] crypto: move 'opaque' parameter to (nearly) the end of parameter list Previous commit moved 'opaque' to be the 2nd parameter in the list: commit 375092332eeaa6e47561ce47fd36144cdaf964d0 Author: Fam Zheng Date: Fri Apr 21 20:27:02 2017 +0800 crypto: Make errp the last parameter of functions Move opaque to 2nd instead of the 2nd to last, so that compilers help check with the conversion. this puts it back to the 2nd to last position. Reviewed-by: Eric Blake Reviewed-by: Fam Zheng Signed-off-by: Daniel P. Berrange --- block/crypto.c | 6 +++--- crypto/block-luks.c | 13 ++++++++----- include/crypto/block.h | 6 +++--- tests/test-crypto-block.c | 6 +++--- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/block/crypto.c b/block/crypto.c index 6828180840..10e5ddccaa 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -56,10 +56,10 @@ static int block_crypto_probe_generic(QCryptoBlockFormat format, static ssize_t block_crypto_read_func(QCryptoBlock *block, - void *opaque, size_t offset, uint8_t *buf, size_t buflen, + void *opaque, Error **errp) { BlockDriverState *bs = opaque; @@ -83,10 +83,10 @@ struct BlockCryptoCreateData { static ssize_t block_crypto_write_func(QCryptoBlock *block, - void *opaque, size_t offset, const uint8_t *buf, size_t buflen, + void *opaque, Error **errp) { struct BlockCryptoCreateData *data = opaque; @@ -102,8 +102,8 @@ static ssize_t block_crypto_write_func(QCryptoBlock *block, static ssize_t block_crypto_init_func(QCryptoBlock *block, - void *opaque, size_t headerlen, + void *opaque, Error **errp) { struct BlockCryptoCreateData *data = opaque; diff --git a/crypto/block-luks.c b/crypto/block-luks.c index d5a31bbaeb..2b97d89a69 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -473,9 +473,9 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, * then encrypted. */ rv = readfunc(block, - opaque, slot->key_offset * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE, splitkey, splitkeylen, + opaque, errp); if (rv < 0) { goto cleanup; @@ -676,9 +676,10 @@ qcrypto_block_luks_open(QCryptoBlock *block, /* Read the entire LUKS header, minus the key material from * the underlying device */ - rv = readfunc(block, opaque, 0, + rv = readfunc(block, 0, (uint8_t *)&luks->header, sizeof(luks->header), + opaque, errp); if (rv < 0) { ret = rv; @@ -1245,7 +1246,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE; /* Reserve header space to match payload offset */ - initfunc(block, opaque, block->payload_offset, &local_err); + initfunc(block, block->payload_offset, opaque, &local_err); if (local_err) { error_propagate(errp, local_err); goto error; @@ -1267,9 +1268,10 @@ qcrypto_block_luks_create(QCryptoBlock *block, /* Write out the partition header and key slot headers */ - writefunc(block, opaque, 0, + writefunc(block, 0, (const uint8_t *)&luks->header, sizeof(luks->header), + opaque, &local_err); /* Delay checking local_err until we've byte-swapped */ @@ -1295,10 +1297,11 @@ qcrypto_block_luks_create(QCryptoBlock *block, /* Write out the master key material, starting at the * sector immediately following the partition header. */ - if (writefunc(block, opaque, + if (writefunc(block, luks->header.key_slots[0].key_offset * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE, splitkey, splitkeylen, + opaque, errp) != splitkeylen) { goto error; } diff --git a/include/crypto/block.h b/include/crypto/block.h index 4a053a3ffa..013a435f1b 100644 --- a/include/crypto/block.h +++ b/include/crypto/block.h @@ -30,22 +30,22 @@ typedef struct QCryptoBlock QCryptoBlock; * and QCryptoBlockOpenOptions in qapi/crypto.json */ typedef ssize_t (*QCryptoBlockReadFunc)(QCryptoBlock *block, - void *opaque, size_t offset, uint8_t *buf, size_t buflen, + void *opaque, Error **errp); typedef ssize_t (*QCryptoBlockInitFunc)(QCryptoBlock *block, - void *opaque, size_t headerlen, + void *opaque, Error **errp); typedef ssize_t (*QCryptoBlockWriteFunc)(QCryptoBlock *block, - void *opaque, size_t offset, const uint8_t *buf, size_t buflen, + void *opaque, Error **errp); /** diff --git a/tests/test-crypto-block.c b/tests/test-crypto-block.c index 85e6603d59..95c4bd5da3 100644 --- a/tests/test-crypto-block.c +++ b/tests/test-crypto-block.c @@ -187,10 +187,10 @@ static struct QCryptoBlockTestData { static ssize_t test_block_read_func(QCryptoBlock *block, - void *opaque, size_t offset, uint8_t *buf, size_t buflen, + void *opaque, Error **errp) { Buffer *header = opaque; @@ -204,8 +204,8 @@ static ssize_t test_block_read_func(QCryptoBlock *block, static ssize_t test_block_init_func(QCryptoBlock *block, - void *opaque, size_t headerlen, + void *opaque, Error **errp) { Buffer *header = opaque; @@ -219,10 +219,10 @@ static ssize_t test_block_init_func(QCryptoBlock *block, static ssize_t test_block_write_func(QCryptoBlock *block, - void *opaque, size_t offset, const uint8_t *buf, size_t buflen, + void *opaque, Error **errp) { Buffer *header = opaque; From a37278169d0d7d33270c6817f4a2e4f7345e1098 Mon Sep 17 00:00:00 2001 From: Geert Martin Ijewski Date: Wed, 26 Apr 2017 00:15:01 +0200 Subject: [PATCH 4/4] crypto: qcrypto_random_bytes() now works on windows w/o any other crypto libs If no crypto library is included in the build, QEMU uses qcrypto_random_bytes() to generate random data. That function tried to open /dev/urandom or /dev/random and if opening both files failed it errored out. Those files obviously do not exist on windows, so there the code uses CryptGenRandom(). Furthermore there was some refactoring and a new function qcrypto_random_init() was introduced. If a proper crypto library (gnutls or libgcrypt) is included in the build, this function does nothing. If neither is included it initializes the (platform specific) handles that are used by qcrypto_random_bytes(). Either: * a handle to /dev/urandom | /dev/random on unix like systems * a handle to a cryptographic service provider on windows Signed-off-by: Geert Martin Ijewski Signed-off-by: Daniel P. Berrange --- crypto/init.c | 6 ++++++ crypto/random-gcrypt.c | 2 ++ crypto/random-gnutls.c | 3 +++ crypto/random-platform.c | 45 +++++++++++++++++++++++++++++++++------- include/crypto/random.h | 9 ++++++++ 5 files changed, 57 insertions(+), 8 deletions(-) diff --git a/crypto/init.c b/crypto/init.c index f65207e57d..f131c42306 100644 --- a/crypto/init.c +++ b/crypto/init.c @@ -32,6 +32,8 @@ #include #endif +#include "crypto/random.h" + /* #define DEBUG_GNUTLS */ /* @@ -146,5 +148,9 @@ int qcrypto_init(Error **errp) gcry_control(GCRYCTL_INITIALIZATION_FINISHED, 0); #endif + if (qcrypto_random_init(errp) < 0) { + return -1; + } + return 0; } diff --git a/crypto/random-gcrypt.c b/crypto/random-gcrypt.c index 0de9a096df..9f1c9ee60e 100644 --- a/crypto/random-gcrypt.c +++ b/crypto/random-gcrypt.c @@ -31,3 +31,5 @@ int qcrypto_random_bytes(uint8_t *buf, gcry_randomize(buf, buflen, GCRY_STRONG_RANDOM); return 0; } + +int qcrypto_random_init(Error **errp G_GNUC_UNUSED) { return 0; } diff --git a/crypto/random-gnutls.c b/crypto/random-gnutls.c index 04b45a8f8f..5350003a0b 100644 --- a/crypto/random-gnutls.c +++ b/crypto/random-gnutls.c @@ -41,3 +41,6 @@ int qcrypto_random_bytes(uint8_t *buf, return 0; } + + +int qcrypto_random_init(Error **errp G_GNUC_UNUSED) { return 0; } diff --git a/crypto/random-platform.c b/crypto/random-platform.c index 82b755afad..0eddb915b7 100644 --- a/crypto/random-platform.c +++ b/crypto/random-platform.c @@ -22,14 +22,16 @@ #include "crypto/random.h" -int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED, - size_t buflen G_GNUC_UNUSED, - Error **errp) -{ - int fd; - int ret = -1; - int got; +#ifdef _WIN32 +#include +static HCRYPTPROV hCryptProv; +#else +static int fd; /* a file handle to either /dev/urandom or /dev/random */ +#endif +int qcrypto_random_init(Error **errp) +{ +#ifndef _WIN32 /* TBD perhaps also add support for BSD getentropy / Linux * getrandom syscalls directly */ fd = open("/dev/urandom", O_RDONLY); @@ -41,6 +43,25 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED, error_setg(errp, "No /dev/urandom or /dev/random found"); return -1; } +#else + if (!CryptAcquireContext(&hCryptProv, NULL, NULL, PROV_RSA_FULL, + CRYPT_SILENT | CRYPT_VERIFYCONTEXT)) { + error_setg_win32(errp, GetLastError(), + "Unable to create cryptographic provider"); + return -1; + } +#endif + + return 0; +} + +int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED, + size_t buflen G_GNUC_UNUSED, + Error **errp) +{ +#ifndef _WIN32 + int ret = -1; + int got; while (buflen > 0) { got = read(fd, buf, buflen); @@ -59,6 +80,14 @@ int qcrypto_random_bytes(uint8_t *buf G_GNUC_UNUSED, ret = 0; cleanup: - close(fd); return ret; +#else + if (!CryptGenRandom(hCryptProv, buflen, buf)) { + error_setg_win32(errp, GetLastError(), + "Unable to read random bytes"); + return -1; + } + + return 0; +#endif } diff --git a/include/crypto/random.h b/include/crypto/random.h index a101353202..a07229ce96 100644 --- a/include/crypto/random.h +++ b/include/crypto/random.h @@ -40,5 +40,14 @@ int qcrypto_random_bytes(uint8_t *buf, size_t buflen, Error **errp); +/** + * qcrypto_random_init: + * @errp: pointer to a NULL-initialized error object + * + * Initializes the handles used by qcrypto_random_bytes + * + * Returns 0 on success, -1 on error + */ +int qcrypto_random_init(Error **errp); #endif /* QCRYPTO_RANDOM_H */