Bug #2381
closed
Wrong authorization check
Description
This authorization check does not work correctly:
private function saveCategoryEdit( $id, $user, $plugin ) {
...
$row = new cbgjCategory( $_CB_database );
if ( $id ) {
$row->load( $id );
}
$authorized = cbgjClass::getAuthorization( $row, null, $row->user_id );
if ( in_array( 'cat_create', $authorized ) || in_array( 'mod_lvl1', $authorized ) ) {
It checks that the owner of the category has right to save, and not the saving logged-in user. This means that anyone can save any category by giving its id, which is a vuln.
Please review all authorizations to WHOM the check is made. Actually, probably you want to check against the logged-in user all the time, and not pass the user id to the getAuthorization() function ?
Updated by beat about 13 years ago
private function showCategoryMessage( $id, $user, $plugin ) {
has same issue.
Probably others too. So all getAuthorization uses need a full review.
Updated by krileon about 13 years ago
- Status changed from New to Feedback
- Assignee changed from krileon to beat
This is not true, myId is ALWAYS used during the authorization checks. The 3rd variable where user object or user_id can be specified is only to determine if say the categories user_id is equal to myId.
Updated by beat about 13 years ago
- Status changed from Feedback to Assigned
- Assignee changed from beat to krileon
Ok, sorry, didn't review the getAuthorization function before this. Right.
However, the case where $user->id is === 0 and $myId === 0 (guest) will still add 'usr_me' to the accesses:
if ( isset( $user->id ) ) {
if ( $user->id == $myId ) {
$access[] = 'usr_me'; // Me
}
}
Updated by krileon about 13 years ago
- Status changed from Assigned to Feedback
- Assignee changed from krileon to beat
This wouldn't be the case as non-registered users can not make it that far, this check is surrounded with the below check.
if ( in_array( 'usr_reg', $access ) ) {
which means the below must be true
if ( $myId ) {
for the access to have usr_reg otherwise is usr_guest, which won't make it to if ( isset( $user->id ) ) {. It's fine for user->id to be 0, but $myId will always have a value for it to make it that far.
Updated by beat about 13 years ago
- Status changed from Feedback to Rejected
- % Done changed from 0 to 100
great, then closing this one as rejected :-)