2
\$\begingroup\$

I've developed my shopping cart further. This is related to my earlier post: Securing PHP shopping cart

In addition to those functionalities, I've proceeded to make integration to Paytrail payment gateway. This post will add option to handle customer's personal data and to send that data to payment gateway. All credit card info/payments are handled by Paytrail on their servers.

Merchant id/secret are Paytrail's test credentials, if anyone thinks, I'm revealing those accidentally.

My questions are the following:

  1. Is my output data being visible in pure HTML considered insecure? This includes an MD5 hash, which is calculated based on shopping cart items/user input data (output.html).
    1. If this is insecure, then how should I proceed with changing my code, such that it'll be less likely to be abused?
  2. Are there any vulnerabilities on handling customer input data (addresses, names etc.)?
  3. Also, please state if your improvements/changes are purely an alternate way of doing things or are they crucial changes.

This script is free to use for anyone who wishes to make their own integration with Paytrail gateway.

test_input

function test_input($data)
{
   $data = trim($data);
   $data = htmlspecialchars($data);
   return $data;
}

order.php

<?php 

if ($_SERVER["REQUEST_METHOD"] == "POST") {

$error = false;

// CUSTOMER ADDRESS VARIABLES
$firstname = 
$lastname = 
$address = 
$postnumber = 
$city = 
$country = 
$company = 
$homenumber = 
$worknumber = 
$email = "";


// PAYMENT DATA VARIABLES
$merchant_secret = '6pKF4jkv97zmqBJ3ZL8gUw5DfT2NMQ';
$merchant_id = '13466';
$order_number = '123456';
$reference_number = '';
$order_description = 'Testitilaus';
$currency = 'EUR';
$return_address = 'http://www.esimerkki.fi/success';
$cancel_address = 'http://www.esimerkki.fi/cancel';
$pending_address = '';
$notify_address = 'http://www.esimerkki.fi/notify';
$type = 'E1';
$culture = 'fi_FI';
$preselected_method = '';
$mode = '1';
$visible_methods = '';
$group = '';

// CART ITEMS DATA  
$vat = 0;
$cart_tax = 0;
$cart_discount = 0;
$cart_type = 1;
$items_data = '';

$count = 0;
foreach($_SESSION['cart']['id'] as $key => $value) {
        $count = count($_SESSION['cart']['id']);
}

## FORM VALIDATION BEGING ##

// VALIDATE FIRSTNAME   
        if (empty($_POST['firstname'])) {$error = true;}
        $firstname = test_input($_POST['firstname']);
        if(!preg_match("/[a-zA-Z]/u", $firstname)) {$error = true;}

// VALIDATE LASTNAME
        if (empty($_POST['lastname'])) {$error = true;}
        $lastname = test_input($_POST['lastname']);
        if(!preg_match("/[a-zA-Z]/u", $lastname)) {$error = true;}

// VALIDATE ADDRESS
        if (empty($_POST['address'])) {$error = true;}
        $address = test_input($_POST['address']);
        if(!preg_match("/[a-zA-Z]/u", $address)) {$error = true;}

// VALIDATE POSTNUMBER
        $postnumber = filter_input(INPUT_POST, 'postnumber', FILTER_SANITIZE_NUMBER_INT);
        if (empty($_POST['postnumber'])) { $error = true; }

// VALIDATE CITY
        if (empty($_POST['city'])) {$error = true;}
        $city = test_input($_POST['city']);
        if(!preg_match("/[a-zA-Z]/u", $city)) {$error = true;}

// VALIDATE COUNTRY
        if (empty($_POST['country'])) {$error = true;}
        $country = test_input($_POST['country']);
        if(!preg_match("/[a-zA-Z]/u", $country)) {$error = true;}

// VALIDATE COMPANY (OPTIONAL)
        if(isset($_POST['company']) && !empty($_POST['company'])) {
            if(empty($_POST['company'])) {$error = true;}
            $company = test_input($_POST['company']);
            if(!preg_match("/[a-zA-Z]/u", $company)) {$error = true;}
        }

// VALIDATE HOMENUMBER (OPTIONAL)
        if(isset($_POST['homenumber']) && !empty($_POST['homenumber'])) {
            if (empty($_POST['homenumber'])) {$error = true;}
            $homenumber = test_input($_POST['homenumber']);
            if(!preg_match("/^[\+0-9\-\(\)\s]*$/", $homenumber)) {$error = true;}
        }

// VALIDATE WORKNUMBER (OPTIONAL)
        if(isset($_POST['worknumber']) && !empty($_POST['worknumber'])) {
            if (empty($_POST['worknumber'])) {$error = true;}
            $worknumber = test_input($_POST['worknumber']);
            if(!preg_match("/^[\+0-9\-\(\)\s]*$/", $worknumber)) {$error = true;}
        }

// VALIDATE EMAIL
            if (empty($_POST["email"])) {$error = true;}
            $email = test_input($_POST["email"]);
            if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {$error = true;}
    }

