Jacques1

Like I said, in a password, every character is significant. You can't just delete whitespace characters. For example, I generate my passwords randomly, so they may very well include whitespace. If you delete those characters, you actually truncate my password and make I weaker. I understand where you're coming from. But in case of passwords check, fuzziness is a terrible bad idea. You want an exact match, not “The password is kinda-sorta correct, come in”. What? Why on earth should users not copy and paste their passwords? That's exactly how password managers work. I wonder where this myth comes from. Some websites do in fact prevent pasting the password into the form field, and that's a major PITA for security-oriented users.

Well, that's not really a good reason for using a function. Sure, “sanitzing a string” sounds great, but does that even mean? It's such an incredibly vague term that it could mean almost anything. When you write code, it's very important that you know exactly what you want to achieve. For example: “I want to help my users recognize errors when they fill out the form.” or “I want to prevent SQL injections.” Then come up with a concrete approach and finally choose the function which does exactly that. In case of the length check, you need mb_strlen(). The rest of your checks may be useless (depending on what you want to achieve). The specification of e-mail addresses wasn't written with SQL injections in mind, so it would be pure luck if e-mail addresses were immune to injections. Unfortunately, they aren't: "'OR(1)#"@foo.com This is actually a valid address which is accepted by FILTER_VALIDATE_EMAIL. But at the same time it will lead to an SQL injection if inserted into an SQL string: SELECT is_admin FROM users WHERE email_address = '$email' This becomes SELECT is_admin FROM users WHERE email_address = '"' OR (1) # @foo.com In other words, it will neutralize the WHERE clause and return the admin status of the first user (who is usually indeed the admin) rather than the status of the current user. This clearly shows that injection attacks are not an input problem. I gave you the exact input you asked for, yet still I can attack your database system. Just because a string is valid according to the e-mail syntax rules doesn't mean that it's harmless in an SQL context. Those are two entirely different things. Yes. Or more specifically: Input filtering has nothing to do with security at all. It you want security, you need to make sure that the input is interpreted correctly in the specific target context (SQL, HTML or whatever). That's why we use prepared statements for SQL queries and HTML-escaping for HTML documents: They guarantuee that the input is treated as data rather than code. Web security is no rocket science. The problem is that there's too much bad advice out there, and PHP itself is full of bullshit. So don't go with ideas or functions just because they sound good. Think about the problem: What do you want to achieve? How do you achieve it? Anyway, it's great that you care about security and want to learn more about it. This is very rare in the PHP world.

While hansford is right about the deprecation of the old mysql_* functions and the importance of prepared statements, the suggested code has several issues: Hard-coding the PHP error configuration in a script is a bad idea. Sure, during development, it makes sense to display the errors right on the screen. But if you forget to remove this code before the site goes into production, you have a problem, because everybody will see the messages. This may leak critical information to attackers, and of course it's very unprofessional and irritating for legitimate users. Dynamic error settings also have no effect on errors which happen before the script runs (syntax errors, startup errors etc.). A much better solution is to put the error configuration into the global php.ini or a site-specific user.ini or .htaccess file. The same applies to MySQL error messages: Do not just print them on the screen. They're meant for developers and administrators, not the end user. If you want to generate internal error messages, use trigger_error() or throw in exception. In the case of MySQLi, you don't have to manually trigger errors at all, because the MySQLi driver can do that for you. Again, it's very important to understand that your database issues are none of the user's business. They don't administer your server, so all they need to know is that there's some technical issue. Do not trim passwords. In a password, every character counts, so don't change it in any way. If you do an isset() check of input parameters, you also need an else part. Simply doing nothing is the worst possible solution, because both the user and the developer will wonder what the hell is going on. The code connects to the MySQL database system with the user-provided credentials. I'm fairly sure this is not what the OP wants. The credentials surely belong to an application-level user account.

