Skip to content

fix(hip): HIPv2-only parameters are accepted inside HIPv1 packets - #1160

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1133-hipv2-params-in-v1
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1133-hipv2-params-in-v1

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Followed the coding style (make pylint, make mypy, make isort): ran the three on the two touched files only. mypy and isort are clean, and pylint reports nothing new on the added lines

  • make test passes, and a test case covers the change. Ran only the modules listed below, not the full suite

  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A, added centrally after the wave

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

Closes #1133. At version 1, DH_GROUP_LIST, HIP_CIPHER, HIT_SUITE_LIST and TRANSPORT_FORMAT_LIST now raise ProtocolError('HIPv1: [ParamNo N] invalid parameter') in both the reader and the maker. The issue asks for a warning, but R1_Counter and HIP_TRANSFORM actually raise on a version mismatch, so this matches the raise.

  • Before: all four build at v1, and they parse back from a v1 header with no error.
  • After: each one raises on both make and read at v1. At v2 they still round-trip.
  • PUZZLE/SOLUTION: no gap. At v1 the readers already require Length 12/20, and the maker fixes RHASH_len at 64.
  • New tests/protocols/internet/test_hip_v2_only_parameters_unit.py: without the fix 8 failed, 3 passed, 4 subtests passed (4 make + 4 read subtests fail). With it 3 passed, 12 subtests passed.
  • Other modules run: test_hip_unit 34 passed / 168 subtests, test_hip_cipher_limit_unit 2, test_hip_r1_counter_width_unit 4, test_option_roundtrip_unit 6 / 360, test_docstring_contract 7, tests/project 389 passed, 1 skipped.

DH_GROUP_LIST (511), HIP_CIPHER (579), HIT_SUITE_LIST (715) and
TRANSPORT_FORMAT_LIST (2049) exist only in RFC 7401. Their readers and
makers now raise ProtocolError("invalid parameter") at version 1, the
same way R1_Counter and HIP_TRANSFORM reject a version mismatch.

Closes #1133.
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 80ede4924: GOOD TO GO (ran on Sonnet; author Opus)

  • Raising instead of warning is correct; the issue text was wrong. hip.py's existing version checks all raise ProtocolError('HIPv{version}: [ParamNo N] invalid parameter'): :910, :1344 and :3059 on main. I re-grepped this myself. The PR copies that message.
  • Effect on real captures: a v1-labelled packet that carries one of the four parameters now decodes as Ethernet:IPv6:IPv6-Ext instead of HIPv1. Extraction does not abort. This is the same degradation HIP_TRANSFORM and R1_Counter already cause for the reverse mismatch, so it is not labelled breaking.
  • The four parameter numbers are exact. Diffing the RFC 5201 and RFC 7401 parameter tables gives 511, 579, 715 and 2049. R1_COUNTER 129 is already covered, and HIP_MAC/HIP_MAC_2 keep their old numbers.
  • The fix is necessary: with hip.py reverted, 8 of 11 tests fail.
  • Follow-up, not a blocker: _make_param_hip_transform has no version check of its own. It is not reachable from users, because HIP(...) round-trips through the reader, which rejects HIP_TRANSFORM at v2 (hip.py:1350).

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.87% (unit tier, Python 3.14, 80ede4924, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1878 91 580 22 94.34%
pcapkit/dumpkit 142 0 42 0 100.00%
pcapkit/foundation 2422 104 842 40 94.24%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15704 188 3976 163 98.19%
pcapkit/toolkit 487 65 144 1 85.42%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 5b09a9e into main Oct 7, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1133-hipv2-params-in-v1 branch October 7, 2026 00:17
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
JarryShaw added a commit that referenced this pull request Oct 7, 2026
- 4 new 1.5.0 entries: HTTP/2 sample stream identifiers (#1161),
  TCP option re-padding (#1163, breaking), format='pcap' output
  (#1164), declared lengths on truncated captures (#1166, breaking)
- extend the PCAP Header (#1159), HIP (#1160) and from_data
  round-trip (#1162) entries rather than duplicating them
- regenerate CHANGELOG.md with util/changelog_md.py
- process.rst: entry-count pin re-measured, 189 -> 193

tests/project: conventions_doc_claims, changelog_md and
documentation_claims pass; changelog_md.py --check is in step.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(hip): HIPv2-only parameters are accepted inside HIPv1 packets

1 participant