jdk.internal.reflect.ReflectionFactory and SecurityManager

[Claes Redestad]

On 2016-12-26 21:39, Peter Levart wrote:
> Hi Claes,
>
> On 12/26/2016 09:11 PM, Claes Redestad wrote:
>> Hi,
>>
>> with strong encapsulation in place this seems perfectly fine to me.
>>
>> The only downside is that we will lose the extra reminder that the
>> ReflectionFactory must not escape to untrusted code, but I think we can
>> all help ensure that doesn't happen, right? :-)
>
> With strong encapsulation in place, if the ReflectionFactory escaped, it
> could only be used by the following modules:
>
>     exports jdk.internal.reflect to
>         java.corba,
>         java.logging,
>         java.sql,
>         java.sql.rowset,
>         jdk.dynalink,
>         jdk.scripting.nashorn,
>         jdk.unsupported;
>
>
> If SecurityManager check was removed, those modules could then also
> obtain ReflectionFactory (they currently don't use it except
> jdk.unsupported). Does this present any new danger?

Right, it shouldn't be possible to do anything with a reference to an
encapsulated object, but let's not challenge this assumption here and
now.

>
> Changes needed to support removal of this SecurityManager check are as
> follows:
>
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/ReflectionFactory.withoutSM/webrev.01/
>
> Changes mostly remove the need to store ReflectionFactory into a
> variable, therefore they encourage style where ReflectionFactory is less
> likely to escape.

That's a good point, and changes looks good to me. I think this could go
as both a standalone cleanup and rolled into a redo of 8062389 etc.

Thanks!

/Claes

>
> Regards, Peter
>
>
>>
>> Thanks!
>>
>> /Claes
>>
>> On 2016-12-26 20:29, Peter Levart wrote:
>>> Hi,
>>>
>>> There are 2 ReflectionFactory classes in JDK 9. The old one is
>>> sun.reflect.ReflectionFactory which ended in jdk.unsupported module and
>>> to which access is restricted with SecurityManager. There is also new
>>> jdk.internal.reflect.ReflectionFactory which is used internally by JDK,
>>> is exported to internal modules only but still uses SecurityManager to
>>> restrict access to itself. I checked all usages and they all use
>>> AccessControler.doPrivileged() for obtaining the instance of
>>> jdk.internal.reflect.ReflectionFactory, which somehow defeats the
>>> purpose of SecurityManager access checks in this API.
>>>
>>> I think this could be simplified by removing the SecurityManager check
>>> from jdk.internal.reflect.ReflectionFactory#getReflectionFactory static
>>> method and change all usages to invoke this method directly without
>>> doPrivileged(). There are already two sensitive internal APIs exposed
>>> without SecurityManager checks: jdk.internal.misc.Unsafe#getUnsafe and
>>> various jdk.internal.misc.SharedSecrets#getXxxAccess methods. So why
>>> wouldn't internal ReflectionFactory be exposed the same way?
>>>
>>> This would make obtaining the ReflectionFactory more robust and not
>>> sensitive to bootstrap issues that surfaced recently after my push of a
>>> fix for issues 8062389, 8029459, 8061950.
>>>
>>> So, what do you think? Is this a worthwhile cleanup and simplification?
>>>
>>> Regards, Peter
>>>
>