[concurrency-interest] Here's why Atomic*FieldReference access checking is broken

[Doug Lea]

Thanks for the careful explanation. In short, the current checks
reject reasonable EE usages, which can lead users to widen
permissions, which can lead to less overall security.

The solution to inspect other caller frames seems OK to me.
But acceptance into OpenJDK requires approval from security folks.
We shouldn't/won't change jsr166 base source until cleared.

So probably the best way to make this happen is to file a CR with
your proposed patch/fix and ask for review?

-Doug


On 10/03/2014 11:13 AM, David M. Lloyd wrote:
> This discussion fizzled out last time it happened (2011 or something?).  So to
> avoid that happening again, I'm going to (hopefully clearly and in simple
> unambiguous terms) explain what the problem is, why it should be fixed, why the
> problem occurs, and exactly what should be done to fix the problem.
>
> What the problem is
> -------------------
> The expectation that users have when creating atomic field updaters is that they
> have full access to any field that plain field accesses would have (I believe
> this was reasonably well-established - or at least implied - by all sides in the
> prior email thread).
>
> The problem, as observed by users, is that when you have code which looks like
> this:
>
>     public class Foo {
>        private volatile int foo;
>        private static final AtomicIntegerFieldUpdater up =
>              AtomicIntegerFieldUpdater.newUpdater(Foo.class, "foo");
>        ...
>     }
>
> ...if you run with a security manager, and Foo.class.getClassLoader() !=
> AtomicIntegerFieldUpdater.class.getClassLoader(), you get an access control
> exception unless Foo.class's ProtectionDomain includes the
> "accessDeclaredMembers".  This is very contrary to the primary expectation of
> users.
>
> Enter the obvious workaround - first grant the requisite permission, then do this:
>
>     public class Foo {
>        private volatile int foo;
>        private static final AtomicIntegerFieldUpdater up =
>              AccessController.doPrivileged(
>                 new PrivilegedAction<AtomicIntegerFieldUpdater>() {
>                    public AtomicIntegerFieldUpdater run() {
>                       return AtomicIntegerFieldUpdater.newUpdater(Foo.class,
> "foo");
>                    }
>                 }
>              );
>        ...
>     }
>
> Problem solved, yes?  No.  The newUpdater() method uses getCallerClass() to get
> the class which is requesting access to the field.  Since the caller class is
> actually Foo$1.class or some such, the equality check in the constructor of the
> implementation fails, causing an extra dynamic check to be run on every invocation.
>
> Here's the better, but still awful, non-obvious workaround:
>
>     public class Foo {
>        private volatile int foo;
>        private static AtomicIntegerFieldUpdater getIt() {
>           return AtomicIntegerFieldUpdater.newUpdater(Foo.class, "foo");
>        }
>
>        private static final AtomicIntegerFieldUpdater up =
>              AccessController.doPrivileged(
>                 new PrivilegedAction<AtomicIntegerFieldUpdater>() {
>                    public AtomicIntegerFieldUpdater run() {
>                       return getIt();
>                    }
>                 }
>              );
>        ...
>     }
>
> This solves the getCallerClass() problem, however it still requires that Foo get
> the dangerous accessDeclaredMembers permission.
>
> Why it is critical to fix this issue
> ------------------------------------
> Any code which uses Atomic*FieldUpdaters cannot run in a security manager
> environment (like, say, a standard Java EE configuration) without potentially
> destructive global permissions.  Period.  This reason should speak for itself.
>
> Why the problem occurs
> ----------------------
> The root of the problem traces back to SecurityManager.checkMemberAccess().
> This method is the one remaining method in all of SecurityManager which uses the
> calling class context (stack) in order to determine the nature of the access
> check that is needed.  Basically it assumes the stack will look like this:
>
>       Class               Method
>    0: SecurityManager     checkMemberAccess
>    1: java.lang.Class     checkMemberAccess
>    2: java.lang.Class     (some reflection API method call)
>    3: ????                <consuming user code>
>
> So, it looks at the class of position 3, and checks that class's class loader
> against the class loader of the class to whom access is being requested.  If the
> class loaders are equal, the check short-circuits and exits (permission
> granted).  Otherwise, we fall back to the permission check on the whole access
> control context.
>
> There are two problems with this.  The first problem is general - the assumption
> that positions 1 and 2 are java.lang.Class calls is never actually verified,
> meaning if some user code (for some reason) wanted to use this method to check
> the accessDeclaredMembers permission, it would not be able to do so (granted
> this is fairly unlikely).
>
> The second problem is specific to construction of Atomic*FieldUpdaters.  In this
> case the call stack looks like this:
>
>       Class                           Method
>    0: SecurityManager                 checkMemberAccess
>    1: java.lang.Class                 checkMemberAccess
>    2: java.lang.Class                 getDeclaredField
>    3: j.u.c.Atomic*FieldUpdater$*Impl <init>
>    4: j.u.c.Atomic*FieldUpdater       newUpdater
>    5: ????                            <consuming user code>
>
> The class loader at position 3 on the stack belongs to JDK code, and thus will
> probably have a null class loader or the system class loader.  If the user code
> is not a part of this class loader (and it almost never is, in the real world),
> then the fast check will always fail, which causes the fallback to the global
> permission check.
>
> What should - no, *must* - be done to fix the problem
> -----------------------------------------------------
> I understand that in the Java 9 SDK we're likely to see improved stack
> introspection APIs and mechanisms, which may allow this problem to be solved in
> other more elegant ways.  However, I'm focusing on the existing code because
> this issue still really needs to be fixed in 8 and 7, and even 6 for anyone who
> may still be maintaining a tree therefor.
>
> The fix must come in two parts.
>
> * Part 1 - checkMemberAccess assumptions should be verified
>
> This should simply amount to adding equality comparisons for frames 1 and 2 of
> the introspected call stack.
>
> * Part 2 - check for atomic field updaters
>
> The code in checkMemberAccess should examine positions 3 and 4 in the call stack
> (after an appropriate bounds check).  If they correspond to an
> Atomic*FieldUpdater implementation and the base class itself, respectively, then
> the class in position 5 should be used to verify the class loader rather than
> position 3.
>
> While this special-case fix is not pretty, it does solve the issue and does not
> measurably worsen the already somewhat fragile existing code. It also solves a
> potentially serious security issue, by removing the need to grant potentially
> dangerous globally effective permissions to any code using these
> otherwise-innocuous atomic constructs.  There is little danger of this kind of
> fix creeping out to other constructs, simply because there are no other similar
> constructs with the same problem in the JDK.
>
> Thank you for your consideration.