3
\$\begingroup\$

I'm trying to make a login system for my website which I'll be releasing in public later on. I have inserted some main samples of my code in here and I would like some useful advise or suggestions. Also if I'm doing anything unsafe or anything which may cause problems in future please let me know. Thanks

login.inc.php:

<?php
    error_reporting(0);
    $error_message = "Website ran into an error";
    session_start();
    if (isset($_SESSION['user_id'])) {
        exit();
    }
    $parameters = json_decode(file_get_contents('php://input'), true);

    if (isset($parameters["inserted_id"]) && isset($parameters["inserted_password"])) {
        // FOR TEST USE I'M USING xampp server, localhost
        $connection = mysqli_connect("localhost", "root", "", "users") or die($error_message);
        $inserted_id = $parameters['inserted_id'];
        $inserted_password = $parameters['inserted_password'];

        $statement = mysqli_stmt_init($connection) or die($error_message);
        if (mysqli_stmt_prepare($statement, "SELECT id, account_name, password FROM user WHERE account_name=? OR email=?;")) {
            mysqli_stmt_bind_param($statement, "ss", $inserted_id, $inserted_id);
            mysqli_stmt_execute($statement) or die($error_message);
            if (($row = mysqli_fetch_assoc(mysqli_stmt_get_result($statement))) == null) {
                echo("No records found with inserted inputs");
                mysqli_stmt_close($statement);
                mysqli_close($connection);
                exit();
            } else {
                if (password_verify($inserted_password, $row['password'])) {
                    $_SESSION['user_id'] = $row['id'];
                    $_SESSION['account_name'] = $row['account_name'];
                    echo("pass");
                } else {
                    echo("Wrong password");
                }
                mysqli_stmt_close($statement);
                mysqli_close($connection);
                exit();
            }
        }
        mysqli_close($connection);
        exit();
    }
?>

login.js:

var request_sent = false;
document.querySelector("button").onclick = function () {
  if (!request_sent) {
    var inserted_id = document.getElementById("inserted_id").value;
    var inserted_password = document.getElementById("inserted_password").value;
    if (inserted_id == "" || inserted_password == "") {
      alert("Both fields need to be filed out");
    } else {
      request_sent = true;
      http_request("login.inc.php", JSON.stringify({"inserted_id":inserted_id, "inserted_password":inserted_password}));
        }
    }
}

function http_request(url, params) {
  $.ajax({
    type: "POST",
    url: url,
    data: params,
    success: function(data) {
      if (data == "pass") {
          window.open("../website", "_self", "", false);
      } else {
          alert(data);
      }
      request_sent = false;
    }
  });
}

login.html:

<html>
<head>
  <title></title>
</head>
<body>
  <input id="inserted_id" type="text" placeholder="Account Name/e-mail"/>
  <input id="inserted_password" type="password" placeholder="Password"/>
  <button>SUBMIT</button>
  <script src="login.js"></script>
</body>
</html>
Improve this question
\$\endgroup\$
3

3 Answers 3

Reset to default
5
\$\begingroup\$

I'll take a more holistic approach than mickmackusa, he already talked about a lot of details.

I don't see any problem with the fact that you use an AJAX call to verify user credentials, and login. There's no real difference, security wise, when I compare this with a normal form submission. Any hacker will simply bypass your Javascript and call login.inc.php directly.

Your PHP code seems to cover the basics well. There are prepared statements, you use hashed passwords and password_verify(), and you don't show system errors to the user (do read the comment by Your Common Sense; 'regarding the topmost line'). But to me the code still looks a bit unorganized.

For instance, you open and close the database connection. I'm sure you'll have other PHP pages where this is needed. It doesn't make sense to have the connection details in every PHP page. I would include another PHP file with functions to open and close the database, so I can simply say: openDatabase();. Then, if I have to change the password of the database, I can change it in one location.

There's also no harm in using functions for other things in your code. It makes it much easier to read:

if (hasLoginDetails($parameters)) {
    list($username, $password) = getLoginDetails($parameters);
    $database = openDatabase();
    if ($user = getUserInfoByName($database, $username)) {
        if (password_verify($password, $user['password'])) {
            setSessionUser($user);
            echo("pass");
        } else {
            echo("Wrong password");
        }
    }
    else {
        echo("No records found with inserted inputs");
    }
    closeDatabase($database);
}

I'm not saying this is the right way to do it, but it is better. Note that I have left out all your exit(). They were used wrongly, because they would have, theoretically, prevented the database to close. Of course, it will close automatically when the script exits, but did you know that?

My point is that structuring your code is very important. It helps you spot problems, and makes it easier to change the code later on.

There's also some, but not much, confusion when it comes to names. An inserted_id seems to have the same content as account_name. That's not obvious. Why not give these more similar names?

Now that I have slightly shortened the code I notice you give two different messages back to the user: "Wrong password" and "No records found with inserted inputs". That means that I can start guessing user names first, and when I get it right, I will get feedback telling me so. That's half my job, of breaking in, done. In other words: These two messages should always be the same. Don't give a hacker any information.

