Permission Bug in AtomicLongFieldUpdater and AtomicIntegerFieldUpdater

[Jeff Nisewanger]

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