HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Encryption with many algorithms using Crypto++

Submitted by: @import:stackexchange-codereview··
0
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.

#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 .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::string
for 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.