Permission Bug in AtomicLongFieldUpdater and AtomicIntegerFieldUpdater

[David Holmes]

Martin Buchholz said the following on 04/16/10 11:38:
> On Thu, Apr 15, 2010 at 17:49, David Holmes <David.Holmes at oracle.com> wrote:
>> 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

Aside: you forgot to call pass().

> The problem happens even for well-behaved classes that only
> use atomic updaters to update their own fields. 

I don't think installing a security manager that permits nothing and 
then getting a SecurityException is a good indicator of a problem.

> 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.

That won't work. The doPrivileged doesn't give them any additional 
permissions in their code, so it has no affect. doPrivileged is only 
useful for bootstrap classes to have operations checked using the 
bootstrap class's permissions rather than the application class (that's 
a loose statement but suffices).

To answer my own question I think the ReflectUtils checks act the same 
way that getDeclaredField() inside a doPrivileged would act. To that end 
perhaps this suggested change is "correct". Further it seems to me that 
the real security check needs to be done when the updater is applied, 
not when it is created.

But I can't help but wonder what purpose this security check is serving, 
and whether circumventing it is the right thing to do. But I'll let the 
security folk chime in on that.

Thanks,
David


> 
> 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