Has anyone spotted the security flaw? The revoke method shouldn't return the Reaper, it should perform it instead before the write lock is released.

At first glance, there appears to be a lot going on in the ecm object, the size of the collections are minimised by the following strategies:

  1. When a Permission goes out of scope, it becomes weakly reachable,
     it, along with the Set of AccessControlContext's are removed from
     the cache by garbage collection.  Most Permission checks create a
     Permission at the time of checking, so the Permission goes out of
     scope quickly.    Remember the Permission checks performed by the
     ECM are intended to be repeated in succession, such that an
     initial check is made and provided the AccessControlContext hasn't
     changed, no further checks are made.
  2. The execution cache keeps track of the Threads that are within the
     try finally block, these are added and removed during very short
     intervals, until at the time of revocation the write lock is
     obtained.  The execution cache is guaranteed to be cleaned as the
     end() method is always executed by finally.
  3. Set's are used to avoid duplication.
  4. Threads cannot be confused with each other, making it easy to
     determine when a thread is in the current execution cache.
  5. The revocation is not considered complete until the policy revoke
     method has completed.  There is a period where trust is reduced,
     during revocation, but revocation is not complete.


Peter Firmstone wrote:
Fred,

Does this look more like it?

Multiple policy, multiple Permission checks, and an execution cache to minimise re checking during revocation.

it's used like this:

ecm.begin(reaper);
try{
   ecm.checkPermission(p);
   ecm.checkPermission(q);
   // Do something briefly
   return something;
} finally {
   ecm.end();
}

/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements.  See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
*      http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.river.imp.security.policy.se;

import java.security.AccessControlContext;
import java.security.AccessControlException;
import java.security.AccessController;
import java.security.Permission;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.apache.river.api.security.ExecutionContextManager;
import org.apache.river.api.security.Reaper;
import org.apache.river.imp.util.ConcurrentWeakIdentityMap;

/**
* Only a Single instance of ECM is required per policy, it is threadsafe.
* Threads will wait until revoke is complete.
*
* @author Peter Firmstone
*/
public class ECM implements ExecutionContextManager{
private final ConcurrentMap<Permission,Set<AccessControlContext>> checkedCache; private final ConcurrentMap<AccessControlContext, Set<Thread>> executionCache; private final ConcurrentMap<Thread, Set<AccessControlContext>> threadAssociation;
   private final ConcurrentMap<Thread, Reaper> association;
   private final ReadWriteLock blockLock;
private final Lock rl; // This lock is held briefly by callers of begin and end.
   private final Lock wl; // This lock is held by revocation.
     ECM(){
checkedCache = new ConcurrentWeakIdentityMap<Permission, Set<AccessControlContext>>(); executionCache = new ConcurrentHashMap<AccessControlContext, Set<Thread>>(); threadAssociation = new ConcurrentHashMap<Thread, Set<AccessControlContext>>();
   association = new ConcurrentHashMap<Thread, Reaper>();
   blockLock = new ReentrantReadWriteLock();
   rl = blockLock.readLock();
   wl = blockLock.writeLock();
   }
     Set<Reaper> revoke(Set<Permission> perms){
   wl.lock();
   try {
       /* This is where we determine what needs to be revoked, we first
        * get all the AccessControlContexts for the Permission class,
        * these are removed from the checkedCache, then we narrow
        * down the AccessControlContext's to only those in the
* execution cache, each AccessControlContext in the execution cache
        * has checkPermission(Permission) called for each revoked
        * permission.  Any that throw AccessControlException will be
        * caught and have their Reaper run, for the Threads referenced
        * from that AccessControlContext.
        *
        * The RevokeableDynamicPolicy will have updated the current
        * Permission's after revocation, so the ProtectionDomain's in
* the AccessControlContext will now throw an AccessControlException
        * if the revocation applied to them.
        *
        * The wl lock will be released, any threads that were interrupted
        * will exit through the finally block and remove themselves
        * from the execution cache.  Those that aren't may throw
        * an exception and bubble up the stack, due to closing sockets
        * etc.
        *
        * The execution cache is comprised of the following fields:
        * executionCache
        * threadAssociation
        */
// Identify Permission's with matching class files to those revoked.
       Set<Class> permClasses = new HashSet<Class>();
       Iterator<Permission> itp = perms.iterator();
       while (itp.hasNext()){
       permClasses.add(itp.next().getClass());
       }
// Remove Permission's and AccessControlContexts from the checked cache.
       Map<Permission, Set<AccessControlContext>> removed =
           new HashMap<Permission, Set<AccessControlContext>>();
       Iterator<Permission> keysIt = checkedCache.keySet().iterator();
       while (keysIt.hasNext()){
       Permission p = keysIt.next();
       if (permClasses.contains(p.getClass())){
           Set<AccessControlContext> a = checkedCache.get(p);
           keysIt.remove();
           removed.put(p, a);
       }              }
       // Match the AccessControlContexts with the execution cache;
       Set<AccessControlContext> exCache = executionCache.keySet();
// Get the AccessControlContext's in the execution cache that fail. Set<AccessControlContext> accFails = new HashSet<AccessControlContext>();
       Iterator<Permission> retests = removed.keySet().iterator();
       while (retests.hasNext()){
       Permission p = retests.next();
       Set<AccessControlContext> rechecks = removed.get(p);
       Iterator<AccessControlContext> recheck = rechecks.iterator();
       while (recheck.hasNext()){
           AccessControlContext a = recheck.next();
           if (accFails.contains(a)) continue;
           // This really narrows down the checks.
           if (exCache.contains(a)){
           try {
               a.checkPermission(p);
           } catch (AccessControlException e){
               accFails.add(a);
           }
           }
       }
       }
       // Identify the threads and prepare reapers.
       Set<Reaper> reapers = new HashSet<Reaper>();
       Iterator<AccessControlContext> failedAcc = accFails.iterator();
       while (failedAcc.hasNext()){
       AccessControlContext fail = failedAcc.next();
       Set<Thread> threads = executionCache.get(fail);
       Iterator<Thread> i = threads.iterator();
       while (i.hasNext()) {
           Thread t = i.next();
           Reaper r = association.get(t);
           r.put(t);
           reapers.add(r);
       }              }
       return reapers;
   } finally {
       wl.unlock();
   }
   }

