Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SecurityServicesFeature unregisters Providers too early? #3664

Closed
zakkak opened this issue Aug 10, 2021 · 8 comments
Closed

SecurityServicesFeature unregisters Providers too early? #3664

zakkak opened this issue Aug 10, 2021 · 8 comments
Assignees

Comments

@zakkak
Copy link
Collaborator

zakkak commented Aug 10, 2021

Describe the issue
SecurityServicesFeature fails to automatically register SunSASL provider and requires the addition of -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider despite being specified in the JCA.

Steps to reproduce the issue

git clone --depth 1 --branch sasl-services-not-included https://github.com/zakkak/issue-reproducers reproducers
cd reproducers
mvn clean package
native-image -H:+TraceSecurityServices -jar target/reproducer-1.0-SNAPSHOT.jar

Describe GraalVM and your environment:

  • GraalVM version: c8285bd
  • JDK major version: 11
  • OS: Fedora 34
  • Architecture: AMD64

More details

Compiling with -H:+TraceSecurityServices and without -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider results in the following report:

The following security providers were deemed to be unused and removed:
    ProviderName - ProviderClass
    SUN - sun.security.provider.Sun
    SunRsaSign - sun.security.rsa.SunRsaSign
    SunEC - sun.security.ec.SunEC
    SunJSSE - com.sun.net.ssl.internal.ssl.Provider
    SunJCE - com.sun.crypto.provider.SunJCE
    SunJGSS - sun.security.jgss.SunProvider
    SunSASL - com.sun.security.sasl.Provider
    XMLDSig - org.jcp.xml.dsig.internal.dom.XMLDSigRI
    SunPCSC - sun.security.smartcardio.SunPCSC
    JdkLDAP - sun.security.provider.certpath.ldap.JdkLDAP
    JdkSASL - com.sun.security.sasl.gsskerb.JdkSASL
    SunPKCS11 - sun.security.pkcs11.SunPKCS11

While adding -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider results in:

Marked provider com.sun.security.sasl.Provider as used
Service factory method javax.crypto.Cipher.getInstance(java.lang.String) is reachable.
    Analysis parsing context: 
        at com.sun.security.ntlm.NTLM.<init>(NTLM.java:83)
        at com.sun.security.ntlm.Client.<init>(Client.java:78)
        at com.sun.security.sasl.ntlm.NTLMClient.<init>(NTLMClient.java:174)
        at com.sun.security.sasl.ntlm.FactoryImpl.createSaslClient(FactoryImpl.java:79)
        at javax.security.sasl.Sasl.createSaslClient(Sasl.java:433)
        at Main.main(Main.java:17)
        at com.oracle.svm.core.JavaMainWrapper.runCore(JavaMainWrapper.java:146)
        at com.oracle.svm.core.JavaMainWrapper.run(JavaMainWrapper.java:182)
 ...

i.e, it appears that javax.crypto.Cipher.getInstance(java.lang.String) becomes reachable only when -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider is used. Adding -H:+PrintAnalysisCallTree we indeed see that when -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider is used the following methods become reachable:

javax.crypto.Cipher.getInstance(String):Cipher
javax.crypto.Mac.getInstance(String):Mac
javax.crypto.SecretKeyFactory.getInstance(String):SecretKeyFactory

resulting in the automatic registration of Sun and SunJCE. Not of SunSasl though which only ends up in the image because we explicitly added it as an additional security provider, indicating another possible issue. Looking at com.oracle.svm.hosted.SecurityServicesFeature#knownServices we observe that all classes except for SaslClientFactory and SaslServerFactory provide the method getInstace, while SaslClientFactory and SaslServerFactory provide createSaslClient and createSaslServer respectively. As a result in order to automatically register Sasl services I believe we should register handlers for those methods like we do for getInstance.

The following prototype of the above :

diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java
index 1c1aa6a8a8d..1fbedebc212 100644
--- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java
+++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java
@@ -491,6 +491,9 @@ public class SecurityServicesFeature extends JNIRegistrationUtil implements Feat
                     checkGetInstanceMethod(method);
                     /* The handler will be executed only once if any of the methods is triggered. */
                     access.registerMethodOverrideReachabilityHandler(handler, method);
+                } else if (method.getName().equals("createSaslClient") || method.getName().equals("createSaslServer")) {
+                    /* The handler will be executed only once if any of the methods is triggered. */
+                    access.registerMethodOverrideReachabilityHandler(handler, method);
                 }
             }
         }

