bare bones file encrypter/decrypter using 128 bit Serpent algorithm

[makobu]

/*
*
*************************************************************************************
* Bare bones program to encrypt and decrypt a file using
mcrypt:serpent *
* Proverbs
25-2
*
* Timothy Makobu, 21st-22nd january,
2007 *
* Make: cc -Wall -O2 -o executable file.c -
lmcrypt *
*
*************************************************************************************
*/

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

/* The help function */
void help(char *name)
{
printf("usage: %s [option: e > encrypt; d > decrypt] file\n",
name);
}

/* File opener */
FILE *open_file(char *path)
{
FILE *infile;
if((infile = fopen(path, "r")) == NULL)
{
fprintf(stderr, "cannot open %s\n",path);
exit(-1);
}
else
return infile;
}

/* crypto */
int crypto(FILE *infile, char *action, char *name)
{
MCRYPT td;
int i;
char * key;
char password[20];
char pass_test[100];
char block_buffer;
char *IV;
int keysize = 19;

key = calloc(1,keysize);
printf("Password[maximum_20 characters]: ");
scanf("%s", pass_test);
strncpy(password, pass_test, 20);
memmove(key, password, strlen(password));
td = mcrypt_module_open("serpent", NULL, "cfb", NULL);
if(td == MCRYPT_FAILED)
return 1;
IV = malloc(mcrypt_enc_get_iv_size(td));
for(i=0; i<mcrypt_enc_get_iv_size(td); i++)
IV = rand();
i = mcrypt_generic_init(td, key, keysize, IV);
if(i<0)
{
mcrypt_perror(i);
return 1;
}

/* Encrypt/Decrypt */
while(fread(&block_buffer, 1, 1, infile) == 1)
{
if(strcmp(action,"e") == 0)
{
mcrypt_generic(td, &block_buffer, 1);
FILE *fname = fopen("encrypted", "a+b");
fwrite(&block_buffer, 1, 1, fname);
fclose(fname);
}
else if(strcmp(action,"d") == 0)
{
mdecrypt_generic (td, &block_buffer, 1);
FILE *fname = fopen("decrypted", "a+b");
fwrite(&block_buffer, 1, 1, fname);
fclose(fname);
}
}
return 0;
}
int main(int argc, char *argv[])
{
char *encrypt = "e";
char *decrypt = "d";

if(argc != 3)
{
help(argv[0]);
printf("%d",1);
exit(-1);
}
else if(!(strcmp(argv[1], encrypt) == 0 || strcmp(argv[1],decrypt)
== 0))
{
help(argv[0]);
printf("%d",2);
exit(-1);
}
if(crypto(open_file(argv[2]), argv[1], argv[2]) != 0)
fprintf(stderr, "Something went wrong , exiting ...\n");
exit(0);
}

/* EOC */

 

[Flash Gordon]

makobu wrote, On 22/01/08 14:00:

So what is your question? I will assume you want it reviewed as you have
not asked one.

* Make: cc -Wall -O2 -o executable file.c -
lmcrypt *

You should limit your line length to avoid line wrapping. Also if using
gcc, which seems likely, try using:
cc -Wall -Wextra -ansi -pedantic -O2 executable file.c -lmcrypt
Or, if you want to use c99 features (gcc is not yet fully C99 compliant)
cc -Wall -Wextra -std=c99 -pedantic -O2 executable file.c -lmcrypt

*
*************************************************************************************
*/

#include <stdio.h>
#include <stdlib.h>
#include <mcrypt.h>

Non-standard header which happens not to be on the Linux box I am using.

#include <string.h>

/* The help function */
void help(char *name)
{
printf("usage: %s [option: e > encrypt; d > decrypt] file\n",
name);
}

/* File opener */
FILE *open_file(char *path)
{
FILE *infile;
if((infile = fopen(path, "r")) == NULL)
{
fprintf(stderr, "cannot open %s\n",path);
exit(-1);

The only standard values of exit are 0, EXIT_SUCCESS and EXIT_FAILURE. 0
indicates success. Since you only use the one value for failure there is
no good reason not to use EXIT_FAILURE so that it is guaranteed to do
what you expect on all implementations.

}
else
return infile;
}

