Jacques1

You can't do that, because then an attacker simply has to obtain the plaintext string from the database and hash it to get the same token you sent to the user (which gives full access to the account). The hash in your scenario is actually entirely useless. It's a plaintext system. You also can't use the hash to look up the original token. So you'd have to hash every single token in your database every time a user comes along, because that's the only way to check if there's a match. Long story short, it doesn't work. The token is the secret which must be protected, and the hash is the less critical residue you keep on your server for validation. Just like with normal passwords.

You need to output the result of bin2hex(). As the name already says, this function converts a binary string into a hexadecimal representation. Now it's your job to print this on the screen. Also note that the output in your browser is not an exact representation of the actual text. For example, all whitespace (tabs, spaces, newlines etc.) is “folded” into a single space character. If you want to know what the actual text looks like, you need to look at the HTML source code. Last but not least, do you understand the concept of character encodings? You generally can't just take text from another site and use it straight in your own application. You have to determine the source encoding, compare it with your own encoding and, if necessary, transcode the data.

The OWASP recommends security questions as additional protection on top of the classical send-secret-token-by-mail approach. It's not a replacement for that. So the advice isn't fundamentally wrong. But like I already said in your other thread, security questions have a lot of issues: You need a supporter on the phone to help users in case they have trouble with the answer (they will). This supporter needs to carefully give hints and ultimately decide if the request is legitimate. Designing good security questions is difficult. If they're too obvious, anybody will know the answer, if they're not obvious enough, nobody will, not even the user. Pedagogically, this is a disaster: We've been telling people to not share personal information and not reuse secrets, now we're doing the exact opposite. We ask them for their mother's maiden name, and the answer probably works on other websites as well. It's also a usability disaster. A lot of people will rather give up their account than call a stranger and have an awkward discussion about their family, their pets or whatever. Security questions may make sense on big sites like Amazon, PayPal etc. They're also helpful for professional users who'll enter a backup passphrase instead. But for the average site and the average user, no.

Destramic, maybe you should tell us what problem you want to solve rather than ask us about the techniques you think solve the problem. What you're saying doesn't make a lot of sense.

The random tokens must not be stored in the database. They're password-equivalent, because knowing them gives you full access to the user accounts. So the tokens must be hashed just like passwords. You don't need bcrypt, though. Since random numbers are high-entropy input, a simple hash algorithm like SHA-256 is enough to make the result immune to brute-force attacks. As always, KISS (keep it short and simple): The user provides their username. You generate a cryptographically secure random token, hash it and store the hash together with the current timestamp. Then you send the token itself (not the hash) to the user by e-mail. The e-mail points the user to a password reset page where they enter the desired password. This password together with the token is sent to your server. You hash the token and check if the hash is present in your database and has neither expired (e. g. < 1 h) nor been used. If the token is valid, you reset the password and deactivate the token. All other features that have been suggested will cause trouble, so you have to carefully compare the problems with the benefits and decide if it makes sense to extend the basic functionality. For example, any kind of security questions means that there has to be a (human) supporter who helps users in case they have trouble with the answer. People will have trouble, because they'll misspell the answer, forget the answer, mix up multiple possible answers etc. Simply locking out those users is not an option on most websites, so a socially competent human needs to sit on the phone, talk to the user, carefully give hints and ultimately decide if the caller is legitimate. Big companies can provide this service, but smaller websites probably not. And of course the whole thing only makes sense in an enviroment where users are actually willing to go through this painful procedure to restore their account. SMS gateways charge money and need to be fully trusted, because they'll get all plaintext tokens. And of course SMS only makes sense on big websites and closed communities. Nobody is going to hand out their phone number to some random website on the Internet. So as great as those extra security features sound, they have specific requirements and problems. For an average web application, KISS.

No, no, no. Have you never wondered why there are strange backslashes in the user input? Wouldn't it make sense to actually fix the problem rather than work around it with nonsense functions like stripslashes()? Random backslashes are not normal. It means there's a fundamental problem with your PHP setup (like Magic Quotes) or your application (like some auto-escaper going berzerk). I strongly recommend that you take care of this. Otherwise you'll run into the same problem over and over again. You may also get into serious trouble: The backslashes are supposed to be a security feature. If you remove them at will, then you might end up with no security at all.

Try $mail->SMTPSecure = "tls" instead of $mail->SMTPSecure = "ssl".