Input filtering is overrated. I understand that people feel to need to “clean up” the incoming data, but this is a rather vague goal. When you look at the hard facts, you'll see that input filtering rarely does what you expect from it and sometimes even causes problems. You want security? Then filtering is not the answer. For example, a perfectly valid e-mail address can still be used for SQL injection or cross-site scripting attacks. And how do you “filter” a password? Passwords are by definition unrestricted, because any kind of limitation would weaken them (well, we do have to limit the lenght sometimes). The thing is this: Injection attacks are an interpretation problem, not an input problem. There's nothing wrong with a user entering a word like “SELECT” or a symbol like “<”. It's all just text. If your application does strange things with this text, then that's the problem. Protection always depends on the specific context. For example, preventing SQL-based attacks is entirely different from preventing HTML-based attacks, because SQL and HTML are two entirely different languages. There is no magical one-size-fits-all filter. Using prepared statements to prevent SQL injections is a much better idea, because it actually makes sure the that data is interpreted correctly, and it's meant for a specific context (SQL). To prevent attacks against your HTML documents, you'll mostly need HTML-escaping. Of course you may use filters to validate the incoming data. But this is not a security feature, and you need to be aware of several problems: Formal validation doesn't prevent users from lying. For example, “bill@microsoft.com” is a perfectly valid e-mail address and probably even exists, but it's obviously not mine. Overzealous validation can frustrate legitimate users. For example, human names and addresses vary greatly accross different countries and cultures. If you expect every name to only contain latin characters, then a large part of the world population won't be able to use your website. If you silently “correct” the user input like in your code above, then users won't get what they meant. This again leads to frustration and can make them leave your site forever. When I enter a username, I want that exact name. If it's somehow not allowed on your site, I want you to tell me so that I can choose a different one. But I certainly don't want you to manipulate my input.

No, no, no. First of all, strlen() usually counts the number of bytes, which is probably not what you want. If you use a modern multi-byte character encoding like UTF-8 (which you should), then you'll get “weird” results. For example, the function will tell you that the length of “Jörg” is 5, because UTF-8 happens to encode the “ö” umlaut with 2 bytes. The implementation of strlen() also depends on the PHP environment, which means you may get different results on different machines. Some enviroments count bytes, some count characters of a particular encoding. Good luck debugging this. Long story short: Do not use this function. If you want to count characters, you need to take the encoding into account. Is it UTF-8? ISO 8859-1? ASCII? Something else? PHP needs to know. The mbstring extension provides an encoding-aware length function which correctly counts the number of characters in a string: <?php $name = 'Jörg'; $name_length = mb_strlen($name, 'UTF-8'); // this says 4 as expected var_dump($name_length); If you actually do want to count bytes, then you still need the mbstring extension: <?php $name = 'Jörg'; $name_byte_length = mb_strlen($name, '8bit'); // 8bit means: count the raw bytes // this says 5 as expected var_dump($name_byte_length); Besides that, FILTER_SANITIZE_STRING is a horribly misnamed and utterly useless fossile from the early days of PHP when the developers had no idea what they're doing. It randomly mangles the input in a desparate attempt to somehow make it secure for HTML contexts. For example, the name Peter's mother, I <3 cookies becomes Peter's mother, I I'm fairly sure this is not what you want. You only picked the filter_input() function because the name sounded good, right? It's great that you're worried about input filtering. But this requires careful analysis of the specific goal and context. There's no magical filter function which somehow makes everything nice and secure (even though the PHP devs like to promise that). For now, I suggest you forget about this filter_input() stuff. It's wrong and useless except for a few special cases.

Escaping makes sure that the browser interprets the variable contents as literal text rather than HTML markup. For example, the character “<” is replaced with the HTML entity “<” which represents a literal less-than sign. If you don't escape the values, than you allow anybody to manipulate the HTML document. They could insert malicious JavaScript code, redirect the form data to another website, deface your page and whatnot.