/* crypto */
int crypto(FILE *infile, char *action, char *name)
{
MCRYPT td;
int i;
char * key;
char password[20];
char pass_test[100];
char block_buffer;
char *IV;
int keysize = 19;

key = calloc(1,keysize);
printf("Password[maximum_20 characters]: ");
scanf("%s", pass_test);

Don't use an unadorned %s in scanf. It is just as bad as gets since if
the user enters more than you allowed for the buffer will overflow.

strncpy(password, pass_test, 20);

strncpy does not do the job here. If the user entered 20 characters or
more then this will NOT null terminate the string.

memmove(key, password, strlen(password));

Why on earth not use strcpy here? It will do exactly the same thing as
the line you entered, including going wrong if the user entered more
than 19 characters (you told the user up to 20 was legal, so this is
likely).

td = mcrypt_module_open("serpent", NULL, "cfb", NULL);
if(td == MCRYPT_FAILED)
return 1;
IV = malloc(mcrypt_enc_get_iv_size(td));
for(i=0; i<mcrypt_enc_get_iv_size(td); i++)
IV = rand();

You have not initialised the random number generator, so your sequence
of "random" numbers will be the same each time the program is run. Even
if you had initialised it rand() is still not suitable for cryptographic
work.

i = mcrypt_generic_init(td, key, keysize, IV);
if(i<0)
{
mcrypt_perror(i);
return 1;
}

/* Encrypt/Decrypt */
while(fread(&block_buffer, 1, 1, infile) == 1)

Why the complexity of fread to read a single byte? Using getc would be
easier.

{
if(strcmp(action,"e") == 0)
{
mcrypt_generic(td, &block_buffer, 1);
FILE *fname = fopen("encrypted", "a+b");

fname is a very misleading name for this variable.

fwrite(&block_buffer, 1, 1, fname);

Why use fwrite to write a single byte?

fclose(fname);

Opening and closing the file every time round the loop is mindbogglingly
stupidly inefficient.

}
else if(strcmp(action,"d") == 0)
{
mdecrypt_generic (td, &block_buffer, 1);
FILE *fname = fopen("decrypted", "a+b");
fwrite(&block_buffer, 1, 1, fname);
fclose(fname);

Same comments apply to this.

}
}
return 0;
}
int main(int argc, char *argv[])
{
char *encrypt = "e";
char *decrypt = "d";

if(argc != 3)
{
help(argv[0]);

argc could be 0 and argc[0] a null pointer (this really can happen on
*nix systems) which will give your help() function problems.

printf("%d",1);

What on earth is this printf for?

exit(-1);
}
else if(!(strcmp(argv[1], encrypt) == 0 || strcmp(argv[1],decrypt)
== 0))
{
help(argv[0]);
printf("%d",2);

Ah, some kind of error code? Printing an error message would be *far*
more helpful.

exit(-1);
}
if(crypto(open_file(argv[2]), argv[1], argv[2]) != 0)
fprintf(stderr, "Something went wrong , exiting ...\n");
exit(0);
}

/* EOC */

For whether you are using mcrypt properly you will have to ask else
where. However, you should fix the issues I have pointed out and next
type actually tell people why you are posting code.

 

[vippstar]

<snip>
/* crypto */
int crypto(FILE *infile, char *action, char *name)

