Patch to fix IDOR vulnerability in userinfo.php#1541
Conversation
Uses a hashed uid to appear in the url instead of the original uid
MekDrop
left a comment
MekDrop
left a comment
There was a problem hiding this comment.
Thanks for the fix! Is very rare for us to get such big PR from first time contributors!
Still I have found few problems that I think should be solved before this can be merged.
|
|
||
| CREATE TABLE users ( | ||
| `uid` mediumint(8) unsigned NOT NULL auto_increment, | ||
| `hash_uid` varchar(64) NOT NULL default '', |
There was a problem hiding this comment.
If we use DB field, it should be unique; just add similar line like 'login_name'
| */ | ||
| public function &getHash($hash_uid) { | ||
| $user = FALSE; | ||
| $sql = "SELECT * FROM " . $this->db->prefix('users') . " WHERE hash_uid = '" . $hash_uid . "'"; |
There was a problem hiding this comment.
hash_uid should be escaped in case of SQL Injection. $this->db->quote($hash_uid) or $this->db->quoteString($hash_uid) should be used here
| */ | ||
| public function &getHash($hash_uid) { | ||
| $user = FALSE; | ||
| $sql = "SELECT * FROM " . $this->db->prefix('users') . " WHERE hash_uid = '" . $hash_uid . "'"; |
There was a problem hiding this comment.
Àdding LIMIT 1 to then end of this SQL can improve execution speed
| $hash_uid = bin2hex(random_bytes(32)); | ||
| $user->assignVar('hash_uid', $hash_uid); | ||
| $sql = sprintf( | ||
| "UPDATE %s SET hash_uid = '%s' WHERE uid = '%u'", | ||
| $this->db->prefix('users'), $hash_uid, (int) $uid | ||
| ); | ||
| $result = $this->db->query($sql); |
There was a problem hiding this comment.
I'm not sure fully how to solve this but I see possible collision for users with same hash_uid problem.
Maybe here everything should be wrapped into transaction that first tries verify if user with such hash_uid exists and then if not try to update. If fails because of unique index violation, retry again.
| include 'mainfile.php'; | ||
| $hash_uid = (int) $_GET['uid']; | ||
| $hash_uid = $_GET['uid']; | ||
|
|
There was a problem hiding this comment.
The input should be filtered and tested before using. In the original code, we force it to be an integer before proceeding
There was a problem hiding this comment.
This is a little better. I recommend reviewing the icms_core_DataFilter::checkVar() method. Strings can still be malicious and validating input and access are the best defense against breaches. You can view how we've utilized it in a lot of places - includes/finduser.php is a good example.
|
I should have said this earlier - thank you! You have done what we encourage everyone to do -
Another thing that makes this important is that you focused on improving security and protecting user data. That is something we are very committed to. This is not a small thing, and we'd like more time to review on how best to assimilate this into our code base. We are in a release cycle and this is the branch we use for packaging and releasing. ImpressCMS is a very user-driven platform and just about everything is linked to a user through their ID. To maintain data integrity, we have to exercise due diligence when changing a key field (a primary, in most cases). Some of the questions we have are about handling changes to existing installations, especially the database changes necessary. Once the release of 2.0 is out, we'll have more time to focus on this. Again - thank you! |
4 participants
This PR prevents the existing IDOR vulnerability at the endpoint userinfo.php/?uid=. Currently hackers are able to gain access to profiles by manipulating the uid field in the url to retrieve any profile in the database. We prevent that by changing what is received in the url.
Changes were made to the database structure, the user object, and the redirect urls to userinfo.php to support implementing a new hash_uid variable. The hash_uid variable is a randomly generated 64 character long hex string. It is saved into the databases and replaces the uid that is shown in the url of the userinfo.php page to prevent users from gaining access to another user’s profile through guessing the uid and crafting the url. This hash_uid is also used to verify if a current user is the same as the profile user to enable editing access. It also retrieves the user’s profile. These changes help remove the IDOR vulnerability at the endpoint userinfo.php/?uid= by crafting the uid in the url.