search

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
892 stars 358 forks source link

NullPointerException in org.apache.sshd.common.kex.MontgomeryCurve.generateKeyPair() #470

Closed zakharovsergey1000 closed 6 months ago

zakharovsergey1000 commented 7 months ago

Version

2.9.2 Edit 2024/03/31: actually the bug shows up in versions up to 2.7.0. Since version 2.8.0 the bug does not manifest.

Bug description

Download the Gerrit code review source code (Edit 2024/03/31: git checkout --recurse-submodules v3.6.4) and run all unit tests several times.

Actual behavior

In every full unit test run multiple random tests will fail with NullPointerException: Caused by: java.lang.NullPointerException at org.bouncycastle.math.ec.rfc7748.X25519.generatePrivateKey(X25519.java:54) at org.bouncycastle.crypto.params.X25519PrivateKeyParameters.(X25519PrivateKeyParameters.java:24) at org.bouncycastle.crypto.generators.X25519KeyPairGenerator.generateKeyPair(X25519KeyPairGenerator.java:23) at org.bouncycastle.jcajce.provider.asymmetric.edec.KeyPairGeneratorSpi.generateKeyPair(KeyPairGeneratorSpi.java:193) at java.base/java.security.KeyPairGenerator$Delegate.generateKeyPair(KeyPairGenerator.java:722) at org.apache.sshd.common.kex.MontgomeryCurve.generateKeyPair(MontgomeryCurve.java:156)

Expected behavior

There should not be random test fails caused by NullPointerException at org.apache.sshd.common.kex.MontgomeryCurve.generateKeyPair(MontgomeryCurve.java:156). All tests should complete successfully every time.

Relevant log output

There was 1 failure:
1) sshKeyEndpoints(com.google.gerrit.acceptance.rest.binding.AccountsRestApiBindingsIT)
org.eclipse.jgit.errors.TransportException: ssh://user1@127.0.0.1:43749: DefaultAuthFuture[ssh-connection]: Failed (NullPointerException) to execute: null
    at org.eclipse.jgit.transport.sshd.SshdSessionFactory.getSession(SshdSessionFactory.java:263)
    at com.google.gerrit.acceptance.SshSessionMina.getMinaSession(SshSessionMina.java:164)
    at com.google.gerrit.acceptance.SshSessionMina.open(SshSessionMina.java:82)
    at com.google.gerrit.acceptance.AbstractDaemonTest.initSsh(AbstractDaemonTest.java:573)
    at com.google.gerrit.acceptance.AbstractDaemonTest.beforeTest(AbstractDaemonTest.java:479)
    at com.google.gerrit.acceptance.AbstractDaemonTest$1$1.evaluate(AbstractDaemonTest.java:231)
    at org.junit.rules.RunRules.evaluate(RunRules.java:20)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runners.Suite.runChild(Suite.java:128)
    at org.junit.runners.Suite.runChild(Suite.java:27)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
    at org.junit.rules.RunRules.evaluate(RunRules.java:20)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.junit.runners.Suite.runChild(Suite.java:128)
    at org.junit.runners.Suite.runChild(Suite.java:27)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at com.google.testing.junit.runner.internal.junit4.CancellableRequestFactory$CancellableRunner.run(CancellableRequestFactory.java:108)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
    at com.google.testing.junit.runner.junit4.JUnit4Runner.run(JUnit4Runner.java:116)
    at com.google.testing.junit.runner.BazelTestRunner.runTestsInSuite(BazelTestRunner.java:148)
    at com.google.testing.junit.runner.BazelTestRunner.main(BazelTestRunner.java:75)
Caused by: org.apache.sshd.common.SshException: DefaultAuthFuture[ssh-connection]: Failed (NullPointerException) to execute: null
    at org.apache.sshd.common.future.AbstractSshFuture.lambda$verifyResult$1(AbstractSshFuture.java:132)
    at org.apache.sshd.common.future.AbstractSshFuture.formatExceptionMessage(AbstractSshFuture.java:190)
    at org.apache.sshd.common.future.AbstractSshFuture.verifyResult(AbstractSshFuture.java:131)
    at org.apache.sshd.client.future.DefaultAuthFuture.verify(DefaultAuthFuture.java:39)
    at org.apache.sshd.client.future.DefaultAuthFuture.verify(DefaultAuthFuture.java:32)
    at org.apache.sshd.common.future.VerifiableFuture.verify(VerifiableFuture.java:68)
    at org.eclipse.jgit.transport.sshd.SshdSession.connect(SshdSession.java:172)
    at org.eclipse.jgit.transport.sshd.SshdSession.connect(SshdSession.java:101)
    at org.eclipse.jgit.transport.sshd.SshdSessionFactory.getSession(SshdSessionFactory.java:256)
    ... 40 more
