View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000556 | XMB1 | Bugs | public | 2020-08-15 05:36 | 2020-10-20 18:17 |
Reporter | simonw | Assigned To | miqrogroove | ||
Priority | normal | Severity | major | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | 1.9.8 SP2 | ||||
Target Version | 1.9.12 | Fixed in Version | 1.9.12 | ||
Summary | 0000556: Insufficient entropy on password generation | ||||
Description | When an account is reset the function mt-srand is called using the output of the microtime() function as a seed. This function is also used elsewhere, e.g. CSRF, but the exploitation is unclear, I would change all instants of mt-rand/mt-srand when a solution is agreed. This means that although it generates a 13 character password with 62 possible characters in each place. It only can 1,000,000 possible passwords. I wrote a trivial PHP function to generate all possible passwords which takes 12 seconds to run. I reset my test server's admin password, and a single threaded brute force attack found the 145245 password was needed in 1h 4m 47 seconds allowing me access to the admin account knowing only account name and email (the email is used in sending out messages from the forum). The attack could be speeded up, since the attacker can know the server time from HTTP headers, and can estimate the value of the microtime() to within a few milliseconds. Because of the problematic way XMB does authentication the password needs to be really strong, since to prevent brute force guessing all authenticated actions would need special controls. Worse case is under 8 hours at this rate of attack. Correctly implemented a 62^13 character password should take ~1000,000,000 times the estimated age of the Universe at this rate of attack. I looked at replace mt-srand(double(microtime()*1000000)) with mt-srand() as the PHP manual page suggests, but the mt-srand() built-in initialisation function is mathematically broken as well. PHP 7 has already reverted one fix for mt-srand() as it breaks backward compatibility (sigh). I'd strongly recommend abandoning mt-srand and related randomisation functions ASAP. The PHP function "random_int" can be used in place of "mt-rand()", and implementation exists for pre-7.0 users https://www.php.net/manual/en/function.random-int.php | ||||
Steps To Reproduce | Reset a password, compare to password to the list generated from: <?php /* Generate all 1,000,000 possible passwords the XMB forum reset can generate */ $chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz"; $get = strlen($chars) - 1; for ($seed = 0 ; $seed < 1000000 ; $seed++){ mt_srand( $seed ); $password = ''; for($i = 0; $i < 13; $i++) { $password .= $chars[mt_rand(0, $get)]; } print $password."\n"; } ?> | ||||
Additional Information | I tested the modified code snippet on Debian GNU/Linux 10 on AMD processor, this created 1000,000 passwords which were unique, strong, and none of them were in the list generated by the snippet above. The list was put through the NIST randomness tests offered by Burp Suite Community Edition and passed, although there is indication of use of pseudorandom number generators, which is expected if it is sourcing /dev/urandom or similar at some points in the process. $chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz"; $get = strlen($chars) - 1; for ($seed = 0 ; $seed < 1000000 ; $seed++){ $password = ''; for($i = 0; $i < 13; $i++) { $password .= $chars[random_int(0, $get)]; } print $password."\n"; } | ||||
Tags | password | ||||
MySQL Version | |||||
PHP Version | |||||
Web Server | |||||
Browser | |||||
Flags | |||||
Original Reporter | |||||
SVN Revision | 2798 | ||||
related to | 0000001 | closed | miqrogroove | Full Session Handling |
related to | 0000559 | closed | miqrogroove | Increase Required PHP Version to 7.0 |
|
This would likely affect all versions. I also think there is a larger issue if XMB is allowing 145,245 login attempts per hour. That's something like 40 hits per second? Maybe I can add a rate limit but I don't recall if the attempt date is stored at all in this schema. |
|
Rate limiting wouldn't be effective on login alone, as you use the password MD5 as an effective session token, an attacker can thus perform any authenticated action as a check on whether a word is the user's current password, you'd need the rate limit check on login, and on all actions that check authentication. I think rate limiting would be good as it stops general password guessing too, but you probably want to also ensure the generation of randomness is correct too, since the attacker can improve the attack by using the HTTP headers to work out the server offset from their clock time, and then measure the time of the HTTP reset request, and know the server microtime() value to within a much narrower range than my proof of concept. |
|
This is all handled by a single function since v1.9.10. Unfortunately, there was never support for rate limiting in the database schema. And worse, we are still at the PHP 4.3.0 support level. So, an overhaul of the XMB session handler is going to require major changes. |
|
The mt_srand() calls have been replaced in the trunk version of XMB. There was also an old ticket regarding the other session handling problems. That older ticket will be used to track those issues. |
Date Modified | Username | Field | Change |
---|---|---|---|
2020-08-15 05:36 | simonw | New Issue | |
2020-08-15 05:36 | simonw | Tag Attached: password | |
2020-08-17 20:10 | miqrogroove | Note Added: 0000385 | |
2020-08-18 02:02 | simonw | Note Added: 0000386 | |
2020-08-18 15:36 | miqrogroove | Note Added: 0000387 | |
2020-08-22 14:34 | miqrogroove | Status | new => confirmed |
2020-08-22 14:34 | miqrogroove | Product Version | 1.9.11.15 => 1.9.8 SP2 |
2020-08-22 14:34 | miqrogroove | Target Version | => 1.9.12 |
2020-08-22 14:34 | miqrogroove | Flags | => Schema Updates |
2020-08-22 14:44 | miqrogroove | Relationship added | related to 0000001 |
2020-08-23 08:48 | miqrogroove | Relationship added | related to 0000559 |
2020-08-28 05:39 | miqrogroove | Assigned To | => miqrogroove |
2020-08-28 05:39 | miqrogroove | Status | confirmed => resolved |
2020-08-28 05:39 | miqrogroove | Resolution | open => fixed |
2020-08-28 05:39 | miqrogroove | Fixed in Version | => 1.9.12 |
2020-08-28 05:39 | miqrogroove | Flags | Schema Updates => |
2020-08-28 05:39 | miqrogroove | SVN Revision | => 2798 |
2020-08-28 05:39 | miqrogroove | Note Added: 0000393 | |
2020-10-20 18:17 | miqrogroove | Status | resolved => closed |