Permission Bug in AtomicLongFieldUpdater and AtomicIntegerFieldUpdater

[Martin Buchholz]

Hi David,

I think you've done a good job of convincing me!

The key idea is that updaters should mimic the access permissions
of regular field updates by callers.  Since those are not affected by
what's further up the stack, neither should the field updaters.

So I'm promoting myself from "messenger" to "advocate".

I've updated the webrev from prototype to "serious".

http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Atomic-security/

containing changes to the three field updater classes and cleanups
to the test case, and javac warning removals.

Arguably the new method getDeclaredFieldPrivileged belongs in
a package-private class like AtomicSupport.java, but I'd rather not
create such a class for just one method.

Jeff, please review.

Martin

On Fri, Apr 16, 2010 at 05:50, David Holmes <David.Holmes at oracle.com> 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.
>
> David
> -----
>
>>
>>>>>>> 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.
>>>
>>> That's not my understanding.  doPrivileged is useful whenever you want to
>>> perform an operation with permissions checked only on the current stack
>>> frame, and not on any other code executing up the stack.
>>>
>>
>> But if David is right about this not always sufficing ...
>>
>>> To be specific it will depend on the exact call stack. In general you'll
>>> get
>>> the least permissions based on your stack. So if application code with no
>>> permissions calls library code with permissions, the doPrivileged in the
>>> library code says "only look at my permissions" - and so the call can
>>> work.
>>> But if the application code with no permissions calls doPrivileged then
>>> it
>>> still has no permissions. A doPrivileged by the application code will
>>> only
>>> have an effect if the application code has the necessary permissions but
>>> there is a frame with fewer permissions further up the stack.
>>
>> ... then it may be necessary to issue the doPriviliged
>> inside the constructor in all cases, just in case it does
>> cross class loaders?
>>
>> -Doug
>>
>>
>