RFR(S) 8166304: Skipping access check for classes generated by core reflection

[harold seigel]

Hi Karen, Lois,

Thanks for your reviews!  Please see this new webrev that incorporates 
your comments:

    http://cr.openjdk.java.net/~hseigel/bug_8166304.2/

Thanks, Harold


On 11/16/2016 9:31 AM, Karen Kinnear wrote:
> Harold,
>
> Creative solution to this problem!
>
> I have to agree with Lois’ comments. I struggled with the non_reflection_class_loader call which is maybe too subtle
> since you were piggybacking on an existing call. The way I understand it,
> when you call non_reflection_class_loader, IF it is a reflection class loader, then it returns the delegating parent class,
> and therefore the result you come back with is different than the class loader you called it with.
>
> It would be cleaner to just check if the class loader is not null, check against reflect_DelegatingClassLoader_klass() as
> Lois suggested, which is the core check you were extracting from non_reflection_class_loader anyway.
>
> thanks,
> Karen
>
>> On Nov 15, 2016, at 3:40 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
>>
>>
>> On 11/15/2016 9:09 AM, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this fix for bug JDK-8166304.  The fix throws an IllegalAccessError exception when attempting to create a class that is a sub-type of a jdk.internal.reflect class, unless the class loader is either the boot loader or the special reflection class loader.  This prevents user classes from being able to extend jdk.internal.reflect classes in order to bypass Reflection.getCallerClass.
>>>
>>> Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8166304/
>> Hi Harold,
>>
>> A couple of comments.
>>
>> src/share/vm/classfile/classFileParser.cpp
>> line #4360 - I think you should reconfigure the if conditional. Make the check for the boot class loader first so a fast exit from the conditional exists if this is the boot class loader.  In other words don't compare the symbol names.  Also, use a Symbol::fast_compare instead of "==" to compare the name of the packages.
>>
>> line #4362 - I would like to understand better the following check
>>
>> 4362           this_klass->class_loader() ==
>> 4363             java_lang_ClassLoader::non_reflection_class_loader(this_klass->class_loader())
>>
>> When this_klass's class loader data's _class_loader field is set the non_reflection_class_loader is not called, so I am having difficulty understanding how this conditional could ever be true.  I am curious why the check isn't instead, "this_klass->class_loader()->is_a(SystemDictionary::reflect_DelegatingClassLoader_klass())"?
>>
>> Thanks,
>> Lois
>>
>>> JBS bug: https://bugs.openjdk.java.net/browse/JDK-8166304
>>>
>>> The fix was tested with the JCK Lang and vm tests, the JTreg hotspot, java/io, java/lang, and java/util tests, the nsk co-located and the non-co-located tests, the RBT tier2 tests, and the test in the webrev.
>>>
>>> A sample error message looks like this:
>>>
>>> java.lang.IllegalAccessError: class FakeMethodAccessor loaded by jdk/internal/loader/ClassLoaders$AppClassLoader cannot access jdk/internal/reflect superclass jdk.internal.reflect.MethodAccessorImpl
>>>
>>> Thanks, Harold
>>>