Fix engineGetCertificateAlias returning the alias of an unrelated certificate - #2385
Closed
rimuln wants to merge 1 commit into
Closed
Fix engineGetCertificateAlias returning the alias of an unrelated certificate#2385rimuln wants to merge 1 commit into
rimuln wants to merge 1 commit into
Conversation
…tificate engineGetCertificateAlias took the certificate from certs.elements() and the alias from certs.keys(), advancing both in lockstep. That relies on the two enumerations visiting the table in the same order, which stopped holding when IgnoresCaseHashtable.keys() began enumerating a copy of the table while elements() kept enumerating the original: new Hashtable(Map) sizes its bucket array from the entry count, whereas the original reached its capacity by incremental rehashing, so for a store with enough entries the layouts differ and the method returns the wrong alias with no error. Look the value up by key instead, which does not depend on the two enumerations agreeing. Applied to the keyCerts loop as well - keyCerts is a plain Hashtable whose keys() and elements() do agree, so it was not affected, but the pairing was the same latent fragility. testGetCertificateAlias covers it with 12 certificate entries plus a negative case; twelve is needed because with only a few entries the table and its copy can share a layout and the defect does not reproduce. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Thanks, merged with minor revision, now up on https://www.bouncycastle.org/betas |
This file contains hidden or 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
2 participants
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.
Fixes #2384.
PKCS12KeyStoreSpi.engineGetCertificateAliaswalkedcerts.elements()andcerts.keys()inlockstep, taking the certificate from one enumeration and the alias from the other. That relies on
both enumerations visiting the table in the same order, which stopped being true in 1.80: a5be993
changed
IgnoresCaseHashtable.keys()to enumeratenew Hashtable(orig)whileelements()stillenumerates
orig.new Hashtable(Map)sizes its bucket array from the entry count, whereasorigreached its capacity by incremental rehashing, so once a store holds enough entries the two layouts
differ and the method returns the alias of an unrelated certificate — silently, with no exception.
Since the copy in
keys()was introduced deliberately, this fixes the caller instead of revertingit: the value is now looked up by key, so the result no longer depends on two enumerations agreeing.
The same change is applied to the
keyCertsloop below it —keyCertsis a plainHashtablewhosekeys()andelements()do agree, so it was not broken, but the pairing was the same latentfragility and removing it costs nothing.
testGetCertificateAliasinPKCS12StoreTestcovers it: 12 certificate entries, then everycertificate is looked up and must return its own alias, plus a negative case for a certificate that
is not in the store. Twelve is chosen so the backing table has grown past its initial capacity — with
only a handful of entries the original and the copy can share a layout and the defect does not
reproduce, which is probably why it went unnoticed. The test fails before this change and passes
after.
Not addressed here: the
prov/src/main/jdk1.3andprov/src/main/jdk1.4copies of this class stillhave
return orig.keys();and so behave correctly today, but theirengineGetCertificateAliascarries the same order-dependent pairing. Happy to extend the PR to those trees if you would prefer
them hardened too.
Prepared with AI assistance (noted in the commit trailer), offered under the Bouncy Castle License
per CONTRIBUTING.md.