13
\$\begingroup\$

I decided to try my hand at making an encryption utility, which I lovingly call "FileScrambler". FileScrambler allows the user to encrypt a file (a master-password list, personal diary, or whatever) using a password, and save the resulting jumbled mess to their computer (same folder as the executable is located). They can then decrypt the file using the same password, revealing all their hidden secrets.

Any and all comments are welcome, especially in regards to optimization and standards compliance.

Being a novice programmer, please be gentle with the criticism. This is only my second completed project.

#include <iostream>
#include <string>
#include <fstream>
#include <ctime>
#include <sstream>
#include <cstdlib>

std::string load(std::string nameOfFile){ //done

    std::ifstream input;
    input.open(nameOfFile.c_str());
    std::string content((std::istreambuf_iterator<char>(input)), std::istreambuf_iterator<char>());
    return content;
}

std::string encrypt(std::string content, std::string password){ //also decrypts! :D

    std::string out;
    int p = 0;
    for (int index = 0; index < content.size(); ++index){
            out += content[index] xor password[p] * p % 30;
            ++p;
            if (p >= password.size())
                p = 0;
        }
    return out;
}

void save(std::string nameOfFile, std::string content){ //saves a file with content Content
    char response;
    std::ifstream in(nameOfFile.c_str());
    std::ofstream of;
    if (!in){ //prevents a file from being overwritten unless the user says it's okay (in the nested IF statement).
        in.close();
        of.open(nameOfFile.c_str());
        of << content;
    } else{
        in.close();
        std::cout << "A file with that name already exists. Overwrite? (y/n)\n";
        std::cin >> response;
        if(response == 'y'){
            of.open(nameOfFile.c_str());
            of << content;
        } else {
            std::cout << "Save cancelled. \n";
            }
    }
    of.close();
}


int main(){

    int answer = 0;
    std::string response, data, nameOfFile, password, encrypted;
    char responseC;

    do{

        switch(answer){

        case 0:
            {
            std::cout << "Available commands:\n";
            std::cout << "'0'. list commands\n---\n";
            std::cout << "'1'. load a file\n---\n";
            std::cout << "'2'. save a file\n---\n";
            std::cout << "'3'. encrypt a file\n---\n";
            std::cout << "'4'. decrypt a file\n---\n";
            std::cout << "'5'. display contents in memory\n---\n";
            std::cout << "'6'. close the program\n---\n";
            std::cout << "Note: You must load a file before saving, encrypting, or decrypting.\n";
            break;
            }

        case 1: //Load a file's content into memory, stored in the 'data' variable.
            {
            std::cout << "enter name of file (with extension) to load:\n";
            std::cin >> nameOfFile;
            data = load(nameOfFile.c_str());
            std::cout << "Display contents? (y/n):\n";
            std::cin >> responseC;
            if (responseC == 'y')
                std::cout << data << "\n";
            break;
            }
        case 2: //Save a file's content.
            {
            std::cout << "save file as: ";
            std::cin >> response;
            save(response, data);
            break;
            }
        case 3: //Encrypt the content of 'data' by XORing it against a password.
            {
            std::cout << "Encrypt " << nameOfFile << "?(y/n)\n";
            std::cin >> responseC;
            if (responseC == 'y'){

                    //responseC = '\n';
                    std::cout << "Enter password to decrypt " << nameOfFile << ':' << "\n";
                    std::cin >> password;
                    std::cout << "Encrypting... \n";
                    data = encrypt(data, password);
            }
            break;
            }
        case 4: //Decrypt the content of 'data' by XORing it against a password. If the password is correct,
            {  //the data actually makes sense. If it isn't, the contents will appear to be gibberish.
            std::cout << "Enter password for file: ";
            std::cin >> password;
            std::cout << "Decrypting...\n";
            data = encrypt(data, password); //uses the same same process as encryption.
            std::cout << "Done.\n";
            break;
            }
        case 5: //Display the contents of the Data variable.
            std::cout << "File contents\n-----\n" << data << "\n-----\n";
            break;
        case 6:
            {
                exit(0);
            }
        default:
            std::cout << "Unrecognized command. Enter '0' to view command list\n";
        }
        std::cout << "Enter command:\n";
        std::cin >> answer;

    }while(answer != 6);

    return 0;
}

