0

I'm following a tutorial, Creating a Secure Login System the Right Way, about how to create a login system. In the code they use mysql_real_escape_string on the username field before passing it to the database as a query, that is,

$username = mysql_real_escape_string($username);

Is this necessary since I am not adding anything to the database, I am simply checking if this user already exists?

The reason I am not just leaving it in anyway is when I use the above code, it renders my text blank and so is sending an empty string to the database. I don't know why this is, so I thought, if I could leave it out, I would.

Below is for advice about database connection being open from a commenter (passwords, etc. been changed):

function dbConnect(){
    $connection = mysql_connect('localhost', 'username', 'password');
    $database=mysql_select_db('database', $connection);

    return $connection;
}

$username = $_POST['username'];
$password = $_POST['password'];
$password = md5($password);

$username = mysql_real_escape_string($username);

$query = mysql_query("SELECT *
      FROM members
      WHERE username = '$username'
      AND password = '$password'",dbConnect());
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Claire
  • 3,683
  • 13
  • 47
  • 74
  • 1
    your query can be canceled or marked as a comment and another one could be attached due to that. also nested queries (subqueries) might work, so every data passed via sql that comes from outer sources should be validated and/or sanitized. – Hajo May 02 '12 at 18:21
  • Ok thanks. Can you please answer the second part please why it renders my string empty? – Claire May 02 '12 at 18:23
  • 2
    `mysql_real_escape_string()` would only return blank if $username were blank. The problem lies elsewhere. – Marcus Adams May 02 '12 at 18:25
  • It's not blank, I do an echo on $username before doing $username = mysql_real_escape_string($username) and it returns a value, after that it doesn't – Claire May 02 '12 at 18:32
  • mysql real escape string needs an open mysql connection and defaults to false on errors. is there a php warning? – Hajo May 02 '12 at 18:34
  • ahhh, I don't think so. I'll edit the code above to show you how I've done it... – Claire May 02 '12 at 18:36
  • @Nicola does the problem with your empty username still exist? if so: php display_errors set to on ? error_reporting set to E_ALL ? – Hajo May 02 '12 at 18:55

3 Answers3

1

You may want to use PDO with prepared statements. Prepared statements are like placeholders in an SQL query and you're later on shipping the data that will then be inserted on those places. This makes escaping strings obsolete.

As I've already mentioned in the comments above: every SQL query with user input is vulnerable to SQL injection attacks.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Hajo
  • 849
  • 6
  • 21
1

The proper code is:

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

$sql = "SELECT * FROM members WHERE username = '$username' AND password = '$password'";
$res = mysql_query($sql) or trigger_error(mysql_error().$sql);
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • thats great thank you, it works now. I wasn't sure about the connection bit, I thought the mysql_query function had to include the database connection – Claire May 02 '12 at 19:27
  • @Nicola it could use an explicit connection too. You could use *a variable* to store a connection and use it with mysql_connect(). Otherwise las opened connection will be used – Your Common Sense May 02 '12 at 19:30
  • right, I think I need to do some reading up on connections and different ways to connect – Claire May 02 '12 at 19:31
0

Yes it is necessary because the username could contain special character not allowed in SQL that need to be escaped like ' or / for instance

Example:

Not escaping ' in the username McDonald's would lead to an illegal SQL statement:

select * from your_table where username = 'McDonald's'
juergen d
  • 201,996
  • 37
  • 293
  • 362