Small one time pad encryption program

This one time pad encryption program I have written (basically just an XOR "encryption" program) seems to be working fine, compiling nicely (gcc -o ./OTP.c), and doing what it's supposed to. However I would like to improve it as much as possible which is why I am posting this.

I am particularly insecure about the memory allocation. Any suggestions regarding improvements are more than welcome!

The code in its entirety is found below and can also be found on Github: PrivacyProject/OTP-Encryption.

#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <sys/mman.h>

int main(int argc, char **argv)
{
struct stat statbuf;
struct stat keybuf;

char buffer [20];
int key;
int data;
int output;
int count;
char ans;
int * buf;
FILE * keyfile;
FILE * sourcefile;
FILE * destfile;

if(geteuid() !=0)
{
printf("Root access is required to run this program\n\n");
exit(0);
}

if(argc<4)
{
printf("\n");
printf("    OTP 1.0 \n\n");

printf("    This program encrypts a  file using a random key\n");
printf("    and  generates an output file with the resulting\n");
printf("    cipher.  Decryption is achieved  by  running the\n");
printf("    output file as source file  with the same key.\n\n");

printf("    WARNING: The security of the encryption provided\n");
printf("    by this program is entirely dependent on the key\n");
printf("    file.  The keyfile should meet the  requirements\n");
printf("    below:\n");
printf("    - Be of the same size or larger than the\n");
printf("      source file.\n");
printf("    - Be completely random, preferably generated by \n");
printf("      a Hardware Random Number Generator.\n");
printf("    - NEVER be reused!\n\n");
printf("    The  author takes no  responsibility  for use of\n");
printf("    this program. Available under GNU General Public\n");
printf("    Licence v.2\n\n");

printf("    USAGE: OTP <source file> <output file> <keyfile>\n\n");
return (0);
}

/* Check number of arguments. */
if(argc>4)
{
printf("Too many arguments.\n");
printf("USAGE: OTP <source file> <output file> <keyfile>\n");
exit(1);
}

/* Allocate memory required by processes */
buf = (int*) malloc (sizeof(int));
if (buf == NULL)
{
perror("Error");
exit(1);
}

/* Lock down pages mapped to processes */
printf("Locking down processes...\n\n");
if(mlockall (MCL_CURRENT | MCL_FUTURE) < 0)
{
perror("mlockall");
exit (1);
}

/* Check if sourcefile can be opened. */
if((sourcefile = fopen(argv[1], "rb"))== NULL)
{
printf("Can't open source file\n");
perror("Error");
printf("USAGE: OTP <source file> <output file> <keyfile>\n");
exit (1);
}

/* Get size of sourcefile */
fstat(fileno(sourcefile), &statbuf);

/* Check if keyfile can be opened. */
if((keyfile = fopen(argv[3], "rb"))== NULL)
{
printf("Can't open keyfile.\n");
perror("Error");
printf("USAGE: OTP <source file> <output file> <keyfile>\n");
exit(1);
}

/* Get size of keyfile */
fstat(fileno(keyfile), &keybuf);

/* Check if keyfile is the same size as, or bigger than the sourcefile */
if((keybuf.st_size) < (statbuf.st_size))
{
printf("Source file is larger than keyfile.\n");
printf("This significantly reduces cryptographic strength.\n");
printf("Do you wish to continue? (Y/N)\n");
fgets(buffer, 20, stdin);
sscanf(buffer, "%c", &ans);
if(ans == 'n' || ans == 'N')
{
exit (1);
}
if(ans == 'y' || ans == 'Y')
{
printf("Proceeding with Encryption/Decryption.\n");
}
else
{
printf("No option selected. Exiting...\n");
exit (1);
}
}

/* Check if destfile can be opened. */
if((destfile = fopen(argv[2], "wb"))== NULL)
{
printf("Can't open output file.\n");
perror("Error");
exit(1);
}

/* Encrypt/Decrypt and write to output file. */
while(count < (statbuf.st_size))
{
key=fgetc(keyfile);
data=fgetc(sourcefile);

output=(key^data);

fputc(output,destfile);
count++;
}

/* Close files. */
fclose(keyfile);
fclose(sourcefile);
fclose(destfile);

printf("Encryption/Decryption Complete.\n\n");

/* delete Source file option. */
printf("Do you wish to delete the source file? (Y/N)\n");
fgets(buffer, 20, stdin);
sscanf(buffer, "%c", &ans);
if(ans == 'y' || ans == 'Y')
{
if ( remove(argv[1]) == 0)
{
printf("File deleted successfully.\n");
}
else
{
printf("Unable to delete the file.\n");
perror("Error");
exit(1);
}
}

/* delete keyfile option. */
printf("Do you wish to delete the keyfile? (Y/N)\n");
fgets(buffer, 20, stdin);
sscanf(buffer, "%c", &ans);
if(ans == 'y' || ans == 'Y')
{
if ( remove(argv[3]) == 0)
{
printf("File deleted successfully.\n");
}
else
{
printf("Unable to delete the file.\n");
perror("Error");
exit(1);
}
}

/* cleanup */
printf("Releasing memory.\n");
free (buf);
return(0);
}