   public void begin(Reaper r) {
   Thread currentThread = Thread.currentThread();
   association.put(currentThread, r);      }

public void checkPermission(Permission p) throws AccessControlException {
   Thread currentThread = Thread.currentThread();
   AccessControlContext executionContext = AccessController.getContext();
   rl.lock();
   try {
       // execution cache.
Set<Thread> exCacheThreadSet = executionCache.get(executionContext);
       if ( exCacheThreadSet == null ){
exCacheThreadSet = Collections.synchronizedSet(new HashSet<Thread>()); Set<Thread> existed = executionCache.putIfAbsent(executionContext, exCacheThreadSet);
       if (existed != null){
           exCacheThreadSet = existed;
       }
       }
       exCacheThreadSet.add(currentThread);// end execution cache.
       // thread association
Set<AccessControlContext> thAssocSet = threadAssociation.get(currentThread);
       if ( thAssocSet == null ){
thAssocSet = Collections.synchronizedSet(new HashSet<AccessControlContext>()); Set<AccessControlContext> existed = threadAssociation.putIfAbsent(currentThread, thAssocSet);
       if (existed != null){
           thAssocSet = existed;
       }
       }
       thAssocSet.add(executionContext); // end thread association.
       // checkedCache - the permission check.
       Set<AccessControlContext> checked = checkedCache.get(p);
       if (checked == null ){
checked = Collections.synchronizedSet(new HashSet<AccessControlContext>()); Set<AccessControlContext> existed = checkedCache.putIfAbsent(p, checked);
       if (existed != null){
           checked = existed;
       }
       }
if ( checked.contains(executionContext)) return; // it's passed before. executionContext.checkPermission(p); // Throws AccessControlException
       // If we get here cache the AccessControlContext.
checked.add(executionContext); // end checkedCache. } finally {
       rl.unlock();
   }
   }

   public void end() {
   // Teardown.
   Thread t = Thread.currentThread();
   rl.lock();
   try {
       association.remove(t);
       Set<AccessControlContext> accSet = threadAssociation.remove(t);
       Iterator<AccessControlContext> it = accSet.iterator();
       while (it.hasNext()){
       AccessControlContext acc = it.next();
       executionCache.get(acc).remove(t);
       }
   }finally {
       rl.unlock();
   }
     }

}



Reply via email to