Jacques1

Hi, counting the rows before the INSERT query (as suggested by iarp) is naive and doesn't survive concurrent requests. Let's say a particular e-mail address is ready to be voted for, and then you get 100 votes for this address at the same time. Since your check sees no rows at that point of time, all votes are accepted. But now you have 100 rows when you only wanted one. This is not just a theoretical issue. If people find your reward worthwhile, they will actively abuse this bug. So it doesn't work like this. If you want to make sure a value is unique, you must put a UNIQUE constraint on the table column. In your application, you first try to insert the row and then check if the constraint has been violated: try { $database->query(' INSERT INTO unique_test SET x = 1 '); } catch (PDOException $insert_exception) { // If the query fails, check the error code; "1062" is a violation of a UNIQUE constraint. if ($insert_exception->errorInfo[1] === 1062) { echo 'Duplicate entry!'; } else { // Otherwise, just pass the exception on. throw $insert_exception; } }

Read the error messages. I mean, really, this is standard English. You don't need a CS degree to understand what they're saying.

Hi, the code you gave us above is not the code you actually use. If you read the error messages, it tells you exactly what is wrong: You have output in line 2 of login.php which prevents the session_start() in line 13 from sending HTTP headers. There must not be any output before session_start(). No whitespace, no HTML doctype, no BOM, absolutely nothing. The PHP tag must start at the very first byte of the file.

No, not really. This is missing the point. The value must be HTML-escaped before you can insert it into your HTML document. Never insert unescaped values. If you're lucky, this will only break your markup (which is what just happened). If you have less luck, an attacker will use this bug to inject malicious JavaScript code. So always escape your input properly: <?php echo '<td><input type="text" name="r_firstname" size="35" value="' . html_escape($arrayvictim['victimFName']). '"></td></tr>'; // Put this into some global file. function html_escape($raw_input) { // If you're not using UTF-8, adjust this accordingly. return htmlspecialchars($raw_input, ENT_QUOTES, 'UTF-8'); }

Instead of discussing the script above, maybe we should rather get back to the OP's question. There are several things to take care of when dealing with sessions: It must not be possible to predict the session IDs. In other words, they have to be sufficiently random (which they're not by default). You must prevent the session ID from being stolen through JavaScript or in transit. An attacker must not be able to set the session ID of a victim (this is called session fixation). It's important to correctly terminate the session in the logout procedure. The lifetime of a session must have an absolute limit 1. Improving the session IDs By default, PHP uses a very weak random number generator to create the session IDs. They're basically derived from the server time and some other trivial factors, which makes it relatively easy to predict them. If you google for it, you'll find plenty of tools for attackers. To prevent this, it's very important that you use the random number generator of your operating system (which is much better) to mix additional randomness into the session IDs. This is done with two INI settings: session.entropy_file specifies the path to the system's random number generator. And session.entropy_length sets the number of bytes which PHP should read from this generator. On a Linux server, you would use something like this: session.entropy_file /dev/urandom session.entropy_length 16 2. Protecting the session cookie The session ID can be stolen in two ways: through malicious JavaScript code in case of a cross-site scripting vulnerability, or by reading unencrypted network traffic. To prevent the ID from being stolen through JavaScript, it's very important to set the HttpOnly flag of the session cookie. This tells the browser to not give JavaScript access to the cookie. You do this by activating the session.cookie_httponly setting. It's also important to use HTTPS so that the network traffic between the client and the server cannot be read by attackers. You can then tell the browser to only transmit the session cookie over HTTPS and never over a plaintext HTTP connection. You do this with the session.cookie_secure setting. So a proper configuration would look like this: session.cookie_httponly on session.cookie_secure on 3. Preventing session fixation attacks If an attacker is able to set the session ID of a victim, they can simply wait until the victim has logged in and then take over the session. The first step to prevent this is by only accepting session IDs from a cookie and never from a URL parameter. You do this by activating both session.use_cookies and session.use_only_cookies: session.use_cookies on session.use_only_cookies on The second step consists of regenerating the session ID after the user has logged in. Even if an attacker has managed to find out the previous session ID, they now don't know the new one. <?php // this goes into your login script right after you've verified the password session_regenerate_id(true); 4. Terminating the session correctly A session consists of three different things: the session file on the server, the session cookie in the user's browser and the $_SESSION array in the current PHP process. All three have to be cleared in the logout procedure: <?php // clear $_SESSION array $_SESSION = array(); // destroy session file on the server session_destroy(); // ask client to delete the session cookie $session_cookie_params = session_get_cookie_params(); setcookie( session_name(), '', time() - 24 * 3600, $session_cookie_params['path'], $session_cookie_params['domain'], $session_cookie_params['secure'], $session_cookie_params['httponly'] ); 5. Setting an absolute time limit In addition to all previous steps, it's important to make sure the session ends after a certain amount of time. Note that it's not enough to simply set the lifetime of the cookie, because the client can extend this forever. You need to enforce the time limit on the server. When the user has just logged in, store the current time in the session file: <?php // this goes into the login script $_SESSION['creation_time'] = time(); Then check the time on every page before you accept the session: <?php // make the session end after 1 hour (or whatever you find appropriate) define('SESSION_MAX_LIFETIME', 3600); // check the lifetime of the session on every page if (isset($_SESSION['user_id'], $_SESSION['creation_time'])) { $session_lifetime = time() - $_SESSION['creation_time']; if ($session_lifetime_seconds <= SESSION_MAX_LIFETIME) { // good to go } else { // the session has expired; terminate it } }

