AES128 Code review C++

Question

So for the past few days I've been working on a AES-128 encrypt/decipher class. I needed something very scaled down from Cryptolib so that I didn't have to constantly import the .lib file on all my programming computers (work, home, laptop1, laptop2) So i decided since I will only every use AES-128 for one my programs (related to NFC desfire stuff) I wanted to make a small and easily portable class. I have the following up for review. I asked a question at stack overflow when I was having a issue and one of the members mentioned that i had memory leaks. Well I'm a C#/Java programmer at heart and C++ is only a few months old in me so sorry about mistakes like that.

I don't have room to put the Galois files, I will however just put the mockup of it

Aes128.h

#pragma once

#include "ByteUtil.h"
#include "Galois.h"

class AES128
{
public:
    void SetKey(const BYTE* key);
    void SetIV(const BYTE* iv);
    void EncryptData(BYTE** outBlock, const BYTE* inBlock, size_t length);
    void DecryptData(BYTE** outBlock, const BYTE* inBlock, size_t length);
private:
    void EncryptBlock(BYTE* outBlock, const BYTE* inBlock, const BYTE* cipherBlock);
    void DecryptBlock(BYTE* outBlock, const BYTE* inBlock, const BYTE* cipherBlock);
    BYTE* Key;
    BYTE* IV;
};

Aes128.cpp

#include "stdafx.h"
#include "AES128.h"

/*
    Public Methods
*/
void AES128::SetKey(const BYTE* key)
{
    Key = (BYTE*)malloc(16);
    memcpy(Key, key, 16);
}
void AES128::SetIV(const BYTE* iv)
{
    IV = (BYTE*)malloc(16);
    memcpy(IV, iv, 16);
}

void AES128::EncryptData(BYTE** outBlock, const BYTE* inBlock, size_t length)
{
    float blockSize = (float)(length/16);
    blockSize = ceilf(blockSize);
    size_t newLength = (size_t)(blockSize*16);
    BYTE* temp = (BYTE*)malloc(newLength);
    BYTE* padd = (BYTE*)malloc(newLength);
    memset(temp, 0, newLength);
    memcpy(padd, inBlock, length);
    EncryptBlock(temp, padd, IV);
    for (int i=1; i<blockSize; i++)
    {
        EncryptBlock(&temp[i*16], &padd[i*16], &temp[(i-1)*16]);
    }
    *outBlock = (BYTE*)malloc(newLength);
    memcpy((*outBlock), temp, newLength);
}
void AES128::DecryptData(BYTE** outBlock, const BYTE* inBlock, size_t length)
{
    float blockSize = (float)(length/16);
    blockSize = ceilf(blockSize);
    size_t newLength = (size_t)(blockSize*16);
    BYTE* temp = (BYTE*)malloc(newLength);
    BYTE* padd = (BYTE*)malloc(newLength);
    memset(temp, 0, newLength);
    memcpy(padd, inBlock, length);
    DecryptBlock(temp, padd, IV);
    for (int i=1; i<blockSize; i++)
    {
        DecryptBlock(&temp[i*16], &padd[i*16], &temp[(i-1)*16]);
    }
    *outBlock = (BYTE*)malloc(newLength);
    memcpy((*outBlock), temp, newLength);
}
/*
    Private Methods
*/
void AES128::EncryptBlock(BYTE* outBlock, const BYTE* inBlock, const BYTE* cipherBlock)
{
    BYTE temp[16] = {0x00};
    Galois::XorBlock(temp, inBlock);
    Galois::XorBlock(temp, cipherBlock);

    BYTE expandedKey[176] = {0x00};
    memcpy(expandedKey, Key, 16);
    Galois::expand_key(expandedKey);

    Galois::XorBlock(temp, expandedKey);
    for(int i=16; i<160; i+=16)
    {
        Galois::DoRound(temp, &expandedKey[i]);
    }
    Galois::SubBytes(temp);
    Galois::ShiftRows(temp);
    Galois::XorBlock(temp, &expandedKey[160]);

    memcpy(outBlock, temp, 16);
}
void AES128::DecryptBlock(BYTE* outBlock, const BYTE* inBlock, const BYTE* cipherBlock)
{
    BYTE temp[16] = {0x00};
    Galois::XorBlock(temp, inBlock);

    BYTE expandedKey[176] = {0x00};
    BYTE* invExpandedKey;
    memcpy(expandedKey, Key, 16);
    Galois::expand_key(expandedKey);
    Galois::InvertExpandedKey(&invExpandedKey, expandedKey);
    Galois::XorBlock(temp, invExpandedKey);
    Galois::InvShiftRows(temp);
    Galois::InvSubBytes(temp);
    for(int i=0x10; i<0xA0; i+=0x10) //change i<0xA0
    {
        Galois::InvDoRound(temp, &invExpandedKey[i]);
    }
    Galois::XorBlock(temp, &invExpandedKey[160]);
    Galois::XorBlock(temp, cipherBlock);
    memcpy(outBlock, temp, 16);
}

Galois.h

class Galois
{
private:
    static BYTE gadd(const BYTE a, const BYTE b);
    static BYTE gmul(const BYTE a, const BYTE b);
    static BYTE gmul_Inverse(const BYTE in);
    static void gmul_mix_column(BYTE* row);
    static void inv_mix_column(BYTE *r);
    //AES Key Expansion Functions
    static void RotWord(BYTE* in);
    static BYTE Rcon(BYTE in);
    static void schedule_core(BYTE* in, BYTE i);
    static void expand_key(BYTE* in);
    static void InvertExpandedKey(BYTE** out, const BYTE* in);
    static void SubBytes(BYTE* dest);
    static void InvSubBytes(BYTE* dest);
    static void ShiftRows(BYTE* dest);
    static void InvShiftRows(BYTE* dest);

