Permission Bug in AtomicLongFieldUpdater and AtomicIntegerFieldUpdater

[Martin Buchholz]

On Thu, Apr 15, 2010 at 17:49, David Holmes <David.Holmes at oracle.com> wrote:
> Hi Martin,
>
> If this proceeds I think you need to include AtomicReferenceFieldUpdater in
> this as well.

Agreed.

I may have created some confusion because the test in my webrev
did not actually demonstrate the problem.
I have since fixed that.

http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Atomic-security/test/java/util/concurrent/atomic/AccessControl.java.html

The problem happens even for well-behaved classes that only
use atomic updaters to update their own fields.  Now, they
could work around the problem by creating their atomic updater
inside a doPrivileged, but probably we intend for them not to
have to do that.

Martin

> But this is not a clear cut issue (security never is!).
>
> If I understand the test program correctly the problem arises when the
> target object's class was loaded by a different class-loader to the class
> doing the AtomicXXXFieldUpdater creation. This seems reasonable as
> getDeclaredField states:
>
>    SecurityException - If a security manager, s, is present and any of the
> following conditions is met:
>        * invocation of s.checkMemberAccess(this, Member.DECLARED) denies
> access to the declared field
>        * the caller's class loader is not the same as or an ancestor of the
> class loader for the current class and invocation of s.checkPackageAccess()
> denies access to the package of this class
>
> So here we would not have package access. What puzzles me is then why our
> following explicit checks pass:
>
>  sun.reflect.misc.ReflectUtil.ensureMemberAccess(
>                  caller, tclass, null, modifiers);
>  sun.reflect.misc.ReflectUtil.checkPackageAccess(tclass);
>
> It seems inconsistent that getDeclaredField's internal checks say you don't
> have permission to access this field, while what should amount to the same
> checks via ReflectUtil say you do. The question is: which check is wrong?
>
> David
> -----
>
>
> Martin Buchholz said the following on 04/16/10 08:34:
>>
>> Hi java.util.concurrent security team,
>>
>> People are using Atomic field updaters to update fields in classes in
>> other classloaders.
>>
>> Toby writes:
>>
>> We received a bug report for App Engine that AtomicLongFieldUpdater
>> (and its sibling) were failing with RuntimePermission
>> accessDeclaredMembers. Looking at the code, it appears that it is a
>> bug for that permission to be required. AtomicLongFieldUpdater (and
>> sibling) are already performing subsequent access checks that ensure
>> that the caller isn't accessing a field it shouldn't be. I've attached
>> a patch which moves the field lookups into doPrivileged blocks.
>>
>> ...
>>
>> There was a bug was filed by ehcache folks. I could follow up with
>> them to see if I could get some pointers to actual usage, if you're
>> looking for motivational reasoning.
>>
>> ---
>>
>> http://code.google.com/p/googleappengine/issues/detail?id=2769
>>
>> GAE Bug Submitter writes:
>>
>> AtomicLongFieldUpdater is broken in the production sandbox.
>>
>> The simple test application that I wrote (atomically increments and prints
>> a volatile long on each get request) works perfectly in the development
>> environment.  In production creating the field updater fails with an
>> AccessControlException when the internal CASUpdater tries to do reflection
>> (on a user class) in order to get a reference to the volatile field.
>>
>> I've attached the test app which works perfectly in development but fails
>> in production.
>>
>> This issue is linked to an associated Ehcache issue:
>>
>> https://jira.terracotta.org/jira/browse/EHC-617
>>
>> I have a modified experimental version of Toby's patch against openjdk7
>> here:
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Atomic-security/
>>
>> I'm afraid to touch this security stuff myself,
>> especially where we might be loosening permissions,
>> but in principle I agree with Toby.
>> (Even though atomic field updaters were designed to be
>> used within a module (whatever that is))
>>
>> The doPrivileged should not be too costly, as it's
>> only performed at updater creation time.
>>
>> Martin
>