XOR encryption program in C

Question

I've written a program that encrypts files (or stdin) with a user-specified key from 1 to 255. This "encryption" is very insecure, but still a bit better than ROT13 (at least for people trying to crack it using pencils and paper). How can I improve this program?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

/*
 * A simple program for encrypting files (or stdin) using the XOR operation.
 * Such encryption is very insecure as there are only 254 valid key values,
 * but is still much more secure than ROT13, where there is only 1 valid key.
 *
 * Using a key value of 0 or 256 is equivalent to no encryption, and is
 * therefore not allowed. Other values are too large to fit into one byte
 * and therefore cannot be properly XORed with a byte.
 */

static void usage(const char *const);

int main(int argc, char *argv[])
{
    FILE *in = NULL;
    FILE *out = NULL;
    int k = 0;
    int c;

    while ((c = getopt(argc, argv, "i:o:k:h")) != -1) {
        switch (c) {
        case 'i':
            in = fopen(optarg, "r");
            break;
        case 'o':
            out = fopen(optarg, "w");
            break;
        case 'k':
            k = atoi(optarg);
            break;
        case 'h':
            usage(argv[0]);
            return EXIT_FAILURE;
        }
    }

    in = in ? in : stdin;
    out = out ? out : stdout;

    if (k <= 0 || k > 255) {
        fprintf(stderr, "%s: must specify a valid key via -k\n",
            argv[0]);
        fprintf(stderr, "%s: valid key values are from 1 to 255\n",
            argv[0]);
        return EXIT_FAILURE;
    }

    while ((c = fgetc(in)) != EOF) {
        fputc(c ^ k, out);
    }

    return EXIT_SUCCESS;
}

void usage(const char *const s)
{
    fprintf(stderr, "usage: %s -k key [-i input] [-o output]\n", s);
}

To compile:

$ clang -O2 -Weverything -Werror -o xor xor.c

Answer 1

Use binary mode

Currently, you are opening your files like this:

    in = fopen(optarg, "r");
    out = fopen(optarg, "w");

On a UNIX system that would be fine, but on a Windows system, that could result in CRLF pairs being converted into a single LF when reading, and single LF characters converted to CRLF pairs when writing. This is particularly important because once you do the xor operation, your output may contain random LF characters.

You should open your files in binary mode instead:

    in = fopen(optarg, "rb");
    out = fopen(optarg, "wb");

Answer 2

It's a relatively small program and pretty well written, so there's not a lot on which to comment, but here are a few things that may help you improve your program.

Handle error conditions

As it stands, if the user specifies an input file that does not exist, fopen will return a NULL value. Rather than emitting some kind of error message, this causes the program to silently use stdin instead. This is not intuitive behavior and could lead the user into thinking that the program has hung rather than that it's just waiting for input.

Reorder functions to eliminate declaration

By putting the definition of usage above that of main where it is first used, the function declaration can be omitted. I think that makes the program easier to read and understand.

Declare local functions static

There is not likely to be any reason that usage needs anything more than static scope. That is, if the function is only going to be used from within this one file, declaring it static clearly conveys that (enforced) intent to the compiler and linker.

Eliminate return EXIT_SUCCESS at the end of main

Since C99, the compiler automatically generates the code corresponding to return EXIT_SUCCESS at the end of main so there is no need to explicitly write it.

Answer 3

For your getopt() operation, you should also check for an invalid option. You can either check for '?', which is returned when an unknown option is found, or you can use a default case, or both. Seeing as your 'h' option already handles program termination, you can just add a '?' case there, but you may still add a default case if the compiler complains about not having one.

Answer 4

One thing I see is that your variable names are somewhat unclear. One letter variables are nearly always discouraged unless you're deliberately obfuscating your code OR are golfing your code for bytelength. And even if it's obfuscation, it's poor obfuscation since it's still easy to determine what the variables are from the source code.

If you HAVE to use one letter variable names for some reason, there is no reason why you can't make your basic source code easy to read and use a pre-build step to minify your code, especially with a program that's as trivial as this one. Instead of k use key. c is a convention and thus can stay like it is.

Sure, this is nitpicking and there are bigger mistakes in there that others have already pointed out, but even though the meaning of k can be inferred easily, it is still a good idea to stick to a good habit, even on smaller projects.

Answer 5

Rather than reassign in and out to stdin/stdout if they're NULL, you could instead just set those as the default values:

FILE *in = stdin;
FILE *out = stdout;

Another thing you should do is handle the case that the user has multiple -i/-o switches, e.g.:

case 'i':
    if (in != stdin) {
        fclose(in);
    }
    in = fopen(optarg, "rb");
    break;
case 'o':
    if (out != stdout) {
        fclose(out);
    }
    out = fopen(optarg, "wb");
    break;

However, in that case, multiple -o switches will create multiple files, so it might be better to just fail as an error. In fact, it might even be better to have the switches store the filenames instead:

char *in_file = NULL;
char *out_file = NULL;

while ((c = getopt(...))) {
    switch (c) {
    case 'i':
        free(in_file); // no-op if already NULL
        in_file = strdup(optarg);
        break;
    case 'o':
        free(out_file);
        out_file = strdup(optarg);
        break;
    // ...
    }
}

/* Some people will take issue with this for having a C99-style variable
   declaration. However, C99 has been around for almost 17 years now. It
   is no longer "too new," IMO. :)
*/
FILE *in = in_file ? fopen(in, "rb") : stdin;
if (!in) {
    perror(in_file);
    return 1;
}
free(in_file);

FILE *out = out_file ? fopen(out, "wb") : stdout;
if (!out) {
    perror(out_file);
    return 1;
}
free(out_file);

Also, it's a soft UNIX convention to just use a positional argument to specify the input file (and often output file), and to use - to indicate stdin/stdout. getopt mutates argc and argv such that they contain just the positional arguments after the while loop is complete.