No offense, Skewled, but WTF? So you blindly accept any user ID people send you in a cookie? What if I create my own cookie with the user ID of the site admin? Does that make me an admin? This is really, really wrong. Never trust the user input. This includes cookies, because anybody can create any cookie they want.

Yes, but that doesn't make it less wrong. It's a well-known fact that a lot of the people who run around writing PHP “tutorials” have absolutely no idea what they're doing. Often times, it's not even their own code. They've just copied and pasted if from somebody who copied and pasted it from somebody who copied and pasted it from somebody. That's why you see the same mistakes being made over and over again and passed from generation to generation. Like I said, this is known problem. But since there are no laws against bad code, there's not much we can do about it. All we can do is give better advice and ask people to choose their sources more carefully.

Hi, like I already said above, you cannot even store data which is related to the password in a cookie or the session. This includes hashes of the password. Knowing the hash allows an attacker to start a brute-force search for the password, so this seemingly harmless extra hash may very well reveal the full password in the end. Long story short, don't do it. The only thing you may do with the password is hash it and store the hash in the user table. SHA-512 is absoutely inappropriate for hashing passwords. Whoever told you to use it doesn't know what they're talking about. None of the general-purpose hash algorithms like MD5, SHA-1 or SHA-2 were ever meant for passwords. They're designed for digital signatures, which is a completely different goal. Calculating signatures should be very fast and work on weak hardware like smartcards, so those algorithms are heavily optimized towards speed and low hardware requirements. This allows an attacker to calculate billions of hashes per second on a stock PC. It's easy to see what that means for passwords: They won't last very long. Searching the entire space of all alphanumerical passwords up to 8 characters takes something from a few minutes to a few days, depending on the algorithm and the hardware. So this is definitely not an option. It doesn't matter which of those algorithms you choose. SHA-2 is just as bad for password hashing as MD5, even though many people run around telling everbody that MD5 is “broken” and that some SHA algorithm with a big number in the name is “strong”. This is an urban legend. It also doesn't help to salt those weak algorithms, because the attacker can easily make up for it with some more dollars or some more time. The only way to protect passwords against the massive computing power of current hardware is to use a specialized password hash algorithm. This algorithm needs three properties: It must be computationally expensive It must have a variable cost factor which can be increased over time It must generate a random salt for every password so that each hash must be attacked individually Currently, the algorithm of choice is bcrypt. It's strong, time-proven and well-supported by PHP. There are other algorithms, but they all have a drawback. If you already have PHP 5.5, you can take advantage of the new Password Hashing API which makes bcrypt very easy to use. If you don't have PHP 5.5 yet but at least 5.3.7, you can include the password_compat library which offers the same functionalities. PHP versions before 5.3.7 have a defective bcrypt implementation and cannot be used. A short example: <?php // bcrypt cannot process more than 72 bytes input, so the user should be warned if the password is too long. define('PASSWORD_MAX_LENGTH', 72); // Adjust the cost factor to your hardware; make it as high as your CPU and your users are willing to accept. define('PASSWORD_HASH_COST', 12); // Create a new hash. $test_password = 'a0WHnUH3IDE7Yl2T'; if (strlen($test_password) <= PASSWORD_MAX_LENGTH) { /* * The result will consist of exactly 60 ASCII characters and look something like this: * * $2y$14$6/F6Y7u/jGAUmrUpHZIQ9e1EVLIj0GUtLQdNUEJGthCm4d77bZTUy * * All information about the hash are included in this string: The "2y" identify the algorithm * (in this case bcrypt), the "14" is the cost factor, the next 29 characters after the "$" are the * salt, and the remaining 31 characters are the actual hash. */ $password_hash = password_hash($test_password, PASSWORD_BCRYPT, array('cost' => PASSWORD_HASH_COST)); } else { echo 'Your password is too long. The maximum length is ' . html_escape(PASSWORD_MAX_LENGTH) . ' bytes.'; } // Verify a password against an existing hash. $password_to_verify = 'a0WHnUH3IDE7Yl2T'; if (password_verify($password_to_verify, $password_hash)) { echo 'The password is correct.'; } else { echo 'The password is wrong.'; } function html_escape($raw_input) { return htmlspecialchars($raw_input, ENT_QUOTES, 'UTF-8'); } Back to your original question: Your check simply makes no sense. Your session contains the user ID, right? And then you look up the password and salt for this user ID? This proves absolutely nothing. You just hash the same user data you hashed a few minutes ago. The only way this check could fail is when the attacker doesn't know the victim's user agent. But this is an absurd assumption. First of all, the UA isn't secret at all. It can easily be found out or simply guessed. And it makes no sense to assume that an attacker who just managed to steal a very strong secret (the session ID) somehow fails to get a much weaker secret (the UA). So this check really doesn't get you anywhere. Instead, you should make sure your session IDs are strong. If you don't know how, simply create a new thread.

