Simple password protection in PHP

Question

I was reading a quite interesting chapter in a "Learning PHP"-book of mine and came across a code example which I wanted to modify and use on my personal website (to protect a simple document, nothing "big", which is why I also don't encrypt the passwords).

I've used the php-sample and I just can't make it work at all.
Here it is (dont get scared by the length, it's really simple):

<?php

if ($_POST['_submit_check']) {
    if ($form_errors = validate_form()) {
        show_form($form_errors);
    } else {
        process_form();
    }
} else {
    show_form();
}

function show_form($errors = '') {
    echo '<form method="POST" action="' . $_SERVER['PHP_SELF'] . '">';

    if ($errors) {
        echo '<br>'
        echo implode('<br>', $errors);
        echo '<br>';
    }

    echo 'Username: ';
    echo '<input type="text" name="username" value="Username goes here">';
    echo '<br>'

    echo 'Password: ';
    echo '<input type="password" name="password">';
    echo '<br>'

    echo '<input type="submit" name="submit" value="Log In">';
    echo '<input type="hidden" name="_submit_check" value="1">'; //when the form is entered, this returns true and the first line of the document is good to go

    echo '</form>';
}

function validate_form() {
    $errors = array();

    $users = array('admin' => 'pass123',
                   'notsoadmin' => 'pass1234');


    if (!array_key_exists($_POST['username']) {
        $errors[] = "Please enter username and password";
    }

    $saved_password = $users[ $_POST['password'] ];
    if ($saved_password != $_POST['password']) {
        echo "Password and username don't match. Please try again";
    }

    return $errors;
}

function process_form() {
    $_SESSION['username'] = $_POST['username'];

    echo "Welcome, $_SESSION[username]";
}

?>

Before my HTML and stuff I also added this:

<?php session_start(); ?>

Clearly I've missed something... Maybe it's $form_errors right at the beginning, that causes the problem (which is "nothing happens"), it was in my book but I'm not sure why/where it comes from?

Answer 1

During validation you should be a little more explict to what you are checking. Always avoid if ($variable) and instead use a function (isset/empty/etc) to check the state of the variable.

if ($_POST['_submit_check']) {
    if ($form_errors = validate_form()) { //always returns an array so will evaluate to true
        show_form($form_errors);
    } else {
        process_form();
    }
} else {
    show_form();
}

//change to

if (isset($_POST['_submit_check'])) {
    $form_errors = validate_form();
    if (!empty($form_errors)) {
        show_form($form_errors);
    } else {
        process_form();
    }
} else {
    show_form();
}

Answer 2

Shouldn't...

    $saved_password = $users[ $_POST['password'] ];
    if ($saved_password != $_POST['password']) {
        ...
    }

actually be..

    $saved_password = $users[ $_POST['username'] ];
    if ($saved_password != $_POST['password']) {
        ...
    }

i.e. you should be looking for username entry in $users not the password

By the way it's really bad practice to store raw passwords like that. Consider HASHing and SALTing them.

Check this question out for information

Answer 3

$saved_password = $users[ $_POST['password'] ];
if ($saved_password != $_POST['password']) {
    echo "Password and username don't match. Please try again";
}

I think the above will not work because $users is an array of username => password. You need to check for the username key:

$saved_password = $users[$_POST['username']] ;

Answer 4

Being the security freak that I am, it would also be a good idea to use hashing to protect your script from being cracked through a vulnerability in your server. Consider something like the sha1() hash; it's very fast and safe.