# Tag Info

31

No. This is just a worse Caeser cypher. steps to break: Convert each string to an int in the output Subtract everything from the min output value - ord('A') Everything is now between 0 and 26 If the message is readable, you are done (this will be true if the message contains an 'a') Otherwise, add 1 to the each character After repeating this between 1 and ...

27

Naming public void lol() { richTextBox1.Text = this.r; } C# isn't LOLCODE, and shouldn't have functions named lol, ever. Use descriptive and meaningful names. Most of your names are pretty bad: public void pk(int print) string rr; string r; namespace pl class Form1 : Form Additionally, this is named check(): private void check() { while (tr....

25

Since you asked for a feedback on the encryption algorithm, too. You are not specialized in cryptography. Do not even attempt to come up with a cryptographic algorithm unless you're doing it for fun and/or to learn and plan to never actually use it. As Bruce Schneier said: Anyone, from the most clueless amateur to the best cryptographer, can create an ...

25

Since there are only three valid key sizes for AES, it makes sense to not even let the AES class be instantiated with any uint16 value. I would introduce an enum similar to this: enum class AesKeyLen { Aes128, Aes192, Aes256 }; And then change the uint16 constructor to this: explicit AES (AesKeyLen keyLen); Sure, they could still pass bad ...

21

I'm looking to improve the code to make it not "DIY-crypto-bad", if at all possible I work in security. This is not my area, but I have a non-zero amount of knowledge on implementing secure cryptographic primitives, and from a cursory look I found some issues. For starters: keyOut[i] = AES::S_BOX[keyOut[i]]; and state[i][j] = AES::S_BOX[state[i][j]]; ...

19

Single responsibility principle A class should be responsible for one thing. The Enigma class is doing too much. It paints a graphical user interface, it works with files, it encrypts and decrypts, and lots of other things. It would be good to split this up aggressively. Start by moving everything out of the class that doesn't need a graphical element. ...

18

It's kind of secure, but it uses older algorithms. Although Benjamin correctly identifies 3DES, I would not call 3 key triple DES "broken". It still delivers a security of about 112 bits which nobody sane will try and break. There is a chance that somebody would try and break your password though, and the shown password is clearly not random enough as it ...

17

NO NO NO NO NO! Besides what others have pointed out, I find some serious flaws in your code. You are not actually using blowfish. To use blowfish, your salt must begin with something like $2y$07$see the PHP Documentation Removing your$blowfish_salt variable from the code makes no difference! This will produce the same output. Therefore, we conclude that ...

17

However, there is a lot of talk about what implementations are secure and not secure. How does my method measure up? Is it secure? Are there more secure methods in PHP for hashing tokens and matching with tokens later on? Since you're specifically asking about security, I think reviewing your security instead of your code is a valid answer in this case. ...

17

