patterncppMinor
Encryption with many algorithms using Crypto++
Viewed 0 times
encryptionwithalgorithmscryptousingmany
Problem
For the last few days I have been working on this code to develop a class that allow me to crypt a block of byte with many algorithme.
One block will be divided into three equal blocks and every block will be encrypted and decrypted by one of the three algorithme.
I want to discover the main weak points of my code especially concerning cryptography within Crypto++.
It is my first prototype and I'm certain there are many point to discuss in this code.
rServer.cpp :
```
#include
#include "../cryptopp562/sha.h"
#include "../cryptopp562/filters.h"
#include "../cryptopp562/hex.h"
#include
#include
#include "../cryptopp562/cryptlib.h"
using CryptoPP::Exception;
#include "../cryptopp562/hex.h"
using CryptoPP::HexEncoder;
using CryptoPP::HexDecoder;
#include "../cryptopp562/filters.h"
using CryptoPP::StringSink;
using CryptoPP::StringSource;
using CryptoPP::StreamTransformationFilter;
#include "../cryptopp562/des.h"
using CryptoPP::DES_EDE2;
#include "rServer.h";
#include "../cryptopp562/modes.h"
using CryptoPP::CBC_Mode;
#include "../cryptopp562/secblock.h"
using CryptoPP::SecByteBlock;
#include
#include
#include "../cryptopp562/modes.h"
#include
One block will be divided into three equal blocks and every block will be encrypted and decrypted by one of the three algorithme.
I want to discover the main weak points of my code especially concerning cryptography within Crypto++.
It is my first prototype and I'm certain there are many point to discuss in this code.
#include "..\PanawaraReader\common.h"
#include "cryptopp_wrapper.h"
class rServer {
public:
public:
/**
rServer class has for roles :
- Creation of a crypted files.
- Creation of Client Applications.
*/
// MILESTONE 1
/**
Returns a crypted block of 1024 bytes with ECB algorithm.
@param _data input array of 1024 bytes.
@return crypted array of 1024 bytes.
*/
DATA CryptBlockWithAESmodeCBC(char _data[1024]);
/**
Returns a crypted block of 1024 bytes with Blowfish algorithm.
@param _data input array of 1024 bytes.
@return crypted array.
*/
DATA CryptBlockWithBlowfish(char _data[1024]);
/**
Returns a crypted block of 1024 bytes with RSA algorithm.
@param _data input array of 1024 bytes.
@return crypted array of 1024 bytes.
*/
DATA CryptBlockWithRSA(char _data[1024]);
};rServer.cpp :
```
#include
#include "../cryptopp562/sha.h"
#include "../cryptopp562/filters.h"
#include "../cryptopp562/hex.h"
#include
#include
#include "../cryptopp562/cryptlib.h"
using CryptoPP::Exception;
#include "../cryptopp562/hex.h"
using CryptoPP::HexEncoder;
using CryptoPP::HexDecoder;
#include "../cryptopp562/filters.h"
using CryptoPP::StringSink;
using CryptoPP::StringSource;
using CryptoPP::StreamTransformationFilter;
#include "../cryptopp562/des.h"
using CryptoPP::DES_EDE2;
#include "rServer.h";
#include "../cryptopp562/modes.h"
using CryptoPP::CBC_Mode;
#include "../cryptopp562/secblock.h"
using CryptoPP::SecByteBlock;
#include
#include
#include "../cryptopp562/modes.h"
#include
Solution
I have never used Crypto++, but can give you several suggestion on how
to improve the overall style and quality of your code.
Missing an include guard:
Your header file doesn't have an include guard. You will need one
to be able to include the header in other
Type and variable naming:
It is unusual to start the name of a type in C++ with a lower-case letter (
Regarding the
Declaring variables at the top of a function:
Declaring all variables at the beginning of a function is considered
bad practice in C++. Variables should be declared when they are first used
or in a way to be visible to the needed scope(s) only. Variables declared
at the top of a function are like mini-globals.
Also, I would group all the
the
Using
Your code is cluttered with log calls to
Magic constants:
What are these supposed to mean?
And these?
That's pretty unclear. Define named constants for those, using a
Using old C-string functions:
Mixing
I suspect you did this because
internally. If that is the case, can't you also use an
for it? It would also be more efficient since you could then avoid
a string copy altogether by just using C++11's
Avoid C-style casts:
C-style casts are ugly, unsafe and provide no compiler diagnostics.
Always use the proper C++ cast operators. The cast above
is wrong BTW. You should never cast away the constess
of a pointer returned by
Clear commented out code:
You have a few commented out blocks. Please remove those
once you are done experimenting. This is only visual pollution.
Be consistent with your comments and var names:
If you are writing code that will be read by programmers around
the world, then all comments and variables/type-names should be in English.
to improve the overall style and quality of your code.
Missing an include guard:
Your header file doesn't have an include guard. You will need one
to be able to include the header in other
.cpp files. Note that #pragma once is also viable.Type and variable naming:
It is unusual to start the name of a type in C++ with a lower-case letter (
rServer). This form of camelCaseing is usually applied to variables and object instances. User defined C++ types are commonly named using PascalCase (first letter uppercase).DATA CryptBlockWithAESmodeCBC(char _data[1024]);Regarding the
_data param, names starting with _ may be reserved for compiler/library use. See this SO thread. It is best to avoid those.Declaring variables at the top of a function:
Declaring all variables at the beginning of a function is considered
bad practice in C++. Variables should be declared when they are first used
or in a way to be visible to the needed scope(s) only. Variables declared
at the top of a function are like mini-globals.
using namespace:using namespace std (or any namespace for that matter) is usually not a good practice because it defeats the purpose of a namespace, which is to allow identical names to coexist without clashes. You can read more about this here.Also, I would group all the
#includes and all the using directives in the
.cpp file. E.g.:#include "../cryptopp562/hex.h"
... all the inslcudes ...
using CryptoPP::StringSink;
... all the usings ...Using
cout directly:Your code is cluttered with log calls to
std::cout. You shouldn't be using it directly inside functions like you did. Most users won't want the verbose output on every execution of the program. If you need them for debugging, a better approach is to wrap those calls in a macro that can be disabled at compile time, e.g.:#define DEBUG_LOG(msg) do { std::cout << msg << std::end; } while(0)Magic constants:
What are these supposed to mean?
std::string key = "0123456789abcdef";
std::string iv = "aaaaaaaaaaaaaaaa";And these?
CryptoPP::Integer n("0xbeaadb3d839f3b5f"), e("0x11"), d("0x21a5ae37b9959db9");That's pretty unclear. Define named constants for those, using a
const char[] or a const std::string, and of course, give the constants meaningful names.Using old C-string functions:
Mixing
std::string with strcpy & friends is totally unnecessary.std::string has all the methods you need. E.g.: operator +, +=, append(), etc.texteChiffre= (char*)crypt.c_str();
strcpy(d.body,texteChiffre);
std::string blKey=hello.getKey();
blKeyChar=(char*)blKey.c_str();
strcpy(d.header.typeAlgo,typeAlgo);
strcpy(d.header.key1,blKeyChar);I suspect you did this because
DATA is using a char array internally. If that is the case, can't you also use an
std::stringfor it? It would also be more efficient since you could then avoid
a string copy altogether by just using C++11's
std::move: data.body = std::move(ciphertext);Avoid C-style casts:
C-style casts are ugly, unsafe and provide no compiler diagnostics.
texteChiffre = (char*)ciphertext.c_str();Always use the proper C++ cast operators. The cast above
is wrong BTW. You should never cast away the constess
of a pointer returned by
std::string::c_str().Clear commented out code:
You have a few commented out blocks. Please remove those
once you are done experimenting. This is only visual pollution.
Be consistent with your comments and var names:
If you are writing code that will be read by programmers around
the world, then all comments and variables/type-names should be in English.
Code Snippets
DATA CryptBlockWithAESmodeCBC(char _data[1024]);#include "../cryptopp562/hex.h"
... all the inslcudes ...
using CryptoPP::StringSink;
... all the usings ...#define DEBUG_LOG(msg) do { std::cout << msg << std::end; } while(0)std::string key = "0123456789abcdef";
std::string iv = "aaaaaaaaaaaaaaaa";CryptoPP::Integer n("0xbeaadb3d839f3b5f"), e("0x11"), d("0x21a5ae37b9959db9");Context
StackExchange Code Review Q#63566, answer score: 7
Revisions (0)
No revisions yet.