Skip to content

Fix DNSRRTSIG length fields not auto-computing#5045

Open
chuenchen309 wants to merge 1 commit into
secdev:masterfrom
chuenchen309:fix/dnsrrtsig-lenfield-autocompute
Open

Fix DNSRRTSIG length fields not auto-computing#5045
chuenchen309 wants to merge 1 commit into
secdev:masterfrom
chuenchen309:fix/dnsrrtsig-lenfield-autocompute

Conversation

@chuenchen309

Copy link
Copy Markdown

Description

DNSRRTSIG declares its two FieldLenField length prefixes with hardcoded defaults — mac_len=20 and other_len=0 — instead of None. FieldLenField only auto-computes the length from its length_of target when the value is None, so these prefixes were frozen and never matched the real mac_data / other_data, breaking the build/dissect round-trip.

Even the zero-argument default can't re-parse its own bytes:

>>> from scapy.layers.dns import DNSRRTSIG
>>> b = bytes(DNSRRTSIG())
>>> DNSRRTSIG(b)
struct.error: unpack requires a buffer of 2 bytes

mac_len is 20 but mac_data is empty, so on dissect the mac_data StrLenField consumes the 20 bytes belonging to the trailing fields, and the next getfield unpacks past the buffer. With a real MAC (e.g. a 32-byte HMAC-SHA256), the length prefix stays 20 and 12 bytes of the MAC are silently truncated on the wire.

Every other FieldLenField in dns.pyDNSRROPT.rdlen, DNSRRNAPTR's flags_len/services_len/regexp_len, and the header counts — uses None. DNSRROPT(rdata=...) round-trips correctly for exactly that reason, which confirms None is the intended pattern and the literal 20/0 are the defect.

Fix: set mac_len and other_len defaults to None so they auto-compute like their siblings (2 lines).

Tests

Two existing dns_dnssec.uts assertions had baked in the buggy \x14 (mac_len=20 with an empty MAC); corrected to \x00, and added a round-trip case asserting a 32-byte MAC + 6-byte other_data survive build→dissect (mac_len/other_len auto-compute to 32/6). The new case is red before the fix and green after. dns.uts (31) and dns_dnssec.uts pass; flake8 clean and mypy unchanged vs baseline.

AI assistance: prepared with Claude Code (Claude Opus 4.8) — disclosed via the AI-Assisted: yes (Claude Opus 4.8) commit trailer. I reproduced the crash, verified the round-trip fix against the sibling FieldLenFields, corrected the bug-baked test expectations, and reviewed every line before opening.

DNSRRTSIG declared its two FieldLenField length prefixes with hardcoded
defaults -- mac_len=20 and other_len=0 -- instead of None. FieldLenField only
derives the length from its length_of target when the value is None, so the
prefixes were frozen and never matched the actual mac_data / other_data.

Consequences on the build/dissect round-trip:
- Even the zero-argument DNSRRTSIG() cannot re-parse its own bytes: mac_len is
  20 but mac_data is empty, so on dissect the mac_data StrLenField consumes the
  20 bytes belonging to the trailing fields and the next getfield unpacks past
  the buffer (struct.error).
- A real MAC (e.g. a 32-byte HMAC-SHA256) is silently truncated to 20 bytes on
  the wire, because the length prefix stays 20.

Every other FieldLenField in dns.py -- DNSRROPT.rdlen, DNSRRNAPTR's
flags_len/services_len/regexp_len, and the header counts -- uses None. Change
mac_len and other_len to None so they auto-compute like their siblings.

Two existing dns_dnssec.uts assertions had baked in the buggy 0x14 (mac_len=20
with an empty MAC); corrected to 0x00 and added a round-trip case that a
32-byte MAC and 6-byte other_data survive build->dissect.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AI-Assisted: yes (Claude Opus 4.8)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes DNSRRTSIG TSIG length-prefix fields (mac_len, other_len) so they auto-compute from their corresponding payload fields during build, restoring correct build→dissect round-tripping and preventing silent MAC truncation / mis-dissection.

Changes:

  • Set DNSRRTSIG.mac_len and DNSRRTSIG.other_len FieldLenField defaults to None so lengths are computed from mac_data / other_data.
  • Update existing TSIG raw-byte expectations that previously baked in the buggy hardcoded mac_len=20.
  • Add a regression test covering non-empty mac_data + other_data round-trip with auto-computed lengths.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
scapy/layers/dns.py Fixes TSIG length fields to auto-compute instead of using hardcoded defaults.
test/scapy/layers/dns_dnssec.uts Updates expectations for the corrected encoding and adds a regression round-trip assertion.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.21%. Comparing base (de33992) to head (e864c32).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5045      +/-   ##
==========================================
+ Coverage   80.13%   80.21%   +0.07%     
==========================================
  Files         388      388              
  Lines       96467    96467              
==========================================
+ Hits        77308    77382      +74     
+ Misses      19159    19085      -74     
Files with missing lines Coverage Δ
scapy/layers/dns.py 83.85% <ø> (ø)

... and 10 files with indirect coverage changes

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

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.

3 participants