current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

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();
    }
}
share|improve this question
    
cryptographically secure, sorry but I just want to check here... do you know what that means? – Simon Forsberg♦ Mar 24 at 17:59
    
@SimonForsberg Yes I do... Why? I am new to this – fraga11 Mar 24 at 18:04
    
I don't know a lot about cryptographically security, but just glancing at your code I can see that you're doing a simple XOR operation, which doesn't really classify as cryptographically secure. That said, I'm sure there's a lot of things you can learn here. Code Review is for all programmers who makes something working, no matter how simple or advanced it is. Welcome to Code Review, hope you get some good answers. – Simon Forsberg♦ Mar 24 at 18:15
1  
Please note that "how secure is..." questions have been discussed before. That being said, just keep in mind that the majority of programmers here are not security experts and may not be able to address that aspect of the code in-depth. – Phrancis Mar 24 at 18:17
    
@PinCrash Thanks, I'm not a security expert either. Unless wikipeida makes me one, haha. I'll also add "security" tag. Maybe you can at least help me with performance? – fraga11 Mar 24 at 18:18
up vote 3 down vote

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.

share|improve this answer
    
+1 for the last sentence. Cannot be stressed enough. – AlexR Mar 27 at 22:15
up vote 1 down vote

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).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.