results in the following report which indicates that SunSASL is being registered due to the analysis and not only due to the explicit addition of it through -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider which is still required to register SunSasl even after the patch:

Service factory method com.sun.security.sasl.ClientFactoryImpl.createSaslClient(java.lang.String[], java.lang.String, java.lang.String, java.lang.String, java.util.Map, javax.security.auth.callback.CallbackHandler) is reachable.
    Analysis parsing context: 
        at javax.security.sasl.Sasl.createSaslClient(Sasl.java:433)
        at Main.main(Main.java:33)
        at com.oracle.svm.core.JavaMainWrapper.runCore(JavaMainWrapper.java:146)
        at com.oracle.svm.core.JavaMainWrapper.run(JavaMainWrapper.java:182)
        at com.oracle.svm.core.code.IsolateEnterStub.JavaMainWrapper_run_5087f5482cc9a6abc971913ece43acb471d2631b(generated:0)
    Reachability of SaslClientFactory service type API triggers registration of following services:
        Type: SaslClientFactory, Provider: SunSASL, Algorithm: CRAM-MD5, Class: com.sun.security.sasl.ClientFactoryImpl
        Type: SaslClientFactory, Provider: SunSASL, Algorithm: DIGEST-MD5, Class: com.sun.security.sasl.digest.FactoryImpl
        Type: SaslClientFactory, Provider: SunSASL, Algorithm: PLAIN, Class: com.sun.security.sasl.ClientFactoryImpl
        Type: SaslClientFactory, Provider: SunSASL, Algorithm: EXTERNAL, Class: com.sun.security.sasl.ClientFactoryImpl
        Type: SaslClientFactory, Provider: JdkSASL, Algorithm: GSSAPI, Class: com.sun.security.sasl.gsskerb.FactoryImpl
        Type: SaslClientFactory, Provider: SunSASL, Algorithm: NTLM, Class: com.sun.security.sasl.ntlm.FactoryImpl

As javax.security.sasl.Sasl#createSaslClient and javax.security.sasl.Sasl#createSaslServer invoke Security.getProviders(String) my intuition is that in the absence of -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider graal doesn't return the SunSasl provider and thus javax.security.sasl.SaslClientFactory#createSaslClient and javax.security.sasl.SaslServerFactory#createSaslServer are not seen as reachable and thus SunSasl is not getting registered. Does this make sense?

@zakkak
Copy link
Collaborator Author

zakkak commented Aug 11, 2021

cc @gradinac

@zakkak
Copy link
Collaborator Author

zakkak commented Jan 20, 2022

@gradinac did you have the chance to take a look at this? I am willing to work on a fix for it, but I would like to get some feedback about my claims from you first.

Thanks

@gradinac
Copy link
Contributor

Hey @zakkak! Sorry for a late answer on this. The security services feature currently automatically registers only services that follow the JCA. Unfortunately, that doesn't seem to be the case for the SASL provider, but, we still would want to support it. The proposed changes seem good - though we will most likely encounter this again in the future - perhaps we could keep these special methods in a set somewhere and iterate over them when checking if we should register a reachability handler.

One thing that confused me a bit is that -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider is still required - is the provider of the Sasl(Client|Server)Factory different to this one or is there another reason why this is still required?

@zakkak
Copy link
Collaborator Author

zakkak commented Jan 25, 2022

One thing that confused me a bit is that -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider is still required - is the provider of the Sasl(Client|Server)Factory different to this one or is there another reason why this is still required?

Hi @gradinac, as mentioned in the description:

As javax.security.sasl.Sasl#createSaslClient and javax.security.sasl.Sasl#createSaslServer invoke Security.getProviders(String) my intuition is that in the absence of -H:AdditionalSecurityProviders=com.sun.security.sasl.Provider graal doesn't return the SunSasl provider and thus javax.security.sasl.SaslClientFactory#createSaslClient and javax.security.sasl.SaslServerFactory#createSaslServer are not seen as reachable and thus SunSasl is not getting registered. Does this make sense?

Explaining in more detail and links to the corresponding code below:

The reproducer invokes Sasl.createSaslClient which conditionally invokes javax.security.sasl.SaslClientFactory#createSaslClient if saslProvider is present. GraalVM however removes all "unused" providers before the analysis. So my understanding is that the analysis marks javax.security.sasl.SaslClientFactory#createSaslClient as unreachable (which I verified with -H:+PrintAnalysisCallTree) and thus saslProvider never gets registered through the registerServices handler.

