Critique my mini-AES

**Hughesz93** · 05-04-2011

This is a mini-AES program I have been working on partly as an independent project and partly for my internship over the past few months. I'm not sure if it works or not, when I try to run it it takes forever, I waited about 30 min and got impatient. Also, I just started programming C about 6 months ago.

I am looking for critique and *corrections* of this program. It will be much appreciated. I also realize some of my methods may not be the most efficient, I was just trying to get it done, and will also welcome any critique in this category.

Code:

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


FILE *fp;
char *buffer;
char *outputBuffer;
unsigned long fileLen;
int c;

//user inputted key - tester
char* KEY = "hi";

//round keys
unsigned int w0, w1, w2, w3, w4, w5, w6, w7, w8, w9, w10, w11;

//text nibbles
unsigned int Nibble1, Nibble2, Nibble3, Nibble4;

int currentBufferPosition = 0;

unsigned int t[2][16];

void readFile(char *name) {

	fp = fopen(name, "rb");

	fseek(fp, 0, SEEK_END);
	fileLen = ftell(fp);
	fseek(fp, 0, SEEK_SET);

	buffer = malloc(fileLen+1);

	int i = 0;

	for(i = 0;i < fileLen - 1;++i) {
		(buffer[i]) = (char)fgetc(fp);;
	}

	fclose(fp);

	}


void getNibbles() {

	Nibble1 = (buffer[currentBufferPosition]) & 0xf;
	Nibble2 = (buffer[currentBufferPosition] << 4) & 0xf0;

	//printf("%x\n", Nibble1);
	//printf("%x\n", Nibble2);

	++currentBufferPosition;

	Nibble3 = (buffer[currentBufferPosition]) & 0xf;
	Nibble4 = (buffer[currentBufferPosition] << 4) & 0xf0;

	//printf("%x\n", Nibble3);
	//printf("%x\n", Nibble4);

	}

void getKeyNibbles(char *k) {
	
	w0 = (k[0]) & 0xf;
	w1 = (k[0] << 4) & 0xf0;

	//printf("%x\n", Nibble1);
	//printf("%x\n", Nibble2);

	w2 = (k[1]) & 0xf;
	w3 = (k[1] << 4) & 0xf0;

	//printf("%x\n", Nibble3);
	//printf("%x\n", Nibble4);

}

void setChart() {

	t[1][1] = 0x0;
	t[1][2] = 0x1;
	t[1][3] = 0x2;
	t[1][4] = 0x3;
	t[1][5] = 0x4;
	t[1][6] = 0x5;
	t[1][7] = 0x6;
	t[1][8] = 0x7;
	t[1][9] = 0x8;
	t[1][10] = 0x9;
	t[1][11] = 0xa;
	t[1][12] = 0xb;
	t[1][13] = 0xc;
	t[1][14] = 0xd;
	t[1][15] = 0xe;
	t[1][16] = 0xf;

	t[2][1] = 0xe;
	t[2][2] = 0x4;
	t[2][3] = 0xd;
	t[2][4] = 0x1;
	t[2][5] = 0x2;
	t[2][6] = 0xf;
	t[2][7] = 0xb;
	t[2][8] = 0x8;
	t[2][9] = 0x3;
	t[2][10] = 0xa;
	t[2][11] = 0x6;
	t[2][12] = 0xc;
	t[2][13] = 0x5;
	t[2][14] = 0x9;
	t[2][15] = 0x0;
	t[2][16] = 0x7;

}

unsigned int nibbleSub(unsigned int n) {
	
	int x;

	for(x = 0; x < 16; x++) {

		if (n == t[1][x]) {
			n = t[2][x];
			return n;
		}

	}
	
}

void mixColumn() {

	unsigned int temp1, temp2, temp3, temp4;

	temp1 = Nibble1;
	temp2 = Nibble2;
	temp3 = Nibble3;
	temp4 = Nibble4;

	Nibble1 = (0x3 * temp1) ^ (0x2 * temp2);
	Nibble2 = (0x2 * temp1) ^ (0x3 * temp2);
	Nibble3 = (0x3 * temp3) ^ (0x2 * temp4);
	Nibble4 = (0x2 * temp3) ^ (0x3 * temp4);

}

void shiftRow() {

	unsigned int temp;

	temp = Nibble2;
	Nibble2 = Nibble4;
	Nibble4 = temp;

}

//inputted key must be 16 bits(2 chars)
void setKeys(char *s) {

	unsigned int temp0, temp1, temp2, temp3;

	getKeyNibbles(s);	
	
		w4 = w0 ^ nibbleSub(w3) ^ 0x1;
		w5 = w1 ^ w4;
		w6 = w2 ^ w5;
		w7 = w3 ^ w6;
		w8 = w4 ^ nibbleSub(w7) ^ 0x2;
		w9 = w5 ^ w8;
		w10 = w6 ^ w9;
		w11 = w7 ^ w10;	
}

