fix: bound record payload reads against rdlength overrun by bdraco · Pull Request #1756 · python-zeroconf/python-zeroconf

bdraco · 2026-05-20T15:04:40Z

Summary

The TXT, HINFO, and A payload readers trusted the declared rdlength without checking it against the buffer; an attacker on the LAN could send a record with rdlength=65535 and only a few real bytes of payload, and we would silently truncate the slice and stuff the corrupt record into the cache. Adds the bounds check _read_string and _read_character_string should have had all along; the existing _read_others catch then skips the record, matching how malformed NSEC bitmaps already get rejected.

Details

Stays a pure C unsigned compare under Cython since offset, length, and _data_len are already unsigned int in incoming.pxd; I checked the generated .c, the hot path stays score-0 in the cython -a annotation.
Fixes Security: _read_string/_read_character_string skip remaining-byte validation, enabling silent record-data corruption #1752.

Test plan

poetry run pytest tests/ (384 passed, 3 skipped)
poetry run pre-commit run --files src/zeroconf/_protocol/incoming.py tests/test_protocol.py
New regression tests in tests/test_protocol.py cover TXT, HINFO, and A record overrun.

codspeed-hq · 2026-05-20T15:06:32Z

Merging this PR will not alter performance

✅ 14 untouched benchmarks

_{Comparing bound_read_string_record_length (c6111f9) with master (4ff6540)¹}

No successful run was found on master (f4b5066) during the generation of this report, so 4ff6540 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

codecov · 2026-05-20T15:06:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.77%. Comparing base (b22c8ff) to head (c6111f9).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1756   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files          33       33           
  Lines        3497     3508   +11     
  Branches      489      493    +4     
=======================================
+ Hits         3489     3500   +11     
  Misses          5        5           
  Partials        3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bdraco · 2026-05-20T15:12:54Z

@bluetoothbot review

Copilot

Pull request overview

This PR hardens DNSIncoming record parsing to avoid silently accepting truncated/over-advertised RDATA payloads by adding explicit bounds checks in _read_string / _read_character_string, and adds regression tests for TXT, HINFO, and A record truncation cases.

Changes:

Add packet-end bounds checks to _read_string() and _read_character_string() to raise IncomingDecodeError on overrun.
Add regression tests ensuring malformed TXT/HINFO/A records do not surface in parsed answers.

Reviewed changes

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

File	Description
`src/zeroconf/_protocol/incoming.py`	Adds bounds checks to prevent slice-truncation from producing corrupted RDATA.
`tests/test_protocol.py`	Adds tests for malformed/truncated TXT, HINFO, and A records.

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

bdraco · 2026-05-20T15:25:27Z

Good catch, addressed in c6111f9; added a per-record post-check in _read_others that drops the record and resyncs to the declared rdlength end when the inner reads consume a different number of bytes than the rdlength claimed, plus a regression test with a second valid record proving resync recovers it.

Copilot

Pull request overview

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

Copilot

Pull request overview

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

bluetoothbot · 2026-05-20T16:51:28Z

PR Review — fix: bound record payload reads against rdlength overrun

Solid fix. The two-layer approach — packet-level bounds in _read_string/_read_character_string plus the per-record self.offset != end post-check in _read_others — correctly addresses both the silent-truncation case (PR description) and the record-boundary-overrun case (raised by the Copilot reviewer and addressed in c6111f9). Tests cover TXT, HINFO, and A overruns plus the over-consume/resync case with a follow-on record that proves framing recovers. Type widths stay unsigned int per incoming.pxd, so the comparison stays a pure C unsigned compare in the Cython build with no sign-extension risk. Only nits are CLAUDE.md docstring style on two test cases and a defensive observation about end > _data_len resync; neither blocks merge.

🟢 Suggestions

1. Test docstrings retell production-side rationale (`tests/test_protocol.py`, L895-901)

Per CLAUDE.md: "Test docstrings retelling the production-side story... A test docstring should name what the test pins, in one sentence — not re-explain the bug, the fix, or the surrounding flow."

