Repository navigation
Dnssec detect signature stripping - #161
Merged
Flowdalic merged 7 commits intoOct 20, 2026
Merged
Conversation
The flag was set if, and only if, a DNSKEY was added to selfSigningKeys. Hence it is equivalent to the set not being empty. Use the set instead, so that there is no second piece of state that needs to be kept in sync. Furthermore, only DNSKEYs in selfSigningKeys are now verified as SEP, also if the set is empty. Previously, if the set was empty, all DNSKEYs were verified as SEP, but the result was ignored. Not doing so avoids the DS lookups for those DNSKEYs, and exceptions thrown while verifying a SEP whose result does not matter. The log message "SEP key is not self-signed.", which was logged for every DNSKEY if the set was empty, is replaced by a message that is logged for every DNSKEY that is not considered as SEP, and that names the DNSKEY and the reason.
A DNSKEY RRset is authenticated only through a secure entry point (SEP), that is, a key of the RRset that validly signs the RRset and that is a trust anchor or is referred to by a DS RR in the parent zone (RFC 4035 § 5.2). It is never authenticated by a signature from another zone. However, if a response's DNSKEY RRset was only signed by a key of another zone (a "cross-signature"), or was otherwise not validly self-signed, then verifyAnswer() removed the DNSKEY records from the set of records to be verified without finding a SEP, and reported no unverified reason. As a result, an attacker able to inject DNS responses could take a zone's genuine DNSKEY RRset, add a key of their own, sign the RRset with that key while claiming an unrelated signer name, and have the RRset, and every record signed by the added key, considered authentic. This works even if the DNSKEY RRset contains the key that the parent's DS RR refers to, as that key is not required to sign the RRset. verifyAnswer() now determines whether the answer carries a DNSKEY RRset owned by the queried name. If so, a key of the RRset that validly signs it has to verify as a SEP, otherwise the answer is not authentic. This closes the chain of trust to a DNSKEY RRset that the previous, separate checks for "is signed" and "contains a SEP" left open.
When a DNSKEY RRset of the queried name is not authentic because it is only signed by a key of another zone (a "cross-signature"), and not by one of its own keys, verifyAnswer() reported the generic NoSecureEntryPointReason. That reason suggests a missing DS RR or trust anchor, whereas the actual problem is that the DNSKEY RRset is not self-signed and hence can not be anchored at all, which is a strong indication of an attack or a misconfiguration. verifySignatures() already detects such cross-signatures. It now records the cross-signing zones of the queried name's DNSKEY RRset, so that verifyAnswer() can report the new CrossSignedDnskeyRrsetReason, which names those zones. NoSecureEntryPointReason remains as a fallback.
DnssecClient had a private isParentOrSelf(String, String) helper that split the ACE representations of two names on "." and compared the labels. This is exactly what DnsName.isChildOf() does, which considers a name a child of, or equal to, another name, and which DnssecClient already uses elsewhere. Use it and drop the custom helper.
When a response carries no signatures, DnssecClient reported the generic NoSignaturesReason, regardless of whether the queried name is within a signed zone (where signatures were expected and their absence indicates stripping, a downgrade attack, or a misconfigured name server) or within an unsigned, insecure zone (where their absence is normal). This addresses the long-standing TODO in verifySignatures(). DnssecClient gained a setDetectSignatureStripping() setting, disabled by default. When enabled, a response without signatures is probed by querying the DNSKEY of the queried name: the name is within a signed zone if, and only if, that response is authentic, either as an authentic DNSKEY RRset at a signed zone's apex, or as an authentic denial of existence below a signed apex. In that case the new, more specific SignaturesExpectedButMissingReason is reported instead of NoSignaturesReason. The setting is off by default because the probe performs an additional DNSKEY query. It only refines the reported reason; a response without signatures is not authentic data regardless. The detection is best-effort: it is accurate for zone apexes, and an on-path attacker who also strips the probe response can evade it. A complete defense requires authenticated denial of existence of the delegating DS RR, which MiniDNS does not yet implement.
Coverage Report for CI Build 39398608611Coverage increased (+0.2%) to 63.887%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions40 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
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.
No description provided.