This makes no sense. I'm not sure if it's even possible to get an exclusive lock on a directory (Google suggests otherwise), but in any case, you're missing two things: The /tmp folder is used by lots and lots of processes, not just your PHP application. Making them all wait until you've created your user directories is a terrible idea. Locks obtained through flock() are only advisory, which means other applications are free to ignore them and overwrite your folders nonetheless. So the idea doesn't even work out. Like I said, proper randomness will make all checks and while loops unnecessary. But even if you're super-paranoid and want to rule out any chance of a collision, you still wouldn't do those checks. Simply call mkdir() and see if it failed. It does not overwrite existing folders but will throw an error instead.

The first approach is nonsense. Whether or not the request was triggered by the XMLHTTPRequest interface is completely irrelevant. Will you send a different response or refuse to respond if you somehow find out that the client actually used a different technique? Will you rename the flag when there's some new client API? Why should you? It's always the same HTTP request, and it's always the same JSON data. The (supposed) origin of the request is as unimportant as the user's hair color or their sexual preferences. The point is that the client asks for a JSON representation of the resource. And that's exactly what the second approach expresses: It tells the server to deliver JSON (rather than HTML, XML or whatever). This actually makes sense. Of course both approaches technically work. You might as well use a X_I_LIKE_TURTLES header, the webserver doesn't give a damn. But if you want your application to make sense now and in the future, only the second solution is valid.

What you need to do is use a good random number generator. The rand() function is indeed poor and may produce duplicate results. But if you use the generator of your operating system, you'll get high-quality output and don't have to worry about any collisions. There are several different interfaces: If you have the Mcrypt extension, use mcrypt_create_iv() If you have the OpenSSL extension, use openssl_random_pseudo_bytes() If you have neither, you need to use the low-level API of your operating system. On Unix, there's the /dev/(u)random device. On Windows, it's a bit more complicated. 16 random bytes are enough to make the risk of collision neglectable (so no need for any checks). You then have to encode the raw bytes as hex digits or whatever you like best. For example: <?php $rand_bytes = mcrypt_create_iv(16, MCRYPT_DEV_URANDOM); $rand_hex = bin2hex($rand_bytes); echo $rand_hex; A lot of people will tell you that you should use things like mt_rand() or uniqid(). Don't. While those functions are slightly better than rand(), they're still poor compared to a proper generator like /dev/urandom.

