Login code probably not secure

Question

I'm new to PHP security and have made this login file:

<?php
header('Content-type: application/javascript');
$hostname_localhost = "";
$database_localhost = "";
$username_localhost = "";
$password_localhost = "";
$salt1 = "";
$salt2 = "";
$localhost = mysql_connect($hostname_localhost,$username_localhost,$password_localhost)
or
trigger_error(mysql_error(),E_USER_ERROR);
mysql_select_db($database_localhost, $localhost);
$username = clean($_POST['username']);
$password = $_POST['password'];
$sha1_password = sha1($salt1.$password.$salt2);
$query_search = "select * from users where username = '".$username."' AND password = '".$sha1_password. "'";
$query_exec = mysql_query($query_search) or die(mysql_error());
$rows = mysql_num_rows($query_exec);
if($rows == 0) {
 echo "No Such User Found";
} else {
 $result = mysqli_query($localhost,"select * from users where username = '".$username."'");
 while ($row = mysqli_fetch_array($result)) {
 echo $row['id'];
 }
}
function clean($string){
 if (get_magic_quotes_gpc()) $string = stripslashes($string);
 return mysql_real_escape_string($string);
}
?>

You'll probably all find a million things wrong with it but I need all the help I can get!

• Alex L: There are most definitely things wrong. It appears you followed an old tutorial, so consider finding one that was recently written next time. (Here is one)(sunnytuts.com/article/…) that covers the most important bases, and it looks like it covers security in the second part.
• Elias Van Ootegem: mysql_connect + mysqli_fetch_array... ? which extension are you using, mysql_* or mysqli_*? Note that it is requested you post working code on this site, as code-REVIEW is all about having an extra pair of eyes pass over your code to improve things, not to fix things

Answer 1

Warning
This is a code-review (CR) site. Though my answer/review may seem harsh at points, it is my firm belief that CR can, and should, be tough. I've explained my reasoning at length here.
It is not my intention to hurt yours or anyone else's feelings, be advised that some criticisms may be phrased badly, or bluntly. It is important, then, to keep in mind that my only goal is to help.

Due to the fact that much of CR is criticizing other peoples code, you may get the impression that I see myself as the light of the world, and only source of "good code". Not so. Some critiques are subjective (such as which extension I recommend: PDO and not mysqli_*). TMTOWTDI and to each his own.

---

The first thing that is "bad" (as in: not good practice), is that you're not using the right MySQL extension. What is in fact outright wrong is that you're not consistently using a single extension throughout. Most of the time, you're using the mysql_* extension (which is really, really bad, mkay). But then out of the blue you have this, shall we say, moment of clarity, and you use the MySQL Improved extension (or mysqli_*):

//all your code using mysql_*
$result = mysqli_query($localhost,"select * from users where username = '".$username."'");
while ($row = mysqli_fetch_array($result)) {
 echo $row['id'];
}
//reverting back to deprecated extension

The help section of this site specifically request you post actual, working code to review. If you need this code fixed/debugged: set about trying yourself and, if you can't seem to get it to work, post a question on SO.
Please clarify (by editing your posted code) which extension you're actually using. Also mention any notices, errors or warnings you get, if any.
Regardless of what extension you choose: make sure it's not deprecated, use it as was intended and be consistent.

The second thing is the use of magic quotes. Like the mysql_* extension, these have been deprecated for quite some time and have been removed since PHP 5.4.0. Do not use them. Ever. No, seriously. Stop. I'm not joking. Honest.

Third item on the agenda: at no point are you bothering to check if there is any POST data to be processed, You just blindly assume $_POST['username'] will be there. That can issue notices (undefined index E_NOTICE).
Adding an if (!isset($_POST['username']) || !isset($_POST['password'])) exit(); isn't a lot of work, and prevents all this.
I would assume you're include-ing this file as part of a bigger script, and in case that script does check for isset params, then you should be good, but if that's the case: be careful with that header business. If the headers are already sent, you'll get an error.

Which brings us neatly on to point four: header('Content-type: application/javascript');? Why? What? How? Why are you setting a header that makes no sense? As far as I can tell, you're only echoing text, a more suitable header, then, would be:

header('Content-Type: text/plain');