Applying

diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SecuritySubstitutions.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SecuritySubstitutions.java
index 03c931b8c9b..a2032223019 100644
--- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SecuritySubstitutions.java
+++ b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SecuritySubstitutions.java
@@ -680,7 +680,8 @@ final class Target_sun_security_jca_Providers {
         @Override
         public Object transform(MetaAccessProvider metaAccess, ResolvedJavaField original, ResolvedJavaField annotated, Object receiver, Object originalValue) {
             ProviderList originalProviderList = (ProviderList) originalValue;
-            return SecurityProvidersFilter.instance().cleanUnregisteredProviders(originalProviderList);
+            return originalProviderList;
+//            return SecurityProvidersFilter.instance().cleanUnregisteredProviders(originalProviderList);
         }
     }
 }
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java
index 3f6e23b3ec3..32305919342 100644
--- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java
+++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java
@@ -538,6 +538,9 @@ public class SecurityServicesFeature extends JNIRegistrationUtil implements Feat
                     checkGetInstanceMethod(method);
                     /* The handler will be executed only once if any of the methods is triggered. */
                     access.registerMethodOverrideReachabilityHandler(handler, method);
+                } else if (method.getName().equals("createSaslClient") || method.getName().equals("createSaslServer")) {
+                    /* The handler will be executed only once if any of the methods is triggered. */
+                    access.registerMethodOverrideReachabilityHandler(handler, method);
                 }
             }
         }

makes it work, confirming(?) my theory.

@gradinac
Copy link
Contributor

@zakkak Thank you for a detailed explanation! Upon investigating it a bit further, it seems like Graal indeed doesn't see the createSaslClient method as reachable. This turned out to be a bit of a chicken and egg problem - Sasl#createSaslClient invokes the SaslClient#createSaslClient method - however, since we are filtering out the available providers during analysis, we did some folding in that method that made SaslClient#createSaslClient unreachable.

I've created a PR that should fix the automatic registration of this provider: #4279
If you have the chance to, could you try it out? I've tested it on your reproducer and it seems to work

@zakkak
Copy link
Collaborator Author

zakkak commented Feb 1, 2022

I've created a PR that should fix the automatic registration of this provider: #4279
If you have the chance to, could you try it out? I've tested it on your reproducer and it seems to work

It looks good, thank you @gradinac.

@warrenc5
Copy link

Thank you, adding this on 22.2.0-r11 solves the problem with jboss remoting3 ( 5.0.18.Final )
You can see the actual error if you turn logging to ALL.

failed :Authentication failed: none of the mechanisms presented by the server (DIGEST-MD5) are supported source: null
[[org.jboss.remoting3.remote.ClientConnectionOpenListener$Capabilities.handleEvent(ClientConnectionOpenListener.java:443), org.jboss.remoting3.remote.ClientConnectionOpenListener$Capabilities.handleEvent(ClientConnectionOpenListener.java:244), org.xnio.ChannelListeners.invokeChannelListener(ChannelListeners.java:92), org.xnio.conduits.ReadReadyHandler$ChannelListenerHandler.readReady(ReadReadyHandler.java:66), org.xnio.nio.NioSocketConduit.handleReady(NioSocketConduit.java:89), org.xnio.nio.WorkerThread.run(WorkerThread.java:571), com.oracle.svm.core.thread.PlatformThreads.threadStartRoutine(PlatformThreads.java:705), com.oracle.svm.core.posix.thread.PosixPlatformThreads.pthreadStartRoutine(PosixPlatformThreads.java:202), ...asynchronous invocation...(Unknown Source), org.jboss.remoting3.EndpointImpl.connect(EndpointImpl.java:599), org.jboss.remoting3.EndpointImpl.connect(EndpointImpl.java:561), org.jboss.remoting3.EndpointImpl.connect(EndpointImpl.java:549), org.jboss.remotingjmx.RemotingConnector.internalRemotingConnect(RemotingConnector.java:268), org.jboss.remotingjmx.RemotingConnector.internalConnect(RemotingConnector.java:156), org.jboss.remotingjmx.RemotingConnector.connect(RemotingConnector.java:103), org.jboss.remotingjmx.RemotingConnector.connect(RemotingConnector.java:98),

@zakkak
Copy link
Collaborator Author

zakkak commented Oct 3, 2022

Fixed by #4279, I can no longer reproduce with master (e34853d). Thank you @gradinac !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants