apache / mina-sshd

Apache MINA sshd is a comprehensive Java library for client- and server-side SSH.
https://mina.apache.org/sshd-project/
Apache License 2.0
847 stars 353 forks source link

incorrect classloading of SecurityProviders #502

Open jtnord opened 1 month ago

jtnord commented 1 month ago

Version

2.12.1

Bug description

The mina plugin attempts work out if EdDSA is supported using a different classloader than it actually uses for using EdDSASecurityProviderUtils leading to ClassNotFoundExceptions

Actual behavior

NoClassDefError

Caused by: java.lang.NoClassDefFoundError: net/i2p/crypto/eddsa/spec/EdDSAParameterSpec
    at org.apache.sshd.common.util.security.SecurityUtils.getEDDSAPublicKeyEntryDecoder(SecurityUtils.java:580)
    at org.apache.sshd.common.config.keys.KeyUtils.<clinit>(KeyUtils.java:187)
    at org.apache.sshd.common.config.keys.loader.openssh.OpenSSHRSAPrivateKeyDecoder.<clinit>

Expected behavior

No classloading issues. the code uses the same classloader to load EdDSA as it uses to see if EdDSA is supported.

Relevant log output

Other information

if you have a non flat classloader (e.g. Jenkins, or any modular app server) then the code has a non reflective way of determining if something is supported or not.

e.g.

https://github.com/apache/mina-sshd/blob/9ceba3e7ffaeaf79705c2abe3a3cc8aea86fd017/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityUtils.java#L575-L581

we see a call to isEDDSACurveSupported before we attempt to use a staic method from the class EdDSASecurityProviderUtils

However https://github.com/apache/mina-sshd/blob/9ceba3e7ffaeaf79705c2abe3a3cc8aea86fd017/sshd-common/src/main/java/org/apache/sshd/common/util/security/SecurityUtils.java#L566-L571

looks for the presence of a security provider called "EdDSA" which in no way reflects if the classes are available or not.

If the "EdDSA" provider was statically registered in the bootclasspath this would work, however it is generally registered by EdDSASecurityProviderRegistrar

As can be seen here https://github.com/apache/mina-sshd/blob/9ceba3e7ffaeaf79705c2abe3a3cc8aea86fd017/sshd-common/src/main/java/org/apache/sshd/common/util/security/eddsa/EdDSASecurityProviderRegistrar.java#L88-L101 this uses ThreadUtils.resolveDefaultClass to see if the class is available which adds a the Threads contextClassLoader into the mix to see if the class is there. If the class is available in the contextClassloader but not the current classload, then it would load as the Provider is created (eventually) here which uses reflection and in the case of this issue the Threads contextClassLoader.

The created provider is then registered in the JVM.

This provider is then used as the gate for "are the EdDSA classes available" for the regular (current) classloader, which in this case they are not which causes a :boom:

whilst it is generally safe to create and Provider dynamically like this, the code that uses the classes from the provider as opposed to the provider itself should take a different approach. Either they also need to be loaded by reflection (yikes), or the check to see if they are available should not use the provider, but should check the actual class is present in the current classloader (ie ignore the context classloader). Or providers should not be dynamically registered from a context classloader.

jtnord commented 1 month ago

(of note BouncyCastle also supports ED25519 curves as does JDK > 15) so this whole code loading / registering for EdDSA could be replaced on a modern JVM).