You are using in your code some non-standard loops. for (int i = 1; i <= streamFooter.Length; ++i) There should be a comment explaining why this loop is starting at 1 and not as expected at 0. I guess there is a reason for this. Even better would be to define a const for the 1 that could probably explain this offset without a comment. for (int i = 0; ...

16

Things you did well on: You make good use of comments. You try to make the user experience as smooth as possible, printing out a lot of useful information. Things you could improve: A few notes that others haven't covered: Running your program through Valgrind, I didn't see any memory leaks besides where your if conditions fail and you exit main(). ...

16

Simple does not mean fast, so you cannot judge performance based on how simple the implementation looks. Usually the most efficient way to perform a non-trivial task is not also the simplest way to do it. In this case though, there is a much more efficient solution that is about equally simple, and is probably sufficient. There is a serious problem with ...

15

I know that Hosch250 has already covered naming but I think it's worth letting you in on a secret... Most programmers would prefer "stupid" code that is well written, with good naming and consistent formatting to "clever" code with ridiculous names. I don't want to bash you too hard but button3? I bet you my house, wife and dog that you won't have a clue ...

15

Overall, this is pretty well written. It's very easy to read and understand. I especially like that you annotated the usage() function with _Noreturn. That really helps readability in my opinion. Constants In general, I'd avoid using #define for defining constants. C supports const and it allows you to give types to your constants so the compiler can check ...

14

Well, I don't know much about PHP; but I know a little about security so that is what I am going to review. First I am going to review your encryption algorithm: AES-192. In June 2003, the U.S. Government announced that AES could be used to protect classified information. So that means you are doing okay/decent with your choice. High speed and low RAM ...

14

Bug Letters J & K, U & W, and V & X have the same pattern. This is not what the specification says: either I & J and U & V shares the same pattern, or none of the letters do. Style String literals ain't comments. If you want two lines of comments, you start your two lines with a # each. Also take the habit to put a space after the # ...

14

Interface Design: void encrypt(const uint8 input[], const uint8 key[], uint8 output[]) const override; void decrypt(const uint8 input[], const uint8 key[], uint8 output[]) const override; Your encrypt and decrypt is very limiting. This means you need to load the whole of your input into memory before you can start any operations. It would ...

14

That seems like a very long method to splice and put together the string every even set of characters. You are essentially trying to build a square matrix using the characters from the given string. Python has a very nice slice-and-stride feature which can perform the same task in a single inline operation. I took the liberty of rewriting your encrypt ...

14

I am only looking for some feedback on my coding style. Formatting is good. I hope it is auto formatted. Respect the presentation width Rather than oblige a horizontal scroll bar, auto format to a narrower width to avoid that. Avoid dogma "to ONLY USE ONE return statement in functions and NOT to use things like break, continue or go-to." --> This is a ...

13

Naming: I'm not very comfortable with your naming of variables and methods. Some are quite unclear. For instance: UString, Rid, Sid, raw, those are very vague names. Try to name things in a way to not leave margin for misinterpretation. UString should probably be unencryptedPassword, then you could symmetrically rename Password to encryptedPassword. Rid ...

13

MD5 md55 = System.Security.Cryptography.MD5.Create(); Do yourself a favor, an stick using System.Security.Cryptography; at the top of the code file - then you can shorten this line to MD5 md55 = MD5.Create(); - but md55 is a bad name that doesn't mean anything. Perhaps engine, or hashProvider... anything but md55! char[] words That is a confusing name - ...

13

There have been several mentions of inefficiencies like using String everywhere here, but those are in fact, almost irrelevant compared to the big issue which you've already suspected. The fact that your code runs relatively fine with 10 steps but blows up completely with 20 should be a big hint: Your code has recursion with exponential blowup, and your ...

13

I think the header could be trimmed down a lot. The constant tables belong in the implementation file since they are not needed for the definition of the class. Since the AES class does not hold any state (except for that inherited from BlockCipher), I would not declare the private functions in the header, but only keep them in the implementation file in an ...

13

In addition to Oscar Smith's answer why this code is not secure, here are a few nitpicks on your code itself: While it is legal to end a line with ; in Python, it is not necessary and actively discouraged. You should use list comprehensions: def encrypt(_key, message): key_value = int(''.join(str(ord(x)) for x in str(_key))) return [ord(letter) + ...

13

One of your lines of code is horribly cryptic. I think it's so bad it's worthy of it's own answer. new_string[i] = new_string[i].replace(new_string[i], new_string[i] + string[i]) This is a longwinded way of writing: new_string[i] += string[i] This is something you should have noticed, and should have gone to first. If you change your for loop to use ...

13

No, it's not secure. Your code is using Random instead of SecureRandom, which limits the entropy of the salt to 48 bits. In addition, as an auditor I would immediately reject any "security code" that is implemented directly in the main method. To demonstrate that you understand the building blocks of a cipher, your code has to be structured into manageable ...

12

Number of iterations is pretty low. Since .NET's PBKDF2 implementation is very slow, you can't afford a good number of iterations. But even with it, 20000 should be affordable for server side hashing. For client side hashing you can go much higher. You're outputting more than the natural size (20 bytes for SHA-1), which decreases performance twofold without ...

12

You're obtaining more than 20 bytes from PBKDF2-HMAC-SHA-1 and the attacker doesn't need the data from the second block (12 IV bytes), so your code slows down defenders by a factor 2 without affecting attackers. generate random 16 bit salt using RNG 16 bits is very short. You should use 16 bytes or 128 bits. I suspect this is a typo in the comment, since ...

12

Since I'm more familiar with cryptography than with C#, I'm going to mainly focus on the high-level cryptographic aspects of your code (mostly, key management) rather than on coding style. You should not be using Rfc2898DeriveBytes on every call to Decrypt or Encrypt. The PBKDF2 algorithm used by Rfc2898DeriveBytes is (deliberately) very slow, so you ...

Only top voted, non community-wiki answers of a minimum length are eligible