AES implementation not working

I made a simple AES implementation but it fails to parse data above 100byte for some reason.

Code:

char *dbuf;
dbuf = malloc(10240000);
in = 0;
n = (strlen(fbuffer)) / 16; //cycle times
for (j=0; j<n; j++)
{
for(i = 0; i < 16; i++)
{
 if (j > 0) {
  in = j * 16; };
 ctext[i] = fbuffer[i+in];
};
ctext[16]='\0';
aes_decrypt(ctx, ctext, ptext);
ptext[16]='\0';
strcat(dbuf, ptext);
};
dbuf[(strlen(fbuffer))] = '\0';

For some reason it stops decrypting it correctly above about 100byte and just prints out garbage.

I can't figure out why. Hopefully it is a simple problem to solve.

All I notice (besides the bad spacing and extra seimicolons after end-braces) is that you should initially zero-terminate dbuf (*dbuf = 0) for use with strcat.

Why are you mallocing 10MB for dbuf when you know it needs to be strlen(fbuffer)+1 long?

You should move

in = j * 16

outside of the for i loop since it doesn't need to be set every time. And it doesn't need the if(j>0) protection.

The for i loop can be replaced by strncpy

strncpy(ctext, fbuffer + in, 16);
ctext[16] = 0;

Actually, using strcat to add text to the end of dbuf is wasteful since you know it's being added in 16 char increments.

strncpy(dbuf + in, ptext, 16);

But to fix the bug you mention you probably have to post more code.

The only code that I have before it is just reading the encrypted file. Its nothing special.

I changed the loop to:

Code:

dbuf = malloc((strlen(fbuffer)));
dbuf[(strlen(fbuffer))] = '\0';
for (j=0; j<n; j++)
{
strncpy(ctext, fbuffer + in, 16);
ctext[16] = 0;
aes_decrypt(ctx, ctext, ptext);
ptext[16]='\0';
in = j * 16;
strcpy(dbuf, ptext);
}

And zero terminating dbug before using it.

But now it seems to only loop once.

Originally Posted by coderplus

But now it seems to only loop once.

That's because you're writing to the beginning of dbuf everytime now.
Try something like

Code:

size_t len = strlen(fbuffer);
dbuf = malloc(len + 1);
for (j = 0; j < len / 16; j++) {
  in = j * 16;
  strncpy(ctext, fbuffer + in, 16);
  ctext[16] = 0;
  aes_decrypt(ctx, ctext, ptext);
  ptext[16] = 0; // don't actually need this since strncpy below will only copy 16 chars
  strncpy(dbuf + in, ptext, 16);
}
dbuf[len] = 0;

The only code that I have before it is just reading the encrypted file. Its nothing special.

You never know. For instance, if there's any zero bytes in the buffer then strlen is not going to give a proper length. Also you need to make sure you read it in binary mode if you're on a system where that matters. The zero byte thing may be the source of your original problem of stopping after 100 bytes. What you would need to do is determine the message string length when you read in the buffer (don't use strlen).

The result is the same when I run the program. Additional whenever I print the decrypted buffer it doesn't print the entire section and it seems to be still encrypted plus always I see the same data printed many times over as well. I don't know why it does that either.

Here is the file reading sample:

Code:

printf("Reading file.\n");
f = fopen("testfile.dat", "rb");
if (f == NULL)
{
printf("Cannot find file. Exiting...");
exit(1);
};
fseek(f, 0, SEEK_END);
long s = ftell(f);
rewind(f);
fbuffer = malloc(s);
st = fread(fbuffer, 1, s, f);
fclose(f);
fbuffer[st] = '\0';
printf("Done reading file.\n");

For your encrypted data, you can't use ANY str... functions at all.
This includes trying to put a \0 at the end of the buffer (which is pointless, if you're not going to use any str... functions).

Further, unless you know FOR SURE that the decrypted data is all text, then you shouldn't use any str... functions for that either.

- you read a block of data using fread
- you decrypt 16 bytes
- you write 16 bytes of decrypted data to wherever (fwrite if a file, memcpy if somewhere in memory)

OK, so I just replace the str functions by loops of 16byte data doing?

Code:

textbuf[i] = encrypt[i];

But I still read the entire file at the beginning right, not 16byte at a time?

Well reading a 10MB file all in one go is about as far as I would go, but you would need another plan if say you were trying to process files in the GB range (which you couldn't read in one go anyway). When your process starts to have MB of infrequently used data (you write it once, you read it once), then chances are you're going to get hurt on performance when the OS starts paging bits of your data to/from the swap file.

Once you accept that you need something like
while ((n = fread(buff, 1, size, fp)) > 0 )

as the thing to read part of a file, then my suggestion would be to use BUFSIZ (or BUFSIZ*16 to make sure you can extract an integer number of AES blocks - though in practice, BUFSIZ is almost certain to be a multiple of 16) as your buffer size. The compiler implementers choose BUFSIZ to give reasonably efficient I/O transfers for the system you're on, in a portable way.

Plus, BUFSIZ is small enough that you can declare a local array in your function, without getting messy with malloc issues.

Yes the files will only be under 10mb size. If they will get larger I will read it in the loop format I guess yes.

PS. I forgot to mention earlier that whenever I don't '\0' the data that it encrypts and I use printf to check it (also when decrypted), it will always add extra characters or/and in same cases the key that it encrypted or decrypted it with. That is why I used it so often.

But won't I have that problem now (since you said I wouldn't need it anymore due to not using the str functions)?

Originally Posted by coderplus

PS. I forgot to mention earlier that whenever I don't '\0' the data that it encrypts and I use printf to check it (also when decrypted), it will always add extra characters or/and in same cases the key that it encrypted or decrypted it with. That is why I used it so often.

I wrote a networking module as part of a group project once. When the other guy started using it, he put message boxes in the callback function, and then showed me that it didn't work properly. I told him to stop checking that it worked and just use it, and unsurprisingly it then worked as designed.
Stop trying to check what it's doing, stop treating any of it as text (remove printfs), and just use the darn thing.

Of course unless you're using a form of Cipher Block Chaining, you're significantly weakening the encryption too.