3
\$\begingroup\$

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";
    }
}
\$\endgroup\$
3
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer! I just added the form in the post. There is also other code, but none that should have anything to do with the login page or system. Regarding if it's enough with escaping the inputs before the query, I mean mostly protecting the database of script insertion that can run from there? Obviously any output to the site is a XSS liabilty.but as you say I don't really have any output here. And thanks for the pointers. \$\endgroup\$ – gerre Apr 21 '15 at 16:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.