Caused by: java.lang.NullPointerException
    at org.bouncycastle.math.ec.rfc7748.X25519.generatePrivateKey(X25519.java:54)
    at org.bouncycastle.crypto.params.X25519PrivateKeyParameters.<init>(X25519PrivateKeyParameters.java:24)
    at org.bouncycastle.crypto.generators.X25519KeyPairGenerator.generateKeyPair(X25519KeyPairGenerator.java:23)
    at org.bouncycastle.jcajce.provider.asymmetric.edec.KeyPairGeneratorSpi.generateKeyPair(KeyPairGeneratorSpi.java:193)
    at java.base/java.security.KeyPairGenerator$Delegate.generateKeyPair(KeyPairGenerator.java:722)
    at org.apache.sshd.common.kex.MontgomeryCurve.generateKeyPair(MontgomeryCurve.java:156)
    at org.apache.sshd.common.kex.XDH.calculateE(XDH.java:45)
    at org.apache.sshd.common.kex.AbstractDH.getE(AbstractDH.java:60)
    at org.apache.sshd.client.kex.DHGClient.init(DHGClient.java:98)
    at org.apache.sshd.common.session.helpers.AbstractSession.doKexNegotiation(AbstractSession.java:888)
    at org.apache.sshd.common.session.helpers.AbstractSession.handleKexInit(AbstractSession.java:827)
    at org.apache.sshd.common.session.helpers.AbstractSession.doHandleMessage(AbstractSession.java:567)
    at org.apache.sshd.common.session.helpers.AbstractSession.lambda$handleMessage$0(AbstractSession.java:522)
    at org.apache.sshd.common.util.threads.ThreadUtils.runAsInternal(ThreadUtils.java:68)
    at org.apache.sshd.common.session.helpers.AbstractSession.handleMessage(AbstractSession.java:521)
    at org.apache.sshd.common.session.helpers.AbstractSession.decode(AbstractSession.java:1639)
    at org.apache.sshd.common.session.helpers.AbstractSession.messageReceived(AbstractSession.java:482)
    at org.eclipse.jgit.internal.transport.sshd.JGitClientSession.messageReceived(JGitClientSession.java:197)
    at org.apache.sshd.common.session.helpers.AbstractSessionIoHandler.messageReceived(AbstractSessionIoHandler.java:64)
    at org.apache.sshd.common.io.nio2.Nio2Session.handleReadCycleCompletion(Nio2Session.java:407)
    at org.apache.sshd.common.io.nio2.Nio2Session$1.onCompleted(Nio2Session.java:380)
    at org.apache.sshd.common.io.nio2.Nio2Session$1.onCompleted(Nio2Session.java:375)
    at org.apache.sshd.common.io.nio2.Nio2CompletionHandler.lambda$completed$0(Nio2CompletionHandler.java:38)
    at java.base/java.security.AccessController.doPrivileged(Native Method)
    at org.apache.sshd.common.io.nio2.Nio2CompletionHandler.completed(Nio2CompletionHandler.java:37)
    at java.base/sun.nio.ch.Invoker.invokeUnchecked(Invoker.java:127)
    at java.base/sun.nio.ch.Invoker.invokeDirect(Invoker.java:158)
    at java.base/sun.nio.ch.UnixAsynchronousSocketChannelImpl.implRead(UnixAsynchronousSocketChannelImpl.java:562)
    at java.base/sun.nio.ch.AsynchronousSocketChannelImpl.read(AsynchronousSocketChannelImpl.java:277)
    at java.base/sun.nio.ch.AsynchronousSocketChannelImpl.read(AsynchronousSocketChannelImpl.java:298)
    at org.apache.sshd.common.io.nio2.Nio2Session.doReadCycle(Nio2Session.java:492)
    at org.apache.sshd.common.io.nio2.Nio2Session.doReadCycle(Nio2Session.java:370)
    at org.apache.sshd.common.io.nio2.Nio2Session.startReading(Nio2Session.java:363)
    at org.apache.sshd.common.io.nio2.Nio2Session.startReading(Nio2Session.java:359)
    at org.apache.sshd.common.io.nio2.Nio2Session.startReading(Nio2Session.java:355)
    at org.apache.sshd.common.io.nio2.Nio2Session.startReading(Nio2Session.java:351)
    at org.apache.sshd.common.io.nio2.Nio2Session.startReading(Nio2Session.java:347)
    at org.apache.sshd.common.io.nio2.Nio2Connector$ConnectionCompletionHandler.onCompleted(Nio2Connector.java:163)
    at org.apache.sshd.common.io.nio2.Nio2Connector$ConnectionCompletionHandler.onCompleted(Nio2Connector.java:118)
    at org.apache.sshd.common.io.nio2.Nio2CompletionHandler.lambda$completed$0(Nio2CompletionHandler.java:38)
    at java.base/java.security.AccessController.doPrivileged(Native Method)
    at org.apache.sshd.common.io.nio2.Nio2CompletionHandler.completed(Nio2CompletionHandler.java:37)
    at java.base/sun.nio.ch.Invoker.invokeUnchecked(Invoker.java:127)
    at java.base/sun.nio.ch.Invoker$2.run(Invoker.java:219)
    at java.base/sun.nio.ch.AsynchronousChannelGroupImpl$1.run(AsynchronousChannelGroupImpl.java:112)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:829)