There's a lot wrong with this, and the slashes are really your least problem. First of all, you can't just drop user input into your HTML document. Never heard of cross-site scripting? All dynamic values need to be escaped before they can be inserted. Secondly, HTML attributes need to be quoted. If you don't quote them, then you're almost asking for trouble, because it's more or less unclear where the value will end. In your case, the slash of your HTML tag is also assigned to the form value. Fix the escaping, fix the quotes, and the problem will go away.

This sounds super-weird and super-dangerous. If the query table gets compromised, you've just turned your entire application into a kind of phpmyadmin for hackers. What are trying to achieve? I mean the underlying problem, not what you think solves it. Do you want to build a database administration tool? Or do you simply want users to change data?

None of the above suggestions will work out, and the current code is horriby insecure with all those SQL injection vulnerabilities, plaintext passwords and whatnot. Right now, really all you have is a lot of mumbo-jumbo: The code looks like it's protecting your application, but anybody with a basic understanding of the mechanics can break it. That's useless. It's actually downright harmful, because it creates a false sense of security. Your users will think that their accounts are protected, but in reality you've (inadvertently) published their passwords on the Internet. I understand that you're new to PHP, and that's perfectly fine. But then you shouldn't make security promises you cannot keep. Be honest. Why not make a public website which is open to everybody? This has a lot of advantages: No complicated registration procedure, no stupid passwords. Less code, more time for the actual content of the site. Instead of spending hours on user management details, you can simply make a nice website. Think about it. The Internet would be a better place if we didn't have to enter a frigging password on every frigging website. If you absolutely must have passwords, then I strongly recommend that you postpone the project until you have a basic understanding of web programming. No, you can't just insert user input into query strings (never heard of Little Bobby Tables?). No, you can't store your user passwords as plaintext, because somebody might break into the database and steal them (that's actually very easy in your case). No, you can't store the log-in attempts in a cookie, because cookies are controlled by the user. They can be deleted or manipulated with just a few clicks in the browser. Try it yourself. For PHP basics, I recommend the PHP manual. If you look up your mysql_* functions, for example, you'll see a big red warning sign which says that those functions are long obsolete and have been replaced with the PDO library. For security basics, I recommend “Survive The Deep End” by Pádraic Brady (he also has an excellent blog). Checking the number of log-in attempts is fairly difficult, and even experienced programmers constantly get it wrong. First of all, yes, you have to store the attempts per user, not per cookie or whatever. Secondly, you have to increment and check the counter in a single operation. Otherwise, there's a small time gap between the check and the increment where users can make an arbitrary number of parallel requests. For example, let's say there's a delay of 1 second between your COUNT(*) query and the increment operation. If I make a new attempt every millisecond, then your server will accept all 1000 requests, because the counter still has the old value. Only after this it is finally incremented. This isn't just a theoretical threat. A stock browser and a small piece of JavaScript code are enough to send a large amount of parallel requests to a website (that's what browsers are for). So a simple SELECT followed by an UPDATE doesn't work. You need a single UPDATE query which also fetches the new counter value. As a basic example: The table CREATE TABLE users ( user_id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY, username VARCHAR(50) NOT NULL, password CHAR(60) NOT NULL COMMENT 'bcrypt', login_attempts INT UNSIGNED NOT NULL DEFAULT 0, last_login_attempt DATETIME ) ENGINE InnoDB, CHARSET utf8mb4 ; database.php <?php define('DB_HOST', '...'); define('DB_NAME', '...'); define('DB_USER', '...'); define('DB_PASSWORD', '...'); define('DB_ENCODING', 'utf8mb4'); function get_database_connection() { $dsn = 'mysql:host=' . DB_HOST . ';dbname=' . DB_NAME . ';charset=' . DB_ENCODING . ''; $database = new PDO($dsn, DB_USER, DB_PASSWORD, array( PDO::ATTR_EMULATE_PREPARES => false, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, )); return $database; } login.php <?php require_once __DIR__ . '/database.php'; $database = get_database_connection(); /* * Increment the log-in attempts counter and fetch the new value with a single atomic operation * to prevent race conditions. * * If the last log-in attempt was more than 1 hour ago, the counter is reset. */ $loginAttemptsCheck = $database->prepare(' UPDATE users SET login_attempts = LAST_INSERT_ID(IF(NOW() > last_login_attempt + INTERVAL 1 MINUTE, 1, login_attempts + 1)), last_login_attempt = NOW() WHERE username = :username '); $loginAttemptsCheck->execute(array( 'username' => $_POST['username'], )); $loginAttempts = $database->lastInsertID(); if ($loginAttempts <= 3) { // check the password; if the log-in was successful, reset the counter to 0 } else { echo 'The account is temporarily locked because of more than three failed log-in attemps within one hour.'; } Note that a hard limit allows anybody to lock out your users simply by entering a wrong password again and again. A slightly less user-hostile approach would be a CAPTCHA: If the limit is exceeded, the user can unlock the account by solving some kind of puzzle. This also slows down attackers but doesn't lock out legtimate users entirely. Sounds complicated? Like I said, proper user management is a pain in the ass for both users and developers. Don't do it unless you absolutely have to.

