C++ program to encrypt hardcoded text

Question

This code takes in a hardcoded input buffer, maximum 64 chars, encrypts, decrypts and outputs decrypted buffer using AES CBC mechanism. Entered password is converted to a SHA256 digest and copied into a key Buffer.

Enter password: 7 er87 323nc37z3223
IV: 3EEBA5E2CDB7CBA525849CE812C2648
SHA256 password: 7902699be42c8a8e46fbbb4501726517e86b22c56a189f7625a6da49081b2451
Key buffer: 7902699BE42C8A8E46FBBB4501726517E86B22C56A189F7625A6DA49081B2451
Inp data: HELLO EQ 123566 ABCDEF 1211 34567
Encrypting...
Decrypting...
Decrypted output...
HELLO EQ 123566 ABCDEF 1211 34567

Entry point in main.cpp file

I know about the missing error handling in Crypto operations like new, free, etc with EVP APIs. Was lazy. :) Other than that, could you point out if there are some cpp specific problems in the code? The program works as expected. But I would like to improve my Cpp and programming skills in general. I also want to expand this to handle files, where I can input files to encrypt and outputs an encrypted file. I think it should be doable with the current design? What do you think?

main.cpp

#include "aescrypto.h"
int main() {
 std::string file_password;
 std::cout << "Enter password: ";
 std::cin >> file_password;
 FileCryptoAES objAESCrypto(file_password);
 unsigned char inBuffer[] = "HELLO EQ 123566 ABCDEF 1211 34567";
 // Initialize a vector from the array
 std::vector<std::uint8_t> input_vec(std::begin(inBuffer), std::end(inBuffer));
 std::cout << "Inp data: " << input_vec.data() << std::endl;
 std::cout << "Encrypting..." << std::endl;
 objAESCrypto.encrypt(input_vec);
 std::cout << "Decrypting..." << std::endl;
 objAESCrypto.decrypt();
 std::cout << "Decrypted output..." << std::endl;
 objAESCrypto.print_decrypted_buff();
 return 0;
}

aescrypto.cpp