Notes:

  • This code is a revision of my previous post.
  • I know that this encryption method is (very) weak. I chose to use it because it's the only one I know.
  • The code is also available on GitHub.
\$\endgroup\$
4
  • \$\begingroup\$ This is not related to the question, but I think it would be nice if you create a DLL for it, using C. \$\endgroup\$ Jan 5, 2015 at 11:50
  • \$\begingroup\$ If you are running this on Windows, I would use wstring / wchar instead of string/char due to the fact you can have Unicode characters in filenames and Windows needs the "wide variants" to handle those : see stackoverflow.com/questions/402283/stdwstring-vs-stdstring . \$\endgroup\$
    – Richy B.
    Jan 5, 2015 at 13:03
  • 2
    \$\begingroup\$ I don't think the DLL idea above is a good one. If you were doing it in Windows in Windows idiomatic style, it would look completely different (you'd probably not make a console app to start with; TCHAR/CString everywhere) \$\endgroup\$
    – pjc50
    Jan 5, 2015 at 14:34
  • 2
    \$\begingroup\$ You may already know this and just be doing different for the practice, but in real world work you should basically always use someone-else's library function to do the actual encryption and decryption. Choose a well known algorithm and library which both have been studied by the community and not found too insecure. \$\endgroup\$
    – bdsl
    Jan 5, 2015 at 21:34

4 Answers 4

10
\$\begingroup\$

Your XOR algorithm is excessively basic, and does not XOR as much as it should.

out += content[index] xor password[p] * p % 30

The operator precedence order is * and % have equal precedence, and are both higher precedence than xor: operator precedence. Because * and % are equal-order, they are processed left-to-right.

Your function above, thus, decomposes to the following:

out += content[index] xor ( ( password[p] * p ) % 30)

In essence, your xor operator will only ever have a value from 0 through 29 (inclusive) on the right hand side.

In bits, this means that the high-3 bits will always 'flip' on every operation, and are essentially never encrypted.

I suspect that your intention is to set the precedence for the p % 30 to be higher than the password[p] * p. To do that, you need braces....

\$\endgroup\$
2
  • 6
    \$\begingroup\$ Nice answer, this is yet another reminder that cryptography is hard. \$\endgroup\$
    – shuttle87
    Jan 5, 2015 at 4:55
  • \$\begingroup\$ Good catch! This would become a serious annoyance if found way later in the project. \$\endgroup\$
    – Mast
    Jan 5, 2015 at 13:35
9
\$\begingroup\$

First I'd like to mention I like that you are using std::string and using .c_str() when needed to pass to APIs that require that. Keep doing that! Overall it's so much easier to use std::strings internally and only deal with C-style char* strings when you absolutely need them.

C++ has what's known as value semantics with function parameters, this means that when you pass a variable to a function the default behaviour is to copy the variable. Sometimes however you don't actually need to copy the variable and a reference is a good thing to be able to use in these cases (or more advanced technique is move semantics).

So in the functions that take a string as a parameter you are copying the string to pass to the function. A c++ idiom that is useful when you are passing a parameter but not changing it is to pass by reference to const and save yourself an unnecessary copy, so this function:

