PKCS5_PBKDF1 buffer overflow #874

Activity

1. guidovranken commented on Aug 16, 2019

   

   guidovranken

   on Aug 16, 2019

   Author

   More actions

   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.
2. noloader commented on Aug 16, 2019

   

   noloader

   on Aug 16, 2019

   Last edited by noloader

   Collaborator

   More actions

   @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.
3. noloader

   added a commit that references this issue on Aug 16, 2019

   Fix use of MaxDerivedKeyLength (GH #874)

   ...

   c0a5a06
4. noloader

   closed this as completedon Aug 16, 2019
5. noloader commented on Aug 16, 2019

   

   noloader

   on Aug 16, 2019

   Last edited by noloader

   Collaborator

   More actions

   Cleared at Commit c0a5a06a8285 (pwdbased) and Commit e22700f741af (scrypt and hkdf).
6. noloader commented on Aug 16, 2019

   

   noloader

   on Aug 16, 2019

   Collaborator

   More actions

   @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.
7. noloader

   added a commit that references this issue on Aug 16, 2019

   Fix use of MaxDerivedKeyLength (GH #874)

   e22700f
8. noloader

   added

   Bug

   on Dec 20, 2020