Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
add comment

1 Answer 1

up vote 2 down vote accepted

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.

share|improve this answer
    
thank you very much for your very thoughtful comments. This will most certainly help me. One thing that still gets me is the use of double pointers. The two places that I had this code in a library, which I was calling from a C# code (dumb idea it was...which i gave up on, and just used the C# to call a TestLibrary method in the library which ran over 600+ tests on it from nist.gov) so what I had posted was my attempt to copy it over to where it was going to live. You were very right about the private/public bit (it didn't compile) I'll try out what you have for me. Thank you! –  Robert Snyder Feb 2 '13 at 16:38
    
+1 for stressing reuse of a library instead of self implementing. As a learning exercise it's fine, but if at all possible use a trusted library. –  Jared Paolin Feb 3 '13 at 4:51
    
@JaredPaolin since you are the second one to mention using the library as apposed to making my own it impels me to ask why. if i have unit tests that prove that my method works from 2 different sources (Nist.gov and NXP) then why is it so wrong to make my own? In this case once i fix my memory leaks it will allow me to compile my program as a static library again instead of a shared library ( desired by me and my client). I'm very interested in your reason behind using the library, if you could please elaborate. –  Robert Snyder Feb 3 '13 at 21:33
2  
@RobertSnyder I'm going to steal some words from Yacoset: "Never write your own implementation of a well-known cipher. Even if the algorithm is a standard like AES, the algorithm is just a mathematical concept, meaning any given implementation could have weaknesses that come from things you never knew about your language, platform, or even the hardware (such as side-channel attacks). Somebody else has figured these things out already, such as the vendor or a dedicated third party." –  Yuushi Feb 4 '13 at 1:38
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.