NullPointerException in org.apache.sshd.common.kex.MontgomeryCurve.generateKeyPair() #470
Milestone
Comments
zakharovsergey1000
changed the title
|
Pull request: #467 |
|
Thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
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 #467 for reliability and to prevent undefined behavior from possible future changes to the bouncycastle KeyPairGeneratorSpi class.
The text was updated successfully, but these errors were encountered: