«

/*
*
************************************************** ***********************************
* 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[i] = 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 */

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.

<snip>

> * 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[i] = 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.
--
Flash Gordon

On Jan 22, 4:00 pm, makobu <makobu.mwambir...@gmail.com> wrote:
> <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[i] = 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.

wrote:
> On Jan 22, 4:00 pm, makobu <makobu.mwambir...@gmail.com> wrote:
>> <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.

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

Phil

On Wed, 23 Jan 2008 03:44:46 -0600, Philip Potter wrote
(in article <fn72ae$hmg$>):

> wrote:
>> On Jan 22, 4:00 pm, makobu <makobu.mwambir...@gmail.com> wrote:
>>> <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.
>
> 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.


--
Randy Howard (2reply remove FOOBAR)
"The power of accurate observation is called cynicism by those
who have not got it." - George Bernard Shaw

Randy Howard wrote:

> 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)...

--
Bruce Redlands

On Jan 23, 1:16 pm, Mark Bluemel <mark_blue...@pobox.com> wrote:
> Randy Howard wrote:
> > 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)...
>
> --
> Bruce Redlands

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

On Jan 23, 1:16 pm, Mark Bluemel <mark_blue...@pobox.com> wrote:
> Randy Howard wrote:
> > 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)...
>
> --
> Bruce Redlands

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

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

<snip>

> 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.
--
Flash Gordon

> I pointed out a number of areas where it was
> simply wrong.
> --
> Flash Gordon

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.