This one's probably a big one. We'll have to take deep care to avoid data races, as they can cause data corruption, and in some cases, security exploits.
This is a fairly complex topic which ties into concurrency, but. If we use the example of the file bug I reported for 1.8, whenever a setting is changed the settings.php file is rebuilt. The problem is that if two people change settings at once, then there's a small chance that it'll be rebuilt at the same time causing it to get corrupt.
One method of avoiding this is with a lock. A lock makes it so that only one process / thread can access that file at once. Locks exist in the database too. While a record can't be corrupted in the same way thanks to the database helping us out here, do bear in mind that multiple updates from the same process don't usually happen all at once (aka atomically), so an update from a different process could be slipped in-between those two updates potentially corrupting the state.
Also, a process could be acting upon data which is already stale by the time it does it's update due to another process updating the data it's acting on before that update but after it does it's check.
Bump on this because I'd like to discuss this further and no one even replied at the time.
OP outlined part of the problems with Race Conditions but I think it's something that goes much deeper with MyBB as a problem.
The current MyBB $db class does not use TRANSACTIONS. Basically a way for MySQL to lock rows/table and then commit.
https://dev.mysql.com/doc/refman/5.7/en/commit.html
There is no $db->commit or $db->start_transaction but there should be.
Inside of MyBB if someone makes a double post it's really not a huge deal but it becomes a problem from a plugin point of view such as New Points where you can literally script cheats to send more points than you have. That's just one instance and I'm sure there are many more where you only want submit to occur once and that duplicate submissions might create a problem.
So I propose a simple solution we've come up with at HF for our own plugins.
We created a simple 2 column table with UID and DATELINE (current timestamp).
We call a small function normally right after verify_post_key() in a POST action we call form_post_check().
function form_post_check() {
global $mybb, $db;
$db->write_query("START TRANSACTION");
$query = $db->write_query("SELECT * FROM mybb_form_check WHERE uid='" . (int)$mybb->user['uid'] . "' FOR UPDATE");
$result = $db->fetch_array($query);
if(!$result) {
$db->insert_query("form_check", ["uid" => (int)$mybb->user['uid'], "dateline" => TIME_NOW]);
} else {
if(TIME_NOW - $result['dateline'] < 2) {
$db->write_query("COMMIT");
error("You are trying to submit a form too quickly after a previous form submission. Please go back and try again.");
} else {
$db->update_query("form_check", ["dateline" => TIME_NOW], "uid='" . (int)$mybb->user['uid'] . "'");
}
}
$db->write_query("COMMIT");
}
That function is simple and implements easily. Xerotic and I both feel that it should be part of verify_post_key() if tested properly by MyBB but should in some way be considered as an addition to base.
If MyBB has a better solution please let us know but after our research and testing this is the best we came up with.
(2019-05-07, 06:40 PM)labrocca Wrote: [ -> ]-snip-
In our solution we simply disallow multiple form (or wherever we decide to call our new function) requests within a 1-2 second period. We figured a solution like this could actually just be built into
verify_post_check(). Basically anywhere that a member can perform a site action requiring their
$mybb->post_code is a good place to limit requests to prevent race conditions. This solution still allows members to use multiple tabs without difficult errors based on a changing hash (another solution we considered).
As labrocca said, popular plugins such as NewPoints are vulnerable to race conditions. A member with 100 credits could programmatically send multiple simultaneous "donate" requests for 100 credits and both (if not more) might go through. In this scenario, the member donating would then have -100 (or -200.. etc) credits and the receiving member would have all those extra credits.
We did actually discuss this originally, but it was mostly on Discord if I remember correctly.
We've talked in the past about future plans to change the database library to make use of parameterised queries and PDO (currently there is no PDO version of the DB classes for MySQL or Postgres, only SQLite). As part of this work, I would like to add easy to use APIs to work with transactions, which would help a little. Obviously, we'd also then have to make use of the transaction APIs where it makes sense to do so - something that may be a bigger job.
We'll have a look at whether using a "lock table" makes sense, perhaps verify_post_check() could gain a new (false by default) parameter to disable use of the "lock table" in case there is any scenario found down the line where it causes problems. I know @Devilshakerz was wanting to make some other changes to post keys and session handling as part of 1.9, so he might be able to chime in here.
(2019-05-09, 04:12 PM)Euan T Wrote: [ -> ]I know @Devilshakerz was wanting to make some other changes to post keys and session handling as part of 1.9, so he might be able to chime in here.
The session system is one of the planned under-the-hood changes that are isolated enough to not introduce many incompatibility issues. It might be worked into 1.9 (or later) like the
password storage update, and would likely include:
- tracking user sessions per-device in a database table (currently the table is used for visit statistics; this will allow to list and terminate sessions on user and global level in case of a breach),
- split session keys (with one part designated for querying rows, to prevent database timing attacks on the 2nd part),
- storing hashes/HMACs of session keys only (the 2nd part; will prevent extracting and using keys from database leaks, and HMACs can limit exploitation of maliciously inserted keys),
- session-specific anti-CSRF tokens.
It wouldn't necessarily address race conditions, though.
Per-user locking wouldn't resolve all issues, as mentioned in the OP; we can take a look at quick and global fixes, but if problems are limited to specific sections (like virtual currency) it might be better to introduce transactions there.