fix(hopopt,ipv6-opts): size the ILNP nonce with a real ceiling, not a floored one (#601)

[JarryShaw]

The ILNP Nonce option builders sized the option with math.ceil(nonce.bit_length() // 8) at hopopt.py:1889 and ipv6_opts.py:1892. // floors, and math.ceil of an int is a no-op, so the ceiling was never taken.

This is silent wire corruption, not a cosmetic length field

nonce is packed by a NumberField whose width is that declared len (pcapkit/protocols/schema/internet/hopopt.py:722, ipv6_opts.py:727), so under-declaring the length truncates the nonce on the wire with nothing raised. Measured on the unfixed tree, construct then parse back:

nonce	bit length	declared len	packed	parsed back
9	4	0	b'\x8b\x00'	0 — nonce gone entirely
42	6	0	b'\x8b\x00'	0
127	7	0	b'\x8b\x00'	0
255	8	1	b'\x8b\x01\xff'	255 — control
256	9	1	b'\x8b\x01\x00'	0 — truncated
65536	17	2	b'\x8b\x02\x00\x00'	0 — truncated
0xFFFFFF	24	3	b'\x8b\x03\xff\xff\xff'	0xFFFFFF — control

Small nonces are the worst case rather than boundary values: anything below 256 was declared as zero octets and dropped from the packet. Only an exact multiple of eight gave the right answer.

The fix

max(1, math.ceil(nonce.bit_length() / 8)) at both sites — the form already used at five other sizing sites: hip.py:3156, :3334, :4289, :4315 and mh.py:7947.

The one-octet floor is mh.py's convention (the reasoning is spelled out in the comment at mh.py:7936) and is load-bearing here because nonce defaults to 0, whose bit length is 0: without it, _make_opt_ilnp(code) with no nonce builds an ILNP Nonce option carrying no Nonce Value field at all, collapsing "the nonce is 0" into "there is no nonce" when RFC 6744 gives the option that field. Value round-trip is unaffected either way (0 → zero octets → 0), so this part is structural rather than a data fix. It is called out here rather than folded in silently, and is easy to drop if you would rather keep the change to the division alone.

The read path was never affected: it takes the width from the len octet on the wire rather than recomputing it.

Tests, proven failing then passing, per site

One test per protocol, so each site is pinned independently. Reverting hopopt.py alone:

SUBFAILED(nonce=0) ... test_hopopt_ilnp_nonce_option_is_sized_by_the_ceiling
SUBFAILED(nonce=1) ... SUBFAILED(nonce=9) ... SUBFAILED(nonce=42) ...
SUBFAILED(nonce=127) ... SUBFAILED(nonce=256) ... SUBFAILED(nonce=65536) ...
SUBFAILED(nonce=8589934591) ...
FAILED tests/protocols/internet/test_ipv6_extension_unit.py::IPv6ExtensionUnitTests::test_hopopt_ilnp_nonce_option_is_sized_by_the_ceiling
============ 9 failed, 1 passed, 59 deselected, 1 warning in 1.71s =============

Reverting ipv6_opts.py alone mirrors it exactly (9 failed, the hopopt test passing), with the assertion deltas being precisely the floor-vs-ceiling gap:

E  AssertionError: 0 != 1     (x5)
E  AssertionError: 1 != 2
E  AssertionError: 2 != 3
E  AssertionError: 4 != 5

With both fixes in place, 2 passed. The two byte-aligned controls, 255 and 0xFFFFFF, pass either way.

Why the suite missed it

The only ILNP nonce the suite exercised was 0xFFFFFF (examples/generators/options.py:537) — bit length 24, an exact multiple of eight, precisely where floor division and the ceiling agree. Same blind spot that hid the identical typo in numbers.py (#599/#600).

So every new case bar the two controls has a bit length that is not a multiple of eight, several below 256, and the table guards itself — the test asserts at least six of its own values stay non-byte-aligned and one stays below 256, so rounding them off to convenient constants later cannot quietly disarm the regression.

There is no ILNP entry in EXPECTED_FAILURES (45 entries, confirmed by importing it rather than grepping) and none starts passing, so that file is untouched.

Verification

• tests/protocols/internet/ — 221 passed, 718 subtests, exit 0
• tests/protocols/test_option_roundtrip_unit.py + test_option_coverage_runtime.py — 9 passed, 378 subtests, exit 0
• Coverage of the two changed modules: 99% statement and branch (coverage run --include=..., never pytest-cov). The single uncovered line in each is a pre-existing gap in _make_opt_pad from ipv6/mh: fix option padding, and with it construction, which was wholly broken #398. Statement coverage of the changed line was already 100% before this change — which is exactly why the defect survived; what the new tests add is assertions on the length it produces.
• python util/changelog_md.py --check exits 0.
• Rebased onto 493020f83; one commit. The changelog conflict against fix(corekit): size the width repair with a real ceiling, not a floored one (#599) #600's bullets was resolved keeping all bullets, mine last, and CHANGELOG.md regenerated rather than hand-merged.

Found but deliberately not fixed: hip.py:3200 is also wrong

#601 recorded hip.py:3200's math.ceil(... / 4) as possibly a deliberate nibble count. It is not — it is a defect, but hip.py is outside this change and it is reported rather than folded in.

len=4 + math.ceil(max(random.bit_length(), solution.bit_length()) / 4),

The / 4 is an invalid shorthand for "two fields of ceil(bits/8) octets each". SolutionParameter gives random and solution a width of (pkt['len'] - 4) // 2 each (pcapkit/protocols/schema/internet/hip.py:454,456), and the reader's own comment at hip.py:1090 reads # Length (schema.len) = 4 + RHASH_len / 4 — valid only because the real RHASH_len is a fixed multiple of 8. Substituting an arbitrary bit_length() for it breaks the identity, since ceil(x/4) != 2*ceil(x/8) in general.

The builder emits parameters its own reader rejects. Measured:

rnd=0x1 sol=0xfff maxbits=12 built_len=7 correct_len=8 per_field=1 need=2
  -> ProtocolError: HIPv2: [ParamNo 321] invalid format
rnd=0x1 sol=0x1ff maxbits=9  built_len=7 correct_len=8 per_field=1 need=2
  -> ProtocolError: HIPv2: [ParamNo 321] invalid format
rnd=0xff sol=0xff maxbits=8  built_len=6 correct_len=6 per_field=1 need=1
  -> reader accepted

len=7 makes (schema.len - 4) % 2 != 0, which hip.py:1083-1084 raises on. The correct expression is 4 + 2 * math.ceil(max(random.bit_length(), solution.bit_length()) / 8). Worth its own issue.

Two further things noticed in hip.py and not investigated: _make_param_solution/_make_param_puzzle recompute widths from bit_length() when re-serializing an already-parsed parameter, so leading zero octets in the original encoding are lost on a parse-then-rebuild; and the readers compute padding from schema.len % 8 rather than from the total record length including the 4-octet header.

Fixes #601

[JarryShaw]

✅ GOOD TO MERGE — head 41682cb284f6ae063a633bbf718696dfbe8dcd2a. Silent-data-loss claim, the discriminating-nonce test guard, and the revert/test evidence for both files all independently reproduced. One judgment call on the max(1, ...) addition in the appendix — defensible and disclosed, arguably belongs in its own change, not a blocker.

[JarryShaw]

Cross-review appendix — PR #607

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head 41682cb284f6ae063a633bbf718696dfbe8dcd2a in an isolated worktree (/tmp/pcapkit-review/pr607, removed after this review).

Fixes keyword and CI

closingIssuesReferences = [601]. CI: rollup PENDING, CheckRun tally 14 SUCCESS, 2 SKIPPED, rest QUEUED, 0 FAILURE/CANCELLED.

1. Silent data loss — reproduced exactly, including the coordinator's specific hex

Constructed HOPOPT().pack(next=59, options=[(Option.ILNP_Nonce, {'nonce': 9})]) on the unfixed tree (reverted only the max(1, math.ceil(.../8)) line back to math.ceil(nonce.bit_length() // 8)):

full packet: 3b008b0001020000
parsed:      ILNPOption(..., length=2, nonce=0)   <- nonce silently became 0

Stripping the 2-octet HOPOPT fixed header (3b00 = next-header + hdr-ext-len) leaves 8b0001020000 — an exact match to the coordinator's cited hex. My first attempt compared against the bare option schema bytes only (8b00, 2 bytes) and looked like a mismatch; re-running through the full HOPOPT.pack()/parse path (which appends a PadN alignment option, 010200 in this case) reconciled it exactly. On the fixed tree, the identical call round-trips nonce=9 correctly (3b008b0109010100, parsed nonce=9). Confirms: no exception, no warning, silent truncation to 0 — a materially worse failure mode than a wrong length field.

2. max(1, ...) — judged, not merely noted

Read RFC 6744 directly (rfc-editor.org/rfc/rfc6744.txt, the ILNP Nonce Option spec) rather than accepting the PR's framing. §"Nonce Value" (line ~307) describes it as "An unpredictable cryptographically random value... used to prevent off-path attacks" — the RFC has no discussion of a zero-valued nonce at all, because a genuine cryptographic nonce is never actually zero in practice. The RFC does not mandate any particular wire treatment for nonce=0 — that case only arises from this library's own API default (nonce: int = 0), so whether it should produce a zero-octet (absent) or one-octet (present, value 0x00) Nonce Value field is a library-design convention, not a wire-format requirement.

Separately: max(1, ...) does not touch the floor-vs-ceiling arithmetic #601 is about — ceil(0/8) and floor(0/8) are both 0, so nonce=0 is unaffected by the division bug either way. It's a genuinely independent behavioral change riding along with the fix.

Judgment: this is out of #601's strict scope, but it's honestly and prominently disclosed ("this part is structural rather than a data fix... easy to drop if you would rather keep the change to the division alone"), matches an existing, precedented convention elsewhere in the codebase (mh.py's _make_opt_mn_id, per the PR's own citation), and doesn't complicate the fix or its tests. I would have prÉferred it split into its own one-line PR for strict scope hygiene, but given the explicit disclosure and offered opt-out, this does not rise to a blocking issue.

3. The self-guarding test table — verified to actually work, not just asserted

Read the guard directly (tests/protocols/internet/test_ipv6_extension_unit.py, test_hopopt_ilnp_nonce_option_is_sized_by_the_ceiling):

discriminating = [nonce for nonce, _ in cases if nonce.bit_length() % 8]
self.assertGreaterEqual(len(discriminating), 6)
self.assertTrue(any(nonce < 256 for nonce in discriminating))

Ran this exact logic independently against the 10-entry cases table: 7 nonces are discriminating (1, 9, 42, 127, 256, 65536, 8589934591), comfortably above the >= 6 floor, and 4 of those 7 are below 256 (1, 9, 42, 127), not merely one. Also independently recomputed max(1, math.ceil(n.bit_length() / 8)) for all 10 table entries — every expected width matches. The guard is real and has margin, not a coincidental pass.

Revert-and-test evidence — reproduced for both files independently

Reverted only hopopt.py's line, kept everything else fixed:

9 failed, 1 passed, 59 deselected, 1 warning, 12 subtests passed in 1.66s   (exit 1)

Restored, reverted only ipv6_opts.py's line instead:

9 failed, 1 passed, 59 deselected, 1 warning, 12 subtests passed in 1.70s   (exit 1)

Both fixed:

2 passed, 59 deselected, 1 warning, 20 subtests passed in 1.29s   (exit 0)

Exact match to the PR's claimed per-site evidence in both cases.

Regression

First attempt at tests/protocols/internet/ showed 5 failures — all FileNotFoundError: sample capture ... not found, the missing-fixture trap in a fresh worktree (only 6 of the needed captures existed). Ran examples/generators/make_samples.py, reran: 221 passed, 718 subtests passed, exit 0 — exact match to the claim. python util/changelog_md.py --check exits 0.

The disclosed hip.py:3200 finding — spot-checked, not fully reproduced

Confirmed pcapkit/protocols/internet/hip.py:3200 reads exactly len=4 + math.ceil(max(random.bit_length(), solution.bit_length()) / 4) as quoted, and confirmed hip.py is genuinely outside this PR's file list (hopopt.py, ipv6_opts.py, the test file, changelog only). Did not independently reproduce the three measured ProtocolError repro lines in the PR body — treat that specific measurement as unverified by me, correctly out of scope for this PR either way.

Not independently checked

• The exact coverage percentage (99%, one pre-existing uncovered line in _make_opt_pad from ipv6/mh: fix option padding, and with it construction, which was wholly broken #398) was not rerun.
• The two further hip.py observations ("recompute widths... lose leading zero octets" and "padding computed from schema.len % 8") were read but not exercised.

Disagreement log

None that rise to a defect. One judgment call (the max(1, ...) scope question) resolved in the PR's favor given its own disclosure and the RFC's silence on the zero-nonce case, which I verified myself rather than accepting.