C++ AES Encryption Class

Question

I am working with OpenSSL and writing an AES256 encryption module. My knowledge of cybersecurity is not vast and I would like to know whether there are any obvious holes I'm leaving. The intent is for the user to construct an instance of AESEncDec passing a key. With that instance, they may encrypt and decrypt files using that key with methods defined in the class.

My main concerns:

1. In what ways could an attacker potentially compromise this program while it's running?
2. Does the module's class-based nature somehow weaken its security due to having instances of the class which could be inspected by another process?
3. To further test myself, I was considering adding another two layers of AES, encrypting somewhat like Cipher = ENC(K1, (ENC(K2, (ENC(K1, Plaintext)))). Would such a change aid security in any way?

Notes:

• I am aware that the encrypt and decrypt methods can be combined into one using different EVP functions, I only learned that a bit far into the process and decided to keep the functions separate for now.

• I reviewed a similar question, but the poster appeared to be using a different library, though I did note some of the responses such as adding validation functions for input as well as expanding the class to enable other sized keys. Since I had some additional questions, I made this review request.

aes_enc_dec_class.h:

#include <fstream>
#include <iostream>
#include <string>
#include <cstring>
#include <openssl/evp.h>
#include <openssl/err.h>
#include <openssl/aes.h>
#include <openssl/rand.h>
#include <sstream>

void handleErrors() {
    ERR_print_errors_fp(stderr);
    abort();
}

/**
 * @brief An instance of AESEncDec is tied to a key, with which
 * files can be encrypted or decrypted as desired.
 */
class AESEncDec {
        static const int KEYLEN = 32;
        static const int BLOCKSIZE = 16;
        static const int ITER_COUNT = 10000;
        unsigned char key[KEYLEN];
        unsigned char iv[BLOCKSIZE];
    public:
        AESEncDec(unsigned char* keybase);
        int encrypt_file(const char* path, 
            const char* out);
        int decrypt_file(const char* path,
            const char* out);
};

AESEncDec::AESEncDec(unsigned char* keybase) {
    // set key from keybase
    if (!(EVP_BytesToKey(EVP_aes_256_cbc(), EVP_md5(), NULL,
        keybase, strlen((const char *) keybase), ITER_COUNT, key, iv))) {
        fprintf(stderr, "Invalid key base.\n");
    }
}

int AESEncDec::encrypt_file(const char* path, const char* out) {
    // initialize/open file streams
    std::ifstream plaintext_file;
    std::ofstream ciphertext_file;
    plaintext_file.open(path, std::ios::in | std::ios::binary);
    ciphertext_file.open(out, std::ios::out | std::ios::binary | std::ios::trunc);

    // ensure file is open, exit otherwise
    if (!plaintext_file.is_open()) {
        fprintf(stderr, "Failed to open plaintext.\n");
        return -1;
    }

    // initialize encryption buffers
    unsigned char plaintext[BLOCKSIZE];
    unsigned char ciphertext[BLOCKSIZE + BLOCKSIZE]; // extra space for padding
    
    // initialize encryption context
    EVP_CIPHER_CTX *ctx;
    if (!(ctx = EVP_CIPHER_CTX_new()))
        handleErrors();

    // reinitialize iv to avoid reuse
    if (!RAND_bytes(iv, BLOCKSIZE)) {
        fprintf(stderr, "Failed to initialize IV");
        return -1;
    }

    // set cipher/key/iv
    if (1 != EVP_EncryptInit_ex(ctx, EVP_aes_256_cbc(), NULL, key, iv))     
        handleErrors();

    // for keeping track of result length
    int len;
    int cipherlen = 0;
    int bytes_read;

    // read and encrypt a block at a time, write to file
    while (1) {
        plaintext_file.read((char *) plaintext, BLOCKSIZE);
        bytes_read = plaintext_file.gcount();
        if (1 != EVP_EncryptUpdate(ctx, ciphertext, &len, plaintext, bytes_read))
            handleErrors();
        
        ciphertext_file.write((char *) ciphertext, len);
        cipherlen+=len;
        if (bytes_read < BLOCKSIZE) break;
    }

    // finalize encryption
    if (1 != EVP_EncryptFinal_ex(ctx, ciphertext, &len))
        handleErrors();

    // write final block
    ciphertext_file.write((char *) ciphertext, len);
    cipherlen += bytes_read;

    // clean up
    ciphertext_file.close();
    plaintext_file.close();
    EVP_CIPHER_CTX_free(ctx);
    return cipherlen;
} 

int AESEncDec::decrypt_file(const char* path, 
    const char* out) {
    
    // open files for reading and writing
    std::ifstream ciphertext_file;
    std::ofstream plaintext_file;
    ciphertext_file.open(path, std::ios::in | std::ios::binary);
    plaintext_file.open(out, std::ios::out | std::ios::binary | std::ios::trunc);

    // if opening failed, exit
    if (!ciphertext_file.is_open() || !plaintext_file.is_open()) {
        fprintf(stderr, "One of the files is already open.\n");
        return -1;
    }
    
    // initialize cipher context
    EVP_CIPHER_CTX *ctx;
    if (!(ctx = EVP_CIPHER_CTX_new()))
        handleErrors();

    // reinitialize iv to avoid reuse
    if (!RAND_bytes(iv, BLOCKSIZE)) {
        fprintf(stderr, "Failed to initialize IV");
        return -1;
    }

    // initialize decryption 
    if (1 != EVP_DecryptInit_ex(ctx, EVP_aes_256_cbc(), NULL, key, iv))
        handleErrors();
    
    // keeping track of length of result
    int len;
    int plaintext_len = 0;

    // initialize cipher/plaintext buffers
    unsigned char plaintext[BLOCKSIZE+BLOCKSIZE], ciphertext[BLOCKSIZE];    

    int bytes_read;

    // go through the file one block at a time
    while (1) {
        ciphertext_file.read((char *) ciphertext, BLOCKSIZE);
        bytes_read = ciphertext_file.gcount();

        // decrypt block
        if (1 != EVP_DecryptUpdate(ctx, plaintext, &len, ciphertext, bytes_read))
            handleErrors();
        plaintext_len += len;
        plaintext_file.write((char *) plaintext, len);
        if (bytes_read < BLOCKSIZE) break;        
    } 

    if (1 != EVP_DecryptFinal_ex(ctx, plaintext + len, &len))
        handleErrors();

    plaintext_file.write((char*) plaintext, len);
    plaintext_len += len;
    
    // clean up
    EVP_CIPHER_CTX_free(ctx);
    plaintext_file.close();
    ciphertext_file.close();

    return plaintext_len;
}

test_aes_enc_dec.cc:

#include "aes_enc_dec_class.h"
#include <iostream>
#include <stdio.h>
#include <string>
#include <fstream>

int main (void) {
    unsigned char *key = (unsigned char *) "HardcodedKey!";
    AESEncDec cipher(key);

    const char* test_file_path = "test.txt";
    // open test file for writing, clearing it
    std::ofstream test_file;
    test_file.open(test_file_path, std::ios::out | std::ios::trunc);

    // put a test string into the file
    const char *test_string = "This is our test string!";
    const int test_len = strlen(test_string);
    test_file.write(test_string, test_len);
    test_file.close();

    // encrypt the file
    int enc_result = cipher.encrypt_file("test.txt", "test.enc");
    printf("Encrypted %d bytes.\n", enc_result);
    
    // decrypt the file 
    int dec_result = cipher.decrypt_file("test.enc", "test.dec");
    printf("Decrypted %d bytes.\n", dec_result);

    // check output contents
    std::ifstream result;
    result.open("test.dec", std::ios::in | std::ios::binary);
    char in[test_len + 1];
    result.read(in, test_len);
    in[test_len] = '\0';
    printf("Plaintext: %s\n"
        "Decrypted plaintext: %s\n", test_string, in);
    
    // report results
    int diff;
    if ((diff = strcmp(test_string, in)) != 0) {
        printf("Test failed: difference %d\n", diff);
        return -1
    } else printf("Test passed.\n");
    return 0;
}

Makefile

LIBS = -lssl -lcrypto
CFLAGS = -g -o
all: clean test_AES
test_AES:
    g++ $(CFLAGS) test_AES test_aes_enc_dec.cc $(LIBS)
clean:
    rm -f test_AES test.txt test.enc test.dec

Answer 1

Use authenticated encryption

Your code uses CBC mode to encrypt the file, without adding any authentication code. This means that if an attacker flips some bits in the encrypted file, then decryption still works without any errors, but the decrypted file will no longer match the original plaintext. Therefore, an attacker can perform a bit-flipping attack that goes unnoticed.

The typical solution is to add a message authentication code to the ciphertext, or as is more common nowadays, using an authenticated encryption mode instead. This ensures that when decoding, you can verify that the ciphertext was not modified in any way.

Missing error checking

You are only checking if a file was opened correctly, not whether it was completely read or written succesfully. For reading, check that file.eof() is true after reading all data, if not you did not reach the end of the file. For writing, check that file.good() is true.

Reduce the responsibility of each function

encrypt_file() takes two filenames, and is in charge of both opening those files, reading and writing to them, and the actual encryption. I would try to reduce the number of things that function does. Instead of passing filenames, consider passing a std::istream and a std::ostream instead, so encrypt_file() no longer has to open and close files, and will then also work for any stream object, like a std::stringstream, or even read from std::cin and write to std::cout directly.

Be careful when modifying crypto algorithms

To further test myself, I was considering adding another two layers of AES, encrypting somewhat like Cipher = ENC(K1, (ENC(K2, (ENC(K1, Plaintext)))). Would such a change aid security in any way?

You should be very careful when you start doing things like that. As JDługosz already mentioned, this might not be better than just having a single layer of encryption. Furthermore, some combinations might actually reduce security. Also consider that internally most encryption algorithms already apply cryptographic operations multiple times already, called "rounds". The number of rounds is chosen so it offers a decent margin of protection against attacks to the encryption algorithm.

Encrypting with multiple independent encryption algorithms might be fine, and protects against a fatal flaw in a single algorithm, although this will also complicate things, which in itself is an undesired property for security-conscious programs.

Answer 2

To further test myself, I was considering adding another two layers of AES, encrypting somewhat like Cipher = ENC(K1, (ENC(K2, (ENC(K1, Plaintext)))). Would such a change aid security in any way?

This is overkill for AES-256 which is secure for more than twenty years of attacks. Triple encryption is suggested for weak cipher like DES which has already another weak property of having small blocksize (64-bit block size and 56-bit key size) to be applicable for the sweet32 type birthday attacks.

AES-256 is bruteforce, multi-target, and quantum safe.

• Searching 256-bit space is impossible.
• Multi-target attacks become infeasible when the keys size is 256
• Grover's quantum search attack can reduce the security 128-bit yet the number of oracle calls is infeasible to implement.

So you don't need triple encryption that will require you to store two independent keys, two. One AES-256 is enough.

Actually, any good block cipher like ChaCha20 with 256-bits of the key is safe.

EVP_aes_256_cbc

You are using AES in CBC mode that requires random and unpredictable IV. With CBC mode you can have at most Ind-CPA secure. CBC mode needs padding like PKCS#7. This padding can cause padding oracle attacks, which are applied many times. Since TLS 1.3 we don't have CBC, it is gone forever.

You should use modern cipher modes as TLS 1.3 does. All of the TLS 1.3 modes internally use CTR mode together with an authentication mechanism like AES-GCM and ChaCha20-1305. If properly used they can provide confidentiality, integrity, and authentication whereas CBC can only provide confidentiality.

Neither AES-GCM nor ChaCha20-Poly1305 is perfect, however, apart from the AES-NI performance, ChaCha20-Poly1305 is faster, easy to use, and easy to implement side-channel resisted than AES. If not restricted to AES, use xChaCha20-Poly1305 that supports 192-bit random nonces that enable to use of a single key for a long time.

Answer 3

To further test myself, I was considering adding another two layers of AES, encrypting somewhat like Cipher = ENC(K1, (ENC(K2, (ENC(K1, Plaintext)))). Would such a change aid security in any way?

Maybe. It depends on whether the ENC(k) forms a group. Perhaps there is a single ENC(K3, Plaintext) that gives the same transformation, in which case an attacker will not notice your multiple stages at all.

The right way is to compute two different keystreams using the different encryptions (with block cyphers, can use counter mode to turn it into a stream cypher), then XOR the two keystreams together and use that to encode the plaintext. Read Applied Cryptography for details.

This is provably at least as secure as the more secure of the two individual encodings. That is, it will protect against one of the keys being weak, and will force the attacker to crack both keys. It's probably far harder since they have to be solved together.

However, this is best done using two different encryption algorithms or implementations, to protect against one of them being broken. For example, use AES and TwoFish which is unrelated and probably doesn't share the same weaknesses.

---

fprintf(stderr, "Failed to initialize IV");

Isn't this supposed to be C++? Why would you use a C function, and for that matter one that scans for format replacement codes when it's just a plain string?

NULL

Likewise, don't use the C NULL macro. C++ has a real null pointer, nullptr.

Use std::filesystem::path for the file names, not const char*. Don't pass around const char* at all; use string_view instead, generally.

Use constexpr instead of static const when you mean for the value to be known at compile time.

Don't use int for sizes. Here, you want streamsize.

int main (void) {

Don't ever write (void) for a parameter list in C++. That accepted only for C compatibility, and Strustrup calls it "an abomination". See https://en.cppreference.com/w/cpp/language/main_function for the two ways to declare main.

Answer 4

Makefile review

Make more use of built-in rules.

We should be assigning CXX and CXXFLAGS (instead of CFLAGS), then we can use the built-in rule to compile C++ code from source.

-o on its own isn't a valid flag, and will break as soon as we write CXXFLAGS += -Wall on a later line. That's why the built-in rule has -o $@, so the option and its argument can't be separated.

The program won't be rebuilt if its source or the header is updated, because we haven't specified any dependencies. If we give the source file the same name as the program, but with .cc appended, then we can let Make use its built-in rule and pick up the source file dependency, and we'll only have to tell it about the header.

Assuming we have pkg-config, we should be using that to specify the compilation flags for OpenSSL, so that there's nothing to do if those change next time we upgrade.

And we should turn on more compilation warnings. GCC supports lots of warnings, and we should be using them to help us improve the code.

The clean target should be marked .PHONY so that its commands will run even if a file of that name is present.

---

Improved Makefile

all: test_aes_enc_dec

CXX = g++
CXXFLAGS += -std=c++20
CXXFLAGS += -g
CXXFLAGS += -Wall -Wextra -Wwrite-strings -Weffc++
CXXFLAGS += -Wpedantic -Warray-bounds -Wconversion
CXXFLAGS += -Wno-parentheses
CXXFLAGS += $(shell pkg-config --cflags openssl)
LINK.o = $(LINK.cc)
LDLIBS += $(shell pkg-config --libs openssl)

test_aes_enc_dec.o: aes_enc_dec_class.h

clean:
        $(RM) test_aes_enc_dec *.o test.*

.PHONY: clean

Answer 5

Interface

    int encrypt_file(const char* path,
        const char* out);
    int decrypt_file(const char* path,
        const char* out);

As well as being a poor separation of concerns, this interface is very limiting, because we can only read from and write to named files. We could be more flexible if we provide functions that accept a std::ostream& and a std::istream&; we'd be able to build the above functions from those, as well as to work with network streams and to create real unit tests. A test that depends on external resources such as filesystem isn't a unit test.

In fact, the convenience functions that open files could even be a single function - pass in the stream operation as a pointer to member function.

The return type int is likely too small for file sizes in general - prefer std::streamsize for that.

---

    AESEncDec(unsigned char* keybase);

Is there a good reason we can't accept const unsigned char* here?

In the definition, I'd prefer to see a proper C++ cast where we convert to const char*, as that protects against casting away the const.

---

Error handler

void handleErrors() {
    ERR_print_errors_fp(stderr);
    abort();
}

That's a very inflexible way to deal with errors - the calling program might have a much better strategy (e.g. asking user for a different output file, rather than losing all their work).

We haven't included <cstdio> for the definition of stderr.

We haven't included <cstdlib> (I'm assuming abort() is meant to be std::abort()), and in any case this is a dangerous function to use in C++, because it immediately exits without running any destructors or atexit functions.

---

   fprintf(stderr, "Failed to open plaintext.\n");

We haven't included <cstdio> for a definition of std::printf(). But why not just stream to std::cerr? That's in <iostream>, which we have already included.

---

File handling

std::ifstream plaintext_file;
std::ofstream ciphertext_file;
plaintext_file.open(path, std::ios::in | std::ios::binary);
ciphertext_file.open(out, std::ios::out | std::ios::binary | std::ios::trunc);

Input and output streams automatically include std::ios::in and std::ios::out (respectively) in their options, so no need to specify those.

More importantly, although we check that we open the input file, that seems to be the only operation that's checked. There's no error handling for failed reads, and no checking at all on the output file (and writing is more likely to fail, e.g. due to permissions or device-full).