PasswordUtil.encode() misinterprets {algo} plaintext as ciphertext#35242
Open
una-tapa wants to merge 1 commit into
Open
PasswordUtil.encode() misinterprets {algo} plaintext as ciphertext#35242una-tapa wants to merge 1 commit into
una-tapa wants to merge 1 commit into
Conversation
Passwords starting with a {something} pattern (e.g. vault/PIM-generated
passphrases like '{abc}def' or '{redacted}***{/redacted}') were being
misinterpreted as already-encoded values by PasswordUtil.encode().
Root cause:
In encode(String, String, Map), getCryptoAlgorithm() returns a non-null
token for any string matching the {.*} pattern at the start. The code
then unconditionally attempted to decode the string before re-encoding,
without first checking whether the extracted token is actually a valid
crypto algorithm (xor, aes, hash, or custom).
This caused two failure modes:
1. NullPointerException: passwordDecode() returned null when the bogus
algorithm caused decipher to fail, and decoded_string.trim() at
line 274 threw NPE. Seen with '{redacted}***{/redacted}' inputs.
2. Silent data loss: for XOR, the internal decode returned an empty
string, producing '{xor}' with no ciphertext payload. Confirmed by
Keith Jabcuga's recreation program
Fix:
Add isValidCryptoAlgorithm() guards to both the throw-on-already-encoded
branch and the decode-then-re-encode branch. A {something} pattern in
the input is now only acted upon when 'something' is a known algorithm.
Literal plaintext passwords with curly-brace patterns fall through and
are encoded directly as plaintext, which is the correct behaviour.
Existing behaviour is preserved:
- Already-encoded passwords still throw InvalidPasswordEncodingException.
- Passwords encoded with a different valid algorithm are still decoded
and re-encoded as before.
Tests added in PasswordUtilTest:
- testEncodePasswordStartingWithCurlyBracePattern
- testEncodeWithKeyPasswordStartingWithCurlyBracePattern
Member
Author
|
!build Note: Target locations of links might be accessible only to IBM employees. |
Member
Code analysis and actionsDO NOT DELETE THIS COMMENT.
|
Member
Author
|
Build finished with one warning in javaBatch GUI FAT. Not related to the encryption/decryption.
Also the locally created jar files passed the recreation program created by our support team (Keith) === Example 1: Encoding with XOR and Custom Key === === Example 2: Encoding with XOR and Custom Key === |
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.
release buglabel if applicable: https://github.com/OpenLiberty/open-liberty/wiki/Open-Liberty-Conventions).Overview
Passwords starting with a {something} pattern (e.g. vault/PIM-generated passphrases like '{abc}def' or '{redacted}***{/redacted}') were being misinterpreted as already-encoded values by PasswordUtil.encode().
Root cause:
In encode(String, String, Map), getCryptoAlgorithm() returns a non-null token for any string matching the {.*} pattern at the start. The code then unconditionally attempted to decode the string before re-encoding, without first checking whether the extracted token is actually a valid crypto algorithm (xor, aes, hash, or custom).
This caused two failure modes:
Fix:
Add isValidCryptoAlgorithm() guards to both the throw-on-already-encoded branch and the decode-then-re-encode branch. A {something} pattern in the input is now only acted upon when 'something' is a known algorithm. Literal plaintext passwords with curly-brace patterns fall through and are encoded directly as plaintext, which is the correct behaviour.
Existing behaviour is preserved:
Tests added in PasswordUtilTest:
Fixes #35243