CryptDecrypt returns random characters at the end of decrxpted string?

Question

I am trying to make a simple application which encrypts a string and then decrypts it. So far my code:

int main( int argc, char* argv[] )
{  
char test[ 32 ] = { 0 };  
strcpy( test, "This is a sample string." );  
BYTE buf = NULL;  
DWORD len = strlen( test );  

EncryptData( lpszPassword, test, &len );

return 0; 
}

void EncryptData( TCHAR *lpszPassword, char *pbBuffer, DWORD *dwCount )
{
HCRYPTPROV hProv = 0;
HCRYPTKEY hKey = 0;
HCRYPTHASH hHash = 0;
LPWSTR wszPassword = lpszPassword;
DWORD cbPassword = ( wcslen( wszPassword ) + 1 )*sizeof( WCHAR );

if ( !CryptAcquireContext( &hProv, NULL, MS_ENH_RSA_AES_PROV, PROV_RSA_AES, CRYPT_VERIFYCONTEXT ) )
{
    printf( "Error %x during CryptAcquireContext!\n", GetLastError() );
    goto Cleanup;
}

if ( !CryptCreateHash( hProv, CALG_SHA_256, 0, 0, &hHash ) )
{
    printf( "Error %x during CryptCreateHash!\n", GetLastError() );
    goto Cleanup;
}

if ( !CryptHashData( hHash, ( PBYTE )wszPassword, cbPassword, 0 ) )
{
    printf( "Error %x during CryptHashData!\n", GetLastError() );
    goto Cleanup;
}

if ( !CryptDeriveKey( hProv, CALG_AES_256, hHash, CRYPT_EXPORTABLE, &hKey ) )//hKey
{
    printf( "Error %x during CryptDeriveKey!\n", GetLastError() );
    goto Cleanup;
}
DWORD size = ( DWORD )strlen( pbBuffer ) / sizeof( char );
printf( "\nLength of string = %d", size );
if ( !CryptEncrypt( hKey, 0, TRUE, 0, ( LPBYTE )pbBuffer, &size, BLOCK_SIZE ) )
{
    printf( "Error %x during CryptEncrypt!\n", GetLastError() );
    goto Cleanup;
}
printf( "\nEncrypted bytes  = %d", size );
printf( "\nEncrypted text = %s", pbBuffer );

if ( !CryptDecrypt( hKey, 0, TRUE, 0, ( LPBYTE )pbBuffer, &size ) )
{
    printf( "Error %x during CryptDecrypt!\n", GetLastError() );
    goto Cleanup;
}
printf( "\nDecrypted bytes  = %d", size );
printf( "\nDecrypted text = %s", pbBuffer );

Cleanup:

if ( hKey )
{
    CryptDestroyKey( hKey );
}
if ( hHash )
{
    CryptDestroyHash( hHash );
}
if ( hProv )
{
    CryptReleaseContext( hProv, 0 );
}

}

This produces the output:

Length of string = 24
Encrypted bytes  = 32
Encrypted text = ╨é╖·ç┤╠├ó br.≡·►;╜K/┤E(↓)╫%┤Cà¡╩╠╠╠╠╘)Ñ°♀·L
Decrypted bytes  = 24
Decrypted text = This is a sample string.)╫%┤Cà¡╩╠╠╠╠╘)Ñ°♀·L

So basicly it is almost working, but at the and of the encrypted string there are characters left from the encrypted string.

So my question is, am i doing something wrong or am i just missing something?

Thanks in advance!

Answer 1

The printf function when given "%s" requires a NULL terminated string. Obviously the string is not NULL terminated (actually, the NULL is located who-knows-where, but printf() found it long after the valid portion of the data is printed).

Use the size value you retrieved for the decrypted text. That is the real number of bytes that are valid.

Here is a solution that not only corrects the size and decrypted data issue, but also the issue with usage of goto.

#include <string>
#include <iostream>

using namespace std;

struct CryptStuff
{
     HCRYPTPROV* hProv;
     HCRYPTKEY* hKey;
     HCRYPTHASH* hHash;

