People who code: we want your input. Take the Survey
29

Alright, I'll start off by answering the questions you've asked, and depending if it all get's covered, I might append more at the end. We'll see, I have yet to actually read your questions! So, let's grab the first one... Will this design be handy when code get more and more complicated? More POST requests etc. No. Right now, I see no way of expanding ...


27

Your script is vulnerable to a header-splitting attack. Due to the poor design of PHP's mail() function, it is actually quite easy to introduce that kind of security hole. In summary, if… any part of the mail headers consists of user-supplied input, and you didn't make any effort to prohibit newlines or escape that input, then you will have a program ...


25

First of all, what you have is an encryption function, not a hashing function (encryption can be reversed by knowing the algorithm and optionally a password, hashing is one way). For passwords, you always* want to hash, not encrypt (because if an attacker gets your source code, they can easily decrypt the passwords). You are using a substitution cipher, ...


22

For starters, indent your code consistently — the standard in Ruby is two spaces. That includes indenting the contents of your begin-rescue-end blocks. Normally, I don't like to make such a huge fuss about indentation, but in this case I think it's highly important, because: Your program has a highly unusual outline (infinite loops and a function ...


22

Block syntax This: loop { ... } Causes a syntax error in MRI 2.1. This would fix the syntax error: loop { ... } However, the use of {...} is normally reserved for single-line blocks. Prefer: loop do .. end Methods Use many more methods. It should be possible to figure out what the script does, in broad strokes, by looking only at its main ...


20

Do not store the connection information in a file unencrypted Assuming that you are connecting to a database somewhere on the internet... If you plan on distributing this application, do not store the connection information unencrypted in a file on the user's device. In fact, preferably don't store the database connection information at all. Do not let your ...


19

Your mailer is vulnerable to a header-splitting attack! (Also known as a CRLF-injection attack.) Basically, if one of the fields contains a CRLF, the attacker can insert any arbitrary header into the generated message. For example, if the email parameter is victim1@example.com%0d%0aCc:%20victim2@example.net … then you will compose a message with headers ...


19

I'm not a php expert, but from reading up on the documentation it appears that it may be possible to insert additional from/to addresses using the "Additional Headers" field on the mail() function. Since you're using a regular string concatenation to insert your POST variable into your additional headers field, that is very well possibly the vector for ...


19

By making it more Pythonic for one: string.letters = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()' let1 = random.choice(string.letters) let2 = random.choice(string.letters) let3 = random.choice(string.letters) let4 = random.choice(string.letters) let5 = random.choice(string.letters) let6 = random.choice(string.letters) let7 = random....


19

Some general tips: The runner should use argparse to parse arguments. It most definitely should not hardcode passwords. (object) is redundant in Python 3 class definitions. I'd recommend running any Python code through Black, flake8 and mypy with a strict configuration like this one: [flake8] doctests = true exclude = .git max-complexity = 5 max-line-...


18

I see two more security issues with this code. Executing a string rather than an array of command and parameters When building a command line programmatically, it is dangerous to build it as a string, especially when any of the parameters comes from user input. When calling Kernel#system with a string, a shell interprets the command line; when calling it ...


18

Is it safe? Maybe. Is it the right way to do it? No. Even if it were safe, there’s a big problem with the SQL, and that’s that you’re inserting the dots after escaping, rather than before. I notice that for the input ', it will be escaped to \', and then your dot-insertion turns it into \.'.. Before, the \ was escaping the ', but now it isn’t. Now it’...


18

There's a common credo believed when evaluating the security of software: If it's homemade, it's unlikely secure. Sec.SE has a Q&A about homemade algorithms, which is somewhat germane to your circumstance. I suggest you look into preparing your queries, as that would be your best action to take in this situation. It's essentially what you're trying to ...


18

var You should use them with max_length and salt too. If the type is obvious from the right hand side of the assignment you should use var. From here Under what circumstances is it necessary for a variable's type to be clearly understood when reading the code? Only when the mechanism of the code -- the "how it works" -- is more important to the ...


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

Consider starting the child process in a sandbox. /usr/bin/perl is likely safe from a malicious user since /usr/bin is typically locked down by the root user(s). But /home/demetri/bin/awklikeperl.pl could be replaced if the owner is not careful with permissions. Putting the process in a sandbox will not only protect the rest of your system from attack, ...


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

There are no security vulnerabilities per se present, as far as I can see (however, 200_success did find one rather important one). But if you're security-conscious, you would probably want to practice defensive programming, which implies careful input validation and eschewing potentially buggy constructs. One of the potentially buggy constructs in many ...


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

I understand it is best not to use functions such as SHA1 or MD5. That is true. But sha256 is not that much better. Problem: simple sha256 There are basically two problems with your approach: you don't use a user-based salt, only a site-based salt (also called a pepper). you only use simple sha256, which is way too fast. These are serious issues. ...


16

Enter password to test: premaintenance disdainful hayloft seer too long your password strength is medium Enter password to test: NXJCWGGDVQZO your password strength is weak Enter password to test: Password1 strong Your knowledge of password strength is: weak. Explanation Password strength is normally measured in "bits of entropy" — the idea being that ...


15

This is not secure at all. Apart from the GET issue (use POST) and the hashing issue (use bcrypt) already mentioned, and the fact that == is not timing safe (use hash_equals), an attacker can log in without knowing the password like this: login.php?userpass[]=1 The reason for this is that strcmp("STRING", [ARRAY]) returns NULL, and NULL == 0 is true. You ...


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

Developement oop & design-patterns tags you've used are already saying what needs to be done here. At this moment it's a structural code (with some class syntax) and it will be very difficult to expand. Even though it doesn't look like a big project yet and you don't need deep object structures/layers here, I think you should learn some oop anyway. ...


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

Design So the point of this program is to check a password, right? There are many problems with your approach to verifying a password. To list a few: Password could be brute-forced in a matter of milliseconds (on a slow machine) due to small length Anyone could reverse-engineer your code by de-compiling it and find out how to enter a valid password, even ...


13

No it is not safe: Classic off-by-one error. You reserve space for 3 additional entries with command[argc+3] but you add 4. argv[i+1] and command[i+4] will be out of bounds on the last iteration (last valid index is argv[argc-1] and command[argc+2] respectively) execv expects a NULL terminated array of NULL terminated strings. So the last entry in commands ...


13

Right, I'll be adding to this answer later on, but for a kick-off, here's a few quick remarks/considerations: On the hidden input fields As Martijn said in his comment: most bots will probably parse the DOM, and filter out those fields that are marked as required (looking for labels with a * text node in them), and they'll simply ignore hidden fields. You'...


13

<?PHP Normally it's all lower case, just saying. //Turn off error reporting (Not Necessary) error_reporting(E_ALL ^ E_NOTICE); This is one prime example of confusing and not helpful commenting. If this call is not necessary, why is it there at all? if (!@mysql_connect('localhost', 'user', '') or !@mysql_select_db('comments')) { Stop using the mysql_* ...


13

Preamble As a matter of security, I can safely say that you would fail a professional security audit in seconds. You should be including the hashing library as an external component. You also need to forget everything you know about querying databases and start again. Also, look at Model-View-Controller. The fact you have a templating engine and separate ...


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