When a user registers I clean the password with mysql_real_escape_string as follow
$password = clean($_POST['password']);
Before adding it into database I use :
$hashedpassword = sha1('abcdef'.$password);
and save it into mySQL.
My question is should I clean it or am I protected that the password is hashed before adding it into the DB ?
Well, there is one major misunderstanding.
mysql_real_escape_string() does not clean anything. It has nothing to do with security at all.
This function is used just to escape delimiters and nothing more. It can help you to put string data into SQL query - that's all.
So, every string you're going to put into query (by putting it in quotes), you have to prepare it with this function. In all other cases it would be useless or even harmful.
Thus,
out of this misunderstanding, you're doing 2 major mistakes here:
Also, as a side note, please bear in mind that this function should be always used in conjunction with mysql_set_charset, or otherwise a "_real_" part of this function become useless
If you are applying the clean() function on it (which I assume is an awful mixture of stripslashes/htmlentities/addslashes), then the hash you generate with sha1() might not be the hash for the actual password.
This will work by accident even if the password contained special charactes. But only as long as you always follow the exact same procedure. And actually you should be properly escaping the $sha1 hash afterwards (even if unneeded). And more actually, you should use a scheme that involves a better salt (per-user).
But to answer your question: This is working in your case; and not a problem. It's just not correct.
Yes. Always. Or, better yet use placeholders (which are available in both PDO and mysqli) see best way to stop SQL injection in PHP.
The reason to always use some form of escaping (even the outdated and cumbersome mysql_real_escape_string) is to be consistent. Saving the few cycles because "it isn't needed here" (and it "isn't needed here" because sha1 returns a string of hex characters) is irrelevant if it -- due to lack of consistency -- leads to bugs and/or compromises later.
Bugs/compromises can be introduced by a lack of consistency and forgetting to "escape" a different field later or it may be due to a small code range where the new requirements to "escape" was overlooked. (Imagine if a future version saves a binary or base-64 SHA1 signature.) Both of these trivial vectors can be eliminated through better practices.
Happy coding.
The user enters data. The database stores the data. The point of "escaping" the data (or, better yet, use placeholders/parameterized queries) is making sure the data makes it into the database correctly and safely. If the data needs to be treated specially, then handle this at the data level -- the actual operation of dealing with the SQL should be simple, consistent, and reliable. (Note that mysql_real_escape_string doesn't change the data seen by the database, rather it ensures that the data is the real data -- that which was passed to the mysql_real_escape_string function -- after the database parses the SQL command.)
It is quite sad, but the entire point of needing to "escape" still exists because of the incorrect concatenation of data into SQL strings. This "problem" has been solved for many, many years with the use of placeholders which allows the SQL command and the data to be kept isolated. Prepared statements may also be more efficient, depending upon other factors. And, quite honestly, I can't see how people can stand to look at/write the mess created with string manipulation (even without all the added "escaping" code) of SQL commands in general.
There is NO need to use anything else than the hashing pass afterwards, there is no chance that something would screw your insertion, and XSS would be even more impossible. (note the “more impossible” part)
However... I suggest you to trim() the password before hashing it, so you will be sure that the user entered something. Also, check the trimmed password with strlen() before “hashing”
This code is enough for passwords... Where $pass is the posted password.
$pass = trim($pass);
if (strlen($pass) > 5) // it has to be bigger than 5 chars long.
{
$pass = sha1($pass); // or use crypt() as some have suggested.
// pass is ready to be inserted.
}
else
{
// pass is not correct and you should handle this as you want~
}
Greetings.
In this particularl case, if you only use the hashed form of the string in your query, you'd be safe from SQL injection. SHA1 does not output any SQL metacharacters that would break your string, so this:
$pw = sha1($password);
$sql = "INSERT INTO .... VALUES ('$pw')";
would be "safe".
However, it is good practice to get in the habit of ALWAYS escaping everything, even if it's not strictly required. Better to go a little overboard with it, when it's not needed versus forgetting it in one tiny little spot and your site getting pwned.
Really, in this case you need not mysql_real_escape_string here, because sha1 returns only string of a-f0-9 chars, that must not be escaped.
But it's bad idea to skip it here.
Why if you will change sha1 to other function, that may cause error. Simply, you will forgot to add mysql_real_escape_string. Besides, it's more semantic(I don't even speak about security) to use escaping function for all of your actions: storing in db, printing HTML/XML, executing shell commands.
Besides, as pst says its great idea to use placeholders
