PHP Form Key - Is this looking okay?

[GrumpyYoungMan]

Hello

Code: [Select]

function key_Validate() {
    //  Use the form_key from _SESSION and _POST:
    return $_POST['form_key'] == $_SESSION['form_key'];
}

function key_Output() {
    // Generate the key and store it inside the class
    $formKey = key_Generate();
    // Store the form key in the session
    $_SESSION['form_key'] = $formKey;

    //Output the form key
    echo '<input type="hidden" name="form_key" id="form_key" value="', $formKey, '">';
}

function key_Generate() {
    // Get the IP-address of the user
    $ip = $_SERVER['REMOTE_ADDR'];

    //We use mt_rand() instead of rand() because it is better for generating random numbers.
    //We use 'true' to get a longer string.
    //See http://www.php.net/mt_rand for a precise description of the function and more examples.
    $uniqid = uniqid(mt_rand(), true);

    //Return the hash
    return md5($ip . $uniqid);
}

Used to prevent form refreshing, is it secure though?

Based on the original code from here and updated.

[Jason Knight]

It's not horrible, but there are a few flubs and a few things I'd do diffferent.

Screwups:

The biggest is that it blindly trusts that the session value would even exist. An isEmpty or array_key_exists should be performed on the check routine.

Next it doesn't destroy the session key after verification. Keys should be "use once and discard" when verifying.

There is no reason for a hidden input to even have an ID.

md_5 and mt_rand is an outdated way of doing it. As of PHP 7 we have random_bytes which is more cryptographically secure, that we would bin2hex the output of.

Suggested Improvements

The key is hardcoded. It's possible that you might need more than one such key on a page such as if you have login, register, and content modals. To that end passing it a name to use as an index would help a lot.

You should probably add a means by which the key can expire.

I would not have the markup for said hidden input hardcoded into the routine. That's more of your template's job. Separations and all that.

I'd also have the key_create set it in the session.

Might also be good to be able to manually check a value instead of assuming $_POST.

More verbose error handling could be implemented using the "false on success, string on error" method. One of the many reasons I think shoehorning typecasting into PHP is actually dumb.

Something like; (might have typos, drive-by)

Code: [Select]

function sessionHashCreate($name, $length = 16, $expires = 900) {
if (empty($name)) die("invalid $name passed to sessionHashCreate");
$hashName = 'hash_' . $name;
$_SESSION[$hashName . 'Expires'] = time() + $expires;
return $_SESSION[$hashName] = bin2hex(random_bytes($length));
} // sessionHashCreate

function sessionHashInvalid($name, $value = false) {
if (empty($name)) return 'Invalid $name passed to sessionHashVerify';
$hashName = 'hash_' . $name;
if (!array_key_exists($hashName, $_SESSION)) return 'Invalid session hash';
if (!$value) {
if (!array_key_exists($name, $_POST)) return 'Hash name not found';
$value = $_POST[$name];
}
if ($_SESSION[$hashName] !== $value) return 'Session hash mismatch';
unset($_SESSION[$hashName]);
$expiresName = $hashName . 'Expires';
if (!array_key_exists($expiresName, $_SESSION)) return 'Session expiry missing';
$expires = $_SESSION[$expiresName]);
unset($_SESSION[$expiresName]);
if (time() > $expires) return 'Session Hash Expired';
return false; // our success state
} // sessionHashInvalid

In your template you'd do something like:

Code: [Select]

echo '
  <input
    type="hidden"
    name="loginVerify"
    value="', sessionHashCreate('loginVerify'), '"
>';

And to test you'd do:

Code: [Select]

if ($error = sessionHashInvalid('loginVerify')) {
  // handle the error here
} else {
  // hash was valid.
}

And yes, all those single = are in fact correct. Test on assignment. The types of coders who get their panties in a knot over that or things like short-circuit returns (which those numbnuts call "premature exit") can go teach their grandma to suck eggs.

Also notice I added the ability for the create method to optionally determine how many random bytes to make (returned string is twice that length, default is 16 bytes so 32 characters), and the expiry time in seconds. (default is 900 seconds / 15 minutes). Likewise you can override the value you are comparing to so you can use it with non-post as well.

-- edit -- been away from PHP for so long I forgot it's just "empty" and not "isempty"

[GrumpyYoungMan]

It's not horrible, but there are a few flubs and a few things I'd do diffferent.

Screwups:

The biggest is that it blindly trusts that the session value would even exist. An isEmpty or array_key_exists should be performed on the check routine.

Next it doesn't destroy the session key after verification. Keys should be "use once and discard" when verifying.

There is no reason for a hidden input to even have an ID.

md_5 and mt_rand is an outdated way of doing it. As of PHP 7 we have random_bytes which is more cryptographically secure, that we would bin2hex the output of.

Suggested Improvements

The key is hardcoded. It's possible that you might need more than one such key on a page such as if you have login, register, and content modals. To that end passing it a name to use as an index would help a lot.

You should probably add a means by which the key can expire.

