Permission Bug in AtomicLongFieldUpdater and AtomicIntegerFieldUpdater

[Jeff Nisewanger]

On 4/21/2010 8:35 PM, David Holmes wrote:
> Jeff I'm a little confused about the reasoning here. I was surprised to
> discover that the atomic updaters allow you to perform accesses that
> direct bytecode and reflection do not (perhaps I knew this when we put
> them together but have since forgotten). So contrary to my expectation
> that the real checks occurred at set/get and so construction checks were
> not necessary, it seems that because there are no checks at set/get then
> construction checks are somewhat pointless anyway.
>
> Is that the view here? That the check under question is unnecessarily
> strict given that we don't perform more obvious checks at get/set time?
>

Since the checks during get/set() protect only type system
integrity it is therefore vital that the access security checks
are performed in the constructor. Somewhat similarly, the bytecode
verifier protects the type system and the constant pool resolution
performs the access security checks for compiled code.

My best guess is that the security check implicit in the call to
Class.getDeclaredField() wasn't intended when the original
code for the AtomicIntegerFieldUpdater constructor was
written. I haven't written or tried running any test cases on
the current behavior but have just read the implementation code
for the constructor. The getDeclaredField() security check
ultimately calls SecurityManager.checkMemberAccess() and the
javadoc for this says:

> The default policy is to allow access to PUBLIC members, as well
> as access to classes that have the same class loader as the caller.
> In all other cases, this method calls checkPermission with the
> RuntimePermission("accessDeclaredMembers") permission.

In the case of the constructor for AtomicIntegerFieldUpdaterImpl, the
classloader of the caller is the bootstrap loader. Therefore,
the RuntimePermission check will be skipped if the class
containing the declared field to be updated is a standard
JRE class (one also loaded by the bootstrap loader).
If the target class is from another classloader (the
application or applet loader) then the RuntimePermission check is
performed. That doesn't really make any sense as a security check for
constructing AtomicIntegerFieldUpdater instances.

The behavior of SecurityManager.checkMemberAccess() is
intended to limit the ability to introspect on the internal
implementation fields and field types of code unrelated to the
calling code (code from another classloader). In the case of
AtomicIntegerFieldUpdater, the reflective field discovery is
an internal intermediate result and the construction does not
complete normally unless other security checks validate that
the caller of the constructor could have legally accessed the
field via static bytecode (assuming the field's declaring class
was loadable via the caller's defining class loader).


Jeff



>
> Jeff Nisewanger said the following on 04/22/10 10:30:
>> On 4/16/2010 5:50 AM, David Holmes wrote:
>>> Hi Doug,
>>>
>>> <aside: FYI copies of my replies to security-dev are being held for
>>> approval as I'm not a subscriber.>
>>>
>>> Doug Lea said the following on 04/16/10 21:43:
>>>> On 04/15/10 18:34, Martin Buchholz wrote:
>>>>
>>>>> People are using Atomic field updaters to update fields in classes
>>>>> in other classloaders.
>>>>>
>>>>
>>>> I think the policy on this awaits interpretation by Jeff
>>>> or other members of security team. FWIW, my take is that
>>>> if users know that they may cross class loaders, then they
>>>> should wrap these in doPrivileged anyway. As in ...
>>>
>>> I'm coming around to agreeing with the proposed fix. My take is that
>>> the real security check should take place at the time the field is set:
>>>
>>> field_x_updater.set(obj, val);
>>>
>>> At this point the calling code must have the necessary permissions to
>>> set field x of the given obj of type T. And I believe we do indeed
>>> check this.
>>>
>>> When the AtomicXXXXFieldUpdater constructor binds itself to the Field
>>> object for T.x that's an optimization. There's no reason we couldn't
>>> do this on each call to set() - other than it would perform terribly.
>>> So in that sense the security checks that take place at construction
>>> are incidental** and so we should be as permissive as we can make
>>> them _provided_ that the actual set() call will make the necessary
>>> permission checks.
>>>
>>> ** This particular check is also incidental because we happen to use
>>> a public reflection method to get the Field object. We could just as
>>> easily have used a magic VM hook.
>>>
>>
>> This is describing the security checking philosophy of the
>> java.lang.reflect apis
>> which mimic the security semantics of static bytecode at the point at
>> which they
>> are dynamically invoked. They perform the full security
>> check on every get() or set() method. This has a substantial
>> performance penalty
>> for various reasons but it allows java.lang.reflect.Field instances to
>> be freely passed
>> around internally within an application or library's implementation
>> classes since the
>> actual security check is against the caller of the get/set() method.
>> Static bytecode
>> doesn't have these performance issues since the check is performed
>> once at
>> constant pool resolution time and the calling point is inherently
>> bound to that class.
>>
>> On the other hand, the java.util.concurrent.atomic APIs were designed
>> to allow
>> highly efficient atomic access where performing a full security check
>> on the set()
>> method would be a substantial performance burden. Therefore, all of
>> the access-oriented
>> security checks are performed at construction time and the set()
>> method (for example)
>> only performs type checks to ensure the integrity of the field offset
>> within the
>> enclosing object.
>>
>> I vaguely recall discussions from years past about the need to improve
>> the security-relevant
>> aspects of the javadoc so this distinction would be clear to
>> developers using the API.
>> However, I'm not seeing any of this in the jdk 7 docs. This needs to
>> be fixed. (!)
>>
>> The current webrev looks reasonable to me aside from the need to
>> improve the javadoc.
>>
>>
>> Jeff