patterncppMinor
Encryption wrapper
Viewed 0 times
encryptionwrapperstackoverflow
Problem
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 (
What would I liked reviewed:
```
#pragma once
#include
#include
#include
#include
#include
#include
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);
simEncrypt, simDecrypt) a session ID. Then any communication between the systems uses the stream cipher.What would I liked reviewed:
- Security: Is this a secure way of communicating?
- General: Can the code be easily read and formatting?
```
#pragma once
#include
#include
#include
#include
#include
#include
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);
Solution
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:
What is the purpose of variables
Consider something like
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
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?)Context
StackExchange Code Review Q#106517, answer score: 2
Revisions (0)
No revisions yet.