How can I optimize my login script? I've created an Ajax login for my website but I feel like I can optimize it, but I'm not sure how and where.

Questions:


How can I optimize my code?
Is the code secure? Any ways to break it (injection, etc)?


Also, when I attempt to log in, it currently takes about 1 second to process the login (on localhost). Is this long?

Here's my Ajax call:

$(document).ready(function() {
  $(document).on("submit", "form", function(event) {
    event.preventDefault();
    $.ajax({
      url: 'assets/php/login_script.php',
      type: 'POST',
      data: $(this).serialize(),
      success: function(data) {
        if (data == true) {
          window.location.href = "index.php";
        } else {
          $("input[name=password_field]").focus();
          $(".error").html(data);
        }
      }
    });
  });
});


Here's the PHP script:

&lt;?php

  include_once("access.php");

  $cxn = mysqli_connect($host, $user, $pass, $db) or die ("Couldn't connect to the server. Please try again.");

  $username = $_POST["username"];
  $password = $_POST["password"];
  $date = date('Y-m-d h:i:s', time());
  $ip_address = get_ip_address();
  $expire = time() + 86400 * 365;

  $options = array('cost' =&gt; 12);
  $hash_password = password_hash($password, PASSWORD_BCRYPT, $options);

  /* Log the login request. */
  $stmt = $cxn-&gt;prepare("INSERT INTO login_logs (log_id, username, password, datetime, ip_address) VALUES ('', ?, ?, ?, ?)");
  $stmt-&gt;bind_param('ssss', $username, $hash_password, $date, $ip_address);
  $stmt-&gt;execute();

  /* Get user information from database. */
  $stmt = $cxn-&gt;prepare('SELECT * FROM users WHERE username = ?');
  $stmt-&gt;bind_param('s', $username);
  $stmt-&gt;execute();
  $result = $stmt-&gt;get_result();

  /* If a result exists, continue. */
  if ($result-&gt;num_rows &gt; 0) {
    while ($row = $result-&gt;fetch_assoc()) {
      $db_username = $row['username'];
      $db_password = $row['password'];
      $random_hash = password_hash(time() . $db_username . time(), PASSWORD_BCRYPT, $options); 

      /* Password matches. */
      if (password_verify($password, $db_password)) {

        /* Get user's cookie information in database. */
        $stmt2 = $cxn-&gt;prepare("SELECT * FROM cookies WHERE username = ?");
        $stmt2-&gt;bind_param('s', $db_username);
        $stmt2-&gt;execute();
        $result2 = $stmt2-&gt;get_result();

        /* If a result exists, update the cookie. */
        if ($result2-&gt;num_rows &gt; 0) {
          $stmt = $cxn-&gt;prepare("UPDATE cookies SET hash = ? WHERE username = ?");
          $stmt-&gt;bind_param('ss', $random_hash, $db_username);
          $stmt-&gt;execute();

          setcookie("user", $db_username, $expire, "/");
          setcookie("hash", $random_hash, $expire, "/");
        } else {
          $stmt = $cxn-&gt;prepare("INSERT INTO cookies (cookie_id, username, hash) VALUES ('', ?, ?)");
          $stmt-&gt;bind_param('ss', $db_username, $random_hash);
          $stmt-&gt;execute();

          setcookie("user", $db_username, $expire, "/");
          setcookie("hash", $random_hash, $expire, "/");
        }

        echo true;
      } else {
        echo "Incorrect credentials.";
      }
    }
  } else {
    echo "Incorrect credentials.";
  }

  function get_ip_address() {
    $ip_address = '';
    if (getenv('HTTP_CLIENT_IP'))
      $ip_address = getenv('HTTP_CLIENT_IP');
    else if(getenv('HTTP_X_FORWARDED_FOR'))
      $ip_address = getenv('HTTP_X_FORWARDED_FOR');
    else if(getenv('HTTP_X_FORWARDED'))
      $ip_address = getenv('HTTP_X_FORWARDED');
    else if(getenv('HTTP_FORWARDED_FOR'))
      $ip_address = getenv('HTTP_FORWARDED_FOR');
    else if(getenv('HTTP_FORWARDED'))
      $ip_address = getenv('HTTP_FORWARDED');
    else if(getenv('REMOTE_ADDR'))
      $ip_address = getenv('REMOTE_ADDR');
    else
      $ip_address = 'UNKNOWN';

    return $ip_address; 
  }

?&gt;


How can I optimize my script to look better, be faster, etc?
I've seen quite a few programmers who don't enjoy seeing arrays coming in as arguments into the code (me personally, I'm fine with it as long as there is some sort of documentation that goes with it so you don't forget what the function can and cannot take).

password_hash($password, PASSWORD_BCRYPT, $options);


would instead be:

password_hash($password, PASSWORD_BCRYPT, $cost);


The above is purely an FYI and not my opinion.

When it comes to accepting username/passwords, you want to be sure you're on a secure location. Based on your current code, if you're only a http:// page (typically port 80) then a malicious bot can potentially sniff the username/password. Easiest way to do this, is run a check against $_SERVER['HTTPS'] but if that fails, you do want to redirect the user to another page or something like that.

You shouldn't have more than one user with the same username, so create a LIMIT 1 at the end of the SQL query, otherwise it'll scan the entire table no matter how many results you get.

$stmt = $cxn-&gt;prepare('SELECT * FROM users WHERE username = ? LIMIT 1 ');


Same concept for your cookies table lookup:

$stmt2 = $cxn-&gt;prepare("SELECT * FROM cookies WHERE username = ? LIMIT 1");


Is there a purpose of creating additional variables that are essentially the same values of the initial variables?

e.g.

$username = $_POST['username'];


What that ends up doing is you will use up additional memory blocks when it's not really needed to happen. Also, you could lose track of what $username is down the road, and re-initialize it with a different value. Same idea when you're looking at your $row data.

PASSWORD_BCRYPT is not meant to be fast, but it is meant to be secure. You're running it twice, with different values, is there a reason you're doing that for your logs considering you can't reverse the encryption? I'd just run it once and put it against $hash_password

If you're using time() multiple times across the page, that's when you may want to put that against a variable. Even though it's a very quick function, it'll be faster to look it up against a variable that has a fixed number and is stored in a particular memory block, creates a consistent value across the board in case you have to do additional comparisons against each other (compare something about the expiration date or check the value that got inserted into the log table and since that's a salt, might as well have that as a reference etc.).

If the username/password verification feels like it takes a long time, then it is taking a long time, that's the way I see it. If you don't mind how long it takes, you might be ok (as saying 1 second is relative to the actual speed of the script). However, for bottlenecks, you'd have to test pieces of your code to see where is it slowest.

Hope this helps.