2
\$\begingroup\$

I've prepared a simple login form, using prepared statements to prevent SQL injections. How secure is this ? 

    <head>
    <title>Login</title>
        <?php
        session_start();

        // set up a token
        $form_token = md5(uniqid('auth', true));

        // session form token
        $_SESSION['form_token'] = $form_token;

        if (isset($_SESSION['user_id'])) {
            print "You're already logged in";
        }
    ?>
</head>
<body>
    <h3>Login!</h3>
    <?php
    if (isset($_SESSION['user_id'])) {
        print "<a href='logout.php'>Log Out Here</a>";
        exit;
    }
    ?>
    <form action="login.php" method="post">
        <p>Username :</p>
        <input type="text" name="username" maxlength="20">

        <p>Password :</p>
        <input type="text" name="pswd" maxlength="20">
        <input type="hidden" name="form_token" value="<?php echo $form_token; ?>" />

        <input type="submit" value="Login">
    </form>

    </body>

Nothing too special on the form.. Now, the PHP that process the data: 

    <head>
    <title>Login</title>

    <?php
    session_start();
    $message = "";

    if (isset($_SESSION['user_id'])) {
        $message .= "You're already logged in<br/>";
        print $message;
        print "<a href='logout.php'>Log Out Here</a>";
        exit;
    }

    // 
    // GET THE USER DATA
    // 
    $username = $_POST['username'];
    $pswd = $_POST['pswd'];
    $form_token = $_POST['form_token'];

    // 
    // CHECK THE DAATA
    // 

    if (empty($username) || empty($pswd) || !isset($form_token)) {
        $message .= "Please enter a valid username/password <br/>";
    }
    elseif ($form_token != $_SESSION['form_token']) {
        $message .= "Invalid Form Submission <br/>";
    }
    elseif (strlen($username) > 20 || strlen($username) < 4) {
        $message .= "Invalid length for a username <br/>";
    }
    elseif (strlen($pswd) > 20 || strlen($username) < 4) {
        $message .= "Invalid length for a username <br/>";
    }
    else {

        // encrypting the password

    $pswd = sha1($pswd);

        // *********************
        // CONNECTING TO THE DATABASE
        // *********************

    $servername = "localhost";
    $usrname = "root";
    $password = "";
    $db_name = "auth";

    // connect to the database
    $conn = new mysqli($servername, $usrname, $password, $db_name);
    if ($conn->connect_error) {
        die("Connection failed".$conn->connect_error);
    }

    // statement
    $stmnt = $conn->prepare("SELECT username, pswd FROM users WHERE username = ? AND pswd = ?");
    $stmnt->bind_param('ss', $username, $pswd);

    $stmnt->execute();

    $result = $stmnt->get_result();
    if ($result->num_rows > 0) {
            $user_id = $result;
            $_SESSION['user_id'] = $result;
            $message .= "Succesfully Logged in!";
    }
    else {
        $message .= "Failed to Log In. Invalid Username or Password";
    }

    }

    ?>

</head>
<body>

<?php
    print $message;

?>

</body>
\$\endgroup\$
5
  • \$\begingroup\$ Aren't you suppose to be getting header already sent error due to outputting HTML before session_start()? \$\endgroup\$
    – Marco Aurélio Deleu
    Commented Nov 18, 2015 at 11:44
  • \$\begingroup\$ By virtue of using prepared statements and binding the params you are guarding against SQL injection attacks. \$\endgroup\$
    – Greg Burghardt
    Commented Nov 18, 2015 at 15:46
  • \$\begingroup\$ @MarcoAurélioDeleu uh no? It's in the head tag \$\endgroup\$
    – candh
    Commented Nov 18, 2015 at 16:27
  • \$\begingroup\$ @GregBurghardt yeah.. But rate it!. 10/6 maybe ? \$\endgroup\$
    – candh
    Commented Nov 18, 2015 at 16:34
  • \$\begingroup\$ Head tag does not protect you from sending http headers - output buffering does. You rely on server configuration here and it's not a good programming. \$\endgroup\$
    – shudder
    Commented Dec 30, 2015 at 2:39

1 Answer 1

Reset to default
1
\$\begingroup\$

I'll point out a few things here:

  1. $pswd = sha1($pswd); is cringeworthy. Please use password_hash() if you can. If you can't update your PHP version to comply, my next best suggestion is to use crypt().

  2. Having all of your DB connection code on the page is generally considered poor coding. Place everything between $servername ... // statement in a separate file and require it.

  3. In your select statement, there is no reason to retrieve the password from the DB alongside the username. The less you deal with the password, most likely will be safer. Also, you're putting the hashed password in the session, which I'm not sure is the best idea. A phishing attempt could potentially reveal this and a malicious user could take advantage of this information.

  4. Your method of creating a session token is weak, and I suggest you check out this StackOverflow post for more info.

  5. Format your code. You're missing a lot of indentation and general style rules aren't 100% met.

\$\endgroup\$
3
  • \$\begingroup\$ Very good points. Thanks for guiding a newbie. But, I wanna ask this : Why using sha1 is a bad idea. Most people on the Internet are using it. Thanks again, I will definitely implement these points into my code :) \$\endgroup\$
    – candh
    Commented Dec 30, 2015 at 5:28
  • 1
    \$\begingroup\$ Thanks @candh, to answer your question, visit my first link: "With modern techniques and computer equipment, it has become trivial to "brute force" the output of [sha1], in order to determine the original input." \$\endgroup\$
    – Alex L
    Commented Dec 30, 2015 at 5:30
  • \$\begingroup\$ Hey @candh if you're satisfied with an answer, consider Accepting it with the checkbox next to the answer. This helps clear up what's going on here on Code Review! :) \$\endgroup\$
    – Alex L
    Commented Jan 3, 2016 at 0:05

Your Answer

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

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