if(!empty($error)) {echo "Something fishy is going on.."; exit;}

## FORM VALIDATION ENDS ##

//CREATE STRINGS FROM CUSTOMER AND PAYMENT DATA
$customer = $homenumber. '|' .$worknumber. '|' .$email. '|' .$firstname.   '|' .$lastname. '|' .$company. '|' .$address. '|' .$postnumber. '|' .$city. '|' .$country. '|' .$vat. '|' .$count. '|';
$payment_data = $merchant_secret. '|' .$merchant_id. '|' .$order_number. '|' .$reference_number. '|' .$order_description. '|' .$currency. '|' .$return_address. '|' .$cancel_address. '|' .$pending_address. '|' .$notify_address. '|' .$type. '|' .$culture. '|' .$preselected_method. '|' .$mode. '|' .$visible_methods. '|' .$group. '|' ;   

// PAYMENT DATA
echo "<form action=\"https://payment.paytrail.com/\" method=\"post\" id=\"payment\">";
echo "<input name=\"MERCHANT_ID\" type=\"hidden\" value=\"{$merchant_id}\">";
echo "<input name=\"ORDER_NUMBER\" type=\"hidden\" value=\"{$order_number}\">";
echo "<input name=\"REFERENCE_NUMBER\" type=\"hidden\" value=\"{$reference_number}\">";
echo "<input name=\"ORDER_DESCRIPTION\" type=\"hidden\" value=\"{$order_description}\">";
echo "<input name=\"CURRENCY\" type=\"hidden\" value=\"{$currency}\">";
echo "<input name=\"RETURN_ADDRESS\" type=\"hidden\" value=\"{$return_address}\">";
echo "<input name=\"CANCEL_ADDRESS\" type=\"hidden\" value=\"{$cancel_address}\">";
echo "<input name=\"PENDING_ADDRESS\" type=\"hidden\" value=\"{$pending_address}\">";
echo "<input name=\"NOTIFY_ADDRESS\" type=\"hidden\" value=\"{$notify_address}\">";
echo "<input name=\"TYPE\" type=\"hidden\" value=\"{$type}\">";
echo "<input name=\"CULTURE\" type=\"hidden\" value=\"{$culture}\">";
echo "<input name=\"PRESELECTED_METHOD\" type=\"hidden\" value=\"{$preselected_method}\">";
echo "<input name=\"MODE\" type=\"hidden\" value=\"{$mode}\">";
echo "<input name=\"VISIBLE_METHODS\" type=\"hidden\" value=\"{$visible_methods}\">";
echo "<input name=\"GROUP\" type=\"hidden\" value=\"{$group}\">";

// CUSTOMER ADDRESS
echo "<input name=\"CONTACT_TELNO\" type=\"hidden\" value=\"{$homenumber}\">";
echo "<input name=\"CONTACT_CELLNO\" type=\"hidden\" value=\"{$worknumber}\">";
echo "<input name=\"CONTACT_EMAIL\" type=\"hidden\" value=\"{$email}\">";
echo "<input name=\"CONTACT_FIRSTNAME\" type=\"hidden\" value=\"{$firstname}\">";
echo "<input name=\"CONTACT_LASTNAME\" type=\"hidden\" value=\"{$lastname}\">";
echo "<input name=\"CONTACT_COMPANY\" type=\"hidden\" value=\"{$company}\">";
echo "<input name=\"CONTACT_ADDR_STREET\" type=\"hidden\" value=\"{$address}\">";
echo "<input name=\"CONTACT_ADDR_ZIP\" type=\"hidden\" value=\"{$postnumber}\">";
echo "<input name=\"CONTACT_ADDR_CITY\" type=\"hidden\" value=\"{$city}\">";
echo "<input name=\"CONTACT_ADDR_COUNTRY\" type=\"hidden\" value=\"{$country}\">";