Well, mostly the file extension. Many webservers will execute any file with a “.php” extension, sometimes even if the extension occurs within the filename (like “mypicture.php.jpg”). Another case is a local file inclusion attack: If the user is able to inject a custom path into a dynamic include statement, then the included file will also be executed as a script – regardless of the file extension or the MIME type. For example, PHP will happily execute code embedded into a comment segment of a JPEG image. The protection against that is obvious: Be as restrictive as possible when you set up script execution in the webserver configuration. By default, no files should be executed. Then you create a whitelist of legitimate scripts or script folders. Do not allow arbitrary paths in dynamic include statements. Use a whitelist of valid paths. Restrict script inclusion with the open_basedir setting. If you store the uploaded files outside of the document root and serve them through a PHP script, then of course you could theoretically store the files with no extension at all. However, the extension is important information for humans. Administrators or supporters may have to access the files directly once in a while, and in that case it's useful to know the type without having to invoke MIME detection or look up the meta information in the database. Escaping is fragile and should only be used as the last resort. For example, escapeshellarg() is vulnerable to null byte injections, which can allow users to truncate the shell arguments: <?php header('Content-Type: text/html;charset=utf8'); $_POST['filename'] = "myfile.php\0"; // note the null byte $intended_filename = $_POST['filename'] . '.jpg'; $arg = 'This file should have a .jpg file extension, but the user prevented that: ' . escapeshellarg($intended_filename); echo htmlspecialchars($arg, ENT_QUOTES, 'UTF-8'); So avoid dynamic shell commands whenever possible and use PHP extensions instead. Have you tried the ImageMagick extension, for example?

*lol* So when the client explicitly declares a file type, that's not trustworthy, but when the same client puts a bunch of magic bytes into the file and makes the server guess the type, then we can suddenly rely on it? That doesn't make a terrible lot of sense. It also seems we're going off on a tangent here. NotionCommotion asked about security, not data validation for well-meaning users. Using MIME type detection as a security feature is fundamentally wrong, because malicious file uploads are not an input problem. A perfectly valid image can blow up the server, and a super-shady PHP script can be a harmless piece of source code. The question is how the file is interpreted. And, no, you do not want to rely on the OS or webserver to figure it out for you. You tell the server and client how to interpret the data and prevent any guesswork from the beginning. If we leave aside security and get into data validation, sure, you can check the MIME type. Whether you use the declaration of the client or the server's guess is irrelevant. Either way, the validation won't be very useful. The average user has Windows, and Windows is all about file extensions. Even if the client somehow ends up with a wrong extension on their PC, they'll quickly notice, because they won't be able to open the file properly. So if you get a “.jpg” file, it's safe to assume that the user actually means a JPEG image and not a Word document.

