1
\$\begingroup\$

This is a trade script for an online virtual currency exchange I have been developing. We have a team of 3, and we all work in our own trees. Since we're all young and not very experienced, we would like some feedback and advice prior to hiring penetration testing.

if (isUserLoggedIn()) 
{ 
//get balances
}

function GetMoney($user, $currency)
{
    $user2 = $user;
    $sql   = @mysql_query("SELECT * FROM balances WHERE `User_ID`='$user2' AND `Coin`='$currency'")
    $id    = @mysql_result($sql, 0, "id");
    if ($id < 1) {
        $old = mysql_result($sql, 0, "Amount");
        return $old;
    } else {
        $old = mysql_result($sql, 0, "Amount");
        return $old;
    }
}
$id = mysql_real_escape_string($_GET["market"]);
$result = mysql_query("SELECT * FROM Wallets WHERE `Id`='$id'");
$name = mysql_real_escape_string(mysql_result($result, 0, "Acronymn"));
$fullname = mysql_real_escape_string(mysql_result($result, 0, "Name"));
if($id == 1) {
    header( 'Location: index.php?page=account' ) ;
    die();
}
if($name == NULL) {
    header( 'Location: index.php?page=invalid_market' ) ;
    die();
}
$market_id = mysql_result($result, 0, "Market_Id");
$SQL2 = mysql_query("SELECT * FROM Wallets WHERE `Id`='$market_id'");
$Currency_1a = mysql_result($SQL2, 0, "Acronymn");
$Currency_1 = mysql_result($SQL2, 0, "Id");
//--------------------------------------
if(isset($_POST["price2"]))
{
    if ($_POST["price2"] > 0 && $_POST["Amount2"] > 0) 
    {
        $PricePer = mysql_real_escape_string($_POST["price2"]);
        $Amount = mysql_real_escape_string($_POST["Amount2"]);
        $X = $PricePer * $Amount;
        $Total = file_get_contents("/system/calculatefees.php?P=" . $X);
        $Fees = file_get_contents("/system/calculatefees2.php?P=" . $X);
        $user_id = $loggedInUser->user_id; 
        if(TakeMoney($Total,$user_id,$Currency_1) == true)
        {
            AddMoney($Fees,101,$Currency_1);
            mysql_query("INSERT INTO trades (`To`,`From`,`Amount`,`Value`,`User_ID`,`Type`,`Fee`,`Total`)VALUES ('$name','$Currency_1a','$Amount','$PricePer','$user_id','$name','$Fees','$Total');");
        }
        else
        {
            echo "<p class='notify-red' id='notify'>You cannot afford that!</p>";
        }
    }
    else
    {
        echo "<p class='notify-red' id='notify'>Please fill in all the forms!!</p>";
    }
}
//--------------------------------------
if (isset($_GET["cancel"])) {

    $ids      = mysql_real_escape_string($_GET["cancel"]);
    $tradesql = @mysql_query("SELECT * FROM trades WHERE `Id`='$ids'");
    $from     = @mysql_result($tradesql, 0, "From");
    $owner    = @mysql_result($tradesql, 0, "User_ID");
    $Fee = @mysql_result($tradesql,0,"Fee");
    $Total = @mysql_result($tradesql,0,"Total");
    $sql = mysql_query("SELECT * FROM Wallets WHERE `Acronymn`='$from'");
    $from_id = mysql_result($sql,0,"Id");
    if(TakeMoney($Fee,101,$from_id,true) == true)
    {
        AddMoney($Total,$owner,$from_id);
    }
    else
    {
        echo "<p class='notify-red' id='notify'>You cannot afford that!</p>";
    }
    mysql_query("DELETE FROM trades WHERE `Id`='$ids'");

}
//--------------------------------------
if(isset($_POST["Amount"]))
{
    if ($_POST["price1"] > 0 && $_POST["Amount"] > 0) 
    {
        $PricePer = mysql_real_escape_string($_POST["price1"]);
        $Amount = mysql_real_escape_string($_POST["Amount"]);
        $Fees = file_get_contents("/system/calculatefees2.php?P=" . $Amount);
        $Total = $Fees + $Amount;
        echo $Total;
        $user_id = $loggedInUser->user_id; 
        if(TakeMoney($Total,$user_id,$id) == true)
        {
            AddMoney($Fees,101,$id);
            mysql_query("INSERT INTO trades (`To`,`From`,`Amount`,`Value`,`User_ID`,`Type`,`Fee`,`Total`)VALUES ('$Currency_1a','$name','$Amount','$PricePer','$user_id','$name','$Fees','$Total');");
        }
        else
        {
            echo "<p class='notify-red' id='notify'>You cannot afford that!</p>";
        }
    }
    else
    {
        echo "<p class='notify-red' id='notify'>Please fill in all the forms!!</p>";
    }
}