-

Things you did well on:

• You make good use of comments.

• You try to make the user experience as smooth as possible, printing out a lot of useful information.

Things you could improve:

A few notes that others haven't covered:

• Running your program through Valgrind, I didn't see any memory leaks besides where your if conditions fail and you exit main().

    if((sourcefile = fopen(argv[1], "rb")) == NULL)
{
printf("Can't open source file\n");
perror("Error");
printf("USAGE: OTP <source file> <output file> <keyfile>\n");
exit(1);
}


The majority of modern (and all major) operating systems will free memory not freed by the program when it ends. However, relying on this is bad practice and it is better to free it explicitly. Relying on the operating system also makes the code less portable.

if((sourcefile = fopen(argv[1], "rb")) == NULL)
{
puts("Can't open source file.");
perror("Error");
free(buf)
return -5;
}

• Your encryption method is secure, banking on the fact that the all of the conditions for your program are met.

But I always think worst case scenario, where the user doesn't follow your program's optimal conditions. If you want to use secure encryption techniques, you can benefit from a cryptography expert's work. This means you will have to implement an external library (such as OpenSSL). Here is an example if you want to go that route.

• You have an implicit declaration of function geteuid(), which is invalid in the C99 standard.

#include <unistd.h>

• fopen(), a widely-used file I/O functions that you are using, got a facelift in C11. It now supports a new exclusive create-and-open mode (“...x“). The new mode behaves like O_CREAT|O_EXCL in POSIX and is commonly used for lock files. The “...x” family of modes includes the following options:

• wx create text file for writing with exclusive access.

• wbx create binary file for writing with exclusive access.

• w+x create text file for update with exclusive access.

• w+bx or wb+x create binary file for update with exclusive access.

Opening a file with any of the exclusive modes above fails if the file already exists or cannot be created. Otherwise, the file is created with exclusive (non-shared) access. Additionally, a safer version of fopen() called fopen_s() is also available. That is what I would use in your code if I were you, but I'll leave that up for you to decide and change.

• You can cut down on a few lines of code by initializing similar types on one line. This will keep you more organized.

   struct stat statbuf;
struct stat keybuf;

char buffer [20];
int key;
int data;
int output;
int count;
char ans;
int * buf;
FILE * keyfile;
FILE * sourcefile;
FILE * destfile;


Also, initialize your int values when you declare them. Pointer values will default to NULL.

struct stat statbuf, keybuf;

char buffer [20];
char ans;
int key = 0, data = 0, output = 0, count = 0;
int *buf;
FILE *keyfile, *sourcefile, *destfile;

• You check if argc is greater or less than 4.

if(argc<4)
{
// huge printf() block
return (0);
}
// ...
if(argc>4)
{
printf("Too many arguments.\n");
printf("USAGE: OTP <source file> <output file> <keyfile>\n");
exit(1);
}


Just check for the inequality of 4 and print the block statement.

if (argc != 4)
{
// printf() block
return 0;
}

• I would have extracted all of the encryption to one method and all of the file validation in another method, but I'll leave that for you to implement.

• You can remove the != 0 for maximum C-ness, but this is to your discretion and tastes.

   if(geteuid() != 0)

• It is more common to return 0; rather than to exit(0). Both will call the registered atexit handlers and will cause program termination though. You have both spread throughout your code. Choose one to be consistent. Also, you should return different values for different errors, so you can pinpoint where something goes wrong in the future.

• Use puts() instead of printf() with a bunch of '\n' characters.

   printf("    and  generates an output file with the resulting\n");

puts("    and generates an output file with the resulting");

• You compare some pointers to NULL in some test conditions.

     if (buf == NULL)


You can simplify them.

if (!buf)

• Your perror() messages could be more descriptive.

• You compare some input to the uppercase and lowercase versions of characters.

   if (ans == 'n' || ans == 'N')


You could use the tolower() function in <ctype.h> to simplify it a bit.

if (tolower(ans) == 'n')

• Already mentioned, but please indent your code properly. It will make your code a lot easier to read and maintain. You can find IDE's out there that will do it automatically for you when told.

Final code (with my changes implemented):

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <ctype.h>
#include <sys/stat.h>
#include <sys/mman.h>