Checking the MIME type doesn't help at all. For example, even a perfectly valid JPEG image can contain malicious code. What matters is how the files are treated. There are three problems: Your server may misinterpret the uploaded files and execute embedded code. The browsers of your users may erroneously execute the files. Some file types are inherently dangerous (HTML, SVG, Flash etc.) Unfortunately, there's no definite solution. In fact, some webservers (like Apache) and some browsers (like Internet Explorer) will actively work against you and sabotage your efforts with all kinds of weird “features”. The only way to deal with that is to use defense in depth and protect the application with multiple security layers: Never accept the user-provided filename. Generate a random name and pick an extension from a pre-defined whilelist. Avoid direct downloads whenever possible. Instead, serve the files through a script. If this is not an option, make sure that the webserver will not any execute any scripts in the upload folder (and make sure this configuration cannot be overriden with a custom .htaccess file). Serve the files via a separate (sub)domain to isolate them from the actual application. Tell the client to not execute scripts. Use Content Security Policy, for example. Not all browsers support this, but for the ones that do, it's a very powerful feature. Of course processing the files with LibreOffice, Imagemagick and whatnot creates additional risks, because attackers may be able to exploit specific bugs in those applications. But if you need this feature, you'll have to live with that. I hope you use proper PHP extension for the processing? If you do it on the shell, you also have to worry about shell injections.

You've asked pretty much the same question a couple of months ago, and the answer is still the same: You use an SQL join. It might actually be a good idea to spend some time on the basics of PHP and SQL. That code is ... weird. I see PHPHTML spaghetti code, ancient functions (mysql_query() etc.), odd syntax (like the while statement), useless code (like stuffing all contents of the row into separate variables) and of course bad practices like dumping raw variables into SQL queries. This is not how PHP works.

I'd be very careful with random “tutorials” you found somewhere on the Internet. A lot of those people have absolutely no idea what they're doing and will teach you dangerous nonsense.which can actually compromise your entire server. To be honest, I think the whole concept of programming-by-numbers is a bad idea. You don't learn a lot, and there's a huge risk of adopting all those bad practices that have been around for years. My personal strategy for learning is this: I learn the basics (like accessing an SQL database with PHP). I use the manual a lot, because that's where you get first-hand information. I read up on best practices (like properly hashing a password). I think about what I've learned: Do I agree or disagree? Is there maybe a better way? I write my own code. I check what other people have to say. In my experience, this works very well. It leads to a high code quality at an early stage and an actual understanding of the concepts behind PHP.

With all due respect: The entire code is a mess. You have PHPHTML spaghetti code from hell, cryptic variable names, arcane data structures, variables coming from nowhere, cross-site scripting vulnerabilities etc. Yes, you could write a workaround for this one bug and hope that it will buy you some time until the next strange thing happens. But I'd rather take care of the underlying issue and clean up the mess. How about a sane data structure which uses meaningful indexes instead of random numbers? What's the caching supposed to do, by the way? How about proper code which passes values in a predictable way, follows best practices and can actually be read by humans? How about untangling the PHP code and the HTML markup? Ideally, this will not only fix the current bug but also prevent future bugs from happening.

There's obviously a misunderstanding. Validation is not a security feature, let alone a replacement for escaping. At best, it's a way to reject nonsense data and make your users aware of input errors. I'd actually argue that validation is grossly overrated. I understand that people feel the need to “clean up” the incoming data in the hopes to make their application secure. But this is fundamentally wrong. Injection attacks are not an input problem. They're caused by incorrect handling of the input. For example, it's perfectly fine if the user input happens to include SQL keywords like “delete” or “drop”. Those are normal English words. It's your fault if you take those words and send them to the database server where they cause trouble. So forget about this filter stuff and learn to use prepared statements. If you insist on limiting the username to 30 characters, you can of course do that. But don't randomly apply filters. This will do more harm than good. For example, you must not restrict the password, because this makes it easier for an attacker to find it.

