1

so I found the implementation of RC4 in pure C, which I was using on my website. It was working super good except when I input a 6 characters string. Then I get the Internal Error Page. Figured out that only this length causes a problem.

1.Crypt.c

unsigned char S[256];
unsigned int i, j;
void swap(unsigned char *s, unsigned int i, unsigned int j) {
unsigned char temp = s[i];
s[i] = s[j];
s[j] = temp;
}

/* KSA */
void rc4_init(unsigned char *key, unsigned int key_length) {
for (i = 0; i < 256; i++)
    S[i] = i;

for (i = j = 0; i < 256; i++) {
    j = (j + key[i % key_length] + S[i]) & 255;
    swap(S, i, j);
}

i = j = 0;
}

/* PRGA */
unsigned char rc4_output() {
i = (i + 1) & 255;
j = (j + S[i]) & 255;

swap(S, i, j);

return S[(S[i] + S[j]) & 255];
}

char *rc4_e(char *text, size_t text_length)
{
char *dup=(char *)malloc(text_length * sizeof(char));
strcpy(dup,text);
unsigned char *vector[2] = {"key", dup}; 
    int y;
    rc4_init(vector[0], strlen((char*)vector[0]));
char *out=(char *)malloc(text_length * sizeof(char) );
char *ptr=out;
    for (y = 0; y < strlen((char*)vector[1]); y++)
       ptr += sprintf(ptr,"%02X",vector[1][y] ^ rc4_output());
*(ptr + 1) = '\0';
return out;
}

2.Main

#define SIZE 1000
char* pass=(char*)malloc(SIZE * sizeof(char));
char *RC4_pass=(char*)malloc(getSize(pass) * sizeof(char)); 
 strcpy(RC4_pass,rc4_e(pass,sizeof(pass)));

Any advice or thoughts are extremely welcome. Just want to know whether it is the function itself that is bad or the rest of my C code. Thank!

FisherDisinformation
  • 159
  • 3
  • 7
  • make a command line test function and run it under a debugger – pm100 Apr 14 '15 at 23:33
  • 1
    regarding calls to malloc(): 1) do not cast the returned value 2) always check (!=NULL) the returned value to assure successful operation 3) do not use 'sizeof(char)' in the parameter list as that value is always 1 and all it does is clutter the code. – user3629249 Apr 15 '15 at 00:09
  • 1
    this code: 'sizof(pass)' will always be 4, because pass is defined as a char* I doubt that is actually what you want – user3629249 Apr 15 '15 at 00:11
  • when #define'ing a numeric value, always wrap the numeric value in parens '(' and ')' to avoid several kinds of text replacement problems – user3629249 Apr 15 '15 at 00:13
  • to make the code much more readable by us humans, please indent (say 4 spaces) after every opening brace '{' and un-indent before every closing brace '}'. Do not use tabs for indenting as each editor/wordprocessor has the tabs stops/tab width set differently. – user3629249 Apr 15 '15 at 00:16
  • the 'i' and 'j' parameters mask the global 'i' and 'j' variables. This is a very bad programming practice. suggest enabling all warnings when compiling, so these kinds of problems are caught/fixed now rather than spending many many hours debugging any related problems. the correct parameter for gcc would be '-Wshadow' – user3629249 Apr 15 '15 at 00:50

1 Answers1

2

There is a problem with this line:

char *dup=(char *)malloc(text_length * sizeof(char));

You forgot to add an extra byte for the terminating '\0' at the end of the string. So at the very next line:

strcpy(dup,text);

you're committing an out-of-bounds access in the array dup, which is causing undefined behaviour.

r3mainer
  • 23,981
  • 3
  • 51
  • 88
  • Yeah this one is an error that I should've fixed, so thanks for pointing out. I think my error was not only in this function, but also in the fact that I combined it with a base64, which may be the actual cause of error. Thanks again! – FisherDisinformation Apr 15 '15 at 00:18