Theoretical data race on java.util.logging.Handler.sealed

[Daniel Fuchs]

Hi Peter,

I had a look at your later webrev 03 and it looks like a good
solution to fix the issue. I am glad to see the sealed
variable removed.

About using lambda I don't know whether we have guidelines
for that - in this case it certainly makes the code more
concise. It is good to remember that it might mean more
work in backporting fixes though - for those fixes that will
need to be backported.

It will be good to hear about Mandy's opinion...

best regards,

-- daniel

On 12/8/13 8:19 PM, Peter Levart wrote:
>
> On 12/04/2013 12:27 AM, Mandy Chung wrote:
>>
>> On 12/3/2013 1:44 AM, Peter Levart wrote:
>>> On 12/03/2013 09:51 AM, Peter Levart wrote:
>>>> Hi,
>>>>
>>>> While browsing the code of java.util.logging.Handler, I noticed a
>>>> theoretical possibility that a security check in a
>>>> j.u.l.StreamHandler be circumvented using a data race.
>>>>
>>>> There is a plain boolean instance field 'sealed' in j.u.l.Handler
>>>> that is pre-initialized to 'true' in field initializer.
>>>> StreamHandler sublcass' constructors overwrite this value with
>>>> 'false' at the beginning, then issue some operations which
>>>> circumvent security checks, and finally they reset the 'sealed'
>>>> value back to 'true' at the end.
>>>>
>>>> If a reference to an instance of StreamHandler or subclass is passed
>>>> to some thread without synchronization via data-race, this thread
>>>> can see 'true' or 'false' as the possible values of 'sealed'
>>>> variable, thus it is possible to circumvent security checks.
>>>>
>>>> One possibility to fix this is moving the field to StreamHandler and
>>>> making it final:
>>>>
>>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.01/
>>>>
>>>>
>>>> Just making the field volatile might not work. There is an ongoing
>>>> debate on concurrency-interest which suggests that volatile fields
>>>> are not exceptional in constructors like final fields are...
>>>>
>>>>
>>>> Regards, Peter
>>>>
>>>
>>> The proposed patch is not complete. There are several subclasses of
>>> StreamHandler in the java.util.logging package that also need a way
>>> to bypass security checks for some operations in their constructors.
>>> So here's the updated webrev which updates them with the same code as
>>> StreamHandler. This means that there are two copies of 'sealed' flag
>>> in object of type ConsoleHandler, for example, but only the one
>>> declared in ConsoleHandler is relevant for governing access checks:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.02/
>>>
>>>
>>> Before filing the bug, I'm asking the list whether this can be
>>> considered a bug...
>>>
>>
>> This does look a possible data race that might return a partially
>> constructed object with sealed = false.  I am not sure how likely we
>> will run into this race though.
>>
>> W.r.t. the patch, it might be better to get rid of the sealed field
>> and wrap the calls with doPrivileged with limited privilege (just
>> LoggingPermission("control"))
>>
>> Mandy
>
> H Mandy,
>
> I created an issue for it nevertheless:
>
>      https://bugs.openjdk.java.net/browse/JDK-8029781
>
> You're right, doPrivileged() is a more straight-forward approach than
> 'sealed' variable. Since this might only be considered for inclusion in
> JDK9 when lambdas are already a tried technology, how do you feel about
> using them for platform code like logging? As far as I know (just
> checked), lambda meta-factory is not using any j.u.logging, so there is
> no danger of initialization loops or similar:
>
> http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.03/
>
>
> Regards, Peter
>