Skip to content

Fix RC2 effective-key-length mask per RFC 2268 - #1150

Open
ArockiaRajamanickam wants to merge 2 commits into
digitalbazaar:mainfrom
ArockiaRajamanickam:fix-1120-rc2-effective-key-bits
Open

Fix RC2 effective-key-length mask per RFC 2268#1150
ArockiaRajamanickam wants to merge 2 commits into
digitalbazaar:mainfrom
ArockiaRajamanickam:fix-1120-rc2-effective-key-bits

Conversation

@ArockiaRajamanickam

Copy link
Copy Markdown

Fixes #1120.

The defect

RFC 2268 section 2 defines the effective-key-length mask as

T8 = (T1+7)/8
TM = 255 MOD 2^(8 + T1 - 8*T8)

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.js shifted out T1 & 0x07, which is T1 MOD 8. Those agree only when T1 is 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 T1 from 1 to 1024: the old expression is correct for the 256 multiples of 8 and wrong for the other 768.

TM is applied at L.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. For T1 = 129 the mask should be 0x01 and was 0x7f.

Evidence

Running the eight RFC 2268 section 5 vectors through the public API on main (7a43db9):

eff= 63 keyLen=  8B  want=ebb773f993278eff got=2366aad00deca466  FAIL
eff= 64 keyLen=  8B  want=278b27e42e2f0d49 got=278b27e42e2f0d49  PASS
eff= 64 keyLen=  8B  want=30649edf9be7d2c2 got=30649edf9be7d2c2  PASS
eff= 64 keyLen=  1B  want=61a8a244adacccf0 got=61a8a244adacccf0  PASS
eff= 64 keyLen=  7B  want=6ccf4308974c267f got=6ccf4308974c267f  PASS
eff= 64 keyLen= 16B  want=1a807d272bbe5db1 got=1a807d272bbe5db1  PASS
eff=128 keyLen= 16B  want=2269552ab0f85ca6 got=2269552ab0f85ca6  PASS
eff=129 keyLen= 33B  want=5b78d3a43dfff1f1 got=e6b0ac1b3324deaf  FAIL

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.js has describe.only('jsbn', ...) at line 4, so npm test currently 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Minor bug in RC2 key expansion function

1 participant