?>
<link rel="stylesheet" type="text/css" href="../assets/css/trade.css" />
<link href="assets/css/tables.css" type="text/css" rel="stylesheet" />
<center><h3>Trade <?php echo $fullname; ?></h3></center>
<div id="chart">
<script src="../assets/charts/effects.js"></script>
<script src="../assets/charts/Chart.js"></script>
<script src="../assets/charts/excanvas.js"></script>

<meta name = "viewport" content = "initial-scale = 1, user-scalable = no">



        <canvas id="canvas" height="400" width="700" style="background: #333; margin: 0 auto;" ></canvas>
    <script>

        var lineChartData = {
            labels : ["1 Week","24 Hour","8 Hour","4 Hour","2 Hour","1 Hour","Last Trade"],
            datasets : [
                {
                    //Coin A
                    strokeColor : "#fff",
                    pointColor : "#000",
                    pointStrokeColor : "#fff",
                    data : [<?php echo $trade_data; ?>]
                }
            ]

        }

    var myLine = new Chart(document.getElementById("canvas").getContext("2d")).Line(lineChartData);
    </script>
    </body>
</div>
<div id="boxB">
    <div id="boxA">
        <div id="col1">
            <!-- Sell Form-->
            <?php if (isUserLoggedIn()) { ?>
            <div id="sellform" >
                <center><h2>Sell <?php echo $name; ?></h2></center>
                <form action="index.php?page=trade&market=<?php echo $id; ?>" method="POST" autocomplete="off" history="off"> 
                <label>Amount:</label><input type="text" style="width:100px;" name="Amount" onKeyUp="calculateFees1(this)" id="Amount"/>
                <label>Price:</label><input type="text" style="width:100px;" name="price1" onKeyUp="calculateFees1(this)" id="price1" />
                <label>Receive:</label><input type="text" style="width:100px;" id="earn1"/>
                <label>Total:</label><input type="text" style="width:100px;" id="fee1"/>
                <label>Execute:</label><input type="submit" name="Sell" value="Sell"/>
                </form>
            </div>
            <?php } ?>
            <!--Sell Order Book-->
            <div id="sellorders">
            <?php
                include("open_orders_from.php");
            ?>
            </div>
        </div>
        <div id="col2">
            <!--Buy Form-->
            <?php if (isUserLoggedIn()) { ?>
            <div id="buyform">
                <center><h2>Buy <?php echo $name; ?></h2></center>
                <form action="index.php?page=trade&market=<?php echo $id; ?>" method="POST" autocomplete="off" history="off">
                <label>Amount:</label><input type="text" style="width:100px;" onKeyUp="calculateFees2()" name="Amount2" id="Amount2"/>
                <label>Price:</label><input type="text" style="width:100px;" id="price2" onKeyUp="calculateFees2()" onKeyUp="calculateFees2()" name="price2"/>
                <label>Total:</label><input type="text" style="width:100px;" id="fee2"/>
                <label>Execute:</label><input type="submit" name="Buy" id="Buy" value="Buy"/>
                </form>
            </div>
            <?php  } ?>
            <!--Buy Order Book-->
            <div id="buyorders">
            <?php
                include("open_orders_to.php");
            ?>
        </div>
        </div>
    </div>
</div>
<?php if (isUserLoggedIn()) { ?>
<div id="user-orders">
<?php include("open_orders.php"); ?>
</div>
<?php }  ?>

Generation of Market list/Last price in index.php

