Fix potential overriding of cipher by other libraries #3751
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request forces the use of specific RSA options, i.e. ECB and PKCS1Padding for cipher.
Specifying "RSA" without specific options will use the default options described above, however libraries such as BouncyCastle provided by various plugins may override this behavior, making it impossible to compare the secret correctly.
In this particular case, the default "RSA" implementation in BouncyCastle uses the following options "RSA/ECB/NoPadding", which results in any secret being impossible to compare, since the padding is not removed from it.
I also considered forcing the "SunJCE" provider in cipher, which also fixes this issue, however I think it is better to allow other libraries to influence this behavior, especially if it can improve performance.
In addition, this pull request also replaces the use of Arrays#equals with MessageDigest#isEqual which should have a positive impact on security.