4
\$\begingroup\$

I just made this simple XOR encryptor and decryptor. It needs a file with the "private" key string inside.

usage:
encryptor "file to encrypt/decrypt" "file with the private key" "output file"

To decrypt, just do the same but for the inputfile, use the already encrypted file and the same file with the key.

 /*
 * Encryptor
 * File encryptor/decryptor.
 * Works on any kind of file.
 * Uses XOR encryption supplying a private key on a separate file
 * that very same key file is used for both encrypt and decrypt
 * example:
 * create a file named key.txt and inside of it, write these characters: mZq4t7w!
 * save the file and use it as a key to encrypt and decrypt any file
 *
 * For encryption:
 * Supply the "raw" file to be encrypted, use the key file and name the output file
 *
 * For decryption:
 * Supply the output file (encrypted), use the key file and the output file will be "raw" again.
 */
#include <cstdio>

// TODO:
// Add a command line option to display outputs
static bool consoleOutput = false;

// function that will encrypt/decrypt the file
// gets 3 args., input file, key file and output file
int Encrypt(const char* file_in, const char* keyFile_in, const char* file_out);

int main(int argc, char** argv)
{
    // if there's no arguments or the user typed more than 3 arguments, tell user to use help though -h option, exit.
    if( (argc == 1) || (argc > 3))
    {
        printf("\nExecute with %s -h for more information\n", argv[0]);
        return 0;
    }
    // if user uses help, indicate the use of the program, exit.
    else if(argc == 2)
    {
        if((argv[1][0] == '-') && (argv[1][1] == 'h'))
        {
            printf("\nUso: %s \"in file\" \"file with key\" \"out file\"\n", argv[0]);
            return 0;
        }
    }
    // user typed 3 arguments, everything ok, do the magic (encryption/decryption)
    Encrypt(argv[1], argv[2], argv[3]);

    return 0;
}

int Encrypt(const char* file_in, const char* keyFile_in, const char* file_out)
{
    //use 3 file pointers, one for input, one for key and other for output
    FILE* mainFile = nullptr;
    FILE* keyFile = nullptr;
    FILE* outFile = nullptr;

    char* inBuffer = nullptr; // buffer to store the whole file contents in memory
    char key[100]; // buffer to hold the key characters found in the key file **needs some future work**
    int mainFileSize = 0; // variable to hold the size of the input file size

    mainFile = fopen(file_in, "rb");

    // if can't open for read, close
    if(mainFile == nullptr)
    {
        printf("Couldn't open file %s!", file_in);
        return -1;
    }

    // go to end of file, get the position in bytes, and store it
    // the position in bytes will be the file size
    fseek(mainFile, 0, SEEK_END);
    mainFileSize = ftell(mainFile);
    rewind(mainFile); // go to beggining of file

    // if the file is 1 byte in size or is empty, exit
    if(mainFileSize <= 1)
    {
        printf("File is empty or the file is really small (not worthy)");
        return -2;
    }

    inBuffer = new char[mainFileSize]; // allocate memory for the file content

    if(inBuffer == nullptr)
    {
        printf("Couldn't allocate %d bytes of memory", mainFileSize);
        return -3;
    }

    // read the whole file on the buffer
    fread(inBuffer,sizeof(char), mainFileSize, mainFile);

    if(consoleOutput) //TODO: if this option is enabled, display the file contents
    {
        for(int i = 0; i < mainFileSize; i++)

        {
            putchar(inBuffer[i]);
        }
        puts("");
    }

    fclose(mainFile);

    keyFile = fopen(keyFile_in, "rb");
    // if can't open for read, close
    if(keyFile == nullptr)
    {
        printf("Couldn't open file %s!", keyFile_in);
        return -1;
    }

    // go to end of file, get the position in bytes, and store it
    // the position in bytes will be the file size
    fseek(keyFile, 0, SEEK_END);
    const int keyFileSize = ftell(keyFile);
    rewind(keyFile); // go to beggining of file

    // read the key characters on the key buffer variable
    fread(key, sizeof(char), keyFileSize, keyFile);

    if(consoleOutput) //TODO: if this option is enabled, display the file contents
    {
        for(int i = 0; i < keyFileSize; i++)
        {
            putchar(key[i]);
        }
        printf("\nSize: %i", keyFileSize);
    }

    fclose(keyFile);

    // output decryption/encryption
    puts("\n\tStarting to do the magic\n");

    // do the XOR encryption
    // for each character in the buffer, XOR it using the key characters
    // use moddulus on the key character array using the key file size to avoid reading outside of array
    // example:
    //      i = 20 keyFileSize = 8 (8 bytes)
    //      i % keyFileSize = 4
    // character in the 5th position of the key array will be use to XOR the buffer character at 21th position
    // write the result in the same buffer
    for(int i = 0; i < mainFileSize; ++i)
    {
        inBuffer[i] = inBuffer[i] ^ key[i%keyFileSize];
    }

    if(consoleOutput) //TODO: if this option is enabled, display the file contents
    {
        for(int i = 0; i < mainFileSize; i++)
        {
            putchar(inBuffer[i]);
        }
        puts("");
    }

    outFile = fopen(file_out, "wb");
    // if can't open, exit
    if(outFile == nullptr)
    {
        printf("Couldn't open file %s!", file_out);
        return -1;
    }

    // write the whole buffer chunk in the output file
    // as data was not added or removed, it is the same size as the input file
    fwrite(inBuffer, sizeof(char), mainFileSize, outFile);

    fclose(outFile);

    // clean up the buffer
    delete[] inBuffer;
    puts("Finished!");

    // bye bye!!
    return 0;
}
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Welcome to Code Review! Why not comment it before asking for a review, though? Reviewers can review your use of comments, and comments will help reviewers understand your code. \$\endgroup\$
    – Null
    Commented Feb 22, 2019 at 21:54
  • \$\begingroup\$ Hi, you're right, editing right now to include comments. Please review :) \$\endgroup\$
    – Vital Zero
    Commented Feb 22, 2019 at 22:48
  • \$\begingroup\$ Thank you for the response, I edited it already with the suggestions. switched to streams and other major stuff. It should work on large files :) \$\endgroup\$
    – Vital Zero
    Commented Feb 26, 2019 at 15:13
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Mast
    Commented Feb 26, 2019 at 15:15
  • \$\begingroup\$ Feel free to post a follow-up question instead. \$\endgroup\$
    – Mast
    Commented Feb 26, 2019 at 15:15

4 Answers 4

3
\$\begingroup\$

In general, your program is very C-like. I would raise at least the following points:

  • When writing C++, you might prefer std::cout to printf. Some points regarding this are raised for instance in this SO question.

  • In contrast to C, when writing C++, you should strive to declare variables as late as possible. This increases readability and advocates efficiency (e.g., if you exit early, was it really worth initializing all those gazillion variables?). For example, don't define keyFile and outFile at the beginning of Encrypt; it's not their place of use. As a C++ programmer, if I see something like keyFile being used, I instinctively look for its type somewhere nearby, perhaps only to discover I have to scroll up considerably before finding it making it harder to understand the code.

  • Luckily, C++ offers classes which do dynamic memory management for you. Instead of using an array of characters (like char key[100]), consider using std::string. With a character array, you have to take care in not overflowing (is 100 characters really sufficient? What happens if not?).

  • Consider avoiding manual memory management via new and delete. Mistakes are easy to make and notoriously hard to catch in larger & more complex systems. Tools like std::shared_ptr and std::vector are specifically designed to help you in these cases.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the comments. I appreciate them. I just wanted to do it right now using c libraries to see the file size (as iostream and string makes big files). It was just the first step :). I'll modify the code to use the c++ STL and streams. For the key char buffer, I just used 100 on the array just for temporary use, I'll make it dynamic with std::string. Now as for the new and delete, let me see std::shared_ptr, I haven't got to that yet (self taught here :) ) \$\endgroup\$
    – Vital Zero
    Commented Feb 23, 2019 at 14:33
  • \$\begingroup\$ Thank you for the response, I edited it already with the suggestions. switched to streams and other major stuff. It should work on large files :) \$\endgroup\$
    – Vital Zero
    Commented Feb 26, 2019 at 15:12
