Database hack

[learnings]

I successfully built a transactional app with PDO MySQL, all transactions that deals with cash inflow and outflow are programmed to record accordingly. But I figure out that someone credited his wallet by himself and the method he uses bypass recording is channel of self funding. I will love who is willing to assist me on this

[ginerjm]

Huh?

[requinix]

We don't know anything about your application. If you wrote it and you don't know what happened, what would we be able to do?

That said, my guess is an unvalidated input somewhere. Do you verify that all transactions must be for positive amounts?

[learnings]

<pre>public function update($table, $data, $where)

    {

        $collection = array_merge($data, $where);

 

        $values = array_values($collection);

 

        $fieldDetails = null;

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

            $fieldDetails .= "$key = ?,";

        }

        $fieldDetails = rtrim($fieldDetails, ',');

 

        $whereDetails = null;

        $i = 0;

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

            $whereDetails .= $i == 0 ? "$key = ?" : " AND $key = ?";

            $i++;

        }

 

        $stmt = $this->run("UPDATE $table SET $fieldDetails WHERE $whereDetails", $values);

 

        return $stmt->rowCount();

    }

</pre>

<pre>$towallet = intval($rgy['mainbalance']) - intval($newamount); 

      $letUpdate = $db->update('mywallet', ['main' => $towallet], ['validate' => $apikill]); </pre>

 

From the screenshot, the third column addresses the amount value. The very one at the top is the previous balance, the middle is the new amount while the third is the new balance. All credit and debit were written to b recorded in the transaction table. All of this works fine for many months, until someone was able to breach it this week over and over. And there's no record for this strange activities. There's no other direct code to interact with my wallet table, what could have been wrong

Edited June 15, 2023 by learnings

[requinix]

Remember the part where I said you might not be validating transaction amounts? Did you look into that?

[learnings]

Yes, I did. Any amount lesser than 100 will have the script terminated. And amount associated inputted value that is not number or negative in value will be terminated. Moreso, when the wallet changes occurred. The user can't determine the amount, the amount is sorted according from the DB. 

[learnings]

My entire code to run update command on the application is as follows

 

public function run($sql, $args = [])

{

    if (empty($args)) {

        return $this->db->query($sql);

    }



    $stmt = $this->db->prepare($sql);

    $stmt->execute($args);



    return $stmt;

}

public function update($table, $data, $where)

{

    $collection = array_merge($data, $where);



    $values = array_values($collection);



    $fieldDetails = null;

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

        $fieldDetails .= "$key = ?,";

    }

    $fieldDetails = rtrim($fieldDetails, ',');



    $whereDetails = null;

    $i = 0;

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

        $whereDetails .= $i == 0 ? "$key = ?" : " AND $key = ?";

        $i++;

    }



    $stmt = $this->run("UPDATE $table SET $fieldDetails WHERE $whereDetails", $values);



    return $stmt->rowCount();

}





$towallet = intval($rgy['mainbalance']) - intval($newamount);

$letUpdate = $db->update('mywallet', ['main' => $towallet], ['validate' => $apikill]);

 

 

[gizmola]

Your code seems to be entirely variable driven dynamic sql, based on user input.  What could possibly go wrong? 🤐

Add logging routines that log the actual SQL statements AND parameters to a file.  I'm sure if this continues to happen, in short order you will see what is being done.

Seeing these routines themselves, it's easy to see that anyone can essentially update any table they want, if they can get the variables to these routines set to be what they want.  

You haven't provided any of the UI code or table structure(s) information for context, so there's not much more we can do to help, when we don't have any idea what those things are or how these routines are called.

[requinix]

22 hours ago, learnings said:

My entire code to run update command on the application is as follows

No, that is not all of your code. Because it doesn't show what $rgy or $newamount are.

The problem isn't the update itself. As gizmola suggested, the problem is going to be whatever sets up the different variables which the update uses.

[mac_gyver]

this implementation has two problems.

1) by getting the existing value (wherever and whenever $rgy['mainbalance'] is being gotten and/or passed to/through the code), modifying it (wherever $newamount is coming from), then executing an update query, if there are concurrent operations on this data, they will all get the same starting value, modify it, then the last update query to be executed will replace any previously updated value. so, if for example there were two (or more) concurrent operations to subtract values, such as 1000 and 100, and the last update query to be executed is for the 100 value, then it would look like the 1000 value was never subtracted, leaving the wallet with a larger value in it then expected. if you continue to just update the value in a column (see the next point), you would modify the value completely within the update query, e.g. SET main = main - ?

2) by updating a value in a column, there is no audit-trail that would let you detect if a programming mistake, duplicate submission (hopefully you are using a run-once token for form submissions), or nefarious activity modified the value or let you produce statements/reports of who, what, when, where, and why the amount got changed. the correct way of doing this is to insert a new row of data into an accounting table for every transaction that affects the value. you would then simply SUM() the +/- amounts in a query whenever you need the current total, the same way that all your banking, credit card, loans, utility accounts, medical billing, ... do it.

also, since the op didn't post all the code necessary to reproduce the problem (he was asked elsewhere for this), the values themselves could be determined from an external submission, rather than only being determined on the server.

Edited June 16, 2023 by mac_gyver