Conversation
Signed-off-by: Zafer Balkan <zafer@zaferbalkan.com>
Member
|
Thanks for the PR. Will check it soon in detail. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
This PR adds wire-format validation for TSIG resource records during DNS message parsing, closing three gaps against RFC 8945 (the TSIG spec):
DnsResourceRecord.cs) - RFC 8945 §4.2 mandatesCLASS=ANYandTTL=0on the TSIG RR. A record read with any other CLASS or a non-zero TTL now throwsDnsClientExceptionbefore its RDATA is even parsed.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).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:try/catchinDnsDatagram.ReadFrom, which sets_parsingExceptionand produces aFormatErrorresponse.DeserializeDomainNameand compares the delta toDnsDatagram.GetSerializeDomainNameLength(_algorithmName)(the exact uncompressed wire length for any label count) - a mismatch can only mean a compression pointer was used.datagram._parsingExceptioninline in the per-record loop rather than throwing, so parsing of the rest of the message continues and the existingParsingExceptionproperty (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 compressedcomment inDnsTSIGRecordData.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.IsSigned/TsigError/TsigKeyName(which only inspect the last Additional record) to confirm no new exception path reaches unrelated code.TechnitiumSoftware/DnsServer(DnsServerApp,DnsServerCore,DnsServerCore.HttpApi,DnsServerCore.ApplicationCommon, plus two app plugins), against this branch's compiled library - 0 errors, 0 warnings. ConfirmedDnsServer.csalready gates onrequest.ParsingExceptionbefore touching TSIG verification, so no downstream code changes are required.Anything else