Encryption wrapper

Question

I am writing an encryption class using Crypto++. The idea is that the client connects using RSA (encrypt, decrypt) with username, password, 32 byte key. The server will then return using the stream cipher (simEncrypt, simDecrypt) a session ID. Then any communication between the systems uses the stream cipher.

What would I liked reviewed:

1. Security: Is this a secure way of communicating?
2. General: Can the code be easily read and formatting?

#pragma once
#include <cstdlib>
#include <crypto++/rsa.h>
#include <crypto++/osrng.h>
#include <crypto++/base64.h>
#include <crypto++/files.h>
#include <crypto++/salsa.h>
using namespace CryptoPP;
class encryption {
 std::string privFileKeyStr;
 std::string publicFileKeyStr;
 RSAES_OAEP_SHA_Decryptor privKey;
 RSAES_OAEP_SHA_Encryptor pubKey;
 AutoSeededRandomPool rng;
 const byte msgGroupDecryptedLength = 40;
 unsigned int msgGroupEncryptedLength;
 std::string SimEncryptDo(std::string key, std::string strShortString, byte iv[8]) {
 byte *plaintextBytes = (byte *)strShortString.c_str();
 byte *ciphertextBytes = new byte[strShortString.length()];
 byte* keyBytes = (byte *)key.substr(0, 32).c_str();
 Salsa20::Encryption salsa;
 salsa.SetKeyWithIV(keyBytes, 32, iv);
 salsa.ProcessData(ciphertextBytes, plaintextBytes, strShortString.length());
 std::string returnStr((const char *)ciphertextBytes, strShortString.length());
 salsa.SetKeyWithIV(keyBytes, 32, iv);
 salsa.ProcessData(plaintextBytes, ciphertextBytes, strShortString.length());
 delete ciphertextBytes;
 return returnStr;
 }
public:
 encryption()
 {
 privFileKeyStr = "privkey.txt";
 publicFileKeyStr = "pubkey.txt";
 }
 encryption(std::string privKeyStr, std::string pubKeyStr) {
 privFileKeyStr = privFileKeyStr;
 publicFileKeyStr = publicFileKeyStr;
 }
 void GenerateKey()
 {
 AutoSeededRandomPool rng;
 InvertibleRSAFunction privkey;
 privkey.Initialize(rng, 1024);
 Base64Encoder privkeysink(new FileSink(privFileKeyStr.c_str()));
 privkey.DEREncode(privkeysink);
 privkeysink.MessageEnd();
 RSAFunction pubkey(privkey);
 Base64Encoder pubkeysink(new FileSink(publicFileKeyStr.c_str()));
 pubkey.DEREncode(pubkeysink);
 pubkeysink.MessageEnd();
 }
 void LoadKeys() {
 RSAES_OAEP_SHA_Encryptor lPubKey(
 FileSource(publicFileKeyStr.c_str(), true, new Base64Decoder)
 );
 pubKey = lPubKey;
 msgGroupEncryptedLength = (unsigned int)pubKey.CiphertextLength(5);
 RSAES_OAEP_SHA_Decryptor lPrivKey(
 FileSource(privFileKeyStr.c_str(), true, new Base64Decoder)
 );
 privKey = lPrivKey;
 }
 std::string Encrypt(std::string strShortString) {
 std::string Builder = "";
 std::string stringPart = "";
 for (unsigned int i = 0; i < (unsigned int)strShortString.size(); i = i + msgGroupDecryptedLength) {
 stringPart = strShortString.substr(i, msgGroupDecryptedLength);
 byte randLength = rng.GenerateByte() / 8;
 byte msgLength = (byte)stringPart.size();
 SecByteBlock randomPadLeft(randLength);
 rng.GenerateBlock(randomPadLeft, randLength);
 std::string randomPadLeftStr((const char*)randomPadLeft.begin(), randomPadLeft.size());
 std::string toEncrypt(1, (unsigned char)randLength);
 toEncrypt.append(std::string(1, (unsigned char)msgLength));
 toEncrypt.append(randomPadLeftStr);
 toEncrypt.append(stringPart);
 size_t paddingRight = 80 - toEncrypt.size();
 SecByteBlock randomPadRight(paddingRight);
 rng.GenerateBlock(randomPadRight, paddingRight);
 std::string randomPadRightStr((const char*)randomPadRight.begin(), randomPadRight.size());
 toEncrypt.append(randomPadRightStr);
 SecByteBlock sbbCipherText(msgGroupEncryptedLength);
 pubKey.Encrypt(
 rng,
 (byte const*)toEncrypt.data(),
 toEncrypt.size(),
 sbbCipherText.begin());
 Builder.append(std::string((const char*)sbbCipherText.begin(), sbbCipherText.size()));
 }
 return Builder;
 }
 std::string Decrypt(std::string strShortString) {
 std::string decryptedBuilder;
 for (unsigned int i = 0; i < (unsigned int)strShortString.size(); i = i + msgGroupEncryptedLength) {
 std::string msgGroup = strShortString.substr(i, msgGroupEncryptedLength);
 SecByteBlock sbbCipherText(msgGroupEncryptedLength);
 privKey.Decrypt(
 rng,
 (byte const*)msgGroup.data(),
 msgGroup.size(),
 sbbCipherText.begin());
 std::string decrypted((const char*)sbbCipherText.begin(), sbbCipherText.size());
 byte ind1 = decrypted.at(0);
 byte ind2 = decrypted.at(1);
 decryptedBuilder.append(decrypted.substr(ind1 + 2, ind2));
 }
 return decryptedBuilder;
 }
 std::string simDecrypt(std::string key, std::string strShortString) {
 std::string keyStr = strShortString.substr(0, 8);
 std::string encryptStr = strShortString.substr(8, strShortString.size() - 8);
 byte* iv = (byte*)keyStr.c_str();
 return SimEncryptDo(key, encryptStr, iv);
 }
 std::string simEncrypt(std::string key, std::string strShortString) {
 byte iv[8];
 rng.GenerateBlock(iv, 8);
 std::string encStr = SimEncryptDo(key, strShortString, iv);
 std::string returnStr(std::string((char *)iv, 8));
 returnStr.append(encStr);
 return returnStr;
 }
};

A brief explanation of AES and Salsa:

AES uses two keys. A message encrypted by one is decrypted by the other but can only encrypt a short amount of characters (which is why the loop is in the two functions).

Salsa takes a key and IV and will encrypt the message. If you put the encrypted message in, it will return the message.

• SuperBiasedMan: Welcome to Code Review! It's helpful to edit your question to explain what you might like reviewed. eg. Performance, readability?
• Simon Forsberg: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .
• Christopher Oicles: delete ciphertextBytes; should be delete [] ciphertextBytes;

Answer 1

First of all - these all unnecessary white lines make the code very hard to read.

Second - don't use magic numbers. In your case, they are: 32, 1024, 8 and 80. Always store them as named variables.

What is the purpose of variables strShortString and encryptStr in simDecrypt method? As far as I understand, strShortString stores string to decrypt (so why the name does not tell anything about it?), and encryptStr stores... string to decrypt but without prefix and suffix. Consider something like stringToDecrypt. And think what is the meaning of the second variable (without first and last 8 characters) - and give it the more descriptive name.

There are some issues of naming convention. Some functions and variables have names starting with capital letter, some not. Try to follow one standard in the code. I recommend also using MSG_GROUP_DECRYPTED_LENGTH convention in const variables - this is quite often used standard and anyone who will read your code, will know that this is const variable. (By the way... are you using this variable anywhere?)

Answer 2

Do not incorporate the type into the variable name, the compiler already takes care of that, stating the type twice is redundant and harmul.

• Gabe Evans: I can't see how stating the type twice is harmful.
• Caridorc: @GabeEvans the compiler never checks the type inside the name, if you get distracted you can write string myInteger = "foo"; you cannot trust types inside names
• Gabe Evans: Honestly, a mistake like that would, to me, signal that I really need to take a break and regain my focus, after making the correction of course. That can't be all bad, right? :P
• Caridorc: @GabeEvans Two sources against your Hungarian notation: joelonsoftware.com/articles/Wrong.html stackoverflow.com/questions/111933/…