<ul id="market-list">
        <?php
        $sql = mysql_query("SELECT * FROM Wallets ORDER BY `Acronymn` ASC");

        while ($row = mysql_fetch_assoc($sql)) {
        if($row["Market_Id"] == 0)
        {

        }
        else
        {
            $sql_2 = mysql_query("SELECT * FROM Trade_History WHERE `Market_Id`='". $row["Id"] . "' ORDER BY Timestamp DESC");

            $last_trade = mysql_result($sql_2,0,"Price");
        ?>
        <li><a href='index.php?page=trade&market=<?php echo $row["Id"]; ?>'><?php echo $row["Acronymn"]; ?> &rarr; BTC <?php echo "<font class='price'>".sprintf("%2.8f",$last_trade)."</font>"; ?></a></li>
        <?php
        }
        }
        ?>          
      </ul>

Chat from index.php

<div id="messages"></div>
<?php

//check if user is banned.


//check if user is logged in then display forms.
if (isUserLoggedIn()){ 
        $username = $loggedInUser->display_username;

    //define color of usernames.
    if(!isUserAdmin($id)) {
        $color = "#999999";
        }else{
        $color = "#FF9900";
        }

echo'
<form id="ajaxPOST" history="off" autocomplete="off">
    <input type="hidden" value="'.$color.'" name="color" />
    <input type="hidden" value="'.$username.'" name="username" />
    <input type="text" id="message" maxlength="255" name="message" />
    <input type="submit" id="chat-submit" value="Post Message" />   
</form>';

} else {

echo'
<div id="LoggedOut"></br><b><center>You must be logged in to chat</center></b></div>';


} 

?>
</div>

jQuery code (index.php)

$(document).ready(function() {
    //load messages
    $('#messages').load('chat/ajaxLOAD.php');


    $('#ajaxPOST').submit(function() {
        $.post('chat/ajaxPOST.php', $('#ajaxPOST').serialize(), function(data){
        //clear the message field
        $('#message').val('');
        //reload messages
        $('#messages').delay(1000).load('chat/ajaxLOAD.php').delay(1000).scrollTop($("#messages")[0].scrollHeight);

        });
        return false; 
    });
});

ajaxPOST.php

include 'chat.config.php';

$color = $_POST['color'];
$username = $_POST['username']; 
$message = $_POST['message'];
if ($_POST['message'] == null) 
{

}else{

$db->Query("INSERT INTO messages (color, username, message) VALUES ('".$color."','".$username."','".$message."')");
}

ajaxLOAD.php

    include 'chat.config.php';

$db->query('SELECT * FROM messages ORDER BY (id) ASC');

$data = $db->GET();

