Re: [Qemu-devel] [PATCH v2 03/17] crypto: add support for PBKDF2 algorithm

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Feb 04, 2016 at 03:14:10PM -0700, Eric Blake wrote:
> On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> > The LUKS data format includes use of PBKDF2 (Password-Based
> > Key Derivation Function). The Nettle library can provide
> > an implementation of this, but we don't want code directly
> > depending on a specific crypto library backend. Introduce
> > a include/crypto/pbkdf.h header which defines a QEMU
> 
> 'an include/...', or maybe 'a new include/...'?
> 
> > API for invoking PBKDK2. The initial implementations are
> > backed by nettle & gcrypt, which are commonly available
> > with distros shipping GNUTLS.
> > 
> > The test suite data is taken from the cryptsetup codebase
> > under the LGPLv2.1+ license. This merely aims to verify
> > that whatever backend we provide for this function in QEMU
> > will comply with the spec.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> 
> In addition to Fam's review,
> 
> > +++ b/crypto/pbkdf-gcrypt.c
> 
> > +int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
> > +                   const uint8_t *key, size_t nkey,
> > +                   const uint8_t *salt, size_t nsalt,
> > +                   unsigned int iterations,
> > +                   uint8_t *out, size_t nout,
> > +                   Error **errp)
> > +{
> > +    static const int hash_map[QCRYPTO_HASH_ALG__MAX] = {
> > +        [QCRYPTO_HASH_ALG_MD5] = GCRY_MD_MD5,
> > +        [QCRYPTO_HASH_ALG_SHA1] = GCRY_MD_SHA1,
> > +        [QCRYPTO_HASH_ALG_SHA256] = GCRY_MD_SHA256,
> > +    };
> 
> If QCRYPTO_HASH_ gains future enum values, those elements of the array
> will be 0-initialized.
> 
> > +    int ret;
> > +
> > +    if (hash > G_N_ELEMENTS(hash_map)) {
> > +        error_setg(errp, "Unexpected hash algorithm %d", hash);
> > +        return -1;
> > +    }
> 
> This checks for beyond the bounds of the array, but not for an element
> that was 0-initialized.  Is that a problem we need to worry about?

I'll add  '||  hash_map[hash] == GCRY_MD_NONE' to this condition


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|