Permission Bug in AtomicLongFieldUpdater and AtomicIntegerFieldUpdater
martinrb at google.com
Thu Apr 15 18:38:16 PDT 2010
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.
I may have created some confusion because the test in my webrev
did not actually demonstrate the problem.
I have since fixed that.
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.
> 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:
> caller, tclass, null, modifiers);
> 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?
> 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.
>> 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:
>> I have a modified experimental version of Toby's patch against openjdk7
>> 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.
More information about the security-dev