#include "aescrypto.h"
#include <iomanip>
#include <openssl/evp.h>
#include <openssl/rand.h>
void FileCryptoAES::hex_to_bytes(const std::string &hex) {
 unsigned char bytes[kLenKey];
 for (size_t i = 0; i < kLenKey; i++) {
 sscanf(hex.c_str() + (i * 2), "%2hhx", &bytes[i]);
 }
 // copy from unsigned char buffer to vector
 mKeyBuffer.assign(bytes, bytes + kLenKey);
 std::cout << "Key buffer: ";
 for (int i = 0; i < kLenKey; i++)
 printf("%02X", mKeyBuffer[i]);
 printf("\n");
}
bool FileCryptoAES::calc_sha256() {
 std::string hashedpass;
 EVP_MD_CTX *mdCtx = EVP_MD_CTX_new();
 unsigned char mdVal[EVP_MAX_MD_SIZE];
 unsigned int mdLen;
 EVP_DigestInit_ex(mdCtx, EVP_sha256(), NULL);
 EVP_DigestUpdate(mdCtx, get_file_password().c_str(),
 get_file_password().length());
 EVP_DigestFinal_ex(mdCtx, mdVal, &mdLen);
 EVP_MD_CTX_free(mdCtx);
 std::stringstream ss;
 for (unsigned int i = 0; i < mdLen; ++i) {
 ss << std::hex << std::setw(2) << std::setfill('0') << (int)mdVal[i];
 }
 hashedpass = ss.str();
 set_hashed_pass(hashedpass);
 return true;
}
void FileCryptoAES::generate_key_from_pass() {
 calc_sha256();
 std::cout << "SHA256 password: " << get_hashed_pass() << std::endl;
 hex_to_bytes(get_hashed_pass());
}
void FileCryptoAES::encrypt(const std::vector<std::uint8_t> &inp_vec) {
 unsigned char *pKeyBuff = &mKeyBuffer[0];
 unsigned char *pIVBuff = &mInitialisationVector[0];
 unsigned char *pOutBuff = &mOutputBuffer[0];
 const unsigned char *pInpBuff =
 reinterpret_cast<const unsigned char *>(inp_vec.data());
 EVP_CIPHER_CTX *enc_Ctx = EVP_CIPHER_CTX_new();
 EVP_EncryptInit_ex(enc_Ctx, EVP_aes_256_cbc(), NULL, pKeyBuff, pIVBuff);
 EVP_EncryptUpdate(enc_Ctx, pOutBuff, &mOutLen, pInpBuff, inp_vec.size());
 EVP_EncryptFinal_ex(enc_Ctx, pOutBuff + mOutLen, &mFinalOutLen);
 mOutLen += mFinalOutLen; // Ensure final output length is correct
 EVP_CIPHER_CTX_free(enc_Ctx);
}
void FileCryptoAES::decrypt() {
 unsigned char *pKeyBuff = &mKeyBuffer[0];
 unsigned char *pIVBuff = &mInitialisationVector[0];
 unsigned char *pOutBuff = &mOutputBuffer[0];
 unsigned char *pDecBuff = &mDecryptedBuffer[0];
 EVP_CIPHER_CTX *dec_ctx = EVP_CIPHER_CTX_new();
 EVP_DecryptInit_ex(dec_ctx, EVP_aes_256_cbc(), NULL, pKeyBuff, pIVBuff);
 EVP_DecryptUpdate(dec_ctx, pDecBuff, &mDecLen, pOutBuff, mOutLen);
 EVP_DecryptFinal_ex(dec_ctx, pDecBuff + mDecLen, &mFinalDecLen);
 mDecLen += mFinalDecLen;
 EVP_CIPHER_CTX_free(dec_ctx);
}
void FileCryptoAES::print_decrypted_buff() {
 for (size_t i = 0; i < mDecryptedBuffer.size(); i++)
 printf("%c", mDecryptedBuffer[i]);
 printf("\n");
}
void FileCryptoAES::fill_iv_buffer() {
 unsigned char *pIVBuffer = &mInitialisationVector[0];
 RAND_bytes(pIVBuffer, kIVLen);
 std::cout << "IV: ";
 for (int i = 0; i < kIVLen; i++)
 printf("%X", mInitialisationVector[i]);
 printf("\n");
}

aescrypto.h

#include <iostream>
#include <vector>
class FileCryptoAES {
private:
 static constexpr uint8_t kBuffSize = 64;
 static constexpr uint8_t kLenKey = 32;
 static constexpr uint8_t kIVLen = 16;
 std::string mfilePassword;
 std::string mhashedPassword;
 std::vector<std::uint8_t> mKeyBuffer;
 std::vector<std::uint8_t> mInitialisationVector;
 std::vector<std::uint8_t> mOutputBuffer;
 std::vector<std::uint8_t> mDecryptedBuffer;
 std::vector<std::uint8_t> mInputBuffer;
 int mOutLen, mFinalOutLen = 0;
 int mDecLen, mFinalDecLen;
 // Convert the first 32 hex characters (64 chars) into 32 raw bytes for AES
 // key
 void hex_to_bytes(const std::string &hex);
 const std::string get_file_password() { return mfilePassword; }
 bool calc_sha256();
 void set_hashed_pass(std::string &hashed_pass) {
 mhashedPassword = hashed_pass;
 }
 std::string get_hashed_pass() { return mhashedPassword; }
 void fill_iv_buffer();
 void generate_key_from_pass();
public:
 FileCryptoAES(std::string password)
 : mKeyBuffer(kLenKey), mInitialisationVector(kIVLen),
 mOutputBuffer(kBuffSize), mDecryptedBuffer(kBuffSize),
 mInputBuffer(kBuffSize) {
 mfilePassword = password;
 // Initialisation vector
 fill_iv_buffer();
 // Generate Key from entered password
 generate_key_from_pass();
 }
 void encrypt(const std::vector<std::uint8_t> &inp_vec);
 void decrypt();
 void print_decrypted_buff();
};

