Skip to content

Fix NTP Authenticator parsing for non-MD5 digest types#4918

Draft
Copilot wants to merge 5 commits intomasterfrom
copilot/fix-ntp-authenticator-parsing
Draft

Fix NTP Authenticator parsing for non-MD5 digest types#4918
Copilot wants to merge 5 commits intomasterfrom
copilot/fix-ntp-authenticator-parsing

Conversation

Copy link

Copilot AI commented Feb 14, 2026

Implementation Plan for NTP Authenticator Parsing Fix

  • Add XStrField to imports in scapy/layers/ntp.py
  • Create _ntp_auth_tail_size() helper function to dynamically compute tail size
  • Update _NTPAuthenticatorPaddingField to use dynamic tail size computation
  • Update NTPAuthenticator.dgst from XStrFixedLenField to XStrField
  • Update NTPExtPacketListField.getfield() to use dynamic tail size detection
  • Update NTPHeader.guess_payload_class() to include SHA384 (48-byte) digest
  • Add test cases for SHA1 (24-byte) NTP authenticator parsing
  • Add test cases for SHA256 (36-byte) NTP authenticator parsing
  • Address code review feedback (add constants, clarify comments)
  • Run security checks (0 vulnerabilities found)
  • Fix code health issues (flake8 compliance)

Summary

Successfully fixed NTP Authenticator parsing to support multiple digest types (MD5, SHA1, SHA256, SHA384, SHA512) instead of being hardcoded to MD5's 16-byte digest size.

Changes:

  • Added dynamic MAC size detection via _ntp_auth_tail_size() helper
  • Defined MAC size constants for better maintainability
  • Updated all parsing logic to use dynamic sizes
  • Changed NTPAuthenticator.dgst from fixed-length to variable-length field
  • Added comprehensive tests with round-trip verification
  • Fixed all code health issues (flake8 compliance)

Test Results:

  • All 77 NTP regression tests pass
  • New tests verify SHA1 (24-byte) and SHA256 (36-byte) parsing
  • Security scan: 0 vulnerabilities
  • Code health: All flake8 checks pass

Security Summary:

No security vulnerabilities detected by CodeQL analysis.

Original prompt

The PR #4917 addresses an issue with NTP Authenticator parsing in the Scapy project. The problem arises because the NTP Authenticator fields were hardcoded to MD5's 16-byte digest size, leading to misaligned parsing for other digest types such as SHA1, SHA256, SHA384, and SHA512. Specifically, the _NTPAuthenticatorPaddingField wrongly assumed a 20-byte tail (4 key_id + 16 MD5 digest). For instance, in the case of SHA1 (24 bytes total), the first 4 bytes of the digest were wrongly consumed as padding, causing incorrect parsing of fields like key_id and dgst.

Required Fixes:

  1. Update _NTPAuthenticatorPaddingField to compute the tail size dynamically by matching against known valid MAC sizes (20/24/36/52/68) instead of using a hardcoded value _NTP_AUTH_MD5_TAIL_SIZE.
  2. Update NTPAuthenticator.dgst to replace XStrFixedLenField with XStrField to ensure it consumes all remaining bytes after the key_id.
  3. Modify NTPExtPacketListField.getfield() to implement dynamic tail size detection when separating extensions from the MAC.
  4. Update NTPHeader.guess_payload_class() to include the recognized SHA384 (48-byte) digest size.
  5. Update NTPExtensions so that _NTPAuthenticatorPaddingField uses dynamic size calculation by adding a new _ntp_auth_tail_size() helper.

Testing:

You should also add test cases to verify the correct parsing and round-trip build/parse of SHA1 (24-byte) and SHA256 (36-byte) NTP authenticators.

Commit these changes to the secdev/scapy repository on the copilot/check-oldest-issues branch. Retain the existing PR title and description, and include a message summarizing the new implementation and tests.

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits February 14, 2026 12:50
- Add XStrField import
- Add _ntp_auth_tail_size() helper for dynamic MAC size detection
- Update _NTPAuthenticatorPaddingField to use dynamic tail size
- Change NTPAuthenticator.dgst from XStrFixedLenField to XStrField
- Update NTPExtPacketListField.getfield() for dynamic tail detection
- Add SHA384 (48-byte) to NTPHeader.guess_payload_class()

Co-authored-by: guedou <11683796+guedou@users.noreply.github.com>
- Add SHA1 (24-byte) authenticator parsing test with round-trip verification
- Add SHA256 (36-byte) authenticator parsing test with round-trip verification
- Update existing MD5 authenticator test to reflect correct SHA1 parsing for 24-byte payloads
- Update NTPControl test to expect SHA1 parsing for 24-byte authenticator data

Co-authored-by: guedou <11683796+guedou@users.noreply.github.com>
…ents

- 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>
Copilot AI changed the title [WIP] Fix NTP Authenticator parsing for dynamic digest sizes Fix NTP Authenticator parsing for non-MD5 digest types Feb 14, 2026
Copilot AI requested a review from guedou February 14, 2026 13:05
@codecov
Copy link

codecov bot commented Feb 14, 2026

Codecov Report

❌ Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.10%. Comparing base (ded1d73) to head (4dd2c8f).

Files with missing lines Patch % Lines
scapy/layers/ntp.py 70.58% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4918   +/-   ##
=======================================
  Coverage   80.10%   80.10%           
=======================================
  Files         370      370           
  Lines       91727    91739   +12     
=======================================
+ Hits        73477    73488   +11     
- Misses      18250    18251    +1     
Files with missing lines Coverage Δ
scapy/layers/ntp.py 86.60% <70.58%> (-1.05%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@guedou
Copy link
Member

guedou commented Feb 15, 2026

@copilot fix the code health error, please.

- Remove trailing whitespace on blank lines (W293)
- Remove trailing whitespace at end of line (W291)
- Add extra blank line before class definition (E302)
- Move comment to separate line to fix line length (E501)

Co-authored-by: guedou <11683796+guedou@users.noreply.github.com>
Copy link
Author

Copilot AI commented Feb 15, 2026

@copilot fix the code health error, please.

Fixed in commit 4dd2c8f. Addressed all flake8 violations:

  • Removed trailing whitespace on blank lines and at end of line
  • Added blank line before class definition
  • Moved inline comment to separate line to fix line length (88 char limit)

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.

2 participants