void addKey(int round) {

	if (round = 0) {
		Nibble1 = Nibble1 ^ w0;
		Nibble2 = Nibble2 ^ w1;
		Nibble3 = Nibble3 ^ w2;
		Nibble4 = Nibble4 ^ w3;
	} else if (round = 1) {
		Nibble1 = Nibble1 ^ w4;
		Nibble2 = Nibble2 ^ w5;
		Nibble3 = Nibble3 ^ w6;
		Nibble4 = Nibble4 ^ w7;
	} else if (round = 2) {
		Nibble1 = Nibble1 ^ w8;
		Nibble2 = Nibble2 ^ w9;
		Nibble3 = Nibble3 ^ w10;
		Nibble4 = Nibble4 ^ w11;
	}

}

void encrypt(char *name) {

	readFile(name);
	setKeys(KEY);
	setChart();

	do {
		getNibbles;
		addKey(0);
		Nibble1 = nibbleSub(Nibble1);
		Nibble2 = nibbleSub(Nibble2);
		Nibble3 = nibbleSub(Nibble3);
		Nibble4 = nibbleSub(Nibble4);
		shiftRow();
		mixColumn();
		addKey(1);
		Nibble1 = nibbleSub(Nibble1);
		Nibble2 = nibbleSub(Nibble2);
		Nibble3 = nibbleSub(Nibble3);
		Nibble4 = nibbleSub(Nibble4);
		shiftRow();
		addKey(2);
		
		outputBuffer = outputBuffer + Nibble1 + Nibble2 + Nibble3 + Nibble4;
		currentBufferPosition++;

	} while (currentBufferPosition < fileLen - 1);

}

void printBuffer() {

	int counter;

	for (counter = 0; counter < fileLen -1; counter++) {

		printf("%x", outputBuffer[counter]);

	}

}


void main() {
	
	encrypt("text");	
	
}

**~~CommonTater~~** · 05-04-2011

1) It's int main (void) unless you're working on a microcontroller with it's own C variant.

2) because it's int main (void) main has to return a value (usually 0) to the operating system.

3) Global variables are not wrong... but they should be used sparingly to avoid accidental changes in value. It is far smarter to pass a couple of paremeters into a function and have it return a result than to let your functions operate (almost) entirely on global values.

4) ALWAYS check the return values of I/O operations such as fopen(), fscanf(), etc. Ask what happens if the file doesn't open for some reason...

5) Instead of reading characters in a loop, you could load your text buffer with a single call to fread()

6) Your setchart() function could be replaced by using array initializers with your t array when it's defined (no processing overhead)

7) The t array itself only needs to be single dimensional... like this...

Code:

unsigned int t[16] = {0xE,0x4,0xD,0x1,0x2,0xF,0xB,0xA,0x8,0xC,0x5,0x9,0x0,0x7}

8) With the array initialized as shown you can access it much more efficiently in nibblesub()

Code:

unsigned int nibbleSub(unsigned int n) 
  { return t[n]; }

9) your round keys w0 .. w11 should probably be an array.

10) in encrypt your nibbles should also probably be an array as this would allow looping.

11) in encrypt() your assignment to output buffer will fail... you need to access this as an array of characters, C does not "add" strings.

**quzah** · 05-04-2011

1. You use globals pretty much everywhere. You really shouldn't.
2. This:

Code:

void main() {
	
	encrypt("text");	
	
}

Should be:

Code:

int main( void )
{
    encrypt( "text" );
    return 0;
}

3. You are writing to memory you don't have, because you don't understand arrays:

Code:

void setChart() {

	t[1][1] = 0x0;
	t[1][2] = 0x1;
	t[1][3] = 0x2;
	t[1][4] = 0x3;
	t[1][5] = 0x4;
	t[1][6] = 0x5;
	t[1][7] = 0x6;
	t[1][8] = 0x7;
	t[1][9] = 0x8;
	t[1][10] = 0x9;
	t[1][11] = 0xa;
	t[1][12] = 0xb;
	t[1][13] = 0xc;
	t[1][14] = 0xd;
	t[1][15] = 0xe;
	t[1][16] = 0xf;

	t[2][1] = 0xe;
	t[2][2] = 0x4;
	t[2][3] = 0xd;
	t[2][4] = 0x1;
	t[2][5] = 0x2;
	t[2][6] = 0xf;
	t[2][7] = 0xb;
	t[2][8] = 0x8;
	t[2][9] = 0x3;
	t[2][10] = 0xa;
	t[2][11] = 0x6;
	t[2][12] = 0xc;
	t[2][13] = 0x5;
	t[2][14] = 0x9;
	t[2][15] = 0x0;
	t[2][16] = 0x7;

}

Arrays go from 0 to size -1. So you should have t[0] and t[1], not t[1] and t[2].

4. All of your 'Nibble' stuff could just as easily be another array.

That will get you started.

Quzah.

**anduril462** · 05-04-2011

I didn't see this in my scan of Tater's or Quzah's comments:

