Ticket #2004 Fix RC4/RC4Drop hex passphrase parsing and add format validation#2605
Open
alleria173 wants to merge 2 commits into
Open
Ticket #2004 Fix RC4/RC4Drop hex passphrase parsing and add format validation#2605alleria173 wants to merge 2 commits into
alleria173 wants to merge 2 commits into
Conversation
Contributor
Author
|
Hi. I'd appreciate any feedback regarding my PR in case there are any adjustments I can make for it to be accepted. Thanks! |
Move validateFormatInput() out of Utils.convertToByteArray()/convertToByteString(), which are shared by ~50 operations, and into Ciphers.mjs where it's only reachable via parseFormatString() (used by RC4/RC4Drop). This keeps the fix scoped to issue gchq#2004 instead of changing accepted input globally, and avoids the unrelated HMAC/Scrypt regressions the broader validation caused. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Description
Fixes #2004 reported that non-space delimiters in hex passphrase input for RC4 encoding were changing the output (should be ignored, like space) as well as non-hex characters.
Files Changed
src/core/lib/Ciphers.mjsAdded two imports at the top:
(
Utilswas already imported.)Added
parseFormatString()export at the bottom of the file:For Hex format this normalises common delimiter conventions (
1f,10,0x1f,0x10,1f 10,1F:10→1f10) before CryptoJS parses it. For all other formats it delegates to the existingformatmap as before.src/core/operations/RC4.mjsUpdated import line:
Changed passphrase parsing in
run():src/core/operations/RC4Drop.mjsIdentical change to RC4.mjs — same import update and same passphrase line change.
src/core/operations/DeriveEVPKey.mjsRemoved the global
CryptoJS.enc.Hex.parsemonkey-patch (lines 126–147 in the original). This patch had been added to strip whitespace from hex strings before CryptoJS parsing — a partial workaround for exactly this class of bug. It was safe to remove because:DeriveEVPKey.run()usesUtils.convertToByteString()→fromHex()for its own passphrase/salt, notCryptoJS.enc.Hex.parse()directlyCryptoJS.enc.Hex.parse()directly for user input) now go throughparseFormatString()which normalises before parsingsrc/core/Utils.mjsAdded import:
Added
validateFormatInput()static method beforeconvertToByteArray():Added
Utils.validateFormatInput(str, type)as the first line of bothconvertToByteArray()andconvertToByteString(), covering all ~50 crypto operations that use these central conversion functions.Existing Issue
Issue #2004
Screenshots



AI disclosure
Claude Code used to read the codebase and plan fix, as well as create tests.
Test Coverage
tests/operations/tests/RC4.mjs(new file)Nine tests covering:
1f,10(comma-delimited) produces same output as1f100x1f,0x10(0x-prefixed) produces same output as1f101f 10(space-delimited) produces same output as1f101F:10(colon + uppercase) produces same output as1f101fG0(invalid hex char) produces an error message in outputTest vectors computed with a reference Python RC4 implementation against key bytes
[0x1f, 0x10]and plaintext"test".tests/node/tests/operations.mjs(existing file — two tests fixed)Two pre-existing Node API tests were passing non-hex strings to Hex-format inputs. Our validation correctly caught these as bugs in the tests. Both were updated to use appropriate formats with recomputed expected values.
HMAC test (line 661):
Scrypt test (line 885):
Verification
All tests pass: