CODEWITHSUNDEEP
php
Latox
Latox

Reputation: 4705

Form Security & PHP?

I was using something like this:

<input type="hidden" name="ids" value="1, 3, 5" />
<input type="hidden" name="cost" value="350" />

But I was thinking, somebody could just change the cost to say "3" and pay for it for $3.00.. so I was thinking, would a securer(sp) option be setting these values into sessions when they load page, like so:

<?
unset($_SESSION['baskettotal']);
unset($_SESSION['basketids']);
$_SESSION['baskettotal'] = $grand;
$_SESSION['basketids'] = implode(", ", $ids);
?>
<input type="hidden" name="hash" value="<?=md5('stackoverflow'.$_SESSION['baskettotal'].$_SESSION['basketids']);?>" />
<?
if (($_POST['hash']) != (md5('stackoverflow'.$_SESSION['baskettotal'].$_SESSION['basketids']))){
    echo "error";
    die();
}
?>

Is this a good way of doing it? As they cannot edit the sessions, it's defined by whats in their basket, as opposed to storing it in hidden input fields which they could easily manipulate?

Upvotes: 0

Views: 194

Answers (8)

Your Common Sense
Your Common Sense

Reputation: 157989

Your initial code was okay, while further "improvement" makes not much sense.
I see no use for all that hidden hashes stuff.

Why not to just make it

$_SESSION['baskettotal'] = $grand;
$_SESSION['basketids'] = $ids;

and then get these 2 variables from the session upon POST submit. 

$grand = $_SESSION['baskettotal'];
$ids   = $_SESSION['basketids'];

That's ALL!
Why devise all that stuff, unnecessary complicating your code?

Upvotes: 0

quantme
quantme

Reputation: 3657

This is enough for me:

<?php 
session_start();
// thanks to bantam
// http://www.php.net/manual/en/function.mcrypt-encrypt.php#87526
define('SALT', 'whateveryouwant'); // use define, db stored value, var, included, etc.
function encrypt($text)
{
  return trim(base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, SALT, $text, MCRYPT_MODE_ECB, mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND))));
}

function decrypt($text) 
{ 
  return trim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, SALT, base64_decode($text), MCRYPT_MODE_ECB, mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND)));
}

unset($_SESSION['baskettotal']);
unset($_SESSION['basketids']);

$grand = 350;
$ids = array(1, 3, 5);

$_SESSION['baskettotal'] = $grand;
$_SESSION['basketids'] = implode(", ", $ids);

?>
<form method="post" action="<?php $_SERVER['PHP_SELF']?>" enctype="application/x-www-form-urlencoded" >
  <input type="hidden" name="enc" value="<?= encrypt('stackoverflow' . $_SESSION['baskettotal'] . $_SESSION['basketids'] );?>" />
  <input name="send" type="submit" value="Submit">
</form>
<?
if ( decrypt($_POST['enc']) !== 'stackoverflow' . $_SESSION['baskettotal'] . $_SESSION['basketids'] ) die('Not so fast');
else echo 'Go ahead';
?>

On first run you obtain:

Not so fast

When you click on submit button:

Go ahead

Hope it helps. Source

Upvotes: 0

Gaurav Porwal
Gaurav Porwal

Reputation: 513

I think your way is good. but when we are not showing that value on form then there is no need to match hidden field with session value. Do your calculation according to session variable value if someone edit hidden field value that does not effect your calculation..

Upvotes: 1

frostymarvelous
frostymarvelous

Reputation: 2805

If the form is being submitted to an external script. Then your session checks will be useless since you do not do the final data checking.

To be completely secure(or at least a semblance of it) store the prices server side with the IDs in a session, then on checkout, use curl to perform the external call after your final checks.

Upvotes: -1

mpen
mpen

Reputation: 283313

Why are you even storing the price in the form? That should be stored server-side only. You can display the price to the user, but you don't need to have them submit it back to you.

Upvotes: 0

HChen
HChen

Reputation: 2141

What about storing them in the session and not displaying them in the form? (Or just ignore them when you are doing your calculations).

Upvotes: 1

Mike Lewis
Mike Lewis

Reputation: 64177

You are indeed correct, this is a good method of doing this.

1) Create a session that contains the hash of all the values from that form.

2) When the form is submitted, calculate the hash of the values from the form and compare them against the one in the SESSION var. If they are the same, the user didn't change anything... if they differ, obviously the user changed something.

Just to clarify, the user cannot change any of the SESSION variables, however if you stored this in a cookie for example, the user could potentially edit the hash via that cookie (since it is stored client side, as opposed to server side with sessions).

An example might be:

$_SESSION['form_data'] = md5("randomsalt123".$formValue1.$formValue2);

and when the user submits the form:

if($_SESSION['form_data'] == md5('randomsalt123'.$_POST['form_value_1'].$_POST['form_value_2'])){
 // valid submission

} else {
 // invalid
}

Upvotes: 2

JLZenor
JLZenor

Reputation: 1480

There are ways to change the session variables as well. I would store the cost in a database, which the user cannot change, then either check $_POST['cost'] to make sure it matches what is in the database or do away with the hidden field altogether.

If you do not want to use a db you could store it in a file.

Upvotes: 1

Related Questions