test_txt_rdlength_overruns_packet_rejected and test_record_consuming_more_than_rdlength_dropped_and_resyncs both have multi-line docstrings that re-explain the underlying Python slicing behavior, the failure mode, and the fix. The HINFO and A-record tests follow the convention with a single line — same treatment should apply to the other two.

Suggested form: keep just the first sentence ("""A TXT record with rdlength past the buffer must not enter the cache.""") and drop the rationale paragraphs — that history belongs in the PR body and the commit message, which already carry it.

    """A TXT record with rdlength past the buffer must not enter the cache.

    Python slicing silently truncates when the slice end exceeds the buffer,
    so without a bounds check in ``_read_string`` a malformed wire frame
    would land in the cache carrying a payload shorter than the rdlength
    declared, leaving the parser desynchronized for downstream records.
    """

Checklist

Input validation at boundaries (incoming wire data)
No silent truncation of attacker-controlled length fields
Error handling propagates to existing decode-skip path
No resource leaks introduced
Cython type signedness preserved (unsigned int end-to-end)
Test coverage for new branches (overrun + resync)
Test docstrings follow CLAUDE.md style — suggestion ＃1
Resync offset bounded against _data_len — suggestion ＃2

Summary

Solid fix. The two-layer approach — packet-level bounds in _read_string/_read_character_string plus the per-record self.offset != end post-check in _read_others — correctly addresses both the silent-truncation case (PR description) and the record-boundary-overrun case (raised by the Copilot reviewer and addressed in c6111f9). Tests cover TXT, HINFO, and A overruns plus the over-consume/resync case with a follow-on record that proves framing recovers. Type widths stay unsigned int per incoming.pxd, so the comparison stays a pure C unsigned compare in the Cython build with no sign-extension risk. Only nits are CLAUDE.md docstring style on two test cases and a defensive observation about end > _data_len resync; neither blocks merge.

Automated review by Kōand23b029
c6111f9

fix: bound record payload reads against rdlength overrun

d23b029

bdraco temporarily deployed to release May 20, 2026 15:10 — with GitHub Actions Inactive

bdraco marked this pull request as ready for review May 20, 2026 15:12

bdraco requested a review from Copilot May 20, 2026 15:12

Copilot started reviewing on behalf of bdraco May 20, 2026 15:14 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

Comment thread src/zeroconf/_protocol/incoming.py

Comment thread tests/test_protocol.py

Also enforce per-record rdlength boundary in _read_others

c6111f9

bdraco requested a review from Copilot May 20, 2026 15:28

Copilot started reviewing on behalf of bdraco May 20, 2026 15:29 View session

bdraco temporarily deployed to release May 20, 2026 15:31 — with GitHub Actions Inactive

Copilot AI reviewed May 20, 2026

View reviewed changes

bdraco requested a review from Copilot May 20, 2026 15:37

Copilot started reviewing on behalf of bdraco May 20, 2026 15:38 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

bdraco merged commit 5444495 into master May 20, 2026
44 checks passed

bdraco deleted the bound_read_string_record_length branch May 20, 2026 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bound record payload reads against rdlength overrun#1756

fix: bound record payload reads against rdlength overrun#1756
bdraco merged 2 commits into
masterfrom
bound_read_string_record_length

bdraco commented May 20, 2026

Uh oh!

codspeed-hq Bot commented May 20, 2026 •

edited

Loading

Uh oh!

codecov Bot commented May 20, 2026 •

edited

Loading

Uh oh!

bdraco commented May 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

bdraco commented May 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

bluetoothbot commented May 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bdraco commented May 20, 2026

Summary

Details

Test plan

Uh oh!

codspeed-hq Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

codecov Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bdraco commented May 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

bdraco commented May 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

bluetoothbot commented May 20, 2026

PR Review — fix: bound record payload reads against rdlength overrun

🟢 Suggestions

Checklist

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codspeed-hq Bot commented May 20, 2026 •

edited

Loading

codecov Bot commented May 20, 2026 •

edited

Loading