crypto is a bad name, (there's already a 'crypto') I suggest you
choose a differend name.

{
MCRYPT td;
int i;
char * key;
char password[20];
char pass_test[100];
char block_buffer;
char *IV;
int keysize = 19;

key = calloc(1,keysize); You don't check calloc.
printf("Password[maximum_20 characters]: ");
scanf("%s", pass_test);
strncpy(password, pass_test, 20);
memmove(key, password, strlen(password));

^^^
key might be NULL, if calloc() failed.

td = mcrypt_module_open("serpent", NULL, "cfb", NULL);
if(td == MCRYPT_FAILED)
return 1; memory leak
IV = malloc(mcrypt_enc_get_iv_size(td)); you don't check malloc..
for(i=0; i<mcrypt_enc_get_iv_size(td); i++)
IV = rand();
i = mcrypt_generic_init(td, key, keysize, IV);
if(i<0)
{
mcrypt_perror(i);
return 1; memory leak
}

/* Encrypt/Decrypt */
while(fread(&block_buffer, 1, 1, infile) == 1)
{
if(strcmp(action,"e") == 0)
{
mcrypt_generic(td, &block_buffer, 1);
FILE *fname = fopen("encrypted", "a+b");
fwrite(&block_buffer, 1, 1, fname);
fclose(fname);
}
else if(strcmp(action,"d") == 0)
{
mdecrypt_generic (td, &block_buffer, 1);
FILE *fname = fopen("decrypted", "a+b");
fwrite(&block_buffer, 1, 1, fname);
fclose(fname);
}
}
return 0;}

memory leak

There are more errors which mr Gordon has posted about.

 

[Philip Potter]

crypto is a bad name, (there's already a 'crypto') I suggest you
choose a differend name.

Never heard of it. n1256 doesn't mention it, and neither does "man
crypto" on my machine. I *have* heard of crypt(), though.

Phil

 

[Randy Howard]

Never heard of it. n1256 doesn't mention it, and neither does "man
crypto" on my machine. I *have* heard of crypt(), though.

I think it's part of openssl, iirc.

If you limit yourself to names not chosen for any existing lib on the
planet, you're going to be running out of identifiers quickly.

 

[Mark Bluemel]

If you limit yourself to names not chosen for any existing lib on the
planet, you're going to be running out of identifiers quickly.

no - you just choose a naming convention that leads to unreadability

A combination of project name, with vowels removed, and Hungarian
notation perhaps?

I did wonder whether something based on the porn names principle could
be applied (name of your first pet plus the name of the road on which
you grew up)...

 

[makobu]

no - you just choose a naming convention that leads to unreadability

A combination of project name, with vowels removed, and Hungarian
notation perhaps?

I did wonder whether something based on the porn names principle could
be applied (name of your first pet plus the name of the road on which
you grew up)...

Thank you all for your feedback. Any other ways of making the code
more efficient?

 

[makobu]

no - you just choose a naming convention that leads to unreadability

A combination of project name, with vowels removed, and Hungarian
notation perhaps?

I did wonder whether something based on the porn names principle could
be applied (name of your first pet plus the name of the road on which
you grew up)...

Thank you all for your feedback. Any other ways of making the code
more efficient?

 

[Flash Gordon]

makobu wrote, On 23/01/08 11:03:

Thank you all for your feedback. Any other ways of making the code
more efficient?

Before worrying about making it efficient worry about making it easy to
understand and correct. I pointed out a number of areas where it was
simply wrong.

 

[makobu]

I pointed out a number of areas where it was

simply wrong.

The compiler didn't give out any warnings, and the program does what
its supposed to. So there might better ways to code the areas, but
they waren't wrong. Not according to gcc anyway.

 

[santosh]

The compiler didn't give out any warnings, and the program does what
its supposed to. So there might better ways to code the areas, but
they waren't wrong. Not according to gcc anyway.

Compile with the '-Wall', '-W', '-ansi' and '-pedantic' switches. You
can use '-std=c99' instead of '-ansi' for partial conformace to C99.
Read info gcc for further switches.

 

[Bart C]

Never heard of it. n1256 doesn't mention it, and neither does "man crypto"
on my machine. I *have* heard of crypt(), though.

I think it was Superboy's dog.

 

[Nick Keighley]

The compiler didn't give out any warnings,

you didn't ask it to

and the program does what
its supposed to.

wow. perhaps your testing is poor. You fail to check return
values of calloc(), you allow buffer overflow on input, you
misuse strncpy(), you don't seed rand(). You use rand()
for cryptography!?

I hope you don't write software for anything important.

 

[Flash Gordon]

Nick Keighley wrote, On 25/01/08 10:07:

you didn't ask it to

In any case, if not producing any warnings is sufficient to prove a
program is bug free then here is my implementation in standard C of the
"Do whatever you want" program. Run it and it will do whatever you want,
and it must be correct because it compiles without warning.

int main(void) {return 0;}

wow. perhaps your testing is poor. You fail to check return
values of calloc(), you allow buffer overflow on input, you
misuse strncpy(), you don't seed rand(). You use rand()
for cryptography!?

I'm sure there were a few other things I pointed out. Such as a buffer
overflow even if the user followed the instructions.

I hope you don't write software for anything important.

Doesn't mater if he does, for anything important there would be at least
a little QA and any attempt by anyone competent would reject SW like the
OPs.

See above. Compilers are not required to issue diagnostics for all
coding errors, and in fact they cannot because the compiler does not
know what you intend only what you tell it.

 

[makobu]

wow.
Woopa!

perhaps your testing is poor. You fail to check return
values of calloc(), you allow buffer overflow on input, you
misuse strncpy(), you don't seed rand(). You use rand()
for cryptography!?

Did you read the heading? "bare bones", its an example; not production
code.

I hope you don't write software for anything important.

Gee, thanks; i hope you don't write software for anything important
too.

 

[makobu]

Doesn't mater if he does, for anything important there would be at least
a little QA and any attempt by anyone competent would reject SW like the
OPs.

What you guys don't seem to get is that this code i just an example.
Anyone who wants to actually use it needs to make many changes to it,
and add a feature, not to be used as is, hence the words "bare bones"
in the title.

 

[santosh]

What you guys don't seem to get is that this code i just an example.
Anyone who wants to actually use it needs to make many changes to it,
and add a feature, not to be used as is, hence the words "bare bones"
in the title.

Usually "bare bones" means code which is correct in function, but
stripped of all non-essential stuff like error handling, debug output,
etc.

Your code, in addition to being "bare bones", had several errors in it,
which were pointed out to you in more than one response.

There is a subtle difference between "bare bones" code and broken code.

 

[David Thompson]

#if offtopic

makobu wrote, On 22/01/08 14:00:

MCRYPT td;
int i;
char * key;
char *IV;
int keysize = 19;
td = mcrypt_module_open("serpent", NULL, "cfb", NULL);
if(td == MCRYPT_FAILED)
return 1;
IV = malloc(mcrypt_enc_get_iv_size(td));
for(i=0; i<mcrypt_enc_get_iv_size(td); i++)
IV = rand();

You have not initialised the random number generator, so your sequence
of "random" numbers will be the same each time the program is run.

True. And once you do make them different, the IV chosen to encrypt a
given message (here, file) needs to be stored and used for decryption.
The simplest way is just to tack it onto the beginning of the
encrypted data, but many other schemes are possible.

Even > if you had initialised it rand() is still not suitable for cryptographic
work.

Maybe. Assuming this (not-standard) mcrypt does indeed implement
Serpent (or any other decent block cipher) in CFB mode, which seems a
reasonable guess, you want an IV which is unique or at least
duplicated with only negligible probability per key, and I think you
want not influencable by an attacker, but you don't need unpredictable
aka 'crypto random'. Unlike some other crypto parameters, which do.

The seedable state space of the C library prng is not specified, but
it unlikely to be more than 32 bits and may be as little as 15.
Whether this can avoid dangerous duplication depends on your traffic
volume and key management. This would be ontopic in sci.crypt .
As would be limitations of CFB. And human-entered passwords.

<snip other good points>
- formerly david.thompson1 || achar(64) || worldnet.att.net

 

[David Thompson]

I think it's part of openssl, iirc.

'crypto' is one of two (main) source trees, and (thus) library files,
in OpenSSL. But that source tree is divided into numerous modules, and
all the function names identify their module (DES_blah_this,
SHA1_that, EVP_the_other, etc.) so no function is just 'crypto'.

And elsethread, Superboy's dog was (is?) Krypto with a K.

- formerly david.thompson1 || achar(64) || worldnet.att.net