View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0000301 | XMB1 | Bugs | public | 2009-03-15 13:19 | 2010-01-23 18:18 |
| Reporter | John Briggs | Assigned To | miqrogroove | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Product Version | 1.9.8 SP2 | ||||
| Target Version | 1.9.11.05 | Fixed in Version | 1.9.11.05 | ||
| Summary | 0000301: Last Super Admin Self-Demotion | ||||
| Description | This is a basic security issue. Being able to demote yourself or having another super admin demote another who is the primary super admin is abig no no within the logic. | ||||
| Steps To Reproduce | Go to editprofile.php and edit your own account as super admin. Select admin or other status and it will permit you to demote yourself. It will also allow any other super admins to demote themselves or you. This is a logic security issue and should be considered major. Great efforts were made within the admin panel to prevent this type of behavior and now editprofile allows it with ease. | ||||
| Tags | No tags attached. | ||||
| MySQL Version | |||||
| PHP Version | |||||
| Web Server | |||||
| Browser | |||||
| Flags | |||||
| Original Reporter | |||||
| SVN Revision | 1814 | ||||
|
|
It looks like I made a mistake at revision 1184 that changed the last-sa protection to check for deletes but not edits. However, after going through this with a fine-tooth comb, I cannot find any code that would have prevented a "super admin demote another who is the primary super admin". Demotion has always been allowed when there is more than one registered sadmin. |
|
2009-03-16 11:28
|
XMB-1.9.11-last-sadmin-demotion.patch.txt (3,871 bytes)
Index: cp.php
===================================================================
--- cp.php (revision 1745)
+++ cp.php (working copy)
@@ -1728,9 +1728,15 @@
} else {
$pending = '';
}
+
+ if ($member['status'] == 'Super Administrator') {
+ $disabledelete = ' disabled="disabled"';
+ } else {
+ $disabledelete = '';
+ }
?>
<tr bgcolor="<?php echo $altbg2?>" class="tablerow">
- <td align="center"><input type="checkbox" name="delete<?php echo $member['uid']?>" onclick="addUserDel(<?php echo $member['uid']?>, '<?php echo $member['username']?>', this)" value="<?php echo $member['uid']?>" /></td>
+ <td align="center"><input type="checkbox" name="delete<?php echo $member['uid']?>" onclick="addUserDel(<?php echo $member['uid']?>, '<?php echo $member['username']?>', this)" value="<?php echo $member['uid']?>"<?php echo $disabledelete; ?> /></td>
<td><a href="member.php?action=viewpro&member=<?php echo recodeOut($member['username']); ?>"><?php echo $member['username']?></a>
<?php if (X_SADMIN) { ?>
<br /><a href="editprofile.php?user=<?php echo recodeOut($member['username']); ?>"><strong><?php echo $lang['admin_edituseraccount']; ?></strong></a>
@@ -1777,8 +1783,8 @@
<?php
}
} else if (onSubmit('membersubmit')) {
- $query = $db->query("SELECT MIN(uid) FROM ".X_PREFIX."members WHERE status='Super Administrator'");
- $sa_uid = $db->result($query, 0);
+ $query = $db->query("SELECT COUNT(uid) FROM ".X_PREFIX."members WHERE status='Super Administrator'");
+ $sa_count = $db->result($query, 0);
$db->free_result($query);
$query = $db->query("SELECT uid, username, password, status FROM ".X_PREFIX."members $where");
@@ -1793,6 +1799,9 @@
if (!X_SADMIN && ($origstatus == "Super Administrator" || $status == "Super Administrator")) {
continue;
}
+ if ($origstatus == 'Super Administrator' And $status != 'Super Administrator' And $sa_count == 1) {
+ error($lang['lastsadmin'], false, '</td></tr></table></td></tr></table><br />');
+ }
$banstatus = postedVar('banstatus'.$mem['uid']);
$cusstatus = postedVar('cusstatus'.$mem['uid'], '', FALSE);
@@ -1807,7 +1816,7 @@
}
}
- if ($delete == $mem['uid'] && $delete != $self['uid'] && $delete != $sa_uid) {
+ if ($delete == $mem['uid'] && $delete != $self['uid'] && $origstatus != "Super Administrator") {
$dbname = $db->escape_var($mem['username']);
$db->query("DELETE FROM ".X_PREFIX."members WHERE uid=$delete");
$db->query("DELETE FROM ".X_PREFIX."buddys WHERE username='$dbname'");
Index: editprofile.php
===================================================================
--- editprofile.php (revision 1745)
+++ editprofile.php (working copy)
@@ -226,6 +226,13 @@
eval('$editpage = "'.template('admintool_editprofile').'";');
} else {
$status = postedVar('status');
+ $origstatus = $member['status'];
+ $query = $db->query("SELECT COUNT(uid) FROM ".X_PREFIX."members WHERE status='Super Administrator'");
+ $sa_count = $db->result($query, 0);
+ $db->free_result($query);
+ if ($origstatus == 'Super Administrator' And $status != 'Super Administrator' And $sa_count == 1) {
+ error($lang['lastsadmin']);
+ }
$cusstatus = postedVar('cusstatus', '', FALSE);
$langfilenew = postedVar('langfilenew');
$result = $db->query("SELECT devname FROM ".X_PREFIX."lang_base WHERE devname='$langfilenew'");
|
|
|
Attached patch file restores last-sa edit protection, removes ambiguous "primary super admin" code, and adds delete protection for all sadmins (must demote to delete). I would like your feedback before committing this. |
|
|
Looks fine. Apparently at some point the code that prevented demotion in cp was lost or coded over as it did exist at a point before 1.9.11 or even 10. I will test the above patch and return with feedback. Some of the legacy code does indeed exist in the current base code from what I see as well. 1.9.8 may be a good example to look for the older code for prevention. |
|
|
The way I'm reading 1.9.8, it prevents Admins from editing Super Admins, and prevents last-sa status changes. When 2 or more Supers are registered they would be able to edit each other. |
|
|
ah, then that's agood thing I caught it. You are correct. |
|
|
Code approved and tested. It serves it's function. Thanks Rob. |
|
|
As soon as I committed the patch, I realized what MIN(uid) was being used for. It was a guarantee that at least one sadmin would exist after a mass demotion. This does need more work. |
|
|
... And after reading 1.9.8 for the third time, it looks like MIN(uid) was never implemented properly. It was added in 1.9.4 and remained unchanged through 1.9.11. This now appears to be a legacy bug. |
|
2009-03-17 12:47
|
XMB-1.9.11-last-sadmin-part2.diff (2,049 bytes)
Index: cp.php
===================================================================
--- cp.php (revision 1808)
+++ cp.php (working copy)
@@ -1783,12 +1783,26 @@
<?php
}
} else if (onSubmit('membersubmit')) {
- $query = $db->query("SELECT COUNT(uid) FROM ".X_PREFIX."members WHERE status='Super Administrator'");
- $sa_count = $db->result($query, 0);
- $db->free_result($query);
-
$query = $db->query("SELECT uid, username, password, status FROM ".X_PREFIX."members $where");
+ // Guarantee this request will not remove all Super Administrators.
+ if (X_SADMIN And $db->num_rows($query) > 0) {
+ $saquery = $db->query("SELECT COUNT(uid) FROM ".X_PREFIX."members WHERE status='Super Administrator'");
+ $sa_count = $db->result($saquery, 0);
+ $db->free_result($saquery);
+
+ while($mem = $db->fetch_array($query)) {
+ if ($mem['status'] == 'Super Administrator' And postedVar('status'.$mem['uid']) != 'Super Administrator') {
+ $sa_count--;
+ }
+ }
+ if ($sa_count < 1) {
+ error($lang['lastsadmin'], false, '</td></tr></table></td></tr></table><br />');
+ }
+ $db->data_seek($query, 0);
+ }
+
+ // Now execute this request
while($mem = $db->fetch_array($query)) {
$origstatus = $mem['status'];
$status = postedVar('status'.$mem['uid']);
@@ -1799,9 +1813,6 @@
if (!X_SADMIN && ($origstatus == "Super Administrator" || $status == "Super Administrator")) {
continue;
}
- if ($origstatus == 'Super Administrator' And $status != 'Super Administrator' And $sa_count == 1) {
- error($lang['lastsadmin'], false, '</td></tr></table></td></tr></table><br />');
- }
$banstatus = postedVar('banstatus'.$mem['uid']);
$cusstatus = postedVar('cusstatus'.$mem['uid'], '', FALSE);
|
|
2009-03-17 12:50
|
XMB-1.9.11-last-sadmin-v2.patch.txt (4,159 bytes)
Index: cp.php
===================================================================
--- cp.php (revision 1806)
+++ cp.php (working copy)
@@ -1728,9 +1728,15 @@
} else {
$pending = '';
}
+
+ if ($member['status'] == 'Super Administrator') {
+ $disabledelete = ' disabled="disabled"';
+ } else {
+ $disabledelete = '';
+ }
?>
<tr bgcolor="<?php echo $altbg2?>" class="tablerow">
- <td align="center"><input type="checkbox" name="delete<?php echo $member['uid']?>" onclick="addUserDel(<?php echo $member['uid']?>, '<?php echo $member['username']?>', this)" value="<?php echo $member['uid']?>" /></td>
+ <td align="center"><input type="checkbox" name="delete<?php echo $member['uid']?>" onclick="addUserDel(<?php echo $member['uid']?>, '<?php echo $member['username']?>', this)" value="<?php echo $member['uid']?>"<?php echo $disabledelete; ?> /></td>
<td><a href="member.php?action=viewpro&member=<?php echo recodeOut($member['username']); ?>"><?php echo $member['username']?></a>
<?php if (X_SADMIN) { ?>
<br /><a href="editprofile.php?user=<?php echo recodeOut($member['username']); ?>"><strong><?php echo $lang['admin_edituseraccount']; ?></strong></a>
@@ -1777,12 +1783,26 @@
<?php
}
} else if (onSubmit('membersubmit')) {
- $query = $db->query("SELECT MIN(uid) FROM ".X_PREFIX."members WHERE status='Super Administrator'");
- $sa_uid = $db->result($query, 0);
- $db->free_result($query);
-
$query = $db->query("SELECT uid, username, password, status FROM ".X_PREFIX."members $where");
+ // Guarantee this request will not remove all Super Administrators.
+ if (X_SADMIN And $db->num_rows($query) > 0) {
+ $saquery = $db->query("SELECT COUNT(uid) FROM ".X_PREFIX."members WHERE status='Super Administrator'");
+ $sa_count = $db->result($saquery, 0);
+ $db->free_result($saquery);
+
+ while($mem = $db->fetch_array($query)) {
+ if ($mem['status'] == 'Super Administrator' And postedVar('status'.$mem['uid']) != 'Super Administrator') {
+ $sa_count--;
+ }
+ }
+ if ($sa_count < 1) {
+ error($lang['lastsadmin'], false, '</td></tr></table></td></tr></table><br />');
+ }
+ $db->data_seek($query, 0);
+ }
+
+ // Now execute this request
while($mem = $db->fetch_array($query)) {
$origstatus = $mem['status'];
$status = postedVar('status'.$mem['uid']);
@@ -1807,7 +1827,7 @@
}
}
- if ($delete == $mem['uid'] && $delete != $self['uid'] && $delete != $sa_uid) {
+ if ($delete == $mem['uid'] && $delete != $self['uid'] && $origstatus != "Super Administrator") {
$dbname = $db->escape_var($mem['username']);
$db->query("DELETE FROM ".X_PREFIX."members WHERE uid=$delete");
$db->query("DELETE FROM ".X_PREFIX."buddys WHERE username='$dbname'");
Index: editprofile.php
===================================================================
--- editprofile.php (revision 1806)
+++ editprofile.php (working copy)
@@ -226,6 +226,13 @@
eval('$editpage = "'.template('admintool_editprofile').'";');
} else {
$status = postedVar('status');
+ $origstatus = $member['status'];
+ $query = $db->query("SELECT COUNT(uid) FROM ".X_PREFIX."members WHERE status='Super Administrator'");
+ $sa_count = $db->result($query, 0);
+ $db->free_result($query);
+ if ($origstatus == 'Super Administrator' And $status != 'Super Administrator' And $sa_count == 1) {
+ error($lang['lastsadmin']);
+ }
$cusstatus = postedVar('cusstatus', '', FALSE);
$langfilenew = postedVar('langfilenew');
$result = $db->query("SELECT devname FROM ".X_PREFIX."lang_base WHERE devname='$langfilenew'");
|
|
|
New diff and revised full patch attached, to be committed tomorrow. |
|
|
Glad you found it. THanks Rob. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2009-03-15 13:19 | John Briggs | New Issue | |
| 2009-03-16 10:10 | miqrogroove | Status | new => acknowledged |
| 2009-03-16 10:34 | miqrogroove | Note Added: 0000168 | |
| 2009-03-16 10:34 | miqrogroove | Status | acknowledged => confirmed |
| 2009-03-16 10:38 | miqrogroove | Assigned To | => miqrogroove |
| 2009-03-16 10:38 | miqrogroove | Status | confirmed => assigned |
| 2009-03-16 10:38 | miqrogroove | Projection | none => tweak |
| 2009-03-16 10:38 | miqrogroove | ETA | none => < 1 day |
| 2009-03-16 10:38 | miqrogroove | Product Version | 1.9.11.04 => 1.9.11 |
| 2009-03-16 10:38 | miqrogroove | Target Version | => 1.9.11.05 |
| 2009-03-16 10:38 | miqrogroove | Summary | Editprofile Rank CHange Issue => Last Super Admin Self-Demotion |
| 2009-03-16 11:28 | miqrogroove | File Added: XMB-1.9.11-last-sadmin-demotion.patch.txt | |
| 2009-03-16 11:30 | miqrogroove | Note Added: 0000169 | |
| 2009-03-16 17:42 | John Briggs | Note Added: 0000173 | |
| 2009-03-16 17:50 | miqrogroove | Note Added: 0000174 | |
| 2009-03-16 23:04 | John Briggs | Note Added: 0000176 | |
| 2009-03-16 23:15 | John Briggs | Note Added: 0000177 | |
| 2009-03-17 11:29 | miqrogroove | SVN Revision | => 1808 |
| 2009-03-17 11:29 | miqrogroove | Status | assigned => resolved |
| 2009-03-17 11:29 | miqrogroove | Fixed in Version | => 1.9.11.05 |
| 2009-03-17 11:29 | miqrogroove | Resolution | open => fixed |
| 2009-03-17 11:40 | miqrogroove | Note Added: 0000178 | |
| 2009-03-17 11:40 | miqrogroove | Status | resolved => feedback |
| 2009-03-17 11:40 | miqrogroove | Resolution | fixed => reopened |
| 2009-03-17 11:52 | miqrogroove | Note Added: 0000179 | |
| 2009-03-17 11:52 | miqrogroove | Product Version | 1.9.11 => 1.9.8 SP2 |
| 2009-03-17 11:52 | miqrogroove | Fixed in Version | 1.9.11.05 => |
| 2009-03-17 12:47 | miqrogroove | File Added: XMB-1.9.11-last-sadmin-part2.diff | |
| 2009-03-17 12:50 | miqrogroove | File Added: XMB-1.9.11-last-sadmin-v2.patch.txt | |
| 2009-03-17 12:53 | miqrogroove | Note Added: 0000180 | |
| 2009-03-17 12:53 | miqrogroove | Status | feedback => assigned |
| 2009-03-17 16:13 | John Briggs | Note Added: 0000182 | |
| 2009-03-18 10:40 | miqrogroove | SVN Revision | 1808 => 1814 |
| 2009-03-18 10:40 | miqrogroove | Status | assigned => resolved |
| 2009-03-18 10:40 | miqrogroove | Fixed in Version | => 1.9.11.05 |
| 2009-03-18 10:40 | miqrogroove | Resolution | reopened => fixed |
| 2010-01-23 18:18 | miqrogroove | Status | resolved => closed |