After the user has logged in, you start a PHP session, right? Well, that's were you get the correct student ID from: The user logs in with their password You verify the password and start a session; the session contains the user ID On the protected page, you display a welcome message for the user ID from the session

No, this is definitely not an option. As I've already said above, you must not store any password-related data in a cookie. Never. I don't know why anybody would think it's a good idea to put passwords in cookies, but this is pretty much the worst thing you can do. Cookies are simple plaintext files which reside in the user's browser for an indefinite amount of time and travel around the Internet on every request. Besides that, your class is very insecure and has several bugs. Your session IDs are simply derived from the server time, which makes it very easy for an attacker to predict them. You don't have any locking mechanism, which means concurrent requests will trample each other. And you fail to prevent the session cookie from being stolen with a JavaScript attack or in transit (you don't set the HttpOnly and Secure flag). I strongly recommend to use native PHP sessions instead of rolling your own implementation. Custom sessions are only acceptable if you know exactly what you're doing and what you have to take care of.

No, it's not fine, as I've explained above. It would be very nice if you didn't just ignore all other replies. You've even repeated his XSS vulnerability.

Hi, never ever store the password or any password-related data in the session, a cookie or anywhere other than the user table. The password is the user's most sensitive data and must be protected at all cost. The last thing you want is have it flying around in text files on the server or in the user's browser. It even sounds like you store the plaintext password in the database for your calculations. This would be a gross violation of basic security principles. Besides that, I have absolutely no idea what this check is supposed to do. What does this prove? What does it protect against? A login system works like this: Upon registration, the user submits a username and their password. You hash the password with a specialized algorithm like bcrypt (not MD5, SHA or whatever) and store it together with the username. When the user logs in, they again submit their username and password. You verify the password against the stored hash for this username, and if it matches, you store the user ID in the current session. Now the client is connected to this user account for the duration of the session. The session itself is safe as long as nobody other than the user is able to find out the session ID. That of course means the ID needs to be a sufficiently long random number (which depends on your PHP configuration).

Have you read my reply? Your code has a security vulnerability, and it simply makes no sense in its current form.

Hi, first of all, you both have a cross-site scripting vulnerability in your code. Since you insert $_POST["student_id"] straight into the document, an attacker can use this parameter to inject malicious JavaScript code. Every value you want to put into the document must be escaped first: echo 'Welcome, ' . htmlspecialchars($_POST['student_id'], ENT_QUOTES, 'UTF-8'); The target script of your form: Student_Home.php. However, the current program logic doesn't make a lot of sense. The time to welcome the student is after they've logged in and proven their identity, not during the login procedure. I mean, what if I gave you the wrong student ID? Will you still greet me with “Welcome, xyz”? The usual workflow of a login-protected site is like this: You send the data to a script which processes it (this is usually the form script itself). If the login was successful, you redirect the user to the protected page. Otherwise, you send them back to the form.

But do not apply this function when you insert the input into the database. The last thing you want is a database full of messed up strings. Store the original input and escape it when needed. Also be aware that my comment above applies here as well: Manual escaping is very fragile. Make sure to explicitly set the character encoding of the HTML document (preferrably through the Content-Type header). And then set the exact same encoding for the htmlspecialchars() function. For example: <?php header('Content-Type: text/html;charset=utf8'); ?> <h1>XSS Test</h1> <?= htmlspecialchars('<script>alert("XSS")</script>', ENT_QUOTES, 'UTF-8') ?> Without this, there's no guarantee that the function does anything whatsoever. If you want to do more than the bare minimum, you should also use Content Security Policy to block all inline scripts. This serves as a second layer of defense in case you fail to properly escape the values. The concept is simple: If the client's browser supports this feature, it will only accept external JavaScript files from the domains you've marked as trusted. All other code is blocked. So even if an attacker manages to inject JavaScript code into your page, they can't get it to execute. <?php header('Content-Type: text/html;charset=utf8'); // block all JavaScript code and CSS declarations unless they're served from https://yoursite.com header("Content-Security-Policy: default-src 'none'; script-src https://yoursite.com; style-src https://yoursite.com"); ?> <h1>XSS Test</h1> <!-- This will be blocked in all modern browsers --> <script> alert('XSS'); </script>