std::string encrypt(std::string content, std::string password){ //also decrypts! :D

would become:

std::string encrypt(std::string const& content, std::string const& password){
   //rest of function stays the same!
}

In that function you have the following loop:

for (int index = 0; index < content.size(); ++index){

While your compiler will probably optimize for this it would need to look up the value of content.size() every loop iteration. You can save some computational time by just computing this once outside the loop:

int end = content.size();
for (int index = 0; index < end; ++index){

Now additionally if you complied with all warnings your compiler might give a warning about comparing unsigned with signed values, this might seem like a minor annoyance but don't ignore it because it often can be indicative of bugs. The type of .size() is unsigned.

size_t end = content.size();
for (size_t index = 0; index < end; ++index){

Now if you are using c++11 you can further improve the loop by using the range based loop syntax:

for (char& ch: content){
   out += ch xor password[p] * p % 30;

We have now reduced a lot of boilerplate and this aids the readability of the code.

Within that loop I notice you have an if statement without any braces, this is something that I personally dislike and is prohibited in the coding conventions where I work.

In the save function:

void save(std::string nameOfFile, std::string content){

you have some code that tries to save a file and you also have code that is printing to the console the status of the save attempt. This isn't the cleanest separation of concerns, I would instead not print anything in the save function and have the function return a code that indicates the status. Then you can print to the console using the return code. This helps if at a later date you wanted to add logging or perhaps change to a GUI and just generally helps reduce the maintenance overhead of the code.

Going further I think that it would be valuable to have unit tests for a project such as this. Making some test cases that when you say encrypt something then decrypt it you get the same thing back would be a good start. Also breaking the project up into modules with header files is probably a good idea when you get past a certain size.

\$\endgroup\$
4
  • 3
    \$\begingroup\$ .c_str() is still necessary if C++11 isn't in use. \$\endgroup\$
    – Jamal
    Jan 5, 2015 at 4:33
  • 1
    \$\begingroup\$ Isn't optimizing for (int index = 0; index < content.size(); ++index){ premature? \$\endgroup\$
    – Mast
    Jan 5, 2015 at 13:33
  • \$\begingroup\$ @Mast I would tend to agree, especially if the advice earlier in this answer about using const references was followed. As shuttle mentioned though, a smart (or even dumb) compiler would most likely notice that content is never modified regardless. \$\endgroup\$ Jan 6, 2015 at 4:05
  • \$\begingroup\$ @Mast, in the case of a string being iterated over then any decent compiler will optimize this. If however the data structure was different then taking the size might be expensive or might have to happen every loop. As far as it being premature, I'm just in the habit of writing the code for generic containers, if you know that you are always dealing with a string then this is unnecessary. \$\endgroup\$
    – shuttle87
    Jan 6, 2015 at 14:35
8
\$\begingroup\$
  • It doesn't look like you're using <ctime> and <cstdlib>, so just remove them.

  • It may look nicer to have longer comments on their own lines, especially the ones used to describe a function's purpose. This will reduce the horizontal character count and keep them out of the way whenever the code on these lines is modified.

  • It's not really necessary to call close() unless you're also doing error-checking. Omitting them should not leak resources, and the files will still be closed as needed.

  • Although main() is primarily being used as a driver, much of its contents can still be in separate functions called from main(). That will make it easier to maintain this aspect of the program, which may be necessary here since it already looks quite bloated with the switch.

\$\endgroup\$
7
\$\begingroup\$

As a novice, this is actually pretty good. Idiomatic textbook C++. You've entirely avoided pointers and all the related traps, resulting in a fairly stable program that does what it sets out to do.

There are a couple of limitations to consider, beyond the uselessness of XOR as an encryption mechanism:

  • No error checking on load(), or save() if you try to write to a full or write-protected drive
  • The program loads the whole file into memory rather than operating on it as a "stream". In some languages it would be fairly easy to make something that operated as a filter on a stream so you could just pour the ifstream into the ofstream. This is not quite so easy in C++, but you could try adapting the encrypt function to take two input iterators and an output iterator. It would have a similar signature to std::copy: http://en.cppreference.com/w/cpp/algorithm/copy (a templated function).
  • value copy semantics as described in another answer: so in fact you could end up with two copies of the input file and two copies of the encrypted file all in memory at the same time.

(Avoiding loading the whole thing into memory was important when I started coding in C a couple of decades ago. It's less important now but you may still find yourself wanting to process a lot of data on a resource constrained system.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.