Secure login system

Question

The assignment is to create a login system secured for SQL injections and XSS.

It's in PHP and I'm using PDO with prepared statements obviously. Which from my point of view should protect against the simplest injections at least. The ones I'm throwing at the system are ineffectual.

If you believe it's not secure against SQL injection I would gladly take comments and an example of what should work against it.

With regards to XSS I haven't done anything to protect against it yet. From the school I believe it was mostly about html or url-encode the inputs. But would that be enough? From google I also have some articles about sql escaping.

Comments or help on XSS protection could be nice. Or is it only a matter of escaping the inputs before it reaches the SQL query?

<form method="post">
  <input type="text" name="username" id="username">
  <input type="password" name="password" id="password">
  <button type="submit" name="submit" class="login-button">Login</button>
  <a href="register.php">Register!</a>
  <p class="forgot-password"><a href="forgot.php">Forgot login?</a></p>
</form>

function Log_in($username, $password) {
  $db = new PDO("host;dbname", "user", "password");
  $login = $db -> prepare("SELECT * FROM Users WHERE Username =:username and Password =:password");
  $login->bindValue(':username', $username, PDO::PARAM_STR);
  $login->bindValue(':password', $password, PDO::PARAM_STR);
  $login->execute();

  global $count;
  $count = $login->rowCount();

  global $data;
  $data=$login->fetchColumn(2);
  return $count;
  return $data;
}

$username = trim($_POST["username"]);
$password = trim($_POST["password"]);

if (isset($_POST['submit'])) {
    getSalt($username);
    foreach ($rowset as $row) {
        $salt = $row[0];        
    }

    $hashpassword = sha1($password.$salt);
    Log_in($username, $hashpassword);
    if($count==1){
        session_start();
        $_SESSION['email'] = $data;
        header("location:index.php");
    }   else {
        echo "Wrong";
    }
}

Answer 1

Security

Yes, your code is secure against SQL injection, that's good.

You don't echo any variable data, so your code is also not vulnerable to XSS. You left out your form (and possibly other code), so there might be an XSS vulnerability there though. To protect against XSS, you should definitely read this cheat sheet.

is [XSS] only a matter of escaping the inputs before it reaches the sql query?

No, definitely not. XSS is a vulnerability that happens when printing something to the user, so that is when it should be defended against (in PHP eg by using htmlspecialchars).

Also, sha1 isn't really recommended for hashing anymore, you should use something like bcrypt. This also has the added advantage that you don't have to take care of salts, etc. The relevant function in PHP is password_hash.

Structure

globals are evil (not necessarily always, but definitely in this case). If you have to return more than one value return an array, do not set it as global.

I personally would choose a more OOP oriented different approach of having a user object and setting the email there, and then returning true/false instead of an integer from Log_in (because either the login is successful or not, there are no other states).

Misc

• your indentation is off, which makes your code harder to read.
• function names should start with a lowercase character.
• all SQL keywords should be all uppercase (and -> AND).
• data isn't a very good variable name. What is it? If it's just an email address, call it email. count is also very generic (especially for global), rowCount would be better.
• you shouldn't have two returns, especially if you use none of them (in that case, you should have no return).
• it's unclear what $rowset is, but it seems odd that you have to iterate over it to get the salt. You could just access the last item of the array and safe the loop, but it still doesn't seem right.
• don't select * if you don't need all columns, only fetch those that you actually need. If you do select *, I would fetch each column by name, not by an integer, in case a column is added later on.