Does this script parse return parameters?

PHP
1

Upon trying to integrate a PayWithAmazon button to a web site, I am told that this return URL script file "should parse the return parameters, here is an example(provided by amazon support):

Array
(
    [resultCode] => Success
    [signature] => iuhsci7987987^&uygYG87875878.........
    [sellerId] => AAAAAAAAAAAAAAA
    [AWSAccessKeyId] => AAAAAAAAAAAAAAAAA
    [SignatureMethod] => HbacAHS777
   [SignatureVersion] => 2
   [orderReferenceId] => S01-47887779-99776
    [amount] => 0.10
    [currencyCode] => USD
    [paymentAction] => AuthorizeAndCapture
     )

So, my question is, does this file (below) parse the return parameters?

<?php
header('Location: ../index.php');
/// - Database Information
$dbhost = 'localhost'; 
$dbuser = 'XXXXXXXXXXXX';
$dbpass = 'AAAAAAAAAAAA';
$dbname = 'BBBBBBBBBBBB';


/// - Do Not Edit Below This Line

$conn = mysql_connect($dbhost, $dbuser, $dbpass);

 mysql_select_db($dbname);

/////////////////////////////////////////////////////////////////////////////////////////

function getUsername($id) {

$sql1 = "SELECT * FROM member_profile WHERE user_id = $id";
$query1 = mysql_query($sql1) or DIE(mysql_error());
$result = mysql_fetch_array($query1);

return $result['user_name'];
 }
header('Location: ../index.php');
include_once ('../classes/config.php');
include ('../classes/functions.php');
include_once ('../classes/sessions.php'); //gives us access to the user's cookies for validation



$date = date();
$user = $user_id;
$credits = $_GET['description'];
$price = $_GET['amount'];
$username = getUsername($user_id);

$backp = $price;

$sql2 = "INSERT INTO purchases (id, type, user_id, vid_id, date, name, uploader, uploaderID,  title, amount, videoid, descr, promo) VALUES ('', 'purchase', '$user', '0', '$date', '$username', 'none', 'none', 'none', '$backp', 'none', 'none', 'none')";
$query2 = mysql_query($sql2);


$sql1 = "SELECT * FROM credits WHERE user_id = $user";
$query1 = @mysql_query($sql1);

// =========================================================
// Error reporting for the above query is turned off, so we
// don't know if the credits record was even found.
// The following line fixes that issue by inserting a blank
// record if the row count is zero.
// =========================================================
if (mysql_num_rows($query1) == 0)
{
$sql1_I = "INSERT INTO credits (user_id) VALUES ($user)";
$query1_I = mysql_query($sql1_I) or die(mysql_error());
}
// =========================================================


//      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// That is useless code and wasted space considering an entry is made upon initial user registration.
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

$old = @mysql_fetch_array($query1);
$balance = $old['total_credits'] + $credits;
$purchases = $old['total_purchases'] + 1;
$sql = "UPDATE credits SET user_id=$user, total_credits=$balance, pending_credits=0,  last_purchase=$date, total_purchases=$purchases WHERE user_id=$user";
$query = mysql_query($sql);



 $template = "../themes/$user_theme/templates/main_1.htm";
 $inner_template1 = "../themes/$user_theme/templates/amazon1_success.htm"; //middle of page
$TBS = new clsTinyButStrong;
$TBS->NoErr = true; // no more error message displayed.

$TBS->LoadTemplate("$template");
$TBS->MergeBlock('mp', $members_full);

$TBS->Render = TBS_OUTPUT;
$TBS->Show();


?>
2 Likes
2

Hi,

That script reads two request parameters here:

3

It doesn’t look like a script to use anyway? It uses the deprecated mysql_ extension, and it doesn’t do any validation/sanitization of the $_GET parameters before using them in the queries?

1 Like
4

I am no expert, but that php code doesn’t seem to be at all related to the array structure you posted above it. You’d expect at least that it might look at the contents of “ResultCode” to see whether or not it should post the information.

5

Thanks for those replies.

How can I get some help with adding the right parmeters and using the correct mysql_extension and adding some validation/sanitation of the $_GET parameters and making it more likely to work with the array structure? Any additional help will be appreciated.

6

Hi there. Where did you get that PHP code? The comment:

// That is useless code and wasted space considering an entry is made upon initial user registration.

doesn’t give a lot of confidence.

7

Just seeing the “do not edit below” suggests to me that the code could be fragile.

Then seeing the use of mysql_ and “@” error suppression without the code being written in a way to deal with a FAIL would be enough to make me look elsewhere.

2 Likes
8

That was my thinking. I think if you look at it for too long it might fall apart.

9

Not to mention the gaping security holes.

1 Like
10

and that it will not run on the latest version of PHP at all.

11

All these points considered, scrap this and start a new script.

2 Likes
12

Thanks for all of your replies. I won’t be scrapping it, so any constructive suggestions to tweak it will be appreciated.

13

Would replacing:

$price = $_GET['amount'];

With:
$price = mysql_real_escape_string($_GET[‘amount’]);

help?

14

No - definitely not.

  1. That is an escaping function and has nothing to do with validation/sanitizing of tainted inputs such as $_GET.

  2. That function was removed from PHP last month (along with all the others that start mysql_ ) and so no longer exists.

You need something link this as a minimum.

$price = 0;
if (is_numeric($_GET['amount'])) $price = $_GET['amount'];

That will probably still let a lot of invalid values through and so probably needs to be further restricted but is a billion times better than what you currently are using.

1 Like
15

Thanks for your reply and info.

It appears that this:
$price = mysql_real_escape_string($_GET[‘amount’]);
works with the existing php payment script (by adding the purchased credits to the users’ account),

but this does not:

$price = 0;
if (is_numeric($_GET['amount'])) $price = $_GET['amount'];

I know I haven’t provided any details on a payment script, but if have any other suggestions for that line of code, to try, I’d be appreciative. Much thanks

16

Is $_GET['amount'] numeric?
eg. $1.43 is not numeric, 1.43 is.

17

According to the array at the top, [amount] appears to be numeric, with the [CurrencyCode] stating the currency.

Along with any additional credit they want, if they decide to edit the url variable in their browser bar. :wink:

18

If the $_GET value is actually numeric then the code I provided would load it into $price - as it isn’t passing through into the database then the 0 assigned to $price initially must not be being overwritten by a proper numeric value.

All that code does is to pass through the value if it is numeric and to substitute 0 if it isn’t - unlike the code the OP says works which actually does absolutely nothing and so of course ‘works’.

19

Well, if anything, patching up a broken script destined for an early death will be a learning experience.

1 Like
20

Thanks for the replies.
So, you’re saying this doesn’t prevent SQL injection for the GET variables:

$price = mysql_real_escape_string($_GET['amount']);

Also, can you please tell me more about what you mean by:
“Along with any additional credit they want, if they decide to edit the url variable in their browser bar”?