I would not have the markup for said hidden input hardcoded into the routine. That's more of your template's job. Separations and all that.

I'd also have the key_create set it in the session.

Might also be good to be able to manually check a value instead of assuming $_POST.

More verbose error handling could be implemented using the "false on success, string on error" method. One of the many reasons I think shoehorning typecasting into PHP is actually dumb.

Something like; (might have typos, drive-by)

Code: [Select]

function sessionHashCreate($name, $length = 16, $expires = 900) {
if (empty($name)) die("invalid $name passed to sessionHashCreate");
$hashName = 'hash_' . $name;
$_SESSION[$hashName . 'Expires'] = time() + $expires;
return $_SESSION[$hashName] = bin2hex(random_bytes($length));
} // sessionHashCreate

function sessionHashInvalid($name, $value = false) {
if (empty($name)) return 'Invalid $name passed to sessionHashVerify';
$hashName = 'hash_' . $name;
if (!array_key_exists($hashName, $_SESSION)) return 'Invalid session hash';
if (!$value) {
if (!array_key_exists($name, $_POST)) return 'Hash name not found';
$value = $_POST[$name];
}
if ($_SESSION[$hashName] !== $value) return 'Session hash mismatch';
unset($_SESSION[$hashName]);
$expiresName = $hashName . 'Expires';
if (!array_key_exists($expiresName, $_SESSION)) return 'Session expiry missing';
$expires = $_SESSION[$expiresName]);
unset($_SESSION[$expiresName]);
if (time() > $expires) return 'Session Hash Expired';
return false; // our success state
} // sessionHashInvalid

In your template you'd do something like:

Code: [Select]

echo '
  <input
    type="hidden"
    name="loginVerify"
    value="', sessionHashCreate('loginVerify'), '"
>';

And to test you'd do:

Code: [Select]

if ($error = sessionHashInvalid('loginVerify')) {
  // handle the error here
} else {
  // hash was valid.
}

And yes, all those single = are in fact correct. Test on assignment. The types of coders who get their panties in a knot over that or things like short-circuit returns (which those numbnuts call "premature exit") can go teach their grandma to suck eggs.

Also notice I added the ability for the create method to optionally determine how many random bytes to make (returned string is twice that length, default is 16 bytes so 32 characters), and the expiry time in seconds. (default is 900 seconds / 15 minutes). Likewise you can override the value you are comparing to so you can use it with non-post as well.

-- edit -- been away from PHP for so long I forgot it's just "empty" and not "isempty"

Thank you, I wasn’t expecting that answer from you!

I’ll post back soon!

Many Thanks!

[GrumpyYoungMan]

Because if I just copy and paste your code, I would learn nothing, so I tried to use your code as an example and then build up what I understand, so this is what I have come up with (so far..)

Key Functions:

Code: [Select]

function key_Validate($name) {
    //  Use the key from _SESSION and _POST:

    // Key: Sent
    if(!array_key_exists('key_'.$name, $_POST))
        return 'No key detected';

    // Key: Expired
    if(time() > $_SESSION['keys'][$name]['expiry'])
        return 'Key Expired!';

    // Key: Match
    if($_POST['key_'.$name] !== $_SESSION['keys'][$name]['key'])
        return 'Key mis-match detected.';

    // Key: Destroy.
    unset($_SESSION['keys'][$name]['key']);

    return false;
}

function key_Output($name) {
    // Generate the key and store it:
    $key = bin2hex(random_bytes(16));

    // Store the token in the session and give it a unique name
    $_SESSION['keys'][$name] = [ 'key' => $key, 'expiry' => (time()+(60*15))];

    // Output the key
    return $key;
}

Validation Test:

Code: [Select]

if ($keyError = key_Validate('contact'))
        $errors[] = $keyError;

Template:

Code: [Select]

<input
        type="hidden"
        name="key_contact"
        value="', key_Output("contact"), '"
    >


[GrumpyYoungMan]

Slightly updated code:

Code: [Select]

function validateKey($name) {
    //  Use the key from _SESSION and _POST:
    // Key: System Check?
    if (
        !array_key_exists('key_'.$name, $_POST) ||
        !array_key_exists($name, $_SESSION['keys'])
    )
        return 'No valid keys detected!';

    // Key: Expired?
    if (
        time() >
        $_SESSION['keys'][$name]['expiry']
    )
        return 'Key Expired!';

    // Key: Match?
    if (
        $_POST['key_'.$name] !==
        $_SESSION['keys'][$name]['key']
    )
        return 'Key mis-match detected.';

    // Key: Destroy.
    unset($_SESSION['keys'][$name]);

    return false;
}

function generateKey($name) {
    // Generate the key and store it:
    $key = bin2hex(random_bytes(16));

    // Store the token in the session and give it a unique name
    $_SESSION['keys'][$name] = [ 'key' => $key, 'expiry' => (time()+(60*15))];

    // Output the key
    return $key;
}
Basically added a check to make sure the keys exist as it was causing an warning... (this was pointed out by Jason, but I missed it originally)