Encryption algorithm using CTR mode

Question

I am using CTR mode (it is a cipher in itself) in this code. I just wanted to see what you thought about it before I finish it.

(Yes, I know that the plaintext length and key length must be highly specific. I will take care of that with padding later.)

I just wanted to know if you thought if this code was cryptographically secure and if I could speed it up a little.

class Encrypt
{
    public string CTR(string p, string key)
    {
        StringBuilder sb = new StringBuilder();

        for (uint c = 0; c < (p.Length / key.Length); c++ )
        {
            char[] kc = key.ToCharArray();
            char[] E = new char[key.Length];

            char[] cc = c.ToString().ToCharArray();
            int inc = 0;
            foreach (char ch in kc)
            {
                E[inc] = (char)(ch ^ cc[0]); // Something goes wrong here when there are 2 digits in uint c. (CodeReview plz help)
                inc++;
            }

            char[] pc = p.ToCharArray();


            for (int i = 0; i < E.Length; i++)
            {
                sb.Append((char)(pc[i] ^ E[i]));
            }
        }

        return sb.ToString();
    }
}

Answer 1

It looks like you are XORing a mildly modified key with the plaintext, then mildly modifying the key every 10 blocks. As @SimonForsberg has noted, such simplistic schemes just are amateur-grade ciphers, and certainly not cryptographically secure. (Cryptanalysis would be beyond the scope of Code Review, though.)

Only a few experts are capable of producing cryptographically secure algorithms — and even then, they submit their schemes to merciless peer review. There's always someone out there who is smarter than you are. Attacks only get better, never worse. If you want a cryptographically secure scheme, don't roll your own crypto.

Answer 2

tl;dr: The code for review isn't doing encryption. Counter-mode encryption is a real thing, but the reviewed code omitted the actual encryption steps from it. Also, you should be using byte[]/byte's instead of string/char's.

Security

CTR is counter-mode. The gist is that:

1. You encrypt a counter stream.

   • A counter stream is just the stream of values you get from a counter. For example, 1, 2, 3, 4, etc..

2. Assuming that your encryption method was secure, then your counter stream should be pseudo-random to anyone who doesn't know how you encrypted it.

3. So, you can XOR messages with the encrypted counter stream.

   • Note that this is like how a one-time pad (OTP) works.

   • In some sense, counter-mode is a special type of OTP where the pad's values were generated by this known method, which makes it theoretically weaker, but much more convenient.

4. To decrypt the ciphertext, the decrypter generates the same counter stream, encrypts it using the same method (so they should have to know the same encryption key), then XOR's that with the cipher. Because XOR'ing with the same stream twice gives the original result, they get the plaintext.

The problem with your code's security is that you're not encrypting a counter stream. At most, you're just using an unencrypted counter stream. So, you're doing encryption without the actual encryption step.

Incorrect data types

The reviewed code attempts to take string's and operates on them one char-at-a-time.

Instead, you should take byte[]'s and operate on them one-byte-at-time. Both string and char are wrong for this.

For user convenience, you can make an overloaded method call that serializes a string into a byte[] for encryption, then one that returns a string for decryption. But, the actual encryption/decryption methods should operate on byte[]'s, or Stream's, not string's.

Answer 3

Comments

You are missing comments. At the very least I would expect a method comment explaining what CTR stands for (CTR mode? I'm not that familiar with that mode, but if it's like any other mode of operation, you are missing the actual block cipher, it seems).

Ideally, you would link to some documentation of the algorithm you are implementing as well. If none exists, describe your algorithm in-depth yourself.

Variable Naming

Your variable names are pretty bad, making your code very hard to understand.

If these are the same names as names in some mathematical documentation, it may be ok, but you need to link to it.

If these names only occur in your code, you need to change them. key is the only acceptable name you have, p, c, kc, E, cc, inc, ch, pc, i, sb are all not understandable without at least some context.

While your names are way too short, you do apply the c suffix consistently, which is good.

Here are some possible improvement ideas:

• p: plaintext
• pc: plaintextCharacters (you may be able to come up with a better suffix than characters)
• sb: ciphertext (a reader doesn't really care that it's a string builder, but what the string builder actually contains)
• kc: keyCharacters
• ch: keyCharacter (this is one item from the kc array, and the name should reflect this)
• inc: If you make your foreach into a for loop you wouldn't need this.
• c: It's customary to use i as index variable. Now I'm left wondering why you don't follow this custom. If you have a better name than i, you should definitely use it, so if c stands for something, write it out.
• cc: see above, then append Characters
• i: i as index if there is nothing better is fine
• E: modifiedKey or similar might be fitting

Security

I haven't looked at the security aspect in-depth, but this question might interest you. Here it is also suggested to not start c at 0 for every encryption.

Apart from that, Don't be a Dave and Don't roll your own (Although I'm assuming that this is for educational purposes only).