     CryptStuff(HCRYPTPROV* hprov, HCRYPTKEY* hkey, HCRYPTHASH* hash) :
                   hProv(hprov), hKey(hkey), hHash(hash) {}

      ~CryptStuff() 
       { 
         if ( *hKey ) CryptDestroyKey( *hKey );
         if ( *hHash ) CryptDestroyHash( *hHash );
         if ( *hProv ) CryptReleaseContext( *hProv, 0 );
       }
};

void EncryptData( TCHAR *lpszPassword, char *pbBuffer, DWORD *dwCount )
{
    HCRYPTPROV hProv = 0;
    HCRYPTKEY hKey = 0;
    HCRYPTHASH hHash = 0;

    // create an instance of CryptStuff.  This will cleanup the data on return
    CryptStuff cs(&hProv, &hKey, &hHash);

    LPWSTR wszPassword = lpszPassword;
    DWORD cbPassword = ( wcslen( wszPassword ) + 1 )*sizeof( WCHAR );

    if ( !CryptAcquireContext( &hProv, NULL, MS_ENH_RSA_AES_PROV, PROV_RSA_AES, 
                               CRYPT_VERIFYCONTEXT ) )
    {
        return;
    }

    if ( !CryptCreateHash( hProv, CALG_SHA_256, 0, 0, &hHash ) )
    {
        return;
    }

    if ( !CryptHashData( hHash, ( PBYTE )wszPassword, cbPassword, 0 ) )
    {
        return;
    }

    if ( !CryptDeriveKey( hProv, CALG_AES_256, hHash, CRYPT_EXPORTABLE, &hKey ) )
    {
        return;
    }

    DWORD size = ( DWORD )strlen( pbBuffer ) / sizeof( char );
    cout << "\nLength of string = " << size;
    if ( !CryptEncrypt( hKey, 0, TRUE, 0, ( LPBYTE )pbBuffer, &size, BLOCK_SIZE ) )
    {
        return;
    }
    cout << "\nEncrypted bytes  = " << size;
    cout << "\nEncrypted text = ";
    cout.write(pbBuffer, size);

    if ( !CryptDecrypt( hKey, 0, TRUE, 0, ( LPBYTE )pbBuffer, &size ) )
    {
        return;
    }
    cout << "\nDecrypted bytes  = " << size;
    cout << "\nDecrypted text = ";
    cout.write(pbBuffer, size);
}

I wrote this without a compiler handy, so forgive any typos. I also removed the error output for brevity.

The code above first corrects the issue of the decrypted data by using cout.write to output the proper number of characters (denoted by the size value). This guarantees we get the characters outputted that we want. I used cout.write, since it is perfectly acceptable for unencrypted data to contain embedded NULL's, and we don't want to stop on the first NULL that shows up in the string. We want to stop once we hit size number of characters that are outputted.

---

The next thing that was done was to use a technique called RAII (Resource Acquisition Is Initialization) to remove the goto. Note how this was done:

We first created a struct called CryptStuff that contains pointers to the 3 items we want to clean up. In this struct, we have a destructor that cleans up these items. To utilize this struct, we create an instance of it called cs inside of EncryptData, and give the instance on construction the address of the 3 items.

So basically, when EncryptData returns, that cs instance will have its destructor called automatically, which means that we get our handles cleaned up. This is much more advantageous than using things such as goto (practically anything is better than goto) or tricky, redundant coding to clean up the handles. The reason why is that the clean up is automatic -- regardless of the reason for the return of EncryptData, i.e. a return or some function causes an exception to be thrown, we get to clean up the handles.

Also, if at a later time, the code gets more complex, there is no need to remember to "add a goto" or "write that clean up code" over and over again for each new return scenario. Note that the error conditions do a simple return without need for goto.

RAII info can be found here:

What is meant by Resource Acquisition is Initialization (RAII)?

It is an important part in writing C++ code that has to manage resources that are created and must be destroyed consistently.