PHP registration/login form

Question

I'm fairly new to MySQLi and PHP, but I've been working at it for a bit now, and reading up on how to make secure login forms, avoid SQL injection etc. I'm a mere amateur though.

I've created an index.html page (not including the code here, just a basic form though), a register.html page (not including the code here, again it's just a basic form), adduser.php (which gets its call from "action" on the registration form),login.php (obviously gets its "action" from the index.html login form), logout.php, and a restricted.php with a check if session is active.

Everything is working. I'm just looking for some expert eyes to take a gander at my code and let me know if I'm missing anything whether it be major or minor, any security flaws, etc. Currently I don't have any form validation as I'm thinking of doing it client side with jQuery.

adduser.php

<?php

error_reporting(E_ALL);
session_start();
$mysqli = new mysqli ('localhost', '***', '***', 'test');

if($mysqli->connect_errno > 0) {
    die('Unable to connect to database [' . $mysqli->connect_error . ']');
}


if(isset($_POST['submit'])) {

    $errors = array();
    $data = array();
    $fname = $_POST['fname'];
    $lname = $_POST['lname'];
    $username = $_POST['username'];
    $email = $_POST['email'];
    $password = $_POST['password'];
    $confpass = $_POST['confpass'];
    $password_hash = password_hash($password, PASSWORD_DEFAULT);    

    if(!($stmt = $mysqli->prepare("INSERT INTO users (fname, lname, username, email, password) 
        VALUES (?,?,?,?,?)"))){
        echo "Prepare failed: (" . $mysqli->errno . ")" . $mysqli->error;
    }

    if(!$stmt->bind_param('sssss', $fname, $lname, $username, $email, $password_hash)){
     echo "Binding paramaters failed:(" . $stmt->errno . ")" . $stmt->error;
    }

    if(!$stmt->execute()){
     echo "Execute failed: (" . $stmt->errno .")" . $stmt->error;
    }

    if($stmt) {
        header('Location: index.html#testform')

    }
    else{
        echo "Registration failed";
    }

}

$mysqli->close();

login.php

<?php
error_reporting(E_ALL);

$mysqli = new mysqli ('localhost', '***', '***', 'test');

if($mysqli->connect_errno > 0) {
    die('Unable to connect to database [' . $mysqli->connect_error . ']');
}
ob_start();
session_start();

    $username = $_POST['username'];
    $password = $_POST['password'];
if(isset($_POST['submit'])) {

    if(!($stmt = $mysqli->prepare("SELECT username, password FROM users WHERE username = ?"))){
            echo "Prepare failed: (" . $mysqli->errno . ")" . $mysqli->error;
    }




    if(!$stmt->bind_param('s', $username)){
        echo "Bind failed: (" . $stmt->errno . ")" . $stmt->error;
    }

    if(!$stmt->execute()){
     echo "Execute failed: (" . $stmt->errno .")" . $stmt->error;
    }

    $userdata = $stmt->get_result();
    $row = $userdata->fetch_array(MYSQLI_ASSOC);



    $stmt->bind_result($username, $password);
    $stmt->store_result();

             if(password_verify($password, $row['password'])){

            $_SESSION['user'] = $_POST['username'];
            header('Location: restricted.php');
            exit();
        }


    else{
        echo "Login Failed: (" . $stmt->errno .")" . $stmt->error;
    }
$stmt->close();

}

$mysqli->close();

restricted.php (just the top session code, the rest of the page is useless for the sake of this post)

<?php
error_reporting(E_ALL);
ob_start();
session_start();
$db = new mysqli ('localhost', '***', '***', 'test');

if($db->connect_errno > 0) {
    die('Unable to connect to database [' . $db->connect_error . ']');
}
if (!isset($_SESSION['user'])) {
    header('Location: index.html');
}


?>

logout.php

<?php
session_start();
if(session_destroy()) // Destroying All Sessions
{
header("Location: index.html"); // Redirecting To Home Page
}
?>

Consider using PHP Data Objects instead of database-specific calls. — Dave Jarvis
– Dave Jarvis, Commented Jan 24, 2015 at 2:26
@DaveJarvis I was looking at whether or not to do PDO or MySQLi, but in my readings I found that MySQLi is faster, and if you're not using different databases, that PDO isn't necessarily needed. From what I read as well for someone who is fairly amateur that PDO is more difficult to learn right away. What advantages would I have to using PDO opposed to MySQLi? — Alan Hill
– Alan Hill, Commented Jan 24, 2015 at 2:30
@DaveJarvis I've read through that article, and many others. "The core advantage of PDO over MySQLi is in its database driver support. At the time of this writing, PDO supports 12 different drivers, opposed to MySQLi, which supports MySQL only." That's a core advantage that I don't need currently, as I'm only working with MySQL databases. "While both PDO and MySQLi are quite fast, MySQLi performs insignificantly faster in benchmarks - ~2.5% for non-prepared statements, and ~6.5% for prepared ones." Benefit is MySQLi is quicker. Also easier to learn for beginner. It seems like devs are 50/50. — Alan Hill
– Alan Hill, Commented Jan 24, 2015 at 4:28
I'm just not sure what the real advantage is to using PDO if you don't need to use multiple databases. I understand the advantages, I'm just curious as to why you believe it would be necessary to use it. Client side prepared statements, multiple database support, and named paramaters seem to be the main advantages of PDO. But for a small scale database with few users, on a small scale website with < 500 visitors a week, I don't quite understand the need for it. It's something I'd definitely like to learn, but as a beginner would it not be better to stick to MySQLi until I'm more familiar? — Alan Hill
– Alan Hill, Commented Jan 24, 2015 at 4:32
@AlanHill I agree with Dave here, learning PDO would probably be better in the long run. It's much more versatile and you'll have a better knowledge of things afterwards. Plus, it is a good way to get introduced to advanced OOP principles in PHP. — Alex L
– Alex L, Commented Jan 24, 2015 at 5:57

tim · Accepted Answer · 2015-05-26 12:45:51Z

Security

First of, your code looks pretty good, especially from a security point of view. You use prepared statements, and you use them correctly, and you use bcrypt instead of some weaker hashing algorithm such as md5 or sha.

There are a couple of things which might cause problems under certain conditions, and as defense in depth it might be a good idea to include them:

always die after a redirect, because the redirect does not necessarily stop execution of the PHP script (you can easily test this yourself: header('Location: index.html'); echo "secret"; using curl: curl localhost/redirect.php would give you secret, because curl doesn't follow redirects by default).
you have new mysqli ([credentials]) in at least two places. This is not only bad because duplication makes code harder to read and maintain, with credentials it is especially bad because it makes it harder to remember to hide them (when posting code somewhere else, submitting it in version control, etc). It also makes it harder to move those PHP files that do contain credentials outside the web root. Just put the database connection in one file (located outside the web root) and wrap it in a getConnection function (this also makes it possible to use the same database connection for all queries, which improves performance).
with current default PHP.ini settings, this is less important, but it is still always a good idea to regenerate the session id when the state of the session changes (eg when logging in) to prevent session fixation.
as @sjonniesjon said, don't echo actual error messages to the user. It can directly enable some attacks such SQL injections into some keywords (such as insert, update, order by, etc) which only display data in error messages, and it can help an attacker identify vulnerabilities.

Misc

your indentation is sometimes off, leading to harder to read code. Your brackets are also placed inconsistently.
you also have too many newlines in some places.

sjonniesjon · Accepted Answer · 2015-01-24 12:29:46Z

I suggest you sanitize your data server side as well. JavaScript can easily be bypassed. PDO does the escaping which you don't have to worry about, but when a user adds a string which is too long it will be automatically cut off for example (when mysql is in strict mode it will throw an exception). This can corrupt your data. Also one of the first security rules is: never trust user input! So better be safe than sorry ;)

Also, don't use error reporting on your live site (can be disabled in Php.INI or the error reporting command you already use) when the above described exception occurs for example, al lot of database info is shown to the end user. Which could show all possible info needed to acquire access to your database. Plus it looks like complete gibberish to a regular user.

Hope this helps :)

Stack Exchange Network

PHP registration/login form

2 Answers 2

You must log in to answer this question.

Hot Network Questions

PHP registration/login form

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions