Skip to content

Reject malformed TSIG records per RFC 8945 - #68

Open
zbalkan wants to merge 1 commit into
TechnitiumSoftware:masterfrom
zbalkan:fix/rfc8945-tsig-wire-format-validation
Open

zbalkan wants to merge 1 commit into
TechnitiumSoftware:masterfrom
zbalkan:fix/rfc8945-tsig-wire-format-validation

Conversation

@zbalkan

@zbalkan zbalkan commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

What

This PR adds wire-format validation for TSIG resource records during DNS message parsing, closing three gaps against RFC 8945 (the TSIG spec):

  • CLASS/TTL enforcement (DnsResourceRecord.cs) - RFC 8945 §4.2 mandates CLASS=ANY and TTL=0 on the TSIG RR. A record read with any other CLASS or a non-zero TTL now throws DnsClientException before its RDATA is even parsed.
  • Uncompressed algorithm name (DnsTSIGRecordData.cs) - §4.2 also requires the Algorithm Name field to be an uncompressed domain name. Parsing now compares consumed stream bytes against the algorithm name's true uncompressed wire length and rejects a mismatch (i.e., a compression pointer was used).
  • TSIG placement (DnsDatagram.cs) - §5.2 permits TSIG only as the last record of the Additional section. Records of type TSIG found in the Answer or Authority sections, or anywhere but the last slot of Additional, now set _parsingException (the Additional-section case already existed for the "not-last" scenario; this extends the same check to Answer/Authority).

Why

None of these were previously validated, which means a message could carry a TSIG record in the wrong place, with the wrong CLASS/TTL, or with a compressed algorithm name, and still parse "successfully." Since TSIG is the mechanism callers rely on for authenticating DNS requests/updates/zone transfers, a parser that's lenient about its wire format is a plausible spoofing/downgrade surface - a forged or malformed TSIG could reach verification logic in a shape the spec never intended, rather than being rejected outright at the parse boundary.

How

Each check is inserted at the earliest point the malformed data is observable, consistent with the existing parsing pattern in DnsDatagram.ReadFrom:

  • The CLASS/TTL check throws immediately, since it can be evaluated before RDATA is touched - bubbles up through the existing try/catch in DnsDatagram.ReadFrom, which sets _parsingException and produces a FormatError response.
  • The algorithm-name check computes the RDATA-relative stream offset before/after DeserializeDomainName and compares the delta to DnsDatagram.GetSerializeDomainNameLength(_algorithmName) (the exact uncompressed wire length for any label count) - a mismatch can only mean a compression pointer was used.
  • The Answer/Authority placement check follows the non-throwing idiom already used for the Additional-section case: it sets datagram._parsingException inline in the per-record loop rather than throwing, so parsing of the rest of the message continues and the existing ParsingException property (already checked by consumers, e.g. DnsClient.cs) surfaces the rejection.

No wire-format writer code changes - outgoing TSIG records already used CLASS=ANY, TTL=0, and an uncompressed algorithm name (see the pre-existing //MUST NOT be compressed comment in DnsTSIGRecordData.WriteRecordData), so this is purely a parser tightening with no effect on messages this library produces itself.

Testing

  • dotnet build TechnitiumLibrary.Net/TechnitiumLibrary.Net.csproj -c Release - 0 warnings, 0 errors.
  • Manually traced all three new checks against RFC 8945 §4.2/§5.2 text and against IsSigned/TsigError/TsigKeyName (which only inspect the last Additional record) to confirm no new exception path reaches unrelated code.
  • Built the downstream consumer, TechnitiumSoftware/DnsServer (DnsServerApp, DnsServerCore, DnsServerCore.HttpApi, DnsServerCore.ApplicationCommon, plus two app plugins), against this branch's compiled library - 0 errors, 0 warnings. Confirmed DnsServer.cs already gates on request.ParsingException before touching TSIG verification, so no downstream code changes are required.
  • No unit test project exists in this repository yet, so these checks were not covered by an automated regression test as part of this PR.

Anything else

  • This repo currently has no test harness for wire-format parsing edge cases. A natural follow-up would be adding unit tests for the three new rejection paths (bad CLASS/TTL, compressed algorithm name, misplaced TSIG) once a test project exists.
  • Not verified with live TSIG traffic (zone transfer / dynamic update) against a running server - only static analysis and compile-time verification against the downstream consumer.

Signed-off-by: Zafer Balkan <zafer@zaferbalkan.com>
@ShreyasZare

Copy link
Copy Markdown
Member

Thanks for the PR. Will check it soon in detail.

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