Code:

void addKey(int round) {

	if (round == 0) {
        ...
	} else if (round == 1) {
        ...
	} else if (round == 2) {
        ...
	}

}

A single = is for assignment, a double == is for comparison. Learn when to use which one. Your if clauses here need a double ==.

**anduril462** · 05-04-2011

Oh, and you should declare your functions to specifically take a void parameter. There's a difference.

Code:

void foo()

This declares a function foo that returns nothing and takes any amount and type (including none) parameters. This means the compiler can NOT check to make sure you're calling it properly.

Code:

void foo(void)

This declares a function foo that returns nothing and takes exactly zero parameters. This form makes your intentions clear and lets the compiler do it's job of catching any screw-ups you might have made.

**~~CommonTater~~** · 05-04-2011

@anduril ... nice catch!

**quzah** · 05-04-2011

Originally Posted by CommonTater

@anduril ... nice catch!

Indeed. I didn't even see Tater's made it in before mine. Anyway, all of that should be a good starting point.

Quzah.

**~~CommonTater~~** · 05-04-2011

One more I just noticed.... In encrypt() output buffer is a char* that is used in an uninitialized state.

And here's the list from Pelles C

Code:

Building main.obj.
E:\c_Code\Experiments\testconsole\main.c(29): warning #2216: The return value from 'fseek' is never used.
E:\c_Code\Experiments\testconsole\main.c(31): warning #2216: The return value from 'fseek' is never used.
E:\c_Code\Experiments\testconsole\main.c(41): warning #2216: The return value from 'fclose' is never used.
E:\c_Code\Experiments\testconsole\main.c(46): warning #2117: Old-style function definition for 'getNibbles'.
E:\c_Code\Experiments\testconsole\main.c(46): warning #2027: Missing prototype for 'getNibbles'.
E:\c_Code\Experiments\testconsole\main.c(80): warning #2117: Old-style function definition for 'setChart'.
E:\c_Code\Experiments\testconsole\main.c(80): warning #2027: Missing prototype for 'setChart'.
E:\c_Code\Experiments\testconsole\main.c(97): warning #2238: Array index for 'unsigned int [16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(99): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(100): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(101): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(102): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(103): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(104): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(105): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(106): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(107): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(108): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(109): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(110): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(111): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(112): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(113): warning #2238: Array index for 'unsigned int [2][16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(114): warning #2238: Array index for 'unsigned int [16]' is out-of-bounds.
E:\c_Code\Experiments\testconsole\main.c(118): warning #2235: Not all control paths return a value.
E:\c_Code\Experiments\testconsole\main.c(133): warning #2117: Old-style function definition for 'mixColumn'.
E:\c_Code\Experiments\testconsole\main.c(133): warning #2027: Missing prototype for 'mixColumn'.
E:\c_Code\Experiments\testconsole\main.c(149): warning #2117: Old-style function definition for 'shiftRow'.
E:\c_Code\Experiments\testconsole\main.c(149): warning #2027: Missing prototype for 'shiftRow'.
E:\c_Code\Experiments\testconsole\main.c(162): warning #2114: Local 'temp3' is not referenced.
E:\c_Code\Experiments\testconsole\main.c(162): warning #2114: Local 'temp2' is not referenced.
E:\c_Code\Experiments\testconsole\main.c(162): warning #2114: Local 'temp1' is not referenced.
E:\c_Code\Experiments\testconsole\main.c(162): warning #2114: Local 'temp0' is not referenced.
E:\c_Code\Experiments\testconsole\main.c(178): warning #2030: '=' used in a conditional expression.
E:\c_Code\Experiments\testconsole\main.c(183): warning #2030: '=' used in a conditional expression.
E:\c_Code\Experiments\testconsole\main.c(188): warning #2030: '=' used in a conditional expression.
E:\c_Code\Experiments\testconsole\main.c(179): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(180): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(181): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(182): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(183): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(189): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(190): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(191): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(192): warning #2154: Unreachable code.
E:\c_Code\Experiments\testconsole\main.c(204): warning #2046: Expression with no effect removed.
E:\c_Code\Experiments\testconsole\main.c(227): warning #2117: Old-style function definition for 'printBuffer'.
E:\c_Code\Experiments\testconsole\main.c(227): warning #2027: Missing prototype for 'printBuffer'.
E:\c_Code\Experiments\testconsole\main.c(233): warning #2216: The return value from 'printf' is never used.
E:\c_Code\Experiments\testconsole\main.c(240): warning #2117: Old-style function definition for 'main'.
E:\c_Code\Experiments\testconsole\main.c(240): warning #2181: Incorrect signature for entry-point 'main'; expected 'int __cdecl function(void)' but found 'void __cdecl function(void)'.
Building testconsole.exe.
Done.

Thread: Critique my mini-AES

Thread Tools

Search Thread

Display

Critique my mini-AES

Similar Threads

Mini If

Critique my code?

mini-itx

C 101 critique, please?

critique me please