Zeroize a copy of the card user PIN in the YubiKey decryptor factories, not the provider's array - #2444
Open
Arpan0995 wants to merge 1 commit into
Open
Zeroize a copy of the card user PIN in the YubiKey decryptor factories, not the provider's array#2444Arpan0995 wants to merge 1 commit into
Arpan0995 wants to merge 1 commit into
Conversation
…s, not the provider's array
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
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.
BcYubikeyPublicKeyDataDecryptorFactory.requireUserPinandJceYubikeyPublicKeyDataDecryptorFactoryBuilder.requireUserPinreturn thechar[]thatKeyPassphraseProvider.getKeyPasswordhanded them, and each of the three private-key operations in each class clears it in afinallyblock. Both providers BC ships return the application's own array by reference:DefaultKeyPassphraseProviderreturns thechar[]it has cached for the key, and the anonymous provider inOpenPGPApi.editKeyreturns its passphrase argument. So the first card operation destroys the caller's PIN, and since the provider keeps handing back that same array, the next private-key operation presents an all-zero PIN, which a card rejects, spending a user PIN retry.No other
getKeyPasswordconsumer inpg/src/mainzeroizes what it is given, and the simulator card in this module clears nothing; where BC clears achar[]after use it clears a copy it made itself, asJPAKEParticipantandECJPAKEParticipantdo, withOpenPGPKeyGenerator.build(char[])the one exception. The existing YubiKey tests cannot see it, sinceSmartCardTestProperties.getUserPin()already returnsArrays.clone.Reproduced against released 1.86 (no pgsc jar was published for it, so
pgsc/src/main/javawas compiled from ther1rv86tag): a registered PIN of123456does not survive one operation on either factory, the JCE factory printing it back as six NUL characters. Both factory sources are byte-identical in the current 1.87-SNAPSHOT beta.This change:
requireUserPin, so thefinallyblocks clear a copy the factory owns, and records the borrowing convention in thegetKeyPasswordjavadoc, where the provider contract lives; andSmartCardUserPinOwnershipTest, registered inSimulatorTests, which builds each factory with a null card handle so the card call fails immediately while the PIN handling around it runs in full (the fetch before the session is opened, the clear in thefinallyblock after it), asserting that the PIN was fetched and survives, and decrypts two messages in a row through the simulator against one cached PIN. Either factory case fails without the change and both pass with it; the assertions abort at the first failure, so reverting one clone at a time shows each in turn.#2430 removes both files and moves
requireUserPinintoOpenPGPSmartCardunchanged, withYubikeyOpenPGPSmartCardstill clearing the returned array in threefinallyblocks, one of them a signing path; the fix there is a singleArrays.cloneif you would rather take it on that PR.Base tree only, no new imports in the touched main sources and no
module-infochange. Happy to add a release-note entry under whichever version block you prefer.