// CART ITEMS DATA
echo "<input name=\"INCLUDE_VAT\" type=\"hidden\" value=\"{$vat}\" />";
echo "<input name=\"ITEMS\" type=\"hidden\" value=\"{$count}\">";

$i = -1;

foreach ($_SESSION['cart']['id'] as $key => $value) {

    $i++;
    $cart_title = $_SESSION['cart']['name'][$key];
    $cart_id = $i;
    $cart_quantity = $_SESSION['cart']['quantity'][$key];

    $cart_price = $_SESSION['cart']['price'][$key];
    $cart_pricedot = str_replace(',' , '.' , $cart_price);
    $cart_pricedot_trim = trim($cart_pricedot);

    echo "<input name=\"ITEM_TITLE[$i]\" type=\"hidden\" value=\"{$cart_title}\">";         
    echo "<input name=\"ITEM_NO[$i]\" type=\"hidden\" value=\"{$i}\">"; 
    echo "<input name=\"ITEM_AMOUNT[$i]\" type=\"hidden\" value=\"{$cart_quantity}\">"; 
    echo "<input name=\"ITEM_PRICE[$i]\" type=\"hidden\" value=\"{$cart_pricedot_trim}\">"; 
    echo "<input name=\"ITEM_TAX[$i]\" type=\"hidden\" value=\"{$cart_tax}\">"; 
    echo "<input name=\"ITEM_DISCOUNT[$i]\" type=\"hidden\" value=\"{$cart_discount}\">";   
    echo "<input name=\"ITEM_TYPE[$i]\" type=\"hidden\" value=\"{$cart_type}\">";

    $items_data.="$cart_title|$i|$cart_quantity|$cart_pricedot_trim|$cart_tax|$cart_discount|$cart_type|";
}

// START CALCULATING PAYMENT MD5 HASH
$combined = $payment_data. '' .$customer. '' .$items_data;
$combinedsub = substr($combined, 0, -1);
$code = strtoupper(md5($combinedsub));
$trimmeddata = trim($code);

echo "<input name=\"AUTHCODE\" type=\"hidden\" value=\"{$trimmeddata}\">";
echo "<input type=\"submit\" value=\"Siirry maksamaan\">";
echo "</form>";
?>

output.html

    <form action="https://payment.paytrail.com/" method="post" id="payment">
    <input name="MERCHANT_ID" type="hidden" value="13466">
    <input name="ORDER_NUMBER" type="hidden" value="123456">
    <input name="REFERENCE_NUMBER" type="hidden" value="">
    <input name="ORDER_DESCRIPTION" type="hidden" value="Testitilaus">
    <input name="CURRENCY" type="hidden" value="EUR">
    <input name="RETURN_ADDRESS" type="hidden" value="http://www.esimerkki.fi/success">
    <input name="CANCEL_ADDRESS" type="hidden" value="http://www.esimerkki.fi/cancel">
    <input name="PENDING_ADDRESS" type="hidden" value="">
    <input name="NOTIFY_ADDRESS" type="hidden" value="http://www.esimerkki.fi/notify">
    <input name="TYPE" type="hidden" value="E1">
    <input name="CULTURE" type="hidden" value="fi_FI">
    <input name="PRESELECTED_METHOD" type="hidden" value="">
    <input name="MODE" type="hidden" value="1">
    <input name="VISIBLE_METHODS" type="hidden" value="">
    <input name="GROUP" type="hidden" value="">
    <input name="CONTACT_TELNO" type="hidden" value="+5747 5884 7574543">
    <input name="CONTACT_CELLNO" type="hidden" value="0060 55574645">
    <input name="CONTACT_EMAIL" type="hidden" value="esupridf@oooooo.com">
    <input name="CONTACT_FIRSTNAME" type="hidden" value="Özil">
    <input name="CONTACT_LASTNAME" type="hidden" value="Älgebrä">
    <input name="CONTACT_COMPANY" type="hidden" value="Company Ot">
    <input name="CONTACT_ADDR_STREET" type="hidden" value="Kärkel 34">
    <input name="CONTACT_ADDR_ZIP" type="hidden" value="00000">
    <input name="CONTACT_ADDR_CITY" type="hidden" value="Äläbämä">
    <input name="CONTACT_ADDR_COUNTRY" type="hidden" value="FI">
    <input name="INCLUDE_VAT" type="hidden" value="0" />
    <input name="ITEMS" type="hidden" value="2">
    <input name="ITEM_TITLE[0]" type="hidden" value="Lasikengät">
    <input name="ITEM_NO[0]" type="hidden" value="0">
    <input name="ITEM_AMOUNT[0]" type="hidden" value="45">
    <input name="ITEM_PRICE[0]" type="hidden" value="23.43">
    <input name="ITEM_TAX[0]" type="hidden" value="0">
    <input name="ITEM_DISCOUNT[0]" type="hidden" value="0">
    <input name="ITEM_TYPE[0]" type="hidden" value="1">
    <input name="ITEM_TITLE[1]" type="hidden" value="Nahkakengät">
    <input name="ITEM_NO[1]" type="hidden" value="1">
    <input name="ITEM_AMOUNT[1]" type="hidden" value="23">
    <input name="ITEM_PRICE[1]" type="hidden" value="564.44">
    <input name="ITEM_TAX[1]" type="hidden" value="0">
    <input name="ITEM_DISCOUNT[1]" type="hidden" value="0">
    <input name="ITEM_TYPE[1]" type="hidden" value="1">
    <input name="AUTHCODE" type="hidden" value="958C104FA7522E0319214C3AE1147351">
    <input type="submit" value="Siirry maksamaan">
</form>
\$\endgroup\$
5
\$\begingroup\$

As one of your previous answer-ers, I can say this has definitely improved!

I'll work through by the type this time.


Variables:

Your beginning (block) variables are defined like $firstname, but the secondary block variables are defined like $merchant_secret, with an underline instead of a no-space. I, personally, would suggest using underlines for readability, but, if you do choose the first, keep the coding style the same.

It seems like the rest of your script fluctuates too between underline(s) and no space(s), so please do keep the coding style the same.

Below, you initialise $count as zero, and proceed to go through every item in the cart, overriding $count with the same value everytime.

$count = 0;
foreach($_SESSION['cart']['id'] as $key => $value) {
        $count = count($_SESSION['cart']['id']);
}

Just use: $count = count($_SESSION['cart']['id']).

This line here:

if(!empty($error)) {echo "Something fishy is going on.."; exit;}

While empty() works, it's pointless, because they're booleans. So, you can remove empty() and it'll work the exact same.

You then proceed to 'CREATE STRINGS [sic] FROM CUSTOMER AND PAYMENT DATA', and just like the fact I won't comment on why sentences should not become SQL or your usage of plurality, I won't comment on why you concatenate a bunch of strings to make a secure MD5 hash. (It'd certainly be a question I'd vote up on Security.SE, though!)

My beef is (again) with your coding style:

$customer = $homenumber. '|' .$worknumber. '|' .$email. '|' .$firstname.   '|' .$lastname. '|' .$company. '|' .$address. '|' .$postnumber. '|' .$city. '|' .$country. '|' .$vat. '|' .$count. '|';
$payment_data = $merchant_secret. '|' .$merchant_id. '|' .$order_number. '|' .$reference_number. '|' .$order_description. '|' .$currency. '|' .$return_address. '|' .$cancel_address. '|' .$pending_address. '|' .$notify_address. '|' .$type. '|' .$culture. '|' .$preselected_method. '|' .$mode. '|' .$visible_methods. '|' .$group. '|' ;   

compared to here:

$items_data.="$cart_title|$i|$cart_quantity|$cart_pricedot_trim|$cart_tax|$cart_discount|$cart_type|";