• Peter Cordes: Misleading title: the text you're encrypting isn't hardcoded into the source code, only the size of the input. (A fixed-size buffer is normal as long as you loop and reuse it until you've consumed all the input.)
• Abel Tom: @PeterCordes Sorry for the misunderstanding. unsigned char inBuffer[] = "HELLO EQ 123566 ABCDEF 1211 34567"; is hardcoded right? It is a predetermined input, right? I used fixed size buffer because of the crypto libs. And I plan to loop them when I input a file for instance.
• Peter Cordes: Oh, yes, that string variable in main is hardcoded. I was misled by the text talking about an "input buffer". Your whole program doesn't take any input, just your crypto function taking input from main. I'd probably avoid using "hardcoded" and "buffer" together that way; like the program feeds a hardcoded input to a function which takes a vector up to 64 bytes long.

Answer 1

Include guards

Header files should have include guards.

---

Error checking

Don't be lazy with error checking. If something bad can happen, it will, eventually. Probably at the least convenient time!

In C++, this is easier than C (mostly just check your file reads and writes if you don't enable exceptions on the streams).

For the C functions from OpenSSL, consider creating a C++ interface which can throw std::bad_alloc from failed allocation, and owns its resources (so that it cleans uup properly regardless of code path).

As a bare minimum, it's a good idea to use a smart pointer to ensure that "new" and "free" operations are paired, like this:

#include <memory>
bool FileCryptoAES::calc_sha256() {
 auto const mdCtx = std::unique_ptr<EVP_MD_CTX, void(*)(EVP_MD_CTX*)>(EVP_MD_CTX_new(), &EVP_MD_CTX_free);

---

Includes

aescrypto.h needs <cstdint> and <string>; doesn't need <iostream>.

aescrypto.cpp needs <cstdio> and <iostream>.

---

std namespace

We're using std::uint8_t, std::sscanf and std::printf without their namespace qualifiers. That's not something the standard allows us to assume.

---

Avoid fixed size buffers

When we construct a FileCryptoAES, we use fixed-size vectors for input and output buffers, despite knowing that input and output can be of arbitrary length.

This constructor should be explicit, and it should initialise mfilePassword (using std::move(password)) rather than assigning it after default-initialisation.

---

Unnecessary members

mFinalDecLen and mFinalOutLen don't store any useful state, and can be replaced by automatic variables in their respective function scopes.

We wouldn't need to store mfilePassword if we hashed it immediately, and only stored the hash. That also has a small security benefit if the password is re-used anywhere.

---

More const

get_file_password(), get_hashed_pass(), print_decrypted_buff() have no business modifying the file crypto object, so should be declared const.

get_file_password() current returns a const std::string, which is pointless - just return plain string.

---

Careful with formatting

fill_iv_buffer prints numbers back-to-back:

 for (int i = 0; i < kIVLen; i++)
 printf("%X", mInitialisationVector[i]);

That makes it impossible to distinguish between (e.g.) 12, 3 and 1, 23.

print_decrypted_buff() dumps the whole of mDecryptedBuffer, including lots of NUL characters following the text - even though we dutifully updated mOutLen when decrypting.

• Toby Speight: Sorry I don't have time to do a full review; these are just the superficial issues I noticed.
• Abel Tom: Thank you so much for taking the time. I did not understand the one with constructor, should it be declared like so? FileCryptoAES(std::string&& password) and then move it in constructor? I did not know you could pair new and free into a unique_ptr like that. Thats sweet! I used fixed size buffers because I want to enter a file of arbitrary length and process it in chunks. So that i can see exactly how many bytes come in, and are processed, etc. Just for predictability sake.
• Toby Speight: FileCryptoAES(std::string password) : password{std::move(password)} will copy the string's data only if necessary - if you pass an rvalue (perhaps implicitly, by passing a C-style string which converts), then that is moved; if you call with an lvalue, the constructor call copies into the argument. See the article Want speed? Pass by value for explanation and discussion.