Fix RC2 effective-key-length mask per RFC 2268 - #1150
Open
ArockiaRajamanickam wants to merge 2 commits into
Open
Fix RC2 effective-key-length mask per RFC 2268#1150ArockiaRajamanickam wants to merge 2 commits into
ArockiaRajamanickam wants to merge 2 commits into
Conversation
RFC 2268 section 2 defines the mask as T8 = (T1+7)/8 TM = 255 MOD 2^(8 + T1 - 8*T8) and states that TM has its 8 - (8*T8 - T1) least significant bits set. The number of bits shifted out is therefore 8*T8 - T1, which equals (-T1) MOD 8, but the code shifted out T1 MOD 8. The two agree only when T1 is a multiple of 8, which is why the 64- and 128-bit paths and the existing tests never caught it. For T1 = 129 the mask should be 0x01 and was 0x7f. That byte seeds the backward pass of the expansion, so a single wrong mask corrupts the whole 128-byte expanded key and the resulting ciphertext. Two of the eight RFC 2268 section 5 vectors failed before this change and pass after it. Added all the vectors that exercise the boundary. Fixes digitalbazaar#1120
tests/unit/jsbn.js used describe.only, so mocha ran only that file and npm test reported 5 passing. With it removed the suite runs 828 passing and 4 pending, with no failures, so nothing was being hidden other than the tests themselves. This is separable from the RC2 fix; it is here because the new RC2 tests would otherwise never run.
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.
Fixes #1120.
The defect
RFC 2268 section 2 defines the effective-key-length mask as
and says in words: "TM has its 8 - (8*T8 - T1) least significant bits set."
So the number of bits shifted out is
8*T8 - T1, which is(-T1) MOD 8.lib/rc2.jsshifted outT1 & 0x07, which isT1 MOD 8. Those agree only whenT1is a multiple of 8, which is why the 64- and 128-bit paths and the existing 40-bit test never caught it.Checked against the literal RFC formula for every
T1from 1 to 1024: the old expression is correct for the 256 multiples of 8 and wrong for the other 768.TMis applied atL.setAt(128 - T8, piTable[L.at(128 - T8) & TM]), and that byte seeds the backward pass, so one wrong mask corrupts the whole 128-byte expanded key. ForT1 = 129the mask should be0x01and was0x7f.Evidence
Running the eight RFC 2268 section 5 vectors through the public API on
main(7a43db9):Two of eight fail. After the change, zero fail.
The line has been unchanged since RC2 key expansion was added in 2012, so this has been wrong for fourteen years for any non-multiple-of-8 effective key size.
Behaviour change, stated plainly
This changes ciphertext and plaintext for any call where the effective key size is not a multiple of 8 bits. Anything encrypted with the old expansion at such a size will no longer decrypt with this version, and vice versa. Since the old output did not match the RFC, that data was not interoperable with any conforming implementation anyway, but it is a real compatibility break for anyone round-tripping data through forge alone.
The common cases are unaffected: the 128-bit default, 64-bit, 40-bit and every other multiple of 8 produce byte-identical output to before.
The second commit
tests/unit/jsbn.jshasdescribe.only('jsbn', ...)at line 4, sonpm testcurrently runs 5 tests. Any regression test added here would silently never run, which is why it is in this PR rather than a separate one — but it is a distinct change and easy to drop if you would rather take it separately.With it removed the suite is 828 passing, 4 pending, no failures, so nothing was being hidden except the tests themselves. With both commits it is 833 passing.
Tests
Five RFC 2268 vectors added to
tests/unit/rc2.js, chosen to cover the boundary: 63 and 129 effective bits, which failed before, plus 64 and 128 and the single-byte key, which did not. Only the first output block is compared, since forge appends a PKCS#7 padding block.Reverting the one-line source change while keeping the tests fails exactly the 63- and 129-bit cases, so they bind to the defect.
I used an AI assistant while working on this. The RFC derivation, the exhaustive 1..1024 comparison against the spec formula, and the vector runs above are checks I made myself against the RFC text rather than the issue thread.