How to use crypto++ Blowfish correctly

I have been trying all day today to finde out how to read from a binary file and decrypt it.

In my file, the first 4 bytes is describing the file format, the 32 bytes after it is the header, which is encrypted with Blowfish.

so i wrote this code to be able to do this:

string file = "C:\\test.bin";    

byte *header = new byte[32];

FILE *data = fopen(file.c_str(), "r");

if(data == NULL)
{
    return 1; //Error opening file!
}

char type[6];

type[5] = 0;

if(fread(type, sizeof(type) - 1, 1, data) < 1)
{
    return 2;
}

if(strcmp(type, "ABCD") != 0)
{
    return 3;
}

if(fread(header, sizeof(header), 1, data) < 1)
{
    return 2; //Error reading file!
}

vector<byte> key;

key.push_back(0xAA);
key.push_back(0xBB);
key.push_back(0xCC);
key.push_back(0xDD);
key.push_back(0xAA);
key.push_back(0xBB);
key.push_back(0xCC);
key.push_back(0xDD);

ECB_Mode<Blowfish>::Decryption decryption(key.data(), key.size());

byte out[32];

decryption.ProcessData(out, header, 32);

FILE *outer =  fopen("C:\\out.bin", "w");

fwrite (out, sizeof(byte), sizeof(out), outer);

but this is not decrypting the data correctly.

What did i do wrong?

Solution

There's many things a bit smelly here

fopen should use "rb" and "wb" for binary mode
you should use memcmp instead of strcmp
you don't validate that fread actually read 4 bytes
you should prefer unsigned char for binary data (fewer pitfalls to do with sign extension and undefined behaviour on overflow)
if you're using C++, why use cstdlib, cstdio and cstring in the first place?
this is a bug
```
if(fread(header, sizeof(header), 1, data) < 1)
```
sizeof (header) is sizeof(byte*) here, not 32 as you seem to expect

Here's a quick review of the code in c++ style: Update added a length field for my real-life roundtrip test (see below).

decryptor.cpp:

#include <fstream>
#include <algorithm>
#include <iterator>
#include <crypto++/blowfish.h>
#include <crypto++/modes.h>

static std::vector<byte> const key { 's','e','c','r','e','t' };
static byte const SIGNATURE[] = "ABCD"; //{ 'A','B','C','D' };

int main()
{
    if (std::ifstream data {"test.bin", std::ios::binary})
    {
        char type[] = { 0, 0, 0, 0 };

        if (!data.read(type, 4))
        {
            return 2;
        }

        auto mismatch = std::mismatch(std::begin(SIGNATURE), std::end(SIGNATURE), std::begin(type));

        if (mismatch.first != std::end(SIGNATURE))
        {
            return 3;
        }

        uint32_t length = 0;
        if (!data.read(reinterpret_cast<char*>(&length), sizeof(length))) // TODO use portable byte-order
        {
            return 4;
        }

        std::vector<byte> const ciphertext { std::istreambuf_iterator<char>(data), {} };
        // to read 32 bytes: 
        // std::copy_n(std::istreambuf_iterator<char>(data), 32, std::back_inserter(ciphertext));

        assert(data.good() || data.eof());
        assert(ciphertext.size() >= length);
        assert(ciphertext.size() % CryptoPP::Blowfish::BLOCKSIZE == 0);

        CryptoPP::ECB_Mode<CryptoPP::Blowfish>::Decryption decryption(key.data(), key.size());

        std::vector<char> plaintext(ciphertext.size());

        decryption.ProcessData(reinterpret_cast<byte*>(plaintext.data()), ciphertext.data(), plaintext.size());
        plaintext.resize(length); // trim padding

        std::ofstream out("out.bin", std::ios::binary);
        out.write(plaintext.data(), plaintext.size());
    } else
    {
        return 1; //Error opening file
    }
}

I don't have a file yet to test it with.

Update So, I've made an encryptor.cpp now too.

echo "Hello world" | ./encryptor

results in a 40 byte file (sig + length + ciphertext = 4 + 4 + 32 = 40), in base64:

base64 test.bin
QUJDRAwAAABCaDMrpG0WEYePd7fI0wsHAQoNkUl1CjIBCg2RSXUKMg==

Now, decrypting that tests out fine. Note that I found that I needed to ensure padding was done to BLOCKSIZE, and as such I added a length field to store the actual size of the plaintext to avoid trailing garbage after decryption.

You can see the roundtrip by doing

echo 'Bye world!!' | ./encryptor && ./decryptor && cat out.bin

Which does indeed print the greeting back after decryption.

Note specifically the TODO's. You should probably use StreamTransformationFilter which adds padding as required.