Conversation
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.
Summary
This hardens
ntcdns::ResourceRecord::decodeagainst malformed RDATA from a malicious or on-path DNS responder. It addresses a report that crafted TXT RDATA could underflow the TXT sub-buffer length and desynchronize the decoder from the RDATA boundary.The underflow itself was fixed in 6a080a8 ("Fix inverted condition when handling number of bytes read from and DNS TXT record"). Before that commit, a character-string longer than the remaining RDATA wrapped
numBytesRemaining, and the loop kept parsing bytes from the following records as TXT data.Two gaps remained after that fix, and this PR closes them:
checkCoherentRdataLength, once the decoder had already read past the RDATA.Changes
ntcdns_protocol.{h,cpp}rdataLength,ResourceRecord::decodechecks it against the bytes remaining in the message before any type-specific parsing, for every record type.MemoryDecoder::decodeCharacterString(bsl::string* value, bsl::size_t limit). It reads the length byte without consuming it. If1 + lengthexceedslimit(capped at the remaining buffer), it fails and leaves the decoder where it was. The existing one-argument overload now calls it with the remaining buffer as the limit, so its behaviour is unchanged.numBytesRemainingas the limit, so a string can no longer cross the RDATA boundary. The existingnumBytesRead <= numBytesRemainingcheck stays as a second line of defence.rdataLengthand the OS string to what's left of the RDATA.rdataLengthshorter than the address plus protocol (5 bytes) are rejected before anything is decoded.ntcdns_protocol.t.cppThis adds test coverage for TXT and HINFO, which had none, plus malformed-input cases:
verifyCharacterStringLimitverifyTxtverifyTxtMalformedrdataLengthrunning past the end of the message.rdataLengthending partway through a stringverifyHinfoverifyHinfoMalformedrdataLengthending inside the CPU string, right after it, and inside the OS stringverifyWksMalformedrdataLengthshorter than address plus protocolTwo private helpers support these tests:
buildResponse, which builds a response whose declared RDATA length and actual payload can differ, andverifyRoundTrip.