Hi, all mysql_* functions are obsolete since more than 10 years and will be removed in the near future. The PHP manual has a big red warning on every page and a detailed explanation of the two “new” extensions (PDO and MySQLi). Is your code safe from SQL injections? Well, it depends. Manually escaping the input is very fragile, because the function may not recognize the critical characters due to encoding issues. For example, it's well known that using a SET NAMES query together with an exotic encoding like GBK can break the escaping mechanism entirely. A much more secure solution is to use prepared statements. Both PDO and MySQLi support them, but not the old extension.

Yes, they list a lot of good reasons for treating JavaScript with care. Or did you only look at the pictures? Anyway, we obviously have very different opinions about quality and usability. If you're not willing to change anything, and if your customers accept your work, well, then that's how it is.

I gave you the answer: Repair your HTML. I'm actually surprised that this works in any browser. What else did you expect? Some magical window.gimmeThePasswordRememberPopupWindowForFirefox() function? Sorry, there's no such thing.

Hi, regardless of whether this will fix this specific problem, you should always write proper HTML and make the site usable without JavaScript. And get rid of the inline scripting. Contrary to popular belief, not everybody runs around with JavaScript turned all up. That means your magical link may not do anything. If you use a proper form instead and attach JavaScript to the submit event, you don't have this problem.

Where on earth did you dig that script out? This “magic quotes” stuff comes from the early days of PHP and was finally removed around 5 years ago. The whole idea of blindly adding slashes to all input is just nonsense. It's also well-known that addslashes() does not reliably prevent SQL injections, because it fails to take the character encoding into account. That's why back in 2002, the PHP developer added mysql_real_escape_string(). That was 12 years ago! The whole MySQL extension you're using is obsolete since at least a decade and will be removed in the future. I strongly suggest that you update your PHP and the code. Software definitely doesn't get better over time. Nowadays, we use PDO or MySQLi to access databases: http://wiki.hashphp.org/PDO_Tutorial_for_MySQL_Developers

Unfortunately, you seem to have only read the first half of my reply. This does not work. At all. It may seem to work if you have very low traffic and a lot of luck. But this will fall apart at the next opportunity. If that's good enough for you, well, I can't stop you from doing it. But if you care just a tiny bit about correct code, this is simply wrong.

Hi, a COUNT(*) query on the whole table always returns exactly 1 row. Your mysql_num_rows() check makes no sense. Counting the rows before the INSERT query is generally naive and doesn't work when there are multiple requests at the same time. Imagine this scenario: Your table is empty. Then 10 users try to register at the same time, so you count the number of rows. Since it's 0 at that time, you let everybody register. Now you suddenly have 10 users despite your limit of 5. Checks like this are not that simple. Right now, the only solution I could think of would be to put an exclusive lock on the entire table, then count the rows and then insert the row. But this is really, really ugly. Why do you need this weird limit?

Hi, what kind of suggestions do you expect? I mean, your description couldn't be more vague. All we know now is that you wanna import data from some old database to some new database. Since you've posted this in a PHP forum, you've obviously realized already that you may need an import script which pulls the data from the old database, converts it and writes it into the new database. But how exactly that would look like depends entirely on the concrete structure.

You stated that every call of execute() creates a new prepared statement. I'd like to see evidence of this, because this is indeed an exceptional claim. My code above demonstrates that calling prepare() once and execute() 10 times leads to one prepared statement. This is exactly what I expected, but you obviously doubt this result. So where's your code which shows that actually 10 statements get created?

Hi, strip_tags() has absolutely nothing to do with SQL. It removes HTML tags (hence the name). But since doing that is complete nonsense, it's probably best to strike this function from your memory. mysql_real_escape_string() does prevent SQL injections, at least in theory. In reality, many developers fail to use it correctly, or they simply forget to use it once in a while. That's why we hear about a new SQL injection vulnerability every few days. Prepared statements, on the other hand, are a much more robust solution. They completely separate the data from the query itself, which makes it impossible to manipulate the query through the input. I strongly recommend using prepared statement at all times. There's a famous quote by Donald Knuth: “Premature optimization is the root of all evil.” Whether or not reusing a prepared statement saves you a couple of milliseconds is completely irrelevant. Don't waste your time with this. As long as there are no performance issues, there's no need to worry about them. And if you do experience performance issues, you need to investigate the reason. Most of the time, a prepared statement only gets executed once. Yes.