2

Okay I have two variables in PHP

$username;
$password;

which are initialized to the data retrieved from $_POST variable :)

I have this SQL query

$sql = "SELECT * FROM users WHERE username = '" . $username . "' AND password = '" . $password . "')";

But this doesn't works and returns me nothing :(

Can you instruct me into the right direction. Please?

akif
  • 12,034
  • 24
  • 73
  • 85

8 Answers8

12

The query has a closing parenthesis on the end for no reason, it won't work.

Chad Birch
  • 73,098
  • 23
  • 151
  • 149
  • I didn't see that. Was still reeling from shock of seeing such a security risk. However, this is probably the best answer. Removing the parentheses should make the code work but *please* don't use that in production. – Iain Holder Apr 23 '09 at 16:30
7

What's wrong with it?

Everything, unfortunately. In particular it's open to SQL injection attacks.

If that's a verbatim cut&paste, then the reason it's not actually working is a trailing closing bracket. Presumably you're not checking for errors when you call this?

Using the base MySQL API it should be:

$sth = $db->prepare("SELECT COUNT(*) FROM users WHERE username = ? AND password = ?");
$sth->execute($username, $password);
list($count) = $sth->fetchrow();
$authorized = ($count > 0);

or similar (code untested, E&OE, etc...)

Alnitak
  • 334,560
  • 70
  • 407
  • 495
4

eeek! sql injection for one!

EDIT: What's your favorite "programmer" cartoon?

Community
  • 1
  • 1
Iain Holder
  • 14,172
  • 10
  • 66
  • 86
2

Why is there a stray ) at the end of your query? It shouldn't be there.

Oh, and thirded on SQL injection. BAD.

Matt
  • 2,229
  • 12
  • 10
1

You seem to have an excess closing parenthesis at the end of your query string.

[Edit] - for those screaming SQL injection attacks: we don't know what the user has done with their variables before using them in the query. How about benefit of doubt? ;-)

BrynJ
  • 8,322
  • 14
  • 65
  • 89
  • Better to tell him and find that he's already thought about it than to *not* tell him in case he hasn't. – Graeme Perrow Apr 23 '09 at 16:29
  • You should always sanitize the data as it's being passed to the database in addition to validating any input. – Matt Apr 23 '09 at 16:30
  • Of course I advocate proper handling of user entered variables. I've updated the smiley in my post for clarity ;-) – BrynJ Apr 23 '09 at 16:34
1

First of all, never, ever do it like this. Please read about SQL injection and don't write any SQL until you have understood what it says. Sorry, but this is really essential.

That said, your query contains a closing bracket. That looks like a syntax error. Do you get an error executing it?

sleske
  • 81,358
  • 34
  • 189
  • 227
1

There's an extra parenthesis on the right hand side of the query.

Also, if you do not sanitize your code properly you're going to be vulnerable to SQL injection. You should really be using parameterized queries, but in lieu of that at least use mysql_real_escape_string() on $username and $password.

Also, as a bit of ghost debugging, it's very possible that your passwords are MD5 hashed in the database, since you should never store them in plain text.

Try:

$username = mysql_real_escape_string($_POST["username"]);
$password = md5($_POST["password"]);

$sql = "SELECT * FROM users WHERE username = '$username' AND password = '$password'";
John Rasch
  • 62,489
  • 19
  • 106
  • 139
0

In addition to all the other problems noted. The Password in the Users table is stored encrypted. Unless you've run the Password through the MySQL password encryptor, you will never see any data from this query as the passwords won't match.

Thomas Jones-Low
  • 7,001
  • 2
  • 32
  • 36