Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
}
share|improve this question

6 Answers 6

up vote 13 down vote accepted

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>
    
  • 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);
}
share|improve this answer
2  
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 at 21:43
    
how about replace " " with \t –  SSpoke Feb 15 at 21:51
    
@SSpoke Not a bad idea. Unfortunately, I removed the spaces in the final code I posted. –  syb0rg Feb 15 at 21:56
4  
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 at 22:45
2  
Way to got over and above anything I could have hoped for! I have learned a lot from this. –  youjustreadthis Feb 15 at 23:38

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
1  
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 at 21:35
    
@ChrisW OMG you're right. It just bit-flips all of the excess text. –  200_success Feb 15 at 21:50
2  
That might be more secure that reusing the "one-time" pad. –  ChrisW Feb 15 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!)

share|improve this answer
1  
Recycling the key file makes it no longer a one-time pad, and the encryption is no longer unbreakable. –  200_success Feb 16 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 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 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 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.

share|improve this answer
    
Hmm, you make a good point on maintaining the security of the key that I somewhat glossed over. +1 –  syb0rg Feb 17 at 4:43

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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