[jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads

[Peter Firmstone]

Was ever to run with SecurityManager?

When you see an AccessControlException, I'd recommend setting the 
following security debug property, so you can capture the 
ProtectionDomain that failed the access check: 
-Djava.security.debug=access:failure  Clearly there's a ProtectionDomain 
on the calling threads stack that doesn't have the necessary 
permission.  If you knew which one it was, you could just grant it 
java.lang.RuntimePermission "setContextClassLoader" permission in the 
policy file.

In NativeResourceCleaner the original constructor is setting the Context 
ClassLoader of the calling thread to null, prior to calling the Thread 
superclass constructor, so that both the calling thread and new thread 
will nave a null context ClassLoader.  In your new implementation, you 
are asserting the context class loader of the created thread is null, 
without setting the context ClassLoader of the original calling thread, 
is that the intended behaviour?

Alternatively you could set the context ClassLoader of the calling 
thread to null using a PrivilegedAction, prior to creating the new 
thread and restore it?

If the original intent was to set the context ClassLoader of the new 
thread to null, then why not just do that, rather than use an assertion?

If assertions are not enabled it may run with a non null context 
ClassLoader?   What are the consequences?  It appears to me, the fix has 
created a bigger problem, rather than fixed it.  Just my 2 cents.

We use SecurityManager by default following principles of least 
privilege (only the code paths we need to execute), and the original 
reported bug is a non problem for us, we would capture the missing 
permission and grant it, these are permission grants for Java 16:

grant codebase "jrt:/jdk.crypto.cryptoki"
{
     permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util";
};

grant codebase "jrt:/jdk.crypto.ec"
{
     permission java.security.SecurityPermission 
"putProviderProperty.SunEC";
     permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.jca";
     permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.pkcs";
     permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util";
     permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util.math";
     permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util.math.intpoly";
     permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.x509";
};

Good call making NativeResourceCleaner implement Runnable instead of extending Thread though.

-- 
Regards,
  
Peter Firmstone
0498 286 363
Zeus Project Services Pty Ltd.


On 22/06/2021 11:32 pm, Sean Coffey wrote:
> Sufficient permissions missing if this code was ever to run with SecurityManager.
>
> Cleanest approach appears to be use of InnocuousThread to create the cleaner/poller threads.
> Test case coverage extended to cover the SecurityManager scenario.
>
> Reviewer request: @valeriepeng
>
> -------------
>
> Commit messages:
>   - 8269034: AccessControlException for SunPKCS11 daemon threads
>
> Changes: https://git.openjdk.java.net/jdk17/pull/117/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=117&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8269034
>    Stats: 112 lines in 5 files changed: 73 ins; 17 del; 22 mod
>    Patch: https://git.openjdk.java.net/jdk17/pull/117.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/117/head:pull/117
>
> PR: https://git.openjdk.java.net/jdk17/pull/117