HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

RC4 implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationrc4stackoverflow

Problem

I am using the following implementation of the RC4 encryption algorithm in C#

/// 
/// RC4 encryption algorithm
/// 
/// Input bytes
/// Key bytes
/// encrypted bytes
public static byte[] Encrypt(byte[] bytes, byte[] key)
{
    byte[] z = new byte[bytes.Length];
    byte[] s = new byte[256];
    byte[] k = new byte[256];
    byte temp;
    int i, j;

    for (i = 0; i < 256; i++)
    {
        s[i] = (byte)i;
        k[i] = key[i % key.GetLength(0)];
    }

    j = 0;
    for (i = 0; i < 256; i++)
    {
        j = (j + s[i] + k[i]) % 256;
        temp = s[i];
        s[i] = s[j];
        s[j] = temp;
    }

    i = j = 0;
    for (int x = 0; x < z.GetLength(0); x++)
    {
        i = (i + 1) % 256;
        j = (j + s[i]) % 256;
        temp = s[i];
        s[i] = s[j];
        s[j] = temp;
        int t = (s[i] + s[j]) % 256;
        z[x] = (byte)(bytes[x] ^ s[t]);
    }
    return z;
}


Is it possible to optimize my implementation a bit further, eventually using unsafe code, - especially for the variable swapping?
I hope for some constructive criticism.

Solution

public static byte[] Encrypt(byte[] bytes, byte[] key)


bytes doesn't tell me anything that I don't already know from the type. plaintext would tell me the meaning of this variable.

byte[] z = new byte[bytes.Length];


Similarly, this would be more usefully named ciphertext.

byte[] s = new byte[256];
    byte[] k = new byte[256];
    byte temp;
    int i, j;


Is there any benefit to reusing temp, i, and j rather than just redeclaring them in each scope? The benefit to redeclaring them is that it's more transparent that they are new variables.

for (i = 0; i < 256; i++)
    {
        ...
        k[i] = key[i % key.GetLength(0)];
    }

    ...
    for (i = 0; i < 256; i++)
    {
        j = (j + s[i] + k[i]) % 256;
        ...
    }


k is used precisely once, and that's in a loop of the same length as the one which initialises it. IMO it would simplify things to eliminate the middleman and just use key[i % key.Length] in the second loop.

j = (j + s[i] + k[i]) % 256;


i = (i + 1) % 256;
        j = (j + s[i]) % 256;
        ...
        int t = (s[i] + s[j]) % 256;


Here is the big optimisation opportunity. Replacing % 256 with & 255 gives me a 25% reduction in execution time with a large plaintext.

for (int x = 0; x < z.GetLength(0); x++)


I missed this at first: since it's a one-dimensional array, you can use Length instead of GetLength(0). This gives a significant further speedup.

I must say, though, that the code is simple enough to already be very fast. My test case is 512MB of plaintext, and the time drops from 10.2 secs to 5.4 secs.

Code Snippets

public static byte[] Encrypt(byte[] bytes, byte[] key)
byte[] z = new byte[bytes.Length];
byte[] s = new byte[256];
    byte[] k = new byte[256];
    byte temp;
    int i, j;
for (i = 0; i < 256; i++)
    {
        ...
        k[i] = key[i % key.GetLength(0)];
    }

    ...
    for (i = 0; i < 256; i++)
    {
        j = (j + s[i] + k[i]) % 256;
        ...
    }
j = (j + s[i] + k[i]) % 256;

Context

StackExchange Code Review Q#157603, answer score: 2

Revisions (0)

No revisions yet.