    static void MixColumns(BYTE* row);
    static void InvMixColumns(BYTE* row);
    static void xorRow(BYTE* row1, const BYTE* row2);
    static void XorBlock(BYTE* block1, const BYTE* block2);
    static void SubWord(BYTE* in);
    static void SubBytes(BYTE* dest);
    static void InvSubWord(BYTE* in);
    static void InvSubBytes(BYTE* dest);
public:
    static void DoRound(BYTE* dest, const BYTE* roundKey);
    static void InvDoRound(BYTE* dest, const BYTE* roundKey);
};

I plan on refactoring Galois a bit to seperate some of the Rijandel methods and then just use the math from Galois. That should clean it up nicely. So how bad is it?

Answer 1

Before I say anything else, I feel obligated to say that you should use the library. Writing bug-free Crypto code is hard and dangerous. The rule for Crypto code is generally that you shouldn't write it unless you are an expert (and even then, you want a LOT of eyes on it to check it). If this code is going to be used in anything remotely serious, use the library!

With that caveat, there are a few problems with your code.

Memory Leaks

You dynamically allocate both:

Key = (BYTE*)malloc(16);

and

IV = (BYTE*)malloc(16);

Which are both never freed. Firstly, you should prefer new to malloc in C++ code. You also need to remember the rule that whatever you malloc/new, you need to free/delete (unless you're passing a pointer back to the user, in which case it is up to them to do this. This should be infrequent, however). This is generally done in the destructor:

AES128::~AES128()
{
    if(Key != nullptr)
        free(Key);
    if(IV != nullptr)
        free(IV);
}

With delete, the NULL checks are implicit and therefore redundant.

However, in this case, there's not really any reason to use the free store, you should likely just automatically allocate both of these:

private:
    static const std::size_t k_blockSize = 16;
    BYTE Key[k_blockSize];
    BYTE IV[k_blockSize];

You also pass in a BYTE * which you memcpy from, but always only copy 16 bytes, which should be a named constant. I'd also prefer std::copy over memcpy. I'd probably write this like so:

#include <algorithm> //For std::copy

class AES128
{
public:
    void SetKey(const BYTE* key);
    void SetIV(const BYTE* iv);
    //Other public functions
private:
    //Private functions
    static const std::size_t k_blockSize = 16;
    BYTE Key[k_blockSize];
    BYTE IV[k_blockSize];
};

void AES128::SetKey(const BYTE* key)
{
    std::copy(key, key + k_blockSize, Key);
}

void AES128::SetIV(const BYTE* iv)
{
    std::copy(iv, iv + k_blockSize, IV);
}

Namespaces

Firstly, your code seems like it won't compile currently. In Aes128.cpp, you're utilizing things like Galois::XorBlock which is marked as private. Perhaps these functions you're using are meant to be public?

Now, while everything in Java and C# must be a class, C++ has no such restriction. There is no reason to make a class with all static members - they should be free functions within a namespace instead. If you want to make them private, you can put them in an unnamed namespace in a .cpp file. So your Galois class can be rewritten like so:

In Galois.h:

namespace Galois
{
    void DoRound(BYTE* dest, const BYTE* roundKey);
    void InvDoRound(BYTE* dest, const BYTE* roundKey);
}

In your Galois.cpp:

#include "Galois.h"

namespace 
{
    BYTE gadd(const BYTE a, const BYTE b) { ... }
    BYTE gmul(const BYTE a, const BYTE b) { ... }
    BYTE gmul_Inverse(const BYTE in) { ... }
    void gmul_mix_column(BYTE* row) { ... }
    void inv_mix_column(BYTE *r) { ... }
    //AES Key Expansion Functions
    void RotWord(BYTE* in) { ... }
    BYTE Rcon(BYTE in) { ... }
    void schedule_core(BYTE* in, BYTE i) { ... }
    void expand_key(BYTE* in) { ... }
    void InvertExpandedKey(BYTE** out, const BYTE* in) { ... }
    void SubBytes(BYTE* dest) { ... }
    void InvSubBytes(BYTE* dest) { ... }
    void ShiftRows(BYTE* dest) { ... }
    void InvShiftRows(BYTE* dest) { ... }

    void MixColumns(BYTE* row) { ... }
    void InvMixColumns(BYTE* row) { ... }
    void xorRow(BYTE* row1, const BYTE* row2) { ... }
    void XorBlock(BYTE* block1, const BYTE* block2) { ... }
    void SubWord(BYTE* in) { ... }
    void SubBytes(BYTE* dest) { ... }
    void InvSubWord(BYTE* in) { ... }
    void InvSubBytes(BYTE* dest) { ... }
}

void Galois::DoRound(BYTE* dest, const BYTE* roundKey) { ... }
void Galois::InvDoRound(BYTE* dest, const BYTE* roundKey) { ... }

These are called similarly to how you have them now, for example, Galois::DoRound and so on. Note that the functions within the unnamed namespace in Galois.cpp won't be accessible outside of that file, which is exactly what you have now - as mentioned earlier, I'm not sure if this is what you want, as you're calling them from (non-internal) places.