Skip to content

Commit e00c8ab

Browse files
authored
fix(ospf,ngap): keep the parsed OSPF Packet Length and refuse octets after the NGAP-PDU (#1304)
- OSPF: make() takes packet_length, and _make_data passes the parsed len, so the RFC 2328 D.4.3 digest outside Packet Length is no longer counted and a declared Length is not rewritten (#1249). Data without len falls back to 24 + len(payload); a packet_length that is not an int in 0..65535 raises ProtocolError. - OSPF: 8 raw octets are accepted as cryptographic auth_data and unpacked into the schema (#1250); auth_data of any other width raises ProtocolError instead of being padded or cut (#1251, OSPF half). - NGAP: octets after the aligned PER NGAP-PDU raise ProtocolError instead of being dropped (#1252). Closes #1249 Closes #1250 Closes #1252 Refs #1251
1 parent c9676c5 commit e00c8ab

5 files changed

Lines changed: 215 additions & 14 deletions

File tree

‎pcapkit/protocols/application/ngap.py‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -396,8 +396,9 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_NGAP':
396396
Parsed packet data.
397397
398398
Raises:
399-
ProtocolError: If |pycrate|_ is not installed, or if the payload is
400-
not a well formed aligned PER ``NGAP-PDU``.
399+
ProtocolError: If |pycrate|_ is not installed, if the payload is
400+
not a well formed aligned PER ``NGAP-PDU``, or if octets follow
401+
the ``NGAP-PDU``.
401402
402403
"""
403404
if length is None:
@@ -417,7 +418,23 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_NGAP':
417418
# some 700x the ~0.15 ms of the decode itself; and `from_aper()`
418419
# overwrites the stored value outright, so it would buy nothing
419420
# even if it were free.
420-
pdu.from_aper(schema.data)
421+
from pycrate_core.charpy import Charpy # pylint: disable=import-outside-toplevel
422+
423+
# NOTE: decoded from a `Charpy` rather than from the bytes, so
424+
# that what the decoder did not consume can be measured: a bare
425+
# `from_aper(bytes)` ignores a suffix. SCTP preserves message
426+
# boundaries: each user message occupies its own DATA chunk
427+
# (RFC 9260 §1.3), and one with both B and E set is a complete,
428+
# unfragmented message (§3.3.1). The outermost APER encoding is
429+
# padded with zero to seven zero bits up to the next octet
430+
# (X.691 (02/2021) §11.1.4), and the result is the complete
431+
# encoding (§11.1.5). So a whole octet left over is not part of
432+
# the NGAP-PDU.
433+
char = Charpy(schema.data)
434+
pdu.from_aper(char)
435+
trailing = char.len_byte()
436+
if trailing:
437+
raise ProtocolError(f'NGAP: {trailing} octet(s) after the NGAP-PDU')
421438
kind, body = pdu.get_val()
422439

423440
message, message_body = body['value']
@@ -447,9 +464,9 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_NGAP':
447464
)
448465
except ProtocolError:
449466
# NOTE: a ProtocolError raised inside this block is already
450-
# specific about what went wrong, so it must not be caught below
451-
# and relabelled "malformed NGAP-PDU". Nothing here raises one
452-
# now; this keeps that true of the next edit.
467+
# specific about what went wrong, such as the trailing-octet
468+
# check above, so it must not be caught below and relabelled
469+
# "malformed NGAP-PDU".
453470
raise
454471
except Exception as exc:
455472
# NOTE: `Exception` rather than something narrower on purpose.

‎pcapkit/protocols/application/ospf.py‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ def make(self,
197197
type_default: 'Optional[int]' = None,
198198
type_namespace: 'Optional[dict[str, int] | dict[int, str] | Type[StdlibEnum] | Type[AenumEnum]]' = None, # pylint: disable=line-too-long
199199
type_reversed: 'bool' = False,
200+
packet_length: 'Optional[int]' = None,
200201
router_id: 'IPv4Address | str | bytes | bytearray' = '0.0.0.0', # nosec: B104
201202
area_id: 'IPv4Address | str | bytes | bytearray' = '0.0.0.0', # nosec: B104
202203
checksum: 'bytes' = b'\x00\x00',
@@ -215,6 +216,12 @@ def make(self,
215216
type_default: Default value for ``type`` if not specified.
216217
type_namespace: Namespace for ``type``.
217218
type_reversed: Reverse namespace for ``type``.
219+
packet_length: Packet length (header included). If not given, it is
220+
computed as 24 plus the length of ``payload``. Give it
221+
explicitly when ``payload`` carries octets outside the OSPF
222+
packet, such as the message digest appended under
223+
cryptographic authentication (:rfc:`2328#appendix-D.4.3`),
224+
which Packet Length does not count.
218225
router_id: Router ID.
219226
area_id: Area ID.
220227
checksum: Checksum.
@@ -229,23 +236,37 @@ def make(self,
229236
Returns:
230237
Constructed packet data.
231238
239+
Raises:
240+
ProtocolError: If ``auth_data`` is of an invalid type, or is
241+
:obj:`bytes` that are not exactly 8 octets long; or if
242+
``packet_length`` is not an :obj:`int` in ``0..65535``.
243+
232244
"""
233245
type_ = self._make_index(type, type_default, namespace=type_namespace,
234246
reversed=type_reversed, pack=False)
235247
auth_type_ = self._make_index(auth_type, auth_type_default, namespace=auth_type_namespace,
236248
reversed=auth_type_reversed, pack=False)
237249

250+
data: 'bytes | Schema_CryptographicAuthentication'
238251
if auth_type_ == Enum_Authentication.Cryptographic_authentication:
239252
data = self._make_encrypt_auth(auth_data)
240253
else:
241254
if not isinstance(auth_data, bytes):
242255
raise ProtocolError(f'OSPF: invalid type for authentication data: {auth_data!r}')
256+
if len(auth_data) != 8:
257+
raise ProtocolError(f'OSPF: authentication data must be 8 octets: {auth_data!r}')
243258
data = auth_data
244259

260+
if packet_length is None:
261+
packet_length = 24 + len(payload)
262+
elif (not isinstance(packet_length, int) or isinstance(packet_length, bool)
263+
or not 0 <= packet_length <= 0xFFFF):
264+
raise ProtocolError(f'OSPF: invalid packet length: {packet_length!r}')
265+
245266
return Schema_OSPF(
246267
version=version,
247268
type=type_, # type: ignore[arg-type]
248-
length=24 + len(payload),
269+
length=packet_length,
249270
router_id=router_id,
250271
area_id=area_id,
251272
checksum=checksum,
@@ -292,6 +313,7 @@ def _make_data(cls, data: 'Data_OSPF') -> 'dict[str, Any]': # type: ignore[over
292313
return {
293314
'version': data.version,
294315
'type': data.type,
316+
'packet_length': data.get('len'),
295317
'router_id': data.router_id,
296318
'area_id': data.area_id,
297319
'checksum': data.chksum,
@@ -378,18 +400,26 @@ def _read_encrypt_auth(self, schema: 'Schema_CryptographicAuthentication') -> 'D
378400

379401
def _make_encrypt_auth(self,
380402
auth_data: 'bytes | Schema_CryptographicAuthentication | Data_CryptographicAuthentication' # pylint: disable=line-too-long
381-
) -> 'bytes | Schema_CryptographicAuthentication':
403+
) -> 'Schema_CryptographicAuthentication':
382404
"""Make Authentication field when Cryptographic Authentication is employed.
383405
384406
Args:
385407
auth_data: Authentication data.
386408
387409
Returns:
388-
Authentication bytes.
410+
Authentication schema.
411+
412+
Raises:
413+
ProtocolError: If ``auth_data`` is of an invalid type, or is
414+
:obj:`bytes` that are not exactly 8 octets long.
389415
390416
"""
391-
if isinstance(auth_data, (Schema_CryptographicAuthentication, bytes)):
417+
if isinstance(auth_data, Schema_CryptographicAuthentication):
392418
return auth_data
419+
if isinstance(auth_data, bytes):
420+
if len(auth_data) != 8:
421+
raise ProtocolError(f'OSPF: authentication data must be 8 octets: {auth_data!r}')
422+
return Schema_CryptographicAuthentication.unpack(auth_data) # type: ignore[call-arg,misc]
393423
if isinstance(auth_data, Data_CryptographicAuthentication):
394424
return Schema_CryptographicAuthentication(
395425
key_id=auth_data.key_id,

‎tests/protocols/application/test_ngap_unit.py‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -475,9 +475,11 @@ def test_malformed_payloads_raise_protocol_error_not_pycrate_errors(self) -> Non
475475
except ProtocolError as exc:
476476
self.assertIn('NGAP:', str(exc))
477477
else:
478-
# Decoding a prefix or ignoring a suffix is legitimate PER
479-
# behaviour; what matters is that it did not raise something
480-
# raw. Assert it produced a usable model.
478+
# Decoding a prefix is legitimate PER behaviour; what
479+
# matters is that it did not raise something raw. A suffix
480+
# is not: the chunk carries exactly one NGAP-PDU, so octets
481+
# after it are refused rather than dropped (#1252).
482+
self.assertNotEqual(label, 'trailing garbage')
481483
self.assertIsNotNone(info.message)
482484

483485
@unittest.skipUnless(HAS_PYCRATE, 'pycrate not installed')
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
# -*- coding: utf-8 -*-
2+
"""OSPF and NGAP rebuild what they parsed, or refuse it.
3+
4+
GitHub issues:
5+
6+
* #1249: :meth:`OSPF.make <pcapkit.protocols.application.ospf.OSPF.make>`
7+
recomputed Packet Length from the payload and ``_make_data`` dropped the
8+
parsed one, so the 16-octet digest that cryptographic authentication appends
9+
outside the packet (:rfc:`2328#appendix-D.4.3`) was counted in it.
10+
* #1250: ``OSPF(auth_type=2, auth_data=<bytes>)`` raised
11+
:exc:`AttributeError`.
12+
* #1251 (OSPF half): an ``auth_data`` that is not 8 octets was padded or cut.
13+
* #1252: octets after the aligned PER ``NGAP-PDU`` were dropped on rebuild.
14+
15+
Every case builds its own octets in memory and reads no capture. Classes are
16+
imported inside each test, after :func:`~tests._support.reimport_once_per_class`,
17+
c.f. ``tests/protocols/link/test_ethernet_mac_roundtrip_unit.py``.
18+
19+
"""
20+
21+
import importlib.util
22+
import unittest
23+
24+
from tests._support import reimport_once_per_class
25+
26+
HAS_PYCRATE = importlib.util.find_spec('pycrate_asn1dir') is not None
27+
28+
#: Cryptographic authentication: Key ID 1, Auth Data Len 16, sequence 7.
29+
CRYPTO_AUTH = bytes.fromhex('0000011000000007')
30+
#: A 20-octet Hello body.
31+
HELLO_BODY = bytes.fromhex('ffffff00000a020100000028c0a8000100000000')
32+
33+
34+
def ospf_header(length: int, auth_type: int, auth: bytes) -> bytes:
35+
"""OSPFv2 Hello header with the given Packet Length and authentication."""
36+
return (bytes.fromhex('0201') + length.to_bytes(2, 'big')
37+
+ bytes.fromhex('c0a8000100000000' '0000')
38+
+ auth_type.to_bytes(2, 'big') + auth)
39+
40+
41+
#: Packets whose Packet Length is not ``24 + len(payload)``.
42+
OSPF_PACKETS = {
43+
'crypto auth, digest outside Length': ospf_header(0x2c, 2, CRYPTO_AUTH) + HELLO_BODY + bytes(range(16)),
44+
'Length zero': ospf_header(0x00, 0, bytes(8)) + HELLO_BODY,
45+
'Length short of the payload': ospf_header(0x1e, 0, bytes(8)) + HELLO_BODY,
46+
'Length matching the payload': ospf_header(0x2c, 0, bytes(8)) + HELLO_BODY,
47+
}
48+
49+
#: ``NGSetupRequest`` in aligned PER, c.f. ``test_ngap_unit.NGSETUP_REQUEST``.
50+
NGSETUP_REQUEST = bytes.fromhex(
51+
'00150036000004001b00080002f839100001020052400d0500706361706b69742d676e62'
52+
'0066000d00000000010002f839000000080015400140'
53+
)
54+
55+
56+
class TestOSPFRoundTrip(unittest.TestCase):
57+
"""Pin Packet Length and the 8-octet Authentication field."""
58+
59+
def setUp(self) -> None:
60+
reimport_once_per_class(self)
61+
62+
def test_from_data_keeps_parsed_packet_length(self) -> None:
63+
from pcapkit.protocols.application.ospf import OSPF
64+
65+
for name, packet in OSPF_PACKETS.items():
66+
with self.subTest(packet=name):
67+
parsed = OSPF(packet, len(packet))
68+
self.assertEqual(bytes(OSPF.from_data(parsed.info)), packet)
69+
70+
def test_make_packet_length_default_and_explicit(self) -> None:
71+
from pcapkit.protocols.application.ospf import OSPF
72+
73+
self.assertEqual(bytes(OSPF(payload=HELLO_BODY))[2:4], b'\x00\x2c')
74+
self.assertEqual(bytes(OSPF(packet_length=0x1e, payload=HELLO_BODY))[2:4], b'\x00\x1e')
75+
76+
def test_from_data_without_len_computes_packet_length(self) -> None:
77+
from ipaddress import ip_address
78+
79+
from pcapkit.protocols.application.ospf import OSPF
80+
from pcapkit.protocols.data.application.ospf import OSPF as Data_OSPF
81+
82+
data = Data_OSPF.from_dict({
83+
'version': 2, 'type': 1, 'router_id': ip_address('192.168.0.1'),
84+
'area_id': ip_address('0.0.0.0'), 'chksum': b'\x00\x00', 'autype': 0,
85+
'auth': bytes(8),
86+
})
87+
self.assertNotIn('len', data)
88+
self.assertEqual(bytes(OSPF.from_data(data)), ospf_header(24, 0, bytes(8)))
89+
90+
def test_make_rejects_invalid_packet_length(self) -> None:
91+
from pcapkit.protocols.application.ospf import OSPF
92+
from pcapkit.utilities.exceptions import ProtocolError
93+
94+
for packet_length in (-1, 0x10000, True, 24.0, '24'):
95+
with self.subTest(packet_length=packet_length):
96+
with self.assertRaises(ProtocolError) as caught:
97+
OSPF(packet_length=packet_length)
98+
self.assertIn('invalid packet length', str(caught.exception))
99+
self.assertEqual(bytes(OSPF(packet_length=0xFFFF))[2:4], b'\xff\xff')
100+
101+
def test_make_crypto_auth_from_bytes(self) -> None:
102+
from pcapkit.protocols.application.ospf import OSPF
103+
104+
packet = bytes(OSPF(auth_type=2, auth_data=CRYPTO_AUTH))
105+
self.assertEqual(packet[16:24], CRYPTO_AUTH)
106+
107+
info = OSPF(packet, len(packet)).info
108+
self.assertEqual((info.auth.key_id, info.auth.len, info.auth.seq), (1, 16, 7))
109+
self.assertEqual(bytes(OSPF.from_data(info)), packet)
110+
111+
def test_make_rejects_wrong_width_auth_data(self) -> None:
112+
from pcapkit.protocols.application.ospf import OSPF
113+
from pcapkit.utilities.exceptions import ProtocolError
114+
115+
for auth_type in (0, 1, 2):
116+
for auth_data in (b'abc', b'0123456789', b''):
117+
with self.subTest(auth_type=auth_type, auth_data=auth_data):
118+
with self.assertRaises(ProtocolError) as caught:
119+
OSPF(auth_type=auth_type, auth_data=auth_data)
120+
self.assertIn('8 octets', str(caught.exception))
121+
122+
123+
@unittest.skipUnless(HAS_PYCRATE, 'pycrate not installed')
124+
class TestNGAPTrailingOctets(unittest.TestCase):
125+
"""Octets after the ``NGAP-PDU`` are refused rather than dropped."""
126+
127+
def setUp(self) -> None:
128+
reimport_once_per_class(self)
129+
130+
def test_exact_pdu_rebuilds_byte_for_byte(self) -> None:
131+
from pcapkit.protocols.application.ngap import NGAP
132+
133+
parsed = NGAP(NGSETUP_REQUEST, len(NGSETUP_REQUEST))
134+
self.assertEqual(bytes(NGAP.from_data(parsed.info)), NGSETUP_REQUEST)
135+
136+
def test_trailing_octets_raise_protocol_error(self) -> None:
137+
from pcapkit.protocols.application.ngap import NGAP
138+
from pcapkit.utilities.exceptions import ProtocolError
139+
140+
for suffix in (b'\x00', b'\xff\xff', b'\xde\xad\xbe\xef'):
141+
with self.subTest(suffix=suffix):
142+
packet = NGSETUP_REQUEST + suffix
143+
with self.assertRaises(ProtocolError) as caught:
144+
NGAP(packet, len(packet))
145+
self.assertIn(f'{len(suffix)} octet(s) after the NGAP-PDU', str(caught.exception))
146+
147+
148+
if __name__ == '__main__':
149+
unittest.main()

‎tests/protocols/application/test_ospf_unit.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,10 @@ def test_ospf_properties_read_make_and_auth_helpers(self) -> None:
141141
)
142142
self.assertEqual(crypto_schema_from_data.auth_data.key_id, 2)
143143
self.assertIs(maker._make_encrypt_auth(crypto_schema), crypto_schema)
144-
self.assertEqual(maker._make_encrypt_auth(b'\x02' * 8), b'\x02' * 8)
144+
crypto_schema_from_bytes = maker._make_encrypt_auth(b'\x00\x00\x02\x14' + b'\x00\x00\x00\x64')
145+
self.assertIsInstance(crypto_schema_from_bytes, SchemaCryptoAuth)
146+
self.assertEqual((crypto_schema_from_bytes.key_id, crypto_schema_from_bytes.len,
147+
crypto_schema_from_bytes.seq), (2, 20, 100))
145148
self.assertEqual(maker._read_encrypt_auth(crypto_schema).len, 16)
146149
self.assertEqual(maker._read_id_numbers(b'\xc0\x00\x02\x04'), ip_address('192.0.2.4'))
147150
self.assertEqual(maker._make_id_numbers('192.0.2.5'), b'\xc0\x00\x02\x05')

0 commit comments

Comments
 (0)