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

30

You say you're new to Python. Well, that's ok. And a Caeser Cipher is a good place to start since it's complex enough for an absolute beginner while easy enough to understand what goes on, why and when. One thing to keep in mind is that Python comes batteries included. This means a lot of what you want to accomplish is already written, one way or another. ...

27

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

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

23

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]]; ...

23

Here are a number of things you could do to improve the code. Separate interface from implementation The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. The reason is that you might have multiple source files including the .h file ...

18

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

18

I hate to provide an "answer only" code review, but I'd like to expand upon Mast's "Python comes batteries included" point: Python comes with a str.translate, function which will do a letter-for-letter substitution in a string: >>> help(str.translate) Help on method_descriptor: translate(self, table, /) Replace each character in the string ...

18

First, congratulations! Being able to produce something usable with another person is an incredibly important skill, and especially in programming. Please don't worry about the amount of suggestions below – I'm treating this like any other Python review. Specific suggestions Module documentation, like the lines at the top of your file, is usually written ...

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

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

15

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

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

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

14

Your main is a classic arrow anti-pattern. You can use guard clauses to make the code flat and easier to understand. Bare excepts are normally not a good idea. Why would you want to print the usage if the code works fine but the user passes a keyboard interrupt? You may want to use except Exception:. I would prefer if mask and unmask were wrapped up in 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

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 # sign, ...

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

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

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