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

[Peter Levart]

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