| Message ID | 1467715800-20379-2-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/05/2016 04:49 AM, Daniel P. Berrange wrote: > GLib >= 2.16 provides GChecksum API which is good enough > for md5, sha1, sha256 and sha512. Use this as a final > fallback if neither nettle or gcrypt are available. This > lets us remove the stub hash impl, and so callers can > be sure those 4 algs are always available at compile > time. They may still be disabled at runtime, so a check > for qcrypto_hash_supports() is still best practice to > report good error messages. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > crypto/Makefile.objs | 2 +- > crypto/hash-glib.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > crypto/hash-stub.c | 41 ----------------------- > 3 files changed, 95 insertions(+), 42 deletions(-) > create mode 100644 crypto/hash-glib.c > delete mode 100644 crypto/hash-stub.c > > +gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg) > +{ > + if (alg < G_N_ELEMENTS(qcrypto_hash_alg_map) && > + qcrypto_hash_alg_map[alg] != -1) { > + return true; > + } > + return false; > +} > + > + > +int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg, > + const struct iovec *iov, > + size_t niov, > + uint8_t **result, > + size_t *resultlen, > + Error **errp) > +{ > + int i, ret; > + GChecksum *cs; > + > + if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_map) || > + qcrypto_hash_alg_map[alg] == -1) { Worth writing this as 'if (!gcrypto_hash_supports(alg)) {' ? Otherwise, Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, Jul 05, 2016 at 09:03:26AM -0600, Eric Blake wrote: > On 07/05/2016 04:49 AM, Daniel P. Berrange wrote: > > GLib >= 2.16 provides GChecksum API which is good enough > > for md5, sha1, sha256 and sha512. Use this as a final > > fallback if neither nettle or gcrypt are available. This > > lets us remove the stub hash impl, and so callers can > > be sure those 4 algs are always available at compile > > time. They may still be disabled at runtime, so a check > > for qcrypto_hash_supports() is still best practice to > > report good error messages. > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > crypto/Makefile.objs | 2 +- > > crypto/hash-glib.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > crypto/hash-stub.c | 41 ----------------------- > > 3 files changed, 95 insertions(+), 42 deletions(-) > > create mode 100644 crypto/hash-glib.c > > delete mode 100644 crypto/hash-stub.c > > > > > +gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg) > > +{ > > + if (alg < G_N_ELEMENTS(qcrypto_hash_alg_map) && > > + qcrypto_hash_alg_map[alg] != -1) { > > + return true; > > + } > > + return false; > > +} > > + > > + > > +int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg, > > + const struct iovec *iov, > > + size_t niov, > > + uint8_t **result, > > + size_t *resultlen, > > + Error **errp) > > +{ > > + int i, ret; > > + GChecksum *cs; > > + > > + if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_map) || > > + qcrypto_hash_alg_map[alg] == -1) { > > Worth writing this as 'if (!gcrypto_hash_supports(alg)) {' ? This pattern is used in the nettle + gcrypt impls too. I'd be happy to switch to what you suggest in all impls separately. > Otherwise, > Reviewed-by: Eric Blake <eblake@redhat.com> Oh, and BTW the pre-existing test-crypto-hash unit tests will already provide coverage for this implementation & i've checked it passes when --disable-nettle --disable-gcrypt are given to confnigure. Regards, Daniel
On Tue 05 Jul 2016 12:49:59 PM CEST, "Daniel P. Berrange" <berrange@redhat.com> wrote: > GLib >= 2.16 provides GChecksum API which is good enough > for md5, sha1, sha256 and sha512. Use this as a final > fallback if neither nettle or gcrypt are available. This > lets us remove the stub hash impl, and so callers can > be sure those 4 algs are always available at compile > time. They may still be disabled at runtime, so a check > for qcrypto_hash_supports() is still best practice to > report good error messages. Sorry if I missed the explanation, but how do you disable them at runtime ? Berto
On 07/06/2016 05:58 AM, Alberto Garcia wrote: > On Tue 05 Jul 2016 12:49:59 PM CEST, "Daniel P. Berrange" <berrange@redhat.com> wrote: > >> GLib >= 2.16 provides GChecksum API which is good enough >> for md5, sha1, sha256 and sha512. Use this as a final >> fallback if neither nettle or gcrypt are available. This >> lets us remove the stub hash impl, and so callers can >> be sure those 4 algs are always available at compile >> time. They may still be disabled at runtime, so a check >> for qcrypto_hash_supports() is still best practice to >> report good error messages. > > Sorry if I missed the explanation, but how do you disable them at > runtime ? FIPS is a common case where portions of a crypto lib are disabled at runtime based on whether the system is running in FIPS mode or not. I don't think any of the hashes in the glib fallback are necessarily covered by FIPS disabling, so much as the qcrypto interface being interested in generically catering to this behavior across the various implementations.
On Tue 05 Jul 2016 12:49:59 PM CEST, "Daniel P. Berrange" <berrange@redhat.com> wrote: > + cs = g_checksum_new(qcrypto_hash_alg_map[alg]); > + > + for (i = 0; i < niov; i++) { > + g_checksum_update(cs, iov[i].iov_base, iov[i].iov_len); > + } Not too important, but you could do this after checking the hash length and the buffer size. You don't need to create or feed the GChecksum first for those, and that way you would also get rid of the 'goto error'. Either way, Reviewed-by: Alberto Garcia <berto@igalia.com> > + ret = g_checksum_type_get_length(qcrypto_hash_alg_map[alg]); > + if (ret < 0) { > + error_setg(errp, "%s", > + "Unable to get hash length"); > + goto error; > + } > + if (*resultlen == 0) { > + *resultlen = ret; > + *result = g_new0(uint8_t, *resultlen); > + } else if (*resultlen != ret) { > + error_setg(errp, > + "Result buffer size %zu is smaller than hash %d", > + *resultlen, ret); > + goto error; > + } > + > + g_checksum_get_digest(cs, *result, resultlen); > + > + g_checksum_free(cs); > + return 0; > + > + error: > + g_checksum_free(cs); > + return -1; > +} Berto
On Wed, Jul 06, 2016 at 08:53:56AM -0600, Eric Blake wrote: > On 07/06/2016 05:58 AM, Alberto Garcia wrote: > > On Tue 05 Jul 2016 12:49:59 PM CEST, "Daniel P. Berrange" <berrange@redhat.com> wrote: > > > >> GLib >= 2.16 provides GChecksum API which is good enough > >> for md5, sha1, sha256 and sha512. Use this as a final > >> fallback if neither nettle or gcrypt are available. This > >> lets us remove the stub hash impl, and so callers can > >> be sure those 4 algs are always available at compile > >> time. They may still be disabled at runtime, so a check > >> for qcrypto_hash_supports() is still best practice to > >> report good error messages. > > > > Sorry if I missed the explanation, but how do you disable them at > > runtime ? > > FIPS is a common case where portions of a crypto lib are disabled at > runtime based on whether the system is running in FIPS mode or not. I > don't think any of the hashes in the glib fallback are necessarily > covered by FIPS disabling, so much as the qcrypto interface being > interested in generically catering to this behavior across the various > implementations. Yep, currently none of the hashes are disabled by FIPS, but the QEMU crypto API is designed to allow for that in the future, without us needing to change the rest of QEMU code using those APIs Regards, Daniel
diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs index 1f86f4f..e409b89 100644 --- a/crypto/Makefile.objs +++ b/crypto/Makefile.objs @@ -2,6 +2,7 @@ crypto-obj-y = init.o crypto-obj-y += hash.o crypto-obj-$(CONFIG_NETTLE) += hash-nettle.o crypto-obj-$(if $(CONFIG_NETTLE),n,$(CONFIG_GCRYPT)) += hash-gcrypt.o +crypto-obj-$(if $(CONFIG_NETTLE),n,$(if $(CONFIG_GCRYPT),n,y)) += hash-glib.o crypto-obj-y += aes.o crypto-obj-y += desrfb.o crypto-obj-y += cipher.o @@ -30,4 +31,3 @@ crypto-aes-obj-y = aes.o stub-obj-y += random-stub.o stub-obj-y += pbkdf-stub.o -stub-obj-y += hash-stub.o diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c new file mode 100644 index 0000000..81ef7ca --- /dev/null +++ b/crypto/hash-glib.c @@ -0,0 +1,94 @@ +/* + * QEMU Crypto hash algorithms + * + * Copyright (c) 2016 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see <http://www.gnu.org/licenses/>. + * + */ + +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "crypto/hash.h" + + +static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALG__MAX] = { + [QCRYPTO_HASH_ALG_MD5] = G_CHECKSUM_MD5, + [QCRYPTO_HASH_ALG_SHA1] = G_CHECKSUM_SHA1, + [QCRYPTO_HASH_ALG_SHA224] = -1, + [QCRYPTO_HASH_ALG_SHA256] = G_CHECKSUM_SHA256, + [QCRYPTO_HASH_ALG_SHA384] = -1, + [QCRYPTO_HASH_ALG_SHA512] = G_CHECKSUM_SHA512, + [QCRYPTO_HASH_ALG_RIPEMD160] = -1, +}; + +gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg) +{ + if (alg < G_N_ELEMENTS(qcrypto_hash_alg_map) && + qcrypto_hash_alg_map[alg] != -1) { + return true; + } + return false; +} + + +int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg, + const struct iovec *iov, + size_t niov, + uint8_t **result, + size_t *resultlen, + Error **errp) +{ + int i, ret; + GChecksum *cs; + + if (alg >= G_N_ELEMENTS(qcrypto_hash_alg_map) || + qcrypto_hash_alg_map[alg] == -1) { + error_setg(errp, + "Unknown hash algorithm %d", + alg); + return -1; + } + + cs = g_checksum_new(qcrypto_hash_alg_map[alg]); + + for (i = 0; i < niov; i++) { + g_checksum_update(cs, iov[i].iov_base, iov[i].iov_len); + } + + ret = g_checksum_type_get_length(qcrypto_hash_alg_map[alg]); + if (ret < 0) { + error_setg(errp, "%s", + "Unable to get hash length"); + goto error; + } + if (*resultlen == 0) { + *resultlen = ret; + *result = g_new0(uint8_t, *resultlen); + } else if (*resultlen != ret) { + error_setg(errp, + "Result buffer size %zu is smaller than hash %d", + *resultlen, ret); + goto error; + } + + g_checksum_get_digest(cs, *result, resultlen); + + g_checksum_free(cs); + return 0; + + error: + g_checksum_free(cs); + return -1; +} diff --git a/crypto/hash-stub.c b/crypto/hash-stub.c deleted file mode 100644 index 8a9b8d4..0000000 --- a/crypto/hash-stub.c +++ /dev/null @@ -1,41 +0,0 @@ -/* - * QEMU Crypto hash algorithms - * - * Copyright (c) 2016 Red Hat, Inc. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, see <http://www.gnu.org/licenses/>. - * - */ - -#include "qemu/osdep.h" -#include "qapi/error.h" -#include "crypto/hash.h" - -gboolean qcrypto_hash_supports(QCryptoHashAlgorithm alg G_GNUC_UNUSED) -{ - return false; -} - -int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg, - const struct iovec *iov G_GNUC_UNUSED, - size_t niov G_GNUC_UNUSED, - uint8_t **result G_GNUC_UNUSED, - size_t *resultlen G_GNUC_UNUSED, - Error **errp) -{ - error_setg(errp, - "Hash algorithm %d not supported without GNUTLS", - alg); - return -1; -}
GLib >= 2.16 provides GChecksum API which is good enough for md5, sha1, sha256 and sha512. Use this as a final fallback if neither nettle or gcrypt are available. This lets us remove the stub hash impl, and so callers can be sure those 4 algs are always available at compile time. They may still be disabled at runtime, so a check for qcrypto_hash_supports() is still best practice to report good error messages. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- crypto/Makefile.objs | 2 +- crypto/hash-glib.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++ crypto/hash-stub.c | 41 ----------------------- 3 files changed, 95 insertions(+), 42 deletions(-) create mode 100644 crypto/hash-glib.c delete mode 100644 crypto/hash-stub.c