Other information

The keyPairGenerator object in the MontgomeryCurve is a bouncycastle implementation of the java.security.KeyPairGenerator class. The generateKeyPair method in class org.bouncycastle.jcajce.provider.asymmetric.edec.KeyPairGeneratorSpi is not thread safe and calling the generateKeyPair method in org.apache.sshd.common.kex.MontgomeryCurve.generateKeyPair() is not synchronized. Therefore calling method org.apache.sshd.common.kex.MontgomeryCurve.generateKeyPair() by multiple threads will often cause a NullPointerException due to a race conditions.

Consider the following scenario: two threads simultaneously reach the generateKeyPair() method in class org.bouncycastle.jcajce.provider.asymmetric.edec.KeyPairGeneratorSpi. Thread one performs the if (!initialised) check. The "initialised" variable is false. Therefore, the setupGenerator(algorithm) method is executed. In the setupGenerator method, the "initialised" variable is assigned true at the very beginning. And at this moment, execution of the thread one is suspended and execution of the thread two continues. Thread two evaluates the "initialised" variable. The variable is true therefore thread two executes the further command AsymmetricCipherKeyPair kp = generator.generateKeyPair(); the generator is not yet correctly initialized. Thread two continues its execution and eventually throws a NullPointerException, which is the result of the generator not being initialized correctly because the setupGenerator method was not executed to completion even though the "initialised" variable was already set to true. The callstack looks like this: Caused by: java.lang.NullPointerException at org.bouncycastle.math.ec.rfc7748.X25519.generatePrivateKey(X25519.java:54) at org.bouncycastle.crypto.params.X25519PrivateKeyParameters.(X25519PrivateKeyParameters.java:24) at org.bouncycastle.crypto.generators.X25519KeyPairGenerator.generateKeyPair(X25519KeyPairGenerator.java:23) at org.bouncycastle.jcajce.provider.asymmetric.edec.KeyPairGeneratorSpi.generateKeyPair(KeyPairGeneratorSpi.java:193) at java.base/java.security.KeyPairGenerator$Delegate.generateKeyPair(KeyPairGenerator.java:722) at org.apache.sshd.common.kex.MontgomeryCurve.generateKeyPair(MontgomeryCurve.java:156) The org.bouncycastle.math.ec.rfc7748.X25519.generatePrivateKey(SecureRandom random, byte[] k) method executes the random.nextBytes(k) instruction. In this case, the "random" argument is null, since the setupGenerator(algorithm) method was not completed yet. The result is a NullPointerException.

Edit 2024/03/31: The above scenario describes the execution of multi-threaded code in the bouncycastle library version 1.64. How can this be if Gerrit version 3.6.4 uses the sshd library version 2.9.2, and the sshd library version 2.9.2 declares a dependency on the bouncycastle library version 1.70? This is due to the fact that, despite the fact that the sshd library declares a dependency on the bouncycastle library version 1.70, in Gerrit itself, in the tools/deps.bzl file, a dependency on the bouncycastle library version 1.64 is declared and it is the classes of the bouncycastle library version 1.64 that are called in class MontgomeryCurve. In the bouncycastle library, in version 1.69, the KeyPairGeneratorSpi class was refactored, as a result of which it is impossible to fulfill race conditions as described above. Since Gerrit version 3.6.5, a dependency on the bouncycastle library version 1.72 has been declared in the tools/deps.bzl file. Therefore, starting from Gerrit 3.6.5, the flaky tests mentioned above do not appear. However, the KeyPairGeneratorSpi class in the bouncycastle library is still not declared thread safe. Therefore, its subsequent change may again lead to racing conditions similar to those described above. Therefore, it still makes sense to apply the pull request https://github.com/apache/mina-sshd/pull/467 for reliability and to prevent undefined behavior from possible future changes to the bouncycastle KeyPairGeneratorSpi class.

zakharovsergey1000 commented 7 months ago

Pull request: https://github.com/apache/mina-sshd/pull/467

tomaswolf commented 6 months ago

Thank you.