foreach($data as $key => $value) {

    echo "<li id='msg_row'><b style='color: ".$value['color'].";'>".$value['username']."</b> :  <i>".$value['message']."</i></li>";

}
\$\endgroup\$
  • 4
    \$\begingroup\$ Don't use mysql_* functions in new code! \$\endgroup\$ – Vedran Šego Nov 21 '13 at 2:01
  • \$\begingroup\$ why? pdo seems scary to me, and would require us to rewrite the entire application. is what we're doing not safe enough already to prevent injections? \$\endgroup\$ – r3wt Nov 21 '13 at 2:09
  • 2
    \$\begingroup\$ Let me quote from the link I have provided in my first comment: Officially deprecated (as of PHP 5.5. It's likely to be removed in the next major release.) There is much more, so please check the accepted answer where this points to. Why would a professional programmer be afraid of PDO? OOP's been around for 4 or 5 decades (depending on what you pick as the beginning). \$\endgroup\$ – Vedran Šego Nov 21 '13 at 2:54
  • \$\begingroup\$ I did read the link you provided, as well as one that was posted in a similar topic about switching to PDO, and i will gradually migrate toward using it as i learn and grow as a programmer. However, my question was not on the deprecation of mysql functions or about PDO. i asked for any vulnerabilities i might have missed in the code, or any i do not know about. For the record, i haven't even started college yet, so maybe that will help you understand where i'm coming from. I don't have any formal education in programming at this time, just self taught/trial and error. \$\endgroup\$ – r3wt Nov 21 '13 at 3:49
  • 2
    \$\begingroup\$ Using PDO instead of mysql_* is a security improvement! You should always construct queries with placeholders for parameters, and never using string interpolation or concatenation. If you blindly incorporate data strings into your SQL without escaping, the data could be interpreted as code, a.k.a an SQL injection vulnerability. See bobby-tables.com \$\endgroup\$ – 200_success Nov 22 '13 at 0:25
2
\$\begingroup\$

There's quite a few things you can fix here. If it is not already in version control, do that first, and then do a commit for every improvement; that way, if anything goes wrong, you can roll back easily. Git is good, but use whatever you know.

Firstly, the business logic and the page layout need to be separated. Many a tutorial on the web mixes the two up, perhaps because many years ago programmers were just too thrilled with the web's possibilities to remember the benefits of modularisation. So, at the very least, move the first half of the code to a separate file, and then require it in your first line.

That becomes your 'logic' file. Whilst you appear to have a few functions in your logic file, the bulk of this is not split into functions at all, and ought to be. There are too many variables sitting in "global space" that need to be local to a function; this will destroy them when each function ends, which is what we want. That said, a few will need to persist in order to be displayed in your HTML layout; potentially they could be the results of functions, although there are several ways to do it.

One step better would be to use a templating engine of some kind, so that your business logic is wrapped up nicely in objects. However, that might be a case of trying to learn too many things at once, so perhaps leave this one for the time being.

In terms of the logic itself, try to avoid the @ error suppression operator, since it will hide problems from you during development. It is better to test intermediate database operations (from connection, then from the query, then from reading results) and just output an error gracefully.

I noticed also that the if clause in GetMoney has two paths that do exactly the same thing. You may wish to check this.

Where you do a financial transaction, this must be in a database transaction to avoid something being being debited without a corresponding credit - you want this so either everything completes or nothing does.

The file_get_contents refers to an absolute pathname rather than something in the current project. It is better to obtain this path from a configuration file, rather than "hardwiring" it into code. Also, this appears to be a PHP file - note that this PHP will not be run to generate data - file_get_contents will treat it just like a text file.


For the template part (essentially the second half of your code) you may find using the colon form of statements easier to read. As you've discovered, braces do work here, but <?php if (isUserLoggedIn()): ?> and <?php endif ?> is much clearer.

I notice from your live demo that you do not have a DOCTYPE, which means your document will not be treated as standards-based. Just add <!DOCTYPE html> to the first line (assuming you are targetting HTML5) and this will be fixed.

The tag </br> is not valid markup - I think you meant the self-closing tag <br />.

The link tag <a href='index.php?page=trade&market=6'> is not valid; you need to encode the special characters. Thus, it should be <a href='index.php?page=trade&amp;market=6'>. See the "View Source" in Firefox - items it does not understand are shaded in red.

Avoid the <font> tag completely; use <span> with a style class.

\$\endgroup\$
  • \$\begingroup\$ wow, thats a lengthy list. question, on the link tags. why does it even matter? and secondly this is dynamic content generated from a database query. i've added that portion of index.php to my original post. are you suggesting i add the html entity code for "&" like &amp; . won't that break my url/code? \$\endgroup\$ – r3wt Nov 22 '13 at 7:10
  • \$\begingroup\$ In your template code, it looks like you could just write the entity explicitly. If you have a URL in a string, then in your template file you can also do <?php echo htmlspecialchars($url) ?> and that will do the trick as well. It won't break your URL, no; the ampersand entity is understood as being a single character, the ampersand. If it is supplied "bare" then the browser will assume it is the start of an entity, but that it was unfinished. Whilst in most cases the browser will work out what you mean, it is much better to be clear. \$\endgroup\$ – halfer Nov 22 '13 at 9:26
  • \$\begingroup\$ Btw, you can check the above by running your document through the W3C validator - it will pick up this error, the DOCTYPE thing, plus any others I missed. \$\endgroup\$ – halfer Nov 22 '13 at 9:30
  • \$\begingroup\$ template code? what do you mean? \$\endgroup\$ – r3wt Nov 22 '13 at 10:03
  • \$\begingroup\$ Heh, I saw the earlier version of that comment ;), but you misunderstood it. The "template" is a common name given to the browser output part of your code, in the same context as "template engine". \$\endgroup\$ – halfer Nov 22 '13 at 10:50

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.