5
\$\begingroup\$
  • Any time you feel compelled to put a comment like

        // read key file
    

    consider factoring the relevant code into a function, e.g. read_key_file(....). Notice how the comment becomes unnecessary.

  • If you can process a stream, do process a stream. Read stdin, output to stdout. The benefits of stream processing are

    • No need to allocate a (possibly huge) buffer for am input file

    • The program can be part of a pipeline

  • The int mainFileSize; is more than questionable. File size may well exceed the limit of int. It may even exceed the limits of long which ftell returns. Use ftello (which returns off_t, wide enough to accommodate any file size).

  • The only place you do need to know the file size, and allocate a buffer dynamically, is reading of the key file. If the key file is larger than 100 bytes, your code faces an out-of-bound access problems.

  • As a side note, I don't see a reason for consoleOutput flag to exist. There are better debugging techniques. In any case, if you want to use it, provide a command line option to control its value.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you!, I used standard input/ouput to reduce exe size :). Let me work on the streams, and use off_t instead of int. Thanks for all the feedback :) \$\endgroup\$
    – Vital Zero
    Commented Feb 22, 2019 at 23:09
  • \$\begingroup\$ @JonathanMichel To avoid a confusion, I said streams meaning stdin and stdout instead of opening named files, so that the program can be called as encrypt key < infile > outfile \$\endgroup\$
    – vnp
    Commented Feb 22, 2019 at 23:37
  • \$\begingroup\$ Thank you for the response, I edited it already with the suggestions. switched to streams and other major stuff. It should work on large files :) \$\endgroup\$
    – Vital Zero
    Commented Feb 26, 2019 at 15:12
3
\$\begingroup\$

I'll not repeat points that are already mentioned by others, so these are additional to existing answers:

  • If you include <cstdio>, then its identifiers are in the std namespace (i.e. std::printf, std::open, etc). Implementations are allowed to also declare them in the global namespace (and several do), but you can't rely on that in a portable program.

  • Error messages should go to std::cerr or stderr; they should not be mixed in with program output. The same goes for debugging messages (though they should probably be removed, if you've finished debugging).

  • Return a non-zero value from main() if the program is unsuccessful. There's even a handy macro (EXIT_FAILURE) available in <cstdlib>.

  • Don't ignore the return value from Encrypt() - ideally, you'd return exit status so that main() can simply write

        return Encrypt(argv[1], argv[2], argv[3]);
    
  • std::fwrite() and std::fclose() can return errors (and their C++ equivalents can be made to throw exceptions). Don't assume that they have succeeded - the program has certainly failed if it couldn't write its output. Hint: use std::perror() to report the reason for failure.

  • Don't call std::putchar() in a loop when you can simply use std::fwrite() once.

  • End format strings with a newline character, except where you intend to combine several output strings into a single line.

  • When modifying values, we can use combined operate+assign to make it clearer that we're modifying elements:

    inBuffer[i] ^= key[i%keyFileSize];
    
  • sizeof(char) is always 1, since the result is in units of char.

  • Don't leave TODO comments in your code; that's usually a sign that it's not yet ready for review!

  • Consider not measuring either file: the main file can be streamed character by character, and the key file can be treated similarly, but with a std::rewind() call whenever we reach its end.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the response, I edited it already with the suggestions. switched to streams and other major stuff. It should work on large files :) \$\endgroup\$
    – Vital Zero
    Commented Feb 26, 2019 at 15:12
0
\$\begingroup\$

In response on my post, this is the updated version. Thanks to Toby Speight, vnp, Mast and Null for their reviews and suggestions.

Changed:

  • Switched to streams
  • Removed dynamic memory allocation
  • It should work on really large files
  • Uses the return value of the function
  • Have a progress bar if the file size is larger than 5 MB

*blank line

/*
 * Encryptor
 * File encryptor/decryptor.
 * Works on any kind of file.
 * Uses XOR encryption supplying a private key on a separate file
 * that very same key file is used for both encrypt and decrypt
 * example:
 * create a file named key.txt and inside of it, write these characters: mZq4t7w!
 * save the file and use it as a key to encrypt and decrypt any file
 *
 * For encryption:
 * Supply the "raw" file to be encrypted, use the key file and name the output file
 *
 * For decryption:
 * Supply the output file (encrypted), use the key file and the output file will be "raw" again.
 */
// Plans for the future:
// 1.- Add a command line option to display outputs
// 2.- Create a file and a header file, include the key string length into it and compare values, to avoid
//      any modification or "hacking"

#include <iostream>
#include <fstream>
#include <string>

// function that will encrypt/decrypt the file
// gets 3 args., input filename, key filename and output filename
int Encrypt(const std::string& file_in, const std::string& keyFile_in, const std::string& file_out);

