noloader / cryptopp-pem

PEM parsing of keys and parameters for Crypto++ project
42 stars 31 forks source link

Fix error to load DSA Private Key #4

Closed alambrec closed 5 years ago

alambrec commented 5 years ago

Hi,

When we run self-test pem-test, the loading of DSA Private Key failed with this exception :

Load RSA public key
Load RSA private key
Load encrypted RSA private key
Load DSA public key
Load DSA private key
Caught exception : BER decode error

By trying to find the problem, I noticed that this exception proceed from CryptoPP::BERDecodePrivateKey() method into PEM_LoadPrivateKey() :

void PEM_LoadPrivateKey(BufferedTransformation& src, PKCS8PrivateKey& key, bool subjectInfo)
{
    if (subjectInfo)
        key.Load(src);
    else
        key.BERDecodePrivateKey(src, 0, src.MaxRetrievable());
        // Exception here 
    ...
}

This exception appears when the Private Key is bad-encoded.

By reading this link : https://www.cryptopp.com/wiki/Pem_pack#PEM_Load I have found a fix, but I don't know if it can occur a side effect on the rest of code.

The fix is to replace the PEM_LoadPrivateKey() method call into PEM_Load() : Change

void PEM_Load(BufferedTransformation& bt, DSA::PrivateKey& dsa, const char* password, size_t length)
{
    ...
    PEM_LoadPrivateKey(temp, dsa, type == PEM_PRIVATE_KEY);
}

by :

void PEM_Load(BufferedTransformation& bt, DSA::PrivateKey& dsa, const char* password, size_t length)
{
    ...
    PEM_LoadPrivateKey(temp, dsa);
}

This change permit to call a different method, because a multiple declaration of this method exists, with different prototype :

static void PEM_LoadPrivateKey(BufferedTransformation& bt, PKCS8PrivateKey& key, bool subjectInfo = false);
// Crypto++ expects {version,x}; OpenSSL writes {version,x,y,p,q,g}
static void PEM_LoadPrivateKey(BufferedTransformation& bt, DSA::PrivateKey& key);

So, by deleting the third argument, the called method is :

static void PEM_LoadPrivateKey(BufferedTransformation& bt, DSA::PrivateKey& key)

and the problem seems to be fixed.

noloader commented 5 years ago

Thanks @alambrec.

It looks about right. If not we can loop back around to it.