Project

General

Profile

Actions
Copy link

Bug #2381

closed

Wrong authorization check

Added by beat over 13 years ago. Updated over 13 years ago.

Status:
Rejected
Priority:
Normal
Assignee:
beat
Target version:
2.0
Start date:
08 March 2011
Due date:
% Done:

100%

Estimated time:

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 ?

Actions
Copy link
 #1

Updated by beat over 13 years ago

private function showCategoryMessage( $id, $user, $plugin ) {

has same issue.

Probably others too. So all getAuthorization uses need a full review.

Actions
Copy link
 #2

Updated by beat over 13 years ago

actually most have this vuln.

Actions
Copy link
 #3

Updated by krileon over 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.

Actions
Copy link
 #4

Updated by beat over 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
                }
            }
Actions
Copy link
 #5

Updated by krileon over 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.

Actions
Copy link
 #6

Updated by beat over 13 years ago

  • Status changed from Feedback to Rejected
  • % Done changed from 0 to 100

great, then closing this one as rejected :-)

Actions
Copy link

Also available in: Atom PDF