int main(int argc, char** argv)
{
    // if user uses help, indicate the use of the program, exit.
    if(argc == 2)
    {
        if((argv[1][0] == '-') && (argv[1][1] == 'h'))
        {
            std::cout << "\nUso: " << argv[0] <<
                    " \"in file\" \"file with key\" \"out file\"\n" << std::endl;;
        }
    }
    else if(argc == 4)// user typed 3 arguments, everything ok, do the magic
    {
        std::string input(argv[1]);
        std::string key(argv[2]);
        std::string output(argv[3]);

        return Encrypt(input, key, output);
    }
    else // if there's no arguments or the user typed more than 3 arguments,
         //tell user to use help though -h option, exit.
    {
        std::cout << "\nExecute with " << argv[0] <<
                "-h for more information\n" << std::endl;
    }
    return 0;
}

int Encrypt(const std::string& file_in, const std::string& keyFile_in, const std::string& file_out)
{
    const int FileSizeThreshold = 5 * 1024 * 1024; // initialize to 5 MB
    bool biggerThanThreshold = false;

    // Work on the key file
    std::ifstream keyFile(keyFile_in, std::ios_base::in | std::ios_base::binary);
    if(!keyFile.is_open())
    {
        std::cerr << "Couldn't open file " << keyFile_in << std::endl;
        return -1;
    }

    // get the file size and store it, to use it as string length
    // go to beggining of the file and read its contents into a buffer
    keyFile.seekg(0, std::ios_base::end);
    std::streamsize keyFileSize = keyFile.tellg();
    keyFile.seekg(0, std::ios_base::beg); // go to beggining of file

    std::string keyFileBuffer;
    keyFileBuffer.resize(keyFileSize);
    keyFile.read( (char*)&keyFileBuffer[0], keyFileSize);
    keyFile.close();
    // Done working with tye key file

    // Work on the input file
    std::ifstream inputFile(file_in, std::ios_base::in | std::ios_base::binary);
    if(!inputFile.is_open())
    {
        std::cerr << "Couldn't open file " << file_in << std::endl;
        return -1;
    }

    // get the file size and store it, to use it as limit to read and write the files (input / output)
    inputFile.seekg(0, std::ios_base::end);
    std::streamsize inputFileSize = inputFile.tellg();
    inputFile.seekg(0, std::ios_base::beg); // go to beggining of file

    // if the file is 1 byte in size or is empty, exit
    if(inputFileSize <= 1)
    {
        std::cerr << "File is empty or the size is really small (not worthy)" << std:: endl;
        return -2;
    }

    if(inputFileSize > FileSizeThreshold )
    {
        std::cout << "File size is " <<
            inputFileSize/1024/1000 << "," << (inputFileSize/1024)%1000 << " MB... " <<
            "Activating stream mode." << std::endl;

        biggerThanThreshold = true;
    }

    // do the XOR encryption
    // for each character in the buffer, XOR it using the key characters
    // use moddulus on the key character array using the key file size to avoid reading outside of array
    // example:
    //      i = 20 keyFileSize = 8 (8 bytes)
    //      i % keyFileSize = 4
    // character in the 5th position of the key array will be use to XOR the buffer character at 21th position
    // write the result directly to the output files

    // Work on the output file at the same time as the input file
    // so we can avoid to allocate memory
    std::ofstream outFile(file_out, std::ios_base::out | std::ios_base::binary);
    if(!outFile.is_open())
    {
        std::cerr << "Couldn't open file " << file_out << std::endl;
        return -1;
    }

    char charBuffer = 0;
    int tick = inputFileSize / 30;

    if(biggerThanThreshold)
    {
        std::cout << "Progress: ";
    }

    // write directly from the input file, to the output file, without storing any buffer or allocating extra memory
    // if the app fails or crashes, the output file will be incomplete, not a big deal atm
    // it should work with files more than 1 GB
    for(int i = 0; i < inputFileSize; ++i)
    {
        inputFile.get(charBuffer);
        outFile.put(charBuffer ^ keyFileBuffer[i%keyFileSize]);

        // if the file is bigger than the threshold, show some kind of neat progress bar
        if(i % tick == 0 && biggerThanThreshold)
        {
            std::cout << "#";
        }
    }

    if(biggerThanThreshold)
    {
        std::cout << " 100%!!" << std::endl;
    }

    // Close both files and get out!
    inputFile.close();
    outFile.close();

    std::cout << "Finished!" << std::endl;

    return 0;
}
\$\endgroup\$

Your Answer

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

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