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

[Mandy Chung]

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