First, regarding your initial question: the code you have right here is definitely safe against SQL injection. Using prepared queries (which you are doing) will protect you from the majority of SQL injection vulnerabilities. It's worth doing some reading on SQL injection in general, but otherwise the biggest concern you have is for those queries that cannot use prepared queries. This is typically the case if you need a column name to change based on user input, in which case you need to apply a white-list. However, that isn't an issue here, so no need to over complicate things. So while you are immediately protected against SQL injection, there are still some potential concerns here, and some general room for improvement:
- You store the raw user data in the session. In particular, the username is stored without any filtering. This can result in a second-order SQL injection vulnerability. In effect, you are using prepared queries here, but unsanitized data is being stored in the session. Later on when that data is retrieved from the session and used (for instance, to verify the user when they return to the site), you can't see that it is dirty data and may forget to use proper prepared queries. As a result, you may accidentally introduce an SQL injection vulnerability that is hard to see. So remember to abide by two very important rules: 1) Always clean user input before storing and 2) Always use prepared queries. The latter could very well be true already, but make sure and do #1 as well.
- In this particular case though it is better not to worry about cleaning the username so much as halting all processing if you find invalid input. In an ideal situation this would be part of a
check_login function and you would return
false as soon as you determined that the login was invalid. So before you even do anything with the username you first check if it is valid (probably by making sure it doesn't contain any special characters) and return false if you find any problems. This way there is no chance of getting dangerous input into the session in the first place. It is much easier if people login with an email instead of a username, because you can use
filter_var to make sure an actual email address was given, and return false if not. Email addresses, of course, cannot be used for SQL injection.
- Don't use
password_hash, and let it pick the salt for you.
- Don't compare the password in the database. Password hash comparisons are actually difficult to do right, and (while difficult to execute), a direct comparison like you are doing can leave your application vulnerable to timing attacks and other subtle weaknesses. Select the hashed password out of the database and use
password_verify to check if it matches the user input.
- It is probably a bit to soon for you yet, but if you are just learning then now is a good time to start reading about and learning about dependency injection and test automation. Those two will substantially improve the quality of your code in the long run, but obviously aren't being used here.
- Your spacing is very inconsistent. Following good coding conventions is a surprisingly important way of minimizing bugs in the long run and making maintenance easier. Pick a coding convention and stick to it. PSR is the standard in the PHP world, but sticking to any standard is better than being inconsistent.
Code organization is also very important to keep bugs at a minimum in the long run, but since only part of your code is posted here, there isn't much I can say about that at the moment.
Edit to add more about input filtering
I think it is worth a few more minutes to expand upon my discussion about input filtering. The issue is that there are more vulnerabilities to be concerned about than just SQL Injection. When you store user input directly in the application (i.e.
$_SESSION['valid_user'] = $_POST['userid']) you leave a lot of room for security holes to leak in. In this case, because you are checking that the username exists in the database, you are effectively checking the username against a whitelist. Validating against a white-list is the typically the best approach to security, and in this particular case it is unlikely to cause you any specific trouble, but most security vulnerabilities are the more subtle kind that come from failures to properly validate user input at every step of the process.
The concept you want to go for is defense-in-depth. In this case your login code is implicitly relying upon the fact that the registration form performs proper user-input validation. As a result, a simple failure in validation logic on the registration page can potentially open up additional avenues for attack when the login page doesn't do any validation of its own. When each area of the system practices proper security for all data relevant to itself, it becomes much more difficult for an attacker to find an exploitable security bug. As a result, the most secure way to handle user input is:
- Validate user input as strictly as you can, and reject any requests with invalid data
- Always use prepared queries