PKCS5_PBKDF1 buffer overflow

[guidovranken]

#include <sha.h>
#include <pwdbased.h>

int main(void)
{
    unsigned char out[714];
    const unsigned char password[8] = { 0 };
    const unsigned char salt[8] = { 0 };
    ::CryptoPP::PKCS5_PBKDF1<::CryptoPP::SHA1> pbkdf1;
    pbkdf1.DeriveKey(
            out,
            sizeof(out),
            0,
            password,
            sizeof(password),
            salt,
            sizeof(salt),
            4);
    return 0;
}

Results in a buffer overflow in PKCS5_PBKDF1<T>::DeriveKey because:

128     SecByteBlock buffer(hash.DigestSize());
...
...
140     memcpy(derived, buffer, derivedLen);

So with SHA1, 20 bytes are allocated, but because derivedLen is 714, (714-20)=694 too many bytes are copied.

[guidovranken]

Another issue is the following

#include <sha.h>
#include <pwdbased.h>

int main(void)
{
    const unsigned char password[8] = { 0 };
    const unsigned char salt[8] = { 0 };
    ::CryptoPP::PKCS5_PBKDF1<::CryptoPP::SHA1> pbkdf1;
    pbkdf1.DeriveKey(
            nullptr,
            0,
            0,
            password,
            sizeof(password),
            salt,
            sizeof(salt),
            4);
    return 0;
}

(It is a nonsensical use of the API, but my fuzzer tries all parameter combinations)

This causes

   memcpy(derived, buffer, derivedLen);

in PKCS5_PBKDF1<T>::DeriveKey to effectively be

   memcpy(NULL, buffer, 0);

memcpy'ing to NULL even when the size is 0 is technically undefined behavior.

Something like this would be better:

   if ( derivedLen ) memcpy(derived, buffer, derivedLen);

Or just return near the start of the function if derivedLen is 0.

[noloader]

@guidovranken,

Thanks for the find.

Yeah, something looks odd with PKCS5_PBKDF1. It looks like PKCS5_PBKDF1, PKCS5_PBKDF2_HMAC and friends override MaxDerivedKeyLength, but the base class uses MaxDerivedLength. That's a typo from cutting in the KeyDerivationFunction interface. Ugh...

Here is Crypto++ 5.6.2 pwdbased.h.

[noloader]

Cleared at Commit c0a5a06a8285 (pwdbased) and Commit e22700f741af (scrypt and hkdf).

[noloader]

@guidovranken,

PKCS5_PBKDF1 will throw if the requested size is too large. That is the behavior specified by the standard. (It was supposed to throw in the past, but MaxDerivedKeyLength was not properly overridden).

If you need to stretch a key, use PKCS5_PBKDF2_HMAC. Or better, use HKDF class nowadays.