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

[Valerie Peng]

On Tue, 29 Jun 2021 00:07:41 GMT, 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
>
> Sean Coffey has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains four additional 
> commits since the last revision:
> 
>  - Edits from review
>  - Merge remote-tracking branch 'origin/master' into pkcs11-perms
>  - Move TokenPoller to Runnable
>  - 8269034: AccessControlException for SunPKCS11 daemon threads

Update looks good. Thanks, Valerie

-

Marked as reviewed by valeriep (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/117

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

[Sean Coffey]

> 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

Sean Coffey has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains four additional commits since 
the last revision:

 - Edits from review
 - Merge remote-tracking branch 'origin/master' into pkcs11-perms
 - Move TokenPoller to Runnable
 - 8269034: AccessControlException for SunPKCS11 daemon threads

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/117/files
  - new: https://git.openjdk.java.net/jdk17/pull/117/files/03af6494..e961ff09

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=117=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=117=01-02

  Stats: 3102 lines in 121 files changed: 2073 ins; 670 del; 359 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

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

[Seán Coffey]

Hi Valerie,

many thanks for the thorough review. I've taken all your feedback on 
board with the latest push. Some of the test anomalies were a result of 
previous iterations of test edits I had been making.


Regarding the extra edits in 
"src/java.base/share/lib/security/default.policy", I had assumed it 
would be ok to tidy up the module under edit but I've reverted the 
unrelated changes now.


I was doubtful about removing the AccessController.doPrivileged blocks 
around the InnocuousThread.newSystemThread calls given that I wasn't 
sure which path the calling code would come from but on re-examination, 
I think it's ok to remove. The module provides the necessary permissions 
already and use of InnocuousThread solves the original permissions 
issue. Thanks for spotting!


In test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java 
:


> +if (args.length > 0) {
+Policy.setPolicy(new SimplePolicy());
+System.setSecurityManager(new SecurityManager());
+}


Just curious, why split the loop into 2 and set the SecurityManager in 
between the two loops? Can't we just set the policy/security manager 
before the loop?
The test infra requires various permissions including the problem 
setContextClassLoader permission. I figured it was better to set up the 
test infra first and then trigger the security manager.


New edits just pushed for review.

regards,
Sean.


On 25/06/2021 23:31, Valerie Peng wrote:

On Tue, 22 Jun 2021 20:08:03 GMT, 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

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

   Move TokenPoller to Runnable

src/java.base/share/lib/security/default.policy line 131:


129: permission java.lang.RuntimePermission 
"accessClassInPackage.com.sun.crypto.provider";
130: permission java.lang.RuntimePermission 
"accessClassInPackage.jdk.internal.misc";
131: permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.*";

Can we just do necessary changes? I noticed that this file seems to have mixed 
style, i.e. some lines are longer than 80 chars and some break into 2 lines 
with length less than 80 chars. Since the whole file is mixed, maybe just do 
what must be changed.

src/java.base/share/lib/security/default.policy line 142:


140: permission java.security.SecurityPermission 
"clearProviderProperties.*";
141: permission java.security.SecurityPermission "removeProviderProperty.*";
142: permission java.security.SecurityPermission 
"getProperty.auth.login.defaultCallbackHandler";

Same "avoid unnecessary changes" comment here.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
952:


950: AccessController.doPrivileged((PrivilegedAction) () -> {
951: Thread t = InnocuousThread.newSystemThread(
952: "Poller " + getName(),

nit: "Poller " -> "Poller-" (like before)?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
956:


954: assert t.getContextClassLoader() == null;
955: t.setDaemon(true);
956: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1033:


1031: }
1032: cleaner = new NativeResourceCleaner();
1033: AccessController.doPrivileged((PrivilegedAction) () -> {

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> {} is 
un-necessary? I tried your test without it and test still passes.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1039:


1037: assert t.getContextClassLoader() == null;
1038: t.setDaemon(true);
1039: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1212:


1210:
1211: this.token = token;
1212: if (cleaner == null) {

This check seems duplicate to the one in createCleaner() call.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:


54: System.out.println("No NSS config found. Skipping.");
55: return;
56: }

Move this if-check block of code up before the for-loop?

-

PR: 

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

[Valerie Peng]

On Tue, 22 Jun 2021 20:08:03 GMT, 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
>
> Sean Coffey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move TokenPoller to Runnable

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 63:

> 61: Policy.setPolicy(new SimplePolicy());
> 62: System.setSecurityManager(new SecurityManager());
> 63: }

Just curious, why split the loop into 2 and set the SecurityManager in between 
the two loops? Can't we just set the policy/security manager before the loop?

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 137:

> 135: perms.add(new SecurityPermission("insertProvider.*"));
> 136: perms.add(new SecurityPermission("removeProvider.*"));
> 137: }

The test still pass without the following permission:

 perms.add(new RuntimePermission("accessClassInPackage.sun.*"));
 perms.add(new 
RuntimePermission("accessClassInPackage.sun.security.pkcs11.*"));
 perms.add(new SecurityPermission("clearProviderProperties.*"));

Remove them?

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.sh line 142:

> 140: -Dtest.src=${TESTSRC} \
> 141: -Dtest.classes=${TESTCLASSES} \
> 142: -Djava.security.debug=${DEBUG} \

Save these java options and use it for both invocation? This way it's easier to 
tell that there is no difference among these two except for the extra argument.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.sh line 143:

> 141: -Dtest.classes=${TESTCLASSES} \
> 142: -Djava.security.debug=${DEBUG} \
> 143: MultipleLogins ${TESTSRC}${FS}MultipleLogins.policy || exit 11

There is no MultipleLogins.policy file. The test just uses the internal 
SimplePolicy object. Maybe just use a string like "useSimplePolicy".

-

PR: https://git.openjdk.java.net/jdk17/pull/117

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

[Valerie Peng]

On Fri, 25 Jun 2021 19:39:22 GMT, Valerie Peng  wrote:

>> Sean Coffey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Move TokenPoller to Runnable
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
> 952:
> 
>> 950: AccessController.doPrivileged((PrivilegedAction) () -> {
>> 951: Thread t = InnocuousThread.newSystemThread(
>> 952: "Poller " + getName(),
> 
> nit: "Poller " -> "Poller-" (like before)?

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> 
{} is un-necessary? I tried your test without it and test still passes.

-

PR: https://git.openjdk.java.net/jdk17/pull/117

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

[Valerie Peng]

On Tue, 22 Jun 2021 20:08:03 GMT, 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
>
> Sean Coffey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move TokenPoller to Runnable

src/java.base/share/lib/security/default.policy line 131:

> 129: permission java.lang.RuntimePermission 
> "accessClassInPackage.com.sun.crypto.provider";
> 130: permission java.lang.RuntimePermission 
> "accessClassInPackage.jdk.internal.misc";
> 131: permission java.lang.RuntimePermission 
> "accessClassInPackage.sun.security.*";

Can we just do necessary changes? I noticed that this file seems to have mixed 
style, i.e. some lines are longer than 80 chars and some break into 2 lines 
with length less than 80 chars. Since the whole file is mixed, maybe just do 
what must be changed.

src/java.base/share/lib/security/default.policy line 142:

> 140: permission java.security.SecurityPermission 
> "clearProviderProperties.*";
> 141: permission java.security.SecurityPermission 
> "removeProviderProperty.*";
> 142: permission java.security.SecurityPermission 
> "getProperty.auth.login.defaultCallbackHandler";

Same "avoid unnecessary changes" comment here.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
952:

> 950: AccessController.doPrivileged((PrivilegedAction) () -> {
> 951: Thread t = InnocuousThread.newSystemThread(
> 952: "Poller " + getName(),

nit: "Poller " -> "Poller-" (like before)?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
956:

> 954: assert t.getContextClassLoader() == null;
> 955: t.setDaemon(true);
> 956: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1033:

> 1031: }
> 1032: cleaner = new NativeResourceCleaner();
> 1033: AccessController.doPrivileged((PrivilegedAction) () -> {

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> {} is 
un-necessary? I tried your test without it and test still passes.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1039:

> 1037: assert t.getContextClassLoader() == null;
> 1038: t.setDaemon(true);
> 1039: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1212:

> 1210: 
> 1211: this.token = token;
> 1212: if (cleaner == null) {

This check seems duplicate to the one in createCleaner() call.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:

> 54: System.out.println("No NSS config found. Skipping.");
> 55: return;
> 56: }

Move this if-check block of code up before the for-loop?

-

PR: https://git.openjdk.java.net/jdk17/pull/117

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

[Peter Firmstone]

Thanks Seán,

A good explanation. :)

Solaris was a very good platform for exposing and debugging race 
conditions, of course we have very good static analysis now.


Regards,

Peter.

On 23/06/2021 5:10 pm, Seán Coffey wrote:

Thank for the feedback Peter. Comments inline.

On 22/06/2021 22:40, Peter Firmstone wrote:

Was ever to run with SecurityManager?
I found the issue while porting to jdk8u where Solaris uses a 
configuration file with the SunPKCS11 Provider by default - We have 
tests to register Providers while SecurityManager is in place. This 
failed for SunPKCS11.


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.
Yes - that was one of my first actions. [1]. The jdk.crypto.cryptoki 
ProtectionDomain lacks the permission and rightly so IMO. The default 
policy doesn't grant "setContextClassLoader" permission to any JDK 
module. It's not required when we use InnocuousThread.


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?
Use of InnocuousThread is made in various JDK classes for similar 
purpose where daemon threads need to be run with limited privilege. 
Similar use seen in networking and ldap classes.




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?
InnocuousThread sets this to null. The assert is just a belt and 
braces approach which is a useful check during test runs. Again, 
similar approach done in other JDK libraries.


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.


see above. We shouldn't have an issue. A non-null classloader would 
lead to classloader memory leak in some environments.


regards,
Sean.



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.



[1]

access: domain that failed ProtectionDomain (jrt:/jdk.crypto.cryptoki 
)

 jdk.internal.loader.ClassLoaders$PlatformClassLoader@5433274e
 
 java.security.Permissions@7006c658 (
 ("java.io.FilePermission" "<>" "read")
 ("java.net.SocketPermission" "localhost:0" "listen,resolve")
 ("java.security.SecurityPermission" "clearProviderProperties.*")
 ("java.security.SecurityPermission" 
"getProperty.auth.login.defaultCallbackHandler")

 ("java.security.SecurityPermission" "putProviderProperty.*")
 ("java.security.SecurityPermission" "authProvider.*")
 ("java.security.SecurityPermission" "removeProviderProperty.*")
 ("java.util.PropertyPermission" "java.specification.version" "read")
 ("java.util.PropertyPermission" "java.vm.vendor" "read")
 ("java.util.PropertyPermission" "path.separator" "read")
 ("java.util.PropertyPermission" "os.version" "read")
 ("java.util.PropertyPermission" "java.vendor.url" "read")
 ("java.util.PropertyPermission" "java.vm.name" "read")
 ("java.util.PropertyPermission" 

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

[Seán Coffey]

Thank for the feedback Peter. Comments inline.

On 22/06/2021 22:40, Peter Firmstone wrote:

Was ever to run with SecurityManager?
I found the issue while porting to jdk8u where Solaris uses a 
configuration file with the SunPKCS11 Provider by default - We have 
tests to register Providers while SecurityManager is in place. This 
failed for SunPKCS11.


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.
Yes - that was one of my first actions. [1]. The jdk.crypto.cryptoki 
ProtectionDomain lacks the permission and rightly so IMO. The default 
policy doesn't grant "setContextClassLoader" permission to any JDK 
module. It's not required when we use InnocuousThread.


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?
Use of InnocuousThread is made in various JDK classes for similar 
purpose where daemon threads need to be run with limited privilege. 
Similar use seen in networking and ldap classes.




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?
InnocuousThread sets this to null. The assert is just a belt and braces 
approach which is a useful check during test runs. Again, similar 
approach done in other JDK libraries.


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.


see above. We shouldn't have an issue. A non-null classloader would lead 
to classloader memory leak in some environments.


regards,
Sean.



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.



[1]

access: domain that failed ProtectionDomain (jrt:/jdk.crypto.cryptoki 
)

 jdk.internal.loader.ClassLoaders$PlatformClassLoader@5433274e
 
 java.security.Permissions@7006c658 (
 ("java.io.FilePermission" "<>" "read")
 ("java.net.SocketPermission" "localhost:0" "listen,resolve")
 ("java.security.SecurityPermission" "clearProviderProperties.*")
 ("java.security.SecurityPermission" 
"getProperty.auth.login.defaultCallbackHandler")

 ("java.security.SecurityPermission" "putProviderProperty.*")
 ("java.security.SecurityPermission" "authProvider.*")
 ("java.security.SecurityPermission" "removeProviderProperty.*")
 ("java.util.PropertyPermission" "java.specification.version" "read")
 ("java.util.PropertyPermission" "java.vm.vendor" "read")
 ("java.util.PropertyPermission" "path.separator" "read")
 ("java.util.PropertyPermission" "os.version" "read")
 ("java.util.PropertyPermission" "java.vendor.url" "read")
 ("java.util.PropertyPermission" "java.vm.name" "read")
 ("java.util.PropertyPermission" "java.vm.specification.version" "read")
 ("java.util.PropertyPermission" "os.name" "read")
 ("java.util.PropertyPermission" 
"sun.security.pkcs11.allowSingleThreadedModules" "read")
 ("java.util.PropertyPermission" 

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

[Alan Bateman]

On Tue, 22 Jun 2021 20:08:03 GMT, 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
>
> Sean Coffey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move TokenPoller to Runnable

src/java.base/share/classes/module-info.java line 202:

> 200: jdk.charsets,
> 201: jdk.compiler,
> 202: jdk.crypto.cryptoki,

At some point I'd like to see this re-visited so we can avoid exporting this 
package to modules defined to the platform class loader.

-

PR: https://git.openjdk.java.net/jdk17/pull/117

Re: [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=117=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

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

[Sean Coffey]

> 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

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

  Move TokenPoller to Runnable

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/117/files
  - new: https://git.openjdk.java.net/jdk17/pull/117/files/2a168946..03af6494

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=117=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=117=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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

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

[Sean Coffey]

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=117=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