As to the security. There are no other protection mechanisms against hacking in your system. It would happily accept a simple brute force attack. Once I have a valid user name I can try my long list of passwords as fast as your server will process them. Even a simple sleep(4), when the wrong credentials were supplied, will slow this down enormously. I normally give users five changes to enter their credentials and then block them for a longer time (15 minutes?). The user needs to be told this, of course, otherwise real users might find themselves locked out because they simply tried to often. Something like: "You have 3 attempts left before your account will be blocked.". You should not block an account too long, otherwise someone can block users intentionally. Be careful with this. Another strategy is to make failed logins, by the same person, slower and slower, the more they try it. Anything that will make a brute force attack impossible, but not hinder normal users.

Your system is also lacking the usual 'I forgot my password' mechanism. Because users will forget their password. By not having this mechanism, users will have to correspond with you to get it, which can be very slow, and possibly less secure.

Adding features, like the ones I mentioned above, will further increase the complexity of your code, hence the importance of good structuring to keep it readable.

Improve this answer
\$\endgroup\$
3
  • 2
    \$\begingroup\$ Thank you. Only one little nitpick more: error_reporting(0) actually suppresses errors, not preventing them from displaying. You can check the link I posted in the comment under the question, it explains the difference pretty straightforward. \$\endgroup\$
    – Your Common Sense
    Apr 24, 2019 at 8:19
  • 2
    \$\begingroup\$ Yeah, I didn't pay attention to that. I shall refer to your comment in my answer. \$\endgroup\$
    – KIKO Software
    Apr 24, 2019 at 8:22
  • \$\begingroup\$ Thank You. The content in your answer seems useful and I'll try and implement it. \$\endgroup\$
    – aman
    Apr 26, 2019 at 5:37
6
\$\begingroup\$

  • isset() accepts multiple parameters, you can simplify:

    if (isset($parameters["inserted_id"]) && isset($parameters["inserted_password"])) {

    To

    if (isset($parameters["inserted_id"], $parameters["inserted_password"])) {

  • When ever I read anyone's code or database schema and I see any mention of id, I immediately assume that I am dealing with a positive integer. If the value is anything other than a positive integer, I never use id in the field/column/variable name.

  • I never use mysqli's init() call because it is optional. I'll recommend objected-oriented mysqli syntax because it is less verbose.

  • I don't think I'd json encode/decode your form data -- it seems like unnecessary extra data handling. See here: jQuery Ajax POST example with PHP

  • echo doesn't need those parentheses.

  • I am confident that @YourCommonSense will post a comprehensive answer containing the 3 pieces of commented advice and more, so I'll leave those recommendations to him.

  • You don't need to write out all those close() calls, just echo your message and let the script end naturally. If you're going to kill the script at if (isset($_SESSION['user_id'])) { be sure to sure to echo something for consistency.

  • I find javascript alert() boxes to be terribly annoying and a bit trashy to be honest. Give your site some "class" and fashion an attractive presentation for any response messages that you plan to offer.

  • Your .html has form fields, but no form -- that doesn't feel valid to me. I'll recommend adding the form tags and names attributes. Some discussions: https://stackoverflow.com/q/33370025/2943403

Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ No, sorry, I wasn't going to answer, got no time for complete answers atm \$\endgroup\$
    – Your Common Sense
    Apr 24, 2019 at 8:36
  • \$\begingroup\$ @mickmackusa What about any security or any sort of "error" problems which may raise? \$\endgroup\$
    – aman
    Apr 26, 2019 at 5:15
  • \$\begingroup\$ @Aman For extreme cases, I recommend that you never store any personally identifying data in the session array as a consideration regarding the topic of "Session Hijacking". stackoverflow.com/a/42869960/2943403 Between my advice, KIKO's comprehensive answer, and YourCommonSense's comments -- I think we have you pretty well covered. \$\endgroup\$
    – mickmackusa
    Apr 26, 2019 at 12:23
1
\$\begingroup\$

I don't use $_SESSION super global array and don't know much about it, so please correct me if I'm wrong, isn't this code vulnerable to session fixation attacks ?

AFAIK the magic behind $_SESSION is a session id cookie, I think it's default name is PHPSESSID, and in your code when the user logs in you don't change this cookie! I did little test before I write this, and yep, PHP doesn't change this cookie :(

you need to change this

if (password_verify($inserted_password, $row['password'])) {
   //......
}

into something like this

if (password_verify($inserted_password, $row['password'])) {
   session_regenerate_id(true);
   //......
}

The idea is you must change the identification cookies when your clients login, no matter you use $_SERSSION or other way.

This is quoted from MDN

Session fixation should primarily be mitigated by regenerating session cookie values when the user authenticates (even if a cookie already exists) and by tieing any CSRF token to the user.

Improve this answer
\$\endgroup\$
2
  • 1
    \$\begingroup\$ The OP's script is not accepting session identifiers via url, so this is not a concern and does not need to be mitigated. webdigi.co.uk/blog/2009/php-session-fixation-attacks \$\endgroup\$
    – mickmackusa
    Apr 26, 2019 at 12:20
  • \$\begingroup\$ @mickmackusa thank you for the information, yes the most common and easiest to implement technique for fixing the session token on the victims computer is throw URLs, or form fields, which is not happening in the OP's code, however there are still other ways to fix the sessionID on the computer, (e.g: an XSS payload or simply by setting on the victims computer (while he is logged out) and change the cookie by hand). IMHO, I think not changing the session ID after log-ins is a bad practice in general. \$\endgroup\$
    – Accountant م
    Apr 26, 2019 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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