If you choose to join each string with a . instead of just placing them in a string like $items_data, then do so, but please (again) keep your coding style the same.

Your foreach loop, has similar to a for loop style coding attached.
However, normally in a for loop, you increment the value once you've executed the code inside the loop, not before.

Instead of:

$i = -1;
foreach ($array as $key => value){
    $i++;
    some_code_here();
}

Change it to:

$i = 0;
foreach ($array as $key => $value){
    some_code_here()
    $i++;
}

Moving on, you initialise $cart_id as $i, but then never use it, and if you were, it'd be best to replace it with $i seeing as they're the same. So, that line can be removed.

These lines are clutter:

$cart_price = $_SESSION['cart']['price'][$key];
$cart_pricedot = str_replace(',' , '.' , $cart_price);
$cart_pricedot_trim = trim($cart_pricedot);

You can just use:

$cart_price = trim(str_replace(',' , '.' ,$_SESSION['cart']['price'][$key]));

or

$cart_price = $_SESSION['cart']['price'][$key];
$cart_price_formatted = trim(str_replace(',', '.', $cart_price));

Only the last is used later anyway, so, if you like, you can replace them.

Finally (on variables):

$combined = $payment_data. '' .$customer. '' .$items_data;
$combinedsub = substr($combined, 0, -1);
$code = strtoupper(md5($combinedsub));
$trimmeddata = trim($code);

In $combined, you add strings together with empty strings in between. That can get removed.
$combinedsub could be better named to $combined_substring, Nbdy lks abbrevs, yh?
In $code, whilst being misleading, you perform strtoupper() and md5 transforms on it. You then proceed to trim() it.
First, trim()ing after you've MD5 hashed it, literally reduces the chances of having characters trimmed to zero. So, you can put the trim() in front of the md5() instead.
Secondly, if you're performing two transforms in one line, adding a third and condensing is nothing.

While $combinedsub has a misleading name, $trimmeddata has a worse name. While having incorrect grammar and spelling, you manage to double both m & d, making the variable name look even less readable.


String Building:

Here's the most helpful part.

During the course of execution, you echo out quite a few hidden inputs.

Replacing that with a function would greatly be of assistance.

function build_hidden_inputs($type, $value){
  return "<input name=\"$type\" type=\"hidden\" value=\"$value\">";
}

Then, build arrays with the content as keys and values, like so:

$first_content = array(
                     "MERCHANT_ID" => $merchant_id,
                     "ORDER_NUMBER" => $order_number,
                     // So on
                     );

Then, foreach them into the function.

foreach ($first_content as $key => $value){
    echo build_hidden_inputs($key, $value);
}

To answer your questions:

  1. Is it somehow unsecure, that my output data will be visible in pure html? including md5 hash, which is calculated based on shopping cart items/user input data. (output.html)

The visible data will be the data visible in your output.html file, if you don't want to output it, and it's not necessary, then just remove it from the echo statements.
As for md5, you are outputting it, but, cracking hashes works by matching hashes of strings.
Say, for example, if we both md5 hashed 'mypasswordissecure', we'd recieve 'efaafd259c656221c88b22471ac0d61e'.
However, even if I had your hash, I would need to match it, and with a data input as large as yours, would probably1 be pretty hard.

  1. If 1. is unsecure, how should i proceed changing my code, that it'll be less likely to be abused?

Use a more secure encryption than MD5, add a salt coming in from the Database.1

  1. Are there any vulnerabilities on handling customer input data? (addresses, names etc.)

I wouldn't go outputting the tax, price, and the more sensitive information into a HTML field, not necessarily for hackers, but for cheeky users trying to buy things for free.
The AUTHCODE though, probably shouldn't be outputted because of both.
You can always use session variables instead.

  1. Also, please state, if your improvements/changes are purely an alternate way of doing things or are they crucial changes.

Well, if you remove the foreach loop at the beginning that keeps doing the same thing, you'll get faster execution times.
However, these suggestions are closer to improvements than alternate ways of proceeding.
None of these are crucial as such.


1: I'm not an expert on security, unfortunately, so if you're worried, take it to Security.SE, and see what they think.

\$\endgroup\$
  • \$\begingroup\$ Hello, i appreciate your answer. I'll start with the md5 hash. Paytrail is requiring to use md5 hash, which is calculated by combining user data/items data together. Authcode then contains this hash and it's sent with the request. I guess paytrail gateway calculates this same itself and if it's matches the sent hash, i will be directed to payment options. (otherwise i get message, that incorrectly calculated hash etc.). Paytrail integration guide did show the format, on which the data should be sent, so that's why i'm adding the strings up and using | in the middle. \$\endgroup\$ – Griphon May 26 '15 at 10:13
  • \$\begingroup\$ I'm not sure, if i can use other encryption methods, because they would then give different output and paytrail gateway wouldn't accept it. Or does this sound right? \$\endgroup\$ – Griphon May 26 '15 at 10:14
2
\$\begingroup\$

Quill allready points out the problems with code-style (incosistency) you have. So I'm not going to point them out again.

First of, your test_input function is bad. Just bad. It gives you a sense of 'security', but in fact. I does nothing. Did you check UTF-8 support for instance? Apart from it being buggy, it also trims the string and escapes some characters.

  • Is there a reason why you are trimming? If so, write it in a comment of the function. And atleast change the functioname, to trimAndEscape()
  • Do you know what the default values of htmlspecialchars mean? Are you for instance using HTML4.01? Drupal's check_plain for instance uses htmlspecialchars($text, ENT_QUOTES, 'UTF-8');

I think your validation is to strict. What happens when people have a streetname that consists of multiple words? The same for first/last name.


Apart from that, your code is really hard to read. Extremly hard. Some things that (imho) would improve the code:

  • move all empty() checks to the top and place them together.
  • Split up the logic into another file
  • Use Exceptions

When using the functional approach, one thing I like to do is returning an array from a file I Include and catching it's values in a list. For instance:

list($firstName, $lastName) = include 'getData.php';

getData.php :

return array(
    'firstName',
    'lastName'
);

Your getData would then return an array of all the PostData. The list would look similar to this:

list($homenumber, $worknumber, $email, $firstname, $lastname, $company, $address, $postnumber, $city, $country, $vat, $count) = include 'whaterEverTheFileNameIs.php;

For better readability you would probably add some line-breaks:

list(
    $homenumber,
    $worknumber,
    $email,
    $firstname,
    $lastname,
    $company,
    $address,
    $postnumber,
    $city,
    $country,
    $vat
) = include 'whaterEverTheFileNameIs.php;

As sad in the list above, use exceptions. The if(empty($errors)); is a code smell. It will eventually cause for a hard time debugging because you don't know where the error came from. With exceptions, this ll becomes a lot easier. There are a lot of built in exceptions in spl. For instance the InvalidArgumentException. This one is perfect for your validation.

\$\endgroup\$
  • \$\begingroup\$ Thank you a lot. About the test_input function, i've changed it and will be using only htmlspecialchars, since i'm later echoing those variables. About validation, correct me if i'm wrong but /[a-zA-Z]/u accepts any letters, spaces and numbers UTF8? so it shouldn't be problem, if a user enters multiple words. \$\endgroup\$ – Griphon May 28 '15 at 5:32
  • \$\begingroup\$ Quill already pointed the authcode thing but do you have anything to add to that? a quote from paytrail integration guide: "When Paytrail payment service has been integrated using either REST interface or form interface, the payment process can be shortened and made easier from customer point of view by implementing the payment method selection embedding. In that case the payment methods are available directly at the end of a web application's order process instead of having to move first to Paytrail service to select the payment method." I'm using form interface \$\endgroup\$ – Griphon May 28 '15 at 5:36
  • \$\begingroup\$ Would there be an alternate way of embedding it or am i doing it right? since authcode is visible etc. Now i'm just printing out the form (and it's visible in html) but i guess this can't be done on server side with his approach. Also, does this open a door for malicious users or is it safe? \$\endgroup\$ – Griphon May 28 '15 at 5:38
  • \$\begingroup\$ Also, merchant secret is not viewable and it's quite long string of hardcoded random characters, so shouldn't it be enough security to prevent someone from matching the hashes? \$\endgroup\$ – Griphon May 28 '15 at 5:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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