Skip to content

Commit 3ddcd18

Browse files
Copilotguedou
andcommitted
Address code review feedback: add MAC size constants and clarify comments
- Define named constants for MAC sizes (_NTP_AUTH_MD5_SIZE, _NTP_AUTH_SHA1_SIZE, etc.) - Update _ntp_auth_tail_size() to use constants instead of hardcoded values - Clarify test comments to explain SHA1 vs MD5 parsing interpretation Co-authored-by: guedou <11683796+guedou@users.noreply.github.com>
1 parent 892809e commit 3ddcd18

File tree

2 files changed

+25
-9
lines changed

2 files changed

+25
-9
lines changed

scapy/layers/ntp.py

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,14 @@
5858
_NTP_HDR_WITH_EXT_MIN_SIZE = _NTP_AUTH_MD5_MIN_SIZE + _NTP_EXT_MIN_SIZE
5959
_NTP_AUTH_MD5_TAIL_SIZE = 20
6060
_NTP_AUTH_MD5_DGST_SIZE = 16
61+
62+
# Valid NTP MAC sizes (key_id + digest)
63+
_NTP_AUTH_MD5_SIZE = 20 # 4 + 16
64+
_NTP_AUTH_SHA1_SIZE = 24 # 4 + 20
65+
_NTP_AUTH_SHA256_SIZE = 36 # 4 + 32
66+
_NTP_AUTH_SHA384_SIZE = 52 # 4 + 48
67+
_NTP_AUTH_SHA512_SIZE = 68 # 4 + 64
68+
6169
_NTP_PRIVATE_PACKET_MIN_SIZE = 8
6270

6371
# ntpd "Private" messages are the shortest
@@ -82,17 +90,23 @@ def _ntp_auth_tail_size(length):
8290
"""
8391
Dynamically compute the NTP authenticator tail size (key_id + digest).
8492
85-
Valid MAC sizes are: 20, 24, 36, 52, 68 bytes (4-byte key_id + digest)
86-
- 20 bytes: MD5 (4 + 16)
87-
- 24 bytes: SHA1 (4 + 20)
88-
- 36 bytes: SHA256 (4 + 32)
89-
- 52 bytes: SHA384 (4 + 48)
90-
- 68 bytes: SHA512 (4 + 64)
93+
Valid MAC sizes are defined as constants:
94+
- _NTP_AUTH_MD5_SIZE (20): MD5 (4 + 16)
95+
- _NTP_AUTH_SHA1_SIZE (24): SHA1 (4 + 20)
96+
- _NTP_AUTH_SHA256_SIZE (36): SHA256 (4 + 32)
97+
- _NTP_AUTH_SHA384_SIZE (52): SHA384 (4 + 48)
98+
- _NTP_AUTH_SHA512_SIZE (68): SHA512 (4 + 64)
9199
92100
Returns the tail size if it matches a known valid size, otherwise
93101
returns _NTP_AUTH_MD5_TAIL_SIZE as a fallback.
94102
"""
95-
valid_mac_sizes = [20, 24, 36, 52, 68]
103+
valid_mac_sizes = [
104+
_NTP_AUTH_MD5_SIZE,
105+
_NTP_AUTH_SHA1_SIZE,
106+
_NTP_AUTH_SHA256_SIZE,
107+
_NTP_AUTH_SHA384_SIZE,
108+
_NTP_AUTH_SHA512_SIZE
109+
]
96110
# Check for exact match with a known MAC size
97111
if length in valid_mac_sizes:
98112
return length

test/scapy/layers/ntp.uts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,10 @@ assert p.stratum == 2
106106
= NTPAuthenticator - MD5 with padding (old test, updated for correct parsing)
107107

108108
# This packet has 24 bytes of authenticator data
109-
# With the old (broken) code, it was parsed as: 4 padding + 4 key_id + 16 MD5 digest
110-
# With the new (correct) code, it should be parsed as: 4 key_id + 20 SHA1 digest
109+
# The old (incorrect) code interpreted this as: 4 padding + 4 key_id + 16 MD5 digest
110+
# The new (correct) code interprets 24 bytes as SHA1 MAC: 4 key_id + 20 SHA1 digest
111+
# Note: This test packet may have been created with MD5 intent, but with 24 bytes
112+
# total, it's now correctly parsed as SHA1 according to RFC 5905 standards
111113
s = hex_bytes("000c2962f268d094666d23750800450000640db640004011a519c0a80364c0a80305a51e007b0050731a2300072000000000000000000000000000000000000000000000000000000000000000000000000052c7bc1dda64b97d0000000bcdc3825dbf6b7ad02886ff45aa8b2eaf7ac78bc1")
112114
p = Ether(s)
113115
assert NTPAuthenticator in p

0 commit comments

Comments
 (0)