Permission Bug in AtomicLongFieldUpdater and AtomicIntegerFieldUpdater

[Martin Buchholz]

Jeff,

I totally agree with your analysis.

I'd like to see better javadoc for java.util.concurrent.atomic
in general.  I'm not sure I'm the best person to come
up with the kind of security-related javadoc we want to see here.

The key idea is that, like an instance of Unsafe,
an updater is a token that gives its holder special powers
to update the field, inherited from the immediate creator
of the updater.

Martin

On Wed, Apr 21, 2010 at 17:30, Jeff Nisewanger
<jeffrey.nisewanger at oracle.com> wrote:
> 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
>