Unless you restrict the possible classes, this is a gigantic security vulnerability. The session values are not reliable and may have been injected by the user (this shouldn't happen in a properly written application, but it does). If you let your users instantiate arbitrary classes and call their methods, you're in deep trouble. So, no, you can't just put the new operator in front of some input parameter. You need to actually check the class name before you create an instance.

No, it did not. Just because you've gotten the expected result in some cases doesn't mean that your script worked. Depending on the input, it would have either crashed or destroyed your data or even compromised your entire server. Would you use software which does this? Would you say that it works great when it occasionally deletes your files? It's great that you want to fix the security holes, but it's not enough to randomly put a bunch of function calls into your script. You need to use them correctly. Did you read the two examples in the PHP manual? Did you try them out yourself?

This is not a prepared statement. It's an SQL injection masquerading as a prepared statement. Before you write code, make sure you actually understand how the feature works. The whole point of prepared statements is that you have parameters and pass all external values to those parameters. You have no parameters at all, you just dump your variables into the query string like before.

The link contains several examples which you can copy and try out. If you have concrete questions, feel free to ask them.

Isn't this a collection of everything you should not be doing? One aspect you constantly ignore is availability. It's great that you want to prevent brute-force attacks, but this isn't the only threat. If your brute-force protection allows anybody to take down the entire site with a few requests, then it's broken. You've merely replaced one vulnerability with another one. You lock an account after 40 failed attempts and force an admin to manually unlock it? Then I can terminate your entire userbase and drive your admins insane simply by repeatedly entering wrong passwords. It's like you invite everybody for a DoS attack. It's also unclear to me why you still use IP-based bans. This is obviously a bad idea. Once your blacklist includes some big VPNs and proxies, thousands of people who haven't done anything will be unable to reach your site. Even current users may suddenly be blocked. Is your website so incredibly popular that people will accept this and give up their current address just for you? Last but not least, all checks (except the last one) are still subject to race conditions. Like I already said, you can't just do a SELECT query to get the current value and later update the counter. All requests which happen between those two queries still benefit from the old counter value. So the checks don't even work. How many attempts a user can make only depends on the processing power of your webserver.

The braces are wrong, and escaping is entirely useless if you don't quote the value. Manual escaping is actually a rather poor solution, because people constantly forget it (as you just saw) or make some mistake (as you just saw). Better use prepared statements and stop inserting values into query strings altogether.

The php.ini is big, and even the basic settings would require an entire tutorial. Sure, we could randomly pick a few directives, but that's rather useless. To fix obvious mistakes,use an ini scanner. If you want an in-depth discussion, pick a particular aspect like error handling, file uploads or whatever.

It does not show an example otherwise. There's an array of all possible error types, but there's also a comment saying that only a few of them are actually used. A blank value means that you leave the error logging to the server interface. For example, the various PHP/Apache interfaces will invoke the Apache logging mechanism which in turn writes the message to the webserver log. Whether you prefer a separate log for the PHP errors or want them in the general server log is up to you. No, it doesn't send e-mails. I don't understand the question. When PHP emits a 500 status code, then the webserver reacts to that. Typically, it will display one of those ugly Apache error messages. But you can also set up custom error pages instead. Uncaught exceptions simply trigger a fatal error which is processed like any other fatal error (the script is aborted, the error handling mechanism is invoked etc.). The only reason for catching an exception is when you want to do something special with it. For example, in some rare cases you may have a backup plan for a particular problem. Or maybe you want to display a special message for the user (like “This name is already taken.” when the UNIQUE constraint of the name field has been violated). But in general, you do not catch exceptions.

The class doesn't make a lot of sense to me. It's basically just a buggy and incomplete re-implementation of the standard PHP error system. In fact, it's seriously broken: You jump into debug mode if the Host header contains something like “local”, but this header is defined by the client. They can set it to anything they want. Of course “local” won't be a valid virtual host, but many servers accept the request nonetheless and will select some default host. Those localhost detections are generally very problematic, because they may not work in all server setups. For example, if there's a reverse proxy in front of Apache running on the same machine, then the script may think that all requests are local. That's obviously a huge risk. A much better solution is explicitly activate the debug mode through a configuration file (use a constant, for example). You only catch a small subset of all possible errors. Obviously you can't handle errors which happen before the script is executed (syntax errors, startup errors). But even E_ERROR and E_STRICT aren't covered by custom error handlers (see the manual). That means your users will either see a white screen or the raw PHP error message. You don't set an appropriate HTTP status code, which means it's always “200 OK”. That's obviously a lie and a problem for everybody who relies on the code. I strongly recommend that you learn how the standard error system works before you write your own implementation. It's much more powerful than you seem to think, and it's definitely much better than most custom error handlers: The default error handler actually covers all errors, including syntax issues. PHP already takes care of logging. Simply turn on log_errors and set error_log appropriately in the php.ini. No need to manually handle SQL errors. Both PDO and MySQLi automatically throw errors or exceptions if you tell them to. PHP also sets a 500 status code if there's a fatal error and no prior output. All modern webservers support custom error pages, so no need to do that with PHP. There may be some special scenarios where a custom error handler is in fact necessary. But I don't see that in your case. If you do need your own handler, this will be much more complicated than calling set_error_handler().

If product A and product B have a lot in common, it might make sense to create a subclass of product for them. For example: Product is the root class, below that are FooProduct (which in turn is the parent of ProductA and ProductB) and BarProduct (which is the parent of C, …, F). Another approach is to use traits. Unlike inheritance, this does not create a strict hiarchical structure. A trait is basically a method container which can be mixed into arbitrary classes. So ProductA and ProductB would have Product as their parent, but they'd get their model code method from a trait.

Speaking of security: Your code has none. You allow SQL injection. Never heard of Little Bobby Tables? You allow HTML/JavaScript injection. Never heard of escaping? Where do you check if the current user actually has permissions to ban? Or can everbody do that? Changing data in a normal GET request is fundamentally wrong and a very, very bad idea. For example, if I put a picture into this forum with the URL https://www.yoursite.com/banUser.php?user=123, then you'll automatically ban user 123 merely by visiting phpfreaks. Maybe I could even make you ban yourself. You show all your internal PHP errors on the website. This is great info for attackers but very irritating for legitimate users. Unless you're writing a demo application to teach people how to “hack” a website, you're doing it wrong. Has it never occured to you that you need to protect your application? Not every user of the Internet plays nice, you know?

You don't necessarily need inheritance, but what you desparately need is polymorphism. Stuffing all methods of all product variants into a single class and using gigantic if statements to pick the right method whenever you want to do something with a product is an awful approach. It obviously leads to code clutter par excellence, and if you want to add a new variant, you have to go through your entire project and adjust all if statements – hoping that you won't overlook any. This is not how programming works. It's not just bad OOP code. It's bad code in general and shows a lack of abstraction. Think about it: Does the user of a product really need to know every single variant with its specific computation method? Why not leave the computation to the product? The user just asks for the result, and then the current variant (whatever it is) will take the necessary steps. If you want to drive a car, you don't need to know every single car model and how it works internally. You just turn the ignition and leave the rest to the car. It's the same thing with code: As a user, you want an interface for a product. This interface provides various product funitionalities like computation, getting the model code etc. How exactly those functionalities are implemented is entirely up to the variant. The user doesn't care. Like I said above, this doesn't necessarily lead to inheritance. If the products don't share anything except the API, then you want an interface: <?php interface Product { public function compute(); public function getModelCode(); } class ProductA implements Product { public function compute() { return 1 + 1; } public function getModelCode() { return 'AAA'; } } class ProductB implements Product { public function compute() { return 2 + 2; } public function getModelCode() { return 'BBB'; } } Now the user of a product only has to call the API functions without having to distinguish between the different variants: $model_code = $product->getModelCode(); Which getModelCode() method of which class this is doesn't matter. We just want the model code, and the variant knows how to calculate it.

If you need a log-in check to feel secure, keep it super-simple. It's not worth investing any time. For example, make a per-acccount limit and a global limit (based on experience). If the per-account limit is exceeded, you display a CAPTCHA for this account; if the global limit is exceeded, you display a CAPTCHA for every account. You need an atomic counter to correctly count the log-in attempts.

Note that you cannot rely on HTML-based validation at all. The user may choose to ignore it, or the browser may not support it, or the user may not use a browser in the first place. So if you use this, be aware that the data getting sent to your server is still entirely unrestricted. If you ask for 3 digits, you may get 100 Chinese symbols instead. If that's a problem, you need server-side validation or a combination of client-side and server side validation.

Using the IP address for anything but informational purposes is silly, because there's only a very loose connection between people and IP addresses. A single person may have a large pool of different addresses (for legitimate or malicious purposes), and a single IP address may be shared by hundreds or thousands of users. There are big networks behind a single router, there are proxies, there are VPNs, there's Tor, there are botnets. An IP address really doesn't mean anything. If you think it does, you're likely to punish legitimate users while helping attackers. Lockouts of any kind also lead to a major denial-of-service vulnerability: You basically allow anybody to lock out your entire userbase simply by sending a bunch of wrong passwords. A CAPTCHA is less devastating, but of course it's also much less effective. Even good CAPTCHAs can be solved very quickly if only you have enough people who do it for you (knowingly or unknowingly). In addition to that, log-in limits create a false sense of security, because they make the password seem stronger than it actually is: Your articifical delays only work against online attacks. Once the attacker has obtained the raw hashes, they get the full speed of their hardware. And that's when a seemingly OK password may turn out to be extremely weak. And if that wasn't bad enough, it's also very, very difficult to properly implement a log-in limit. No, it's not just a bunch of SELECTs and UPDATEs: You have to worry about race conditions. If there's a gap between checking the current log-in counter and incrementing it, then every request between those two actions still gets the old counter value. So an attacker may be able to circumvent your limit and make hundreds or thousands of attempts simply by sending them in quick succession. You need to take care of different attack styles: The attacker may try many passwords on few accounts, or they try few passwords on many accounts. You'll basically need complex heuristics to recognize any kind of unusual behaviour. It's not enough to just look at each individual account. Those heuristics also need to be damn good. If the detection is too sensitive, you'll piss off legitimate users and introduce the risk of DoS attacks. If it's not sensitive enough, it doesn't provide enough protection. So you'll need a lot of time and knowledge to get this right. And even then the whole approach is dubious at best. I have a much better solution: good passwords. Any decent password hashed with bcrypt is more or less immune against brute-force attacks. So rather than desparately trying to make weak passwords survive a bit longer, you should help your users choose strong passwords. Tell them about the problem, point them to modern password managers (KeePass etc.), explain the concept of passphrases, maybe implement a password meter. Then you don't need any of those half-working log-in gimmicks. I know that log-in checks are extremely popular, and it's the first thing that comes to our mind when we're worried about brute force attacks. But popular doesn't automatically mean good.