You need to echo the actual query generated by your code and check it. Running some other query you made up doesn't help you with the problem. Obviously you still haven't fixed the bug pointed out by sasa and maxxd. You also can't just drop GET parameters into your query string. Never heard of escaping?

Guys, please. I understand that you all just want to help, but the replies are getting worse and worse. Do you honestly suggest that the OP should drop raw POST values into the query string and use SHA-1 to hash passwords? Have you never heard of things like SQL injections and brute-force attacks?

Before you jump to weird optimization attempts, tell us something about the context and the actual problem. Why on earth do you have an array with 5,000 prices? Is this hard-coded into your application? Why don't you use a database? And do you actually have performance issues? Have you measured them and identified the concrete cause? Or is this all just speculation?

I fear you haven't understood a single word of the previous posts.

You may add an (inaccurate) Ajax pre-check as an extra usability feature, but the actual uniqueness check must be done by a database constraint. It cannot be implemented with normal queries.

Any reason why you don't use existing analytics tools?

The point is that a carefully chosen subdomain actually tells us something about the context of the site. This is very important information. Semantically, there's a big difference between, say, “payment.mysite.com” and “payment.users.mysite.com”. Of course not everybody will notice or understand this. But a lot of people do. The subdomain also prevents users from unintentionally reserving critical names. Even if everybody is nice and friendly, people may still choose a name which you actually want yourself. What now? With an extra subdomain, there's no such problem. Just compare this with a file upload: Do you let anybody place arbitrarily named files directly into your document root? Probably not. I'm sure you have an extra folder just for user uploads. Does this solve all security problems? No, but it's still a very important precaution. The subdomain is one security feature amongst others: Make a blacklist and occasionally check the subdomains to recognize obvious abuse. Restrict the content as much as possible. Consider putting the user sites into visual “frames” to mark them as external content. None of this is perfect, but all features combined do reduce the risk of a phising attack massively.

Doing a SELECT query to check the existence of a value is a very, very poor approach and leads to race conditions: If two users ask for an unregistered name at the same time, then your script lets them both use the name. While this may sound unlikely for a low-traffic website, a malicious user might actually do this on purpose in order to trigger bugs in your application. If your code expects the usernames to be unique, who knows how it will react to two users with the same name? So, no, this is not an acceptable solution. The database needs to check the uniqueness of the username. And that's exactly what a UNIQUE constraint does. When you've added the constraint, you simply do your INSERT query. If the database complains about a duplicate value, you know that the name is already taken and tell the user about it: <?php // assign proper names to MySQL error codes define('MYSQL_ERROR_DUPLICATE_ENTRY', 1062); // test values $_POST['name'] = 'foo'; $_POST['password'] = 'bar'; $database = new PDO('mysql:host=localhost;dbname=test', 'YOUR_DB_USER', 'YOUR_DB_PASSWORD', array( PDO::ATTR_EMULATE_PREPARES => false, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, )); $registrationStmt = $database->prepare(' INSERT INTO users SET name = :name, password = :password '); try { $registrationStmt->execute(array( 'name' => $_POST['name'], 'password' => $_POST['password'], )); } catch (PDOException $error) { // check if the name is already taken $errorCode = $error->errorInfo[1]; if ($errorCode == MYSQL_ERROR_DUPLICATE_ENTRY) { echo 'This name is already taken.'; } else { throw $error; } } This is absolutely reliable, plus it is shorter and more efficient (because there's no need for a second query).

The difference between “mysite.com” and “mysite.net” is rather subtle, don't you think? In fact, as a user, I'd be very confused if they delivered entirely different content. And I wouldn't expect that one is the “official” domain while the other one hosts the sites of your users. Anyway, you know my position. Whether you agree or disagree is entirely up to you.