AES-128 encrypt/decipher class

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). I've decided that 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. 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++)
{

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  //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.

StackExchange Code Review Q#21200, answer score: 4