Simple XOR file encryption

Question

In https://stackoverflow.com/a/39299060, I wrote the following program.

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

static std::string str_xor(const std::string &data, const std::string &key) {
  std::string result(data.size(), '\0');

  for (std::size_t i = 0, i < data.size(); i++) {
    result[i] = data[i] ^ key[i % key.size()];
  }
  return result;
}

• Is it idiomatic C++ to evaluate data.size() in every iteration of the loop? Same for key.size().

• Is the use of std::size_t ok here, or do I really have to write std::string::size_type?

• Can I take advantage of move semantics here? I don't know them yet.

int main(int argc, char **argv) {
  if (argc != 3) {
    std::cerr << "usage: xor <datafile> <keyfile>\n";
    return 1;
  }

  std::ifstream data_in(argv[1]);
  std::string data(
    (std::istreambuf_iterator<char>(data_in)),
    (std::istreambuf_iterator<char>()));
  data_in.close();

The error checking is missing here. I would like the code for error checking to be as brief as possible. Ideally one or two lines that check for all errors at once. Is that still possible after closing the stream?

  std::ifstream key_in(argv[2]);
  std::string key(
    (std::istreambuf_iterator<char>(key_in)),
    (std::istreambuf_iterator<char>()));
  key_in.close();

  std::string result = str_xor(data, key);

  std::ofstream data_out(argv[1]);
  data_out << result;
  data_out.close();

  return 0;
}

Anything else?

Answer 1

This strikes me as the sort of code that's likely to make a lot of old C programmers cringe and think that C++ programmers are wasteful. It reads in the entire input file at once, even though it only ever looks at one byte of that file at any given time. Then it creates an output string the same size as the input file (and writes zero bytes to the whole thing). So, if your input file is, say, 20 megabytes, this allocates a minimum of 40 megabytes of memory to process it.

Along with that, it writes its output over the input file. If (for whatever reason) processing stops in the middle, you've destroyed the data. Your file might have some of the encrypted data, or it might have nothing at all.

So, for a moment, let's consider how that C programmer would probably do the job, and then see if we can't do something that's at least sort of close to the same, but with (perhaps) a little more abstraction.

If I were a C programmer, I'd probably do something like this:

FILE *infile = fopen(argv[1]);
FILE *keyfile = fopen(argv[2]);
FILE *outfile = fopen(argv[3]);

char key[BUFFER_SIZE];

size_t key_size = fread(infile, 1, BUFFER_SIZE, keyfile);

size_t current = 0;

int ch;

while ((ch=getchar(infile)) != EOF) {
    putchar(ch ^ key[current], outfile);
    current = (current + 1) % key_size;
}

This reads and processes one character at a time from the input file. Even if the input file is several gigabytes, it allocates only a small, fixed amount of memory.

At the same time, the C streams do actually do buffered reading and writing, so even though this may not be the most efficient code possible, it's usually going to be reasonably fast nonetheless.

The biggest drawback here is that we've set a fixed maximum size for the key. If the user supplies a short key, we're probably wasting quite a bit of memory. If they supply too long of a key, we use only part of it so we lose some security (though, in fairness, unless the key is huge this has no real security to lose).

So what we'd like would be to read the entire key into a string (so we get the whole thing, regardless of size, without wasting a lot of extra space. Other than that, we'd like to process the data directly from input to output, and let the iostreams handle buffering enough to make that reasonably efficient.

At the same time, we'd like a nice iterator-based model for most of the processing. Since we're taking some input, transforming it, and writing the result to an output, we can use std::transform to do most of the dirty work.

The place iterators break down is in working with our key. Normally iterators go from the beginning of a collection to the end (or vice versa), but in this case we need it to (potentially) iterate through the collection holding the key many times. To do that, we can write ourselves a custom iterator, something like this:

template <class Container>
class mod_iterator {
    using value_type = typename Container::value_type;
    using it = typename Container::const_iterator;

    it begin;
    it end;
    it current;
public:
    mod_iterator(Container const &c) : begin(c.cbegin()), end(c.cend()), current(c.cbegin()) { }
    value_type operator*() { return *current; }
    mod_iterator &operator++() { ++current; if (current == end) current = begin; return *this; }
};

To make it a little more convenient to use that, we can add a little builder function template:

template <class T>
mod_iterator<T> make_mod_iterator(T const &t) { return mod_iterator<T>(t); }

To avoid overwriting the input file, we can open a third file to hold the encrypted data. Once we've done the (boring) work of opening all three files, and reading the key into a string, the processing looks something like this:

auto m = make_mod_iterator(key);

std::transform(std::istreambuf_iterator<char>(data_in), {},
    std::ostreambuf_iterator<char>(data_out),
    [&](char c) { char ret = *m ^ c; ++m; return ret; }
);

One minor addition: you want to specify std::ios::binary when you open the file of the encoded data, because the data you produce may no longer be a proper text file. Since you can use the same code to either encrypt or decrypte, that means you want to specify it for both the input file and the output file:

std::ifstream data_in(argv[1], std::ios::binary);
std::ofstream data_out(argv[3], std::ios::binary);

then the processing looks something like this:

auto m = make_mod_iterator(key);

std::transform(std::istreambuf_iterator<char>(data_in), {},
    std::ostreambuf_iterator<char>(data_out),
    [&](char c) { char ret = *m ^ c; ++m; return ret; }
);