Changeset 592

Timestamp:

02/13/14 17:43:02 (10 years ago)

Author:

Kris Deugau

Message:

/trunk

Review, fix, tweak, and fine-tune user permission add/update handling.

Confirm that it now correctly limits a user to granting only those
permissions they already hold, and only warns when the requested
permissions are really greater or mismatched.

Also catch a possible malicious escalation to superuser/admin status.

Confirm it *should* be impossible to generate this warning via normal
web UI actions; someone would have to manually construct a URL or POST
request with the appropriate fields.

See #30.

Location:

trunk

Files:

• DNSDB.pm (modified) (2 diffs)
• dns.cgi (modified) (5 diffs)

trunk/DNSDB.pm

@@ -1735 +1735 @@
 # Returns '>', '<', '=', '!'
 sub comparePermissions {
+  my $self = shift;
   my $p1 = shift;
   my $p2 = shift;
@@ -2816 +2817 @@
     # insert the user...  note we set inherited perms by default since
     # it's simple and cleans up some other bits of state
+##fixme:  need better handling of case of inherited or missing (!!) permissions entries
     my $sth = $dbh->prepare("INSERT INTO users ".
         "(group_id,username,password,firstname,lastname,phone,type,status,permission_id,inherit_perm) ".

trunk/dns.cgi

@@ -1247 +1247 @@
     } else {
 
-      # assemble a permission string - far simpler than trying to pass an
-      # indeterminate set of permission flags individually
-
-      # But first, we have to see if the user can add any particular
-      # permissions;  otherwise we have a priviledge escalation.  Whee.
-
+      my $permstring = 'i';     # start with "inherit"
+
+      # Remap passed checkbox states from webvar to integer/boolean values in %newperms
+      foreach (@permtypes) {
+        $newperms{$_} = (defined($webvar{$_}) && $webvar{$_} eq 'on' ? 1 : 0);
+      }
+
+      # Check for chained permissions.  Some permissions imply others;  make sure they get set.
+      foreach (keys %permchains) {
+        if ($newperms{$_} && !$newperms{$permchains{$_}}) {
+          $newperms{$permchains{$_}} = 1;
+        }
+      }
+
+      # check for possible priviledge escalations
       if (!$permissions{admin}) {
-        my %grpperms;
-        $dnsdb->getPermissions('group', $curgroup, \%grpperms);
-        my $ret = comparePermissions(\%permissions, \%grpperms);
-        if ($ret eq '<' || $ret eq '!') {
-          # User's permissions are not a superset or equivalent to group.  Can't inherit
-          # (and include access user doesn't currently have), so we force custom.
+        if ($webvar{perms_type} eq 'inherit') {
+          # Group permissions are only relevant if inheriting
+          my %grpperms;
+          $dnsdb->getPermissions('group', $curgroup, \%grpperms);
+          my $ret = $dnsdb->comparePermissions(\%permissions, \%grpperms);
+          if ($ret eq '<' || $ret eq '!') {
+            # User's permissions are not a superset or equivalent to group.  Can't inherit
+            # (and include access user doesn't currently have), so we force custom.
+            $webvar{perms_type} = 'custom';
+            $alterperms = 1;
+          }
+        }
+        my $ret = $dnsdb->comparePermissions(\%newperms, \%permissions);
+        if ($ret eq '>' || $ret eq '!') {
+          # User's new permissions are not a subset or equivalent to previous.  Can't add
+          # permissions user doesn't currently have, so we force custom.
           $webvar{perms_type} = 'custom';
           $alterperms = 1;
@@ -1265 +1284 @@
       }
 
-      my $permstring;
+##fixme:
+# could possibly factor building the meat of the permstring out of this if/elsif set, so
+# as to avoid running around @permtypes quite so many times
       if ($webvar{perms_type} eq 'custom') {
         $permstring = 'C:';
@@ -1271 +1292 @@
           if ($permissions{admin} || $permissions{$_}) {
             $permstring .= ",$_" if defined($webvar{$_}) && $webvar{$_} eq 'on';
-            $newperms{$_} = (defined($webvar{$_}) && $webvar{$_} eq 'on' ? 1 : 0);
+          } else {
+            $newperms{$_} = 0;  # remove permissions user doesn't currently have
           }
         }
@@ -1279 +1301 @@
         $dnsdb->getPermissions('user', $webvar{clonesrc}, \%newperms);
         $page->param(perm_clone => 1);
-      } else {
-        $permstring = 'i';
-      }
-      # "Chained" permissions.  Some permissions imply others;  make sure they get set.
+      }
+      # Recheck chained permissions, in the supposedly impossible case that the removals
+      # above mangled one of them.  This *should* be impossible via normal web UI operations.
       foreach (keys %permchains) {
         if ($newperms{$_} && !$newperms{$permchains{$_}}) {
@@ -1312 +1333 @@
                 $webvar{fname}, $webvar{lname}, $webvar{phone});
         if ($code eq 'OK') {
-          $newperms{admin} = 1 if $webvar{accttype} eq 'S';
+          $newperms{admin} = 1 if $permissions{admin} && $webvar{accttype} eq 'S';
           ($code2,$msg2) = $dnsdb->changePermissions('user', $webvar{uid}, \%newperms, ($permstring eq 'i'));
         }