Login Form Security Issue

PHP
1

I’ve built this login form a while back, but now realize that it’s subject to MySQL injection because I didn’t escape any of the characters.

The login form:

if (isset($_POST['submit'])) { // upon submitting login form
	$username = $_POST['username']; // must escape ALL user input
	$password = sha1($_POST['password']);
	if ($user->user_exists($username)) {
		if ($result = $user->authenticate($username, $password)) {
			$user_array = mysql_fetch_array($result);
			$session->login($user_array); // login
			redirect_to("members/homepage.php");
		} else
			$message = "Username/password combination is invalid.";
	} else
		$message = "User does not exist in the database.";	
}

Abridged user class:

	public function user_exists($username="") {
		$sql = "SELECT * FROM " . self::$table_name . " WHERE username = '{$username}'";
		$result = mysql_query($sql) or die("Query failed! " . mysql_error());
		return (mysql_num_rows($result) > 0) ? true : false;
	}  

	public static function authenticate($username="", $password="") {
		global $database;
		// $sql = "SELECT id, username FROM " . self::$table_name . " WHERE strcmp(username,'{$username}') = 0 AND strcmp(password,'{$password}') = 0";
		$sql = "SELECT id, username FROM " . self::$table_name . " WHERE username = '{$username}' AND password = '{$password}'";
		$result = mysql_query($sql) or die("Query failed! " . mysql_error());
		return (mysql_num_rows($result) > 0) ? $result : false;
	}

If I type in a quote mark in the login form

'

I get a MySQL error, meaning that my code is vulnerable to attack (the string is not being escaped properly). This makes sense because I did not escape any of the characters of the input username (I checked, this MySQL error is occuring not in the authenticate function, but in the user_exists function).

As you can see, the $username is directly input into the MySQL database; this is the security risk. A simple solution would be to escape the $username that is input.

Here’s my problem. If a cracker typed this into my login form:

x' OR 't=t

It is interpreted as an actual username (presumably as a string, because my default error message spits out that the Username/Password combination does not match).

Why is this being interpreted as a string when the single quote was being interpreted literally? Does the fact that a cracker can still type in a single quote put my login form at risk, when he cannot type in an actual SQL injection string, like the one above?

Help as always is greatly appreciated, thank you!

Edit:
The following code allows the username to validate:

23' OR '1=1

which is what I expected, and it is solved by applying the escaping function on the user input.

If the user typed that in, he’d be confronted with a “Username/password combination does not work.” Does the password field safely defend against MySQL injection, because the user requires both fields to type in any MySQL string (and the password field is hashed, meaning it won’t come out to anything coherent).

2

I assume you’ve solved your injection problem, so a bit side note, to explain the matters.

// upon submitting login form
// must escape ALL user input

that’s very common wrong assumption
You have absolutely nothing to do with user input.
It must read (and act):

// upon creating SQL query
// must escape ALL SQL data
//no matter where it come from or how

Even hashed password, although it’s already safe
Here’s the only benefit from prepared statements: it won’t ask you if you think your data is safe or not. You have to use prepare&bind anyway.

3

So the rule is: escape EVERY input on your site because a cracker can get in if one of them is left open?

4

No.
Escape everything you put into query.
Even if there were no user input at all.

This is kind of point of view matter. Look at this from the SQL’s point of view, not user input.

On the other hand, sometime you have to print input data back to the browser, instead of putting it into query. Why to escape it in this case? So, escaping EVERY input is just wrong.

5

Alright, got it. How can users possibly inject into your system without having an input area, however (considering, as far as I know, injections only happen through those)?

6

This is not actually injection issue I am talking about.
There can be special characters in the data to be escaped, with no harmful intention. Right?
It is purpose of escaping - to escape special characters, not to defend you from malicious injection. It should be done always, unconditionally, as just a part of syntax.
And, as a side effect, you’d never suffer injection through data too.
Even 2nd level injection, when you have to insert user data from one table to another for some purpose. It is rarely occurs but anyway.
If you escape only user input, you’d have injection this time! With no input area. Is it what you were asking about?
But again, escaping is not for the safety, but for the syntax.

Note that escaping works only with surrounding quotes. Otherwise it help nothing. It is 2 parts of one action and should be always done together! Once you enclose some data into quotes, it should be also escaped. And contrary, once you escape data, it should be quoted.

Also, bear in mind that you can’t escape everything. LIMIT parameters or field names for example.

7

Thank you very much for the clear answer.