int main(int argc, char **argv)
{
struct stat statbuf, keybuf;

char buffer[20];
char ans;
int key = 0, data = 0, output = 0, count = 0;
int *buf;
FILE *keyfile, *sourcefile, *destfile;

if(geteuid() != 0)
{
puts("Root access is required to run this program.");
return -1;
}

if(argc != 4)
{
puts("OTP 1.0 \n");

puts("This program encrypts a  file using a random key");
puts("and  generates an output file with the resulting");
puts("cipher.  Decryption is achieved  by  running the");
puts("output file as source file  with the same key.\n");

puts("WARNING: The security of the encryption provided");
puts("by this program is entirely dependent on the key");
puts("file.  The keyfile should meet the  requirements");
puts("below:");
puts("    - Be of the same size or larger than the");
puts("      source file.");
puts("    - Be completely random, preferably generated by");
puts("      a Hardware Random Number Generator.");
puts("    - NEVER be reused!\n");
puts("The  author takes no  responsibility  for use of");
puts("this program. Available under GNU General Public");
puts("Licence v.2\n");

puts("usage: OTP <source file> <output file> <keyfile>");
return 0;
}

/* Allocate memory required by processes */
buf = (int*) malloc (sizeof(int));
if (!buf)
{
perror("Error");
free(buf);
return -3;
}

/* Lock down pages mapped to processes */
puts("Locking down processes...");
if(mlockall (MCL_CURRENT | MCL_FUTURE) < 0)
{
perror("mlockall");
free(buf);
return -4;
}

/* Check if sourcefile can be opened. */
if((sourcefile = fopen(argv[1], "rb")) == NULL)
{
puts("Can't open source file.");
perror("Error");
free(buf);
return -5;
}

/* Get size of sourcefile */
fstat(fileno(sourcefile), &statbuf);

/* Check if keyfile can be opened. */
if((keyfile = fopen(argv[3], "rb")) == NULL)
{
puts("Can't open keyfile.");
perror("Error");
free(buf);
return -6;
}

/* Get size of keyfile */
fstat(fileno(keyfile), &keybuf);

/* Check if keyfile is the same size as, or bigger than the sourcefile */
if((keybuf.st_size) < (statbuf.st_size))
{
puts("Source file is larger than keyfile.");
puts("This significantly reduces cryptographic strength.");
puts("Do you wish to continue? (Y/N)");
fgets(buffer, 20, stdin);
sscanf(buffer, "%c", &ans);
if (tolower(ans) == 'n')
{
free(buf);
return 0;
}
else if (tolower(ans) == 'y') puts("Proceeding with Encryption/Decryption.");
else
{
puts("No option selected. Exiting...");
free(buf);
return -7;
}
}

/* Check if destfile can be opened. */
if ((destfile = fopen(argv[2], "wb")) == NULL)
{
puts("Can't open output file.");
perror("Error");
free(buf);
return -8;
}

/* Encrypt/Decrypt and write to output file. */
while (count < (statbuf.st_size))
{
key=fgetc(keyfile);
data=fgetc(sourcefile);

output=(key^data);

fputc(output,destfile);
count++;
}

/* Close files. */
fclose(keyfile);
fclose(sourcefile);
fclose(destfile);

puts("Encryption/Decryption Complete.");

/* delete Source file option. */
puts("Do you wish to delete the source file? (Y/N)");
fgets(buffer, 20, stdin);
sscanf(buffer, "%c", &ans);
if (tolower(ans) == 'y')
{
if (remove(argv[1]) == 0) puts("File deleted successfully.");
else
{
puts("Unable to delete the file.");
perror("Error");
free(buf);
return -9;
}
}

/* delete keyfile option. */
puts("Do you wish to delete the keyfile? (Y/N)");
fgets(buffer, 20, stdin);
sscanf(buffer, "%c", &ans);
if(tolower(ans) == 'y')
{
if (remove(argv[3]) == 0) puts("File deleted successfully.");
else
{
puts("Unable to delete the file.");
perror("Error");
free(buf);
return -10;
}
}

/* cleanup */
puts("Releasing memory.");
free (buf);
return(0);
}

-
if (geteuid()) has a connotation that geteuid() returns a notional Boolean value, whereas if (geteuid() != 0) looks more like a check against a particular value. They both generate exactly the same code, but I consider the latter more readable. –  200_success Feb 15 '14 at 21:43
how about replace " " with \t –  SSpoke Feb 15 '14 at 21:51
@SSpoke Not a bad idea. Unfortunately, I removed the spaces in the final code I posted. –  syb0rg Feb 15 '14 at 21:56
Nitpick regarding "Your encryption isn't very secure": As long as the three conditions mentioned in the usage notes are met, the encryption is unbreakable, making the the most secure algorithm possible. –  Heinzi Feb 15 '14 at 22:45
Way to got over and above anything I could have hoped for! I have learned a lot from this. –  youjustreadthis Feb 15 '14 at 23:38

Instead of checking that the EUID is 0, just do mlockall() and check for EPERM. Don't assume that only the root user is allowed to lock memory, and don't assume that the root user will be allowed to lock memory. There may be security modules, such as SELinux, that cause such assumptions to be wrong.

-

Indentation:

Make sure to keep your indentation consistent. You only do it in certain places, mostly towards the end. You should do it everywhere as needed, which will make your code much easier to read and follow.

main() and functions:

You do everything in main(), which is bad practice because it just makes the program hard to read and maintain. That is a beginner thing, so it's understandable. As soon as you can, you should start using separate functions to handle most of the work. This will vastly improve this program and will help with future programs, especially larger ones.

In general, main() should display the introductory text, open files (terminate if it fails), close the files, call other functions to do the cryptography work, return anything if needed, and then terminate. Adding comments is a good start, but is still no substitute for using functions. For the files and command line arguments, they can be passed to the functions and used there.

return and exit()

I prefer returning from main() over calling exit(), but for whichever you choose, you should be consistent with it. This was already mentioned by @syb0rg and in better detail, but is still worth keeping in mind. Consistency is key.

You don't need the parenthesis in return (0). It doesn't require use of it. Just use return 0.

You could optionally return EXIT_SUCCESS or EXIT_FAILURE, which return 0 and 1 respectively.

-

When you "remove" a file, its contents still reside on disk; only its directory entry is removed. Instead of just unlinking the source file, consider overwriting it with random bytes first. (These days, with SSD wear leveling, filesystem snapshots, and log-structured filesystems, it's harder to securely wipe data completely from a filesystem. However, it's still worth trying, in my opinion.)

Your warning is inaccurate. When the key file is shorter than the source file, it doesn't reduce the cryptographic strength; it obliterates it. In my opinion, the most ethical behaviour is to treat it as a fatal error and generate no output file at all.

-
It's processing statbuf.st_size bytes of the source; does it really truncate the output? Or does it xor the extra bytes with invalid but predictable output from fgetc(keyfile) i.e. EOF? –  ChrisW Feb 15 '14 at 21:35
@ChrisW OMG you're right. It just bit-flips all of the excess text. –  200_success Feb 15 '14 at 21:50
That might be more secure that reusing the "one-time" pad. –  ChrisW Feb 15 '14 at 21:54

In terms of algorithm, the following loop has what I would consider a big problem. If keyfile is shorter than sourcefile, then the end of sourcefile ends up being encrypted with EOF and thus the XOR will use -1. It's really much less encrypted that you had in mind, I would think. (Note: I know the code asks you whether you want to go on or not, but I think there is a simple solution, see below.)

/* Encrypt/Decrypt and write to output file. */
while (count < (statbuf.st_size))
{
key=fgetc(keyfile);
data=fgetc(sourcefile);

output=(key^data);

fputc(output,destfile);
count++;
}


Also, you do not set count to zero before looping with it. That being said, I would use something like this instead which does not use count anyway:

while((data = fgetc(sourcefile) != EOF)
{
key = fgetc(keyfile);
if(key == EOF)
{
rewind(keyfile);
key = fgetc(keyfile); // cycle key file
}

output = key ^ data;

fputc(output, destfile);
}


(Side note: count is zero in your case because count is a "primordial variable" and those are using "clean memory" (zeroed) from the kernel, but in a sub-sub-function, you would have noticed the problem!)

-
Recycling the key file makes it no longer a one-time pad, and the encryption is no longer unbreakable. –  200_success Feb 16 '14 at 10:30
I agree, but using EOF to pad the rest of the file is even uglier. If you want to enforce a one-time pad then err if the size of keyfile is less than the size of destfile. –  Alexis Wilke Feb 16 '14 at 10:32
I tried implementing your suggestion, and while compiling it made the program unable to decrypt the output file. I agree that the program should err when too hot keys are used and will implement that. –  youjustreadthis Feb 16 '14 at 10:57
@youjustreadthis, I'm not too sure why it could not decrypt. You could compare the output of your old version and this version using cmp -l old-output new-output to see whether the bytes look different. Of course, if the key is smaller, it will look different from the end of the key to the end of the file being encrypted. –  Alexis Wilke Feb 17 '14 at 0:00

An OTP derives its strength entirely from the secrecy and one-timeness of the key.

Regardless of your code implementation (which looks decent and has some very decent improvements suggested) it is fundamentally flawed as reliable cryptosystem since you have absolutely no way of knowing what's happening (or already happened) to your pad when you "destroy" it. It may have been sniffed on the wire, versioned on a filesystem, backed-up, etc.

This does not address your question (about code improvement), which I think others have answered well, just a reminder that just because your code is good it may not serve its intended purpose.

-
Hmm, you make a good point on maintaining the security of the key that I somewhat glossed over. +1 –  syb0rg Feb 17 '14 at 4:43