[Nestmates] RFR: 8199309: [Nestmates] The new Class nestmate methods should have SecurityManager checks

[David Holmes]

Hi John,

Thanks for looking at this.

On 12/03/2018 2:32 PM, John Rose wrote:
> On Mar 11, 2018, at 6:44 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> webrev:  http://cr.openjdk.java.net/~dholmes/8199309/webrev/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8199309
>>
>> Mandy noted that getNestHost/getNestMembers should have security manager checks consistent with the existing getEnclosingClass et al, and this was confirmed in discussion with the EG.
>>
>> isNestMateOf does not need a security check as it never exposes a Class instance. It is reworked to avoid the security check put inside getNestHost.
>>
>> The existing test is updated to run a second time with the default security manager and policy in use (for which we need to add an empty policy file for jtreg to "use".)
>>
>> A new test is added to trigger the expected SecurityExceptions.
> 
> 
> Looks good, except for one thing:  The checkPackageAccess calls should
> happen *after* the checks for primitives and arrays.
> 
> I don't think primitive or array classes interact well with checkPackageAccess.
> At least, that path (calling cPA from a prim or array) is not well exercised,
> and I'd rather not depend on it working correctly.

You have a point! AFAICS that method is not exercised currently when 
dealing with a primitive or array class as all the methods that utilize 
it - such as getEnclosingClass - return null in that case, and there is 
no security check in the null case. It did appear to work okay though :)

For primitives I expected the check to vacuously pass and wasn't 
concerned about optimising that infrequent case.

I was actually mis-thinking that for arrays we should check as-if 
dealing with the class of the element type, but that's wrong.

> You can return nestHost or nestMembers from a primitive or array without
> security implications, since they are self-hosting, so what comes out
> of the query is only what went in.

True.

> (Hmm… the same is true of *any* self-hosting class.)

"what comes out is what went in" is also true if you call getNestHost() 
on a real nest host.

So, let me exclude the cases of "what comes out is what went in"

http://cr.openjdk.java.net/~dholmes/8199309/webrev.v2/

But that does leave a slight wording issue with the @throws 
SecurityException. The way it reads now, how ever 
getNestHost()/getNestMembers() behaves you should be able to get the 
same result with your own package access check. This isn't in the webrev 
but how about:

* @throws SecurityException
*         If the returned class is not the current class, and
*         if a security manager, <i>s</i>, is present and the caller's
*         class loader is not the same as or an ancestor of the class
*         loader for the returned class and invocation of {@link
*         SecurityManager#checkPackageAccess s.checkPackageAccess()}
*         denies access to the package of the current class

and similarly for getNestMembers:

* @throws SecurityException
*         If any returned class is not the current class, and
*         if a security manager, <i>s</i>, is present and the caller's
*         class loader is not the same as or an ancestor of the class
*         loader for the returned class and invocation of {@link
*         SecurityManager#checkPackageAccess s.checkPackageAccess()}
*         denies access to the package of the current class

It's a bit of a mouthful but ...

Note that the current class and any returned classes are always in the 
same package.

Thanks,
David
-----

> — John
>