If you are going to add markup to the output, change that header to:

header('Content-Type: text/html');

Fifth: mysql_connect(...) or trigger_error(mysql_error(),E_USER_ERROR); vs $query_exec = mysql_query($query_search) or die(mysql_error());. Why are you using that horrible or? and why use trigger_error the first time 'round, only to then switch to that equally horrible die?
If the db connection failed, then the app failed. throw new RuntimeException('Could not connect to db'); from a function that returns a db connection is what you could do, or simply exit(1);
Whatever you do: be consistent.

Lastly, and most importantly: as a result of the things mentioned above, and the fact you're simply concatenating barely validated POST data into a query: You are vulnerable to injection attacks. This site explains what they are.
An easy, reliable and safe way to prevent this sort of attack is to use prepared statements. An example of how this is done with PDO would be:

$dsn = 'mysql:host=127.0.0.1;dbname=default_db';
$user = 'root';
$pass = 'DoNotUseRootUserIfYouDoNotNeedRootAccess';
$pdo = new PDO(
 $dsn,
 $user
 $pass,
 array(//more options here
 PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
 PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_OBJ
 )
);
//no quotes, no data, just the query as-is
$stmt = $pdo->prepare(
 'SELECT * 
 FROM users
 WHERE username = :uname
 AND password = :pass'
);
$stmt->execute(
 array(
 ':uname' => $_POST['username'],
 ':pass' => sha1($salt1.$_POST['password'].$salt2)
 )
);
if (!($row = $stmt->fetch()))
{
 echo 'User not found';
}
else
{
 do
 {
 echo $row->id, PHP_EOL;
 }while($row = $stmt->fetch());
}

Just browse through the documentation for the PDO class, or work out how to use prepared statements using the mysqli_* extension. It's not very hard, and I'd be happy to review your attempts at it, but I don't really like mysqli all too much, so I'm not posting an example here.
To help you choose between the two extensions, there's a special choose-an-api page can be found here

The reason why I prefer PDO, in case you are wondering is simply because its API is clean, and consistent: it's OO-only, whereas mysqli_* offers both a procedural and OO API. That, to my eye, makes for messy code.
mysqli_* uses a bit too much pass-by-reference in its methods:

$foo = 123;
$mysqliStmt->bindParam('i', $foo);//REFERENCE to $foo
$foo = 678;
$mysqliStmt->execute();//will use CURRENT value of $foo -> 678

It, once again, has to do with being consistent.

Last thoughts:
Given that you're still new to PHP, I'll leave it at this. Spend some time on the doc pages, and if it all gets a bit much, perhaps look at the unofficial coding standards: at PHP-FIG. All major players (Zend, Symfony, Doctrine, Drupal, Joomla... the list is on the site) subscribe to these standards. Since I've repeated it many times before here, if you want to be consistent, too: try and adopt these coding standards.

Answer 2

I don't know PHP, but it looks like you have an SQL injection vulnerability here:

$query_search = "select * from users where username = '".$username."' AND password = '".$sha1_password. "'";

And here:

"select * from users where username = '".$username."'"

What if $username is ' OR 1=1?

To fix this issue I suggest using prepared statements for your queries and bind the parameters to your $username and $password values.

• Elias Van Ootegem: While you've pointed out the biggest issue with the OP's code (quite rightly), I don't know if your answer really qualifies as Code review. Sure, you've pointed out a security issue, but what is needed to plug the leak? Also note that mysql_real_escape_string would prevent ' OR 1=1 from messing up the where clause, since it escapes the ' char
• m0skit0: Oh correct, I forgot to point out how to fix it. About mysql_real_escape_string, sorry, as I stated "I don't know PHP".
• Elias Van Ootegem: Well, mysql_real_escape_string is part of the mysql C API, too. I believe most languages that come with mysql extensions/adapters/connectors out of the box have implemented an escaping function at some point. But that's just me being pedantic. Bottom line is: you are right: there is an injection vulnerability, still. I just wanted to let you know that this answer, to me, is sort of in the middle between a comment and and an actual answer, hence no vote (down or up)
• m0skit0: @EliasVanOotegem Yeah, I was in doubt too. I first added it as comment, then posted as answer because I think it points out the major flaw there (from my knowledge).