Skip to content

Support multiple entries in IPOption_Timestamp - #5117

Open
itzzdev09 wants to merge 1 commit into
secdev:masterfrom
itzzdev09:ipoption-timestamp-multiple-entries
Open

Support multiple entries in IPOption_Timestamp#5117
itzzdev09 wants to merge 1 commit into
secdev:masterfrom
itzzdev09:ipoption-timestamp-multiple-entries

Conversation

@itzzdev09

Copy link
Copy Markdown

Fixes #4513

Problem

RFC 791 says the originating host "must compose this option with a large enough timestamp data area to hold all the timestamp information expected", so that each router along the path can append an entry.

IPOption_Timestamp modelled exactly one entry — a single conditional internet_address plus a single timestamp. There was no way to reserve room for more, so a target host had nowhere to record its timestamp and would reply with the overflow flag set.

Change

The data area is now a list, sized from the option length on dissection, following the pattern IPOption_RR already uses for routers:

  • flg=0 (timestamp_only) → timestamps, a list of 32-bit timestamps.
  • flg=1/flg=3pairs, a list of IPOption_Timestamp_Pair (an internet address followed by its timestamp).

Reserving space for four replies now works:

>>> raw(IPOption_Timestamp(pairs=[IPOption_Timestamp_Pair() for _ in range(4)]))
# 36 bytes, length field = 36

and dissection round-trips:

>>> p = IPOption_Timestamp(b'D\x14\x05\x01\xc0\xa8\x0f\x07\x00\x00\x00\x01\n\x00\x00\x01\x00\x00\x00\x02')
>>> [(e.internet_address, e.timestamp) for e in p.pairs]
[('192.168.15.7', 1), ('10.0.0.1', 2)]

Notes for review

Two deliberate decisions worth flagging:

  1. This replaces the internet_address / timestamp fields. Callers move to pairs=[IPOption_Timestamp_Pair(internet_address=..., timestamp=...)] or timestamps=[...]. I couldn't see a way to keep the scalar names while supporting a list of interleaved address/timestamp pairs; happy to add compatibility properties if you'd like.

  2. Defaults are empty lists, matching IPOption_RR, so pointer now defaults to 5 — the RFC minimum, pointing at the first free octet — instead of 9, which assumed one entry was already present. A non-empty default is not safe here: scapy shallow-copies list defaults (value.copy() in Packet.do_init_cached_fields), so a Packet inside a default list would be shared across instances. I verified that with a mutation test while developing this.

The two existing assertions in inet.uts are updated to the new spelling and keep their original expected bytes; the checksum test keeps its 12-byte option (and so its existing checksum) by passing one explicit empty pair.

Test plan

  • UTscapy -t test/scapy/layers/inet.uts67 passed, 0 failed (same as before the change).
  • New assertions cover multi-entry build for both flg shapes and dissection back into the lists.
  • flake8 scapy/layers/inet.py with the project config: 15 pre-existing issues before and after, none introduced.

RFC 791 requires the originating host to compose the timestamp option with a
data area large enough to hold every timestamp it expects back, so that each
router on the path can append one. IPOption_Timestamp modelled only a single
entry, so a target host had no room to record anything and would reply with
the overflow flag set.

Model the data area as a list instead:

- flg 0 (timestamp_only) uses 'timestamps', a list of 32-bit timestamps.
- flg 1/3 use 'pairs', a list of IPOption_Timestamp_Pair, each an internet
  address followed by its timestamp.

Both are sized from the option length on dissection, mirroring IPOption_RR.
The defaults are empty lists, also as in IPOption_RR, so 'pointer' now
defaults to 5 (the RFC minimum, pointing at the first free octet) rather than
to 9, which assumed one entry was already present.

Note this replaces the 'internet_address' and 'timestamp' fields; existing
callers move to 'pairs=[IPOption_Timestamp_Pair(...)]' or 'timestamps=[...]'.
@itzzdev09

Copy link
Copy Markdown
Author

Disclosure, per the "AI-assisted reports and PRs" section of CONTRIBUTING: this PR was drafted with AI assistance (Claude Code, Claude Opus 5). I should have said so in the original description — apologies for the omission.

Everything in it was verified by running it rather than assumed:

  • UTscapy -t test/scapy/layers/inet.uts passes 67/67, the same count as before the change.
  • The multi-entry build and dissection cases in the new assertions were checked against hand-written expected bytes.
  • flake8 scapy/layers/inet.py with the project config reports the same 15 pre-existing issues before and after — none introduced.
  • The two updated assertions keep their original expected bytes, and the checksum test keeps its original checksum by passing one explicit empty pair.

One thing worth calling out, since it drove a design decision: a non-empty PacketListField default is not safe here. Packet.do_init_cached_fields shallow-copies list defaults via value.copy(), so a Packet inside the default list ends up shared across instances. I hit that with a mutation test while developing, which is why the defaults are empty lists as in IPOption_RR.

Happy to adjust anything, including the field naming, if you'd prefer a different shape.

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.

IPOptions_Timestamp does not support multiple IP Address and Timestamp fields

1 participant