🐛 dns: make the wildcard check actually probe and stop breaking mail gateways#2833
🐛 dns: make the wildcard check actually probe and stop breaking mail gateways#2833tas50 wants to merge 3 commits into
Conversation
![mondoo-code-review[bot]](https://avatars.githubusercontent.com/in/2834902?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
tas50
force-pushed
the
tim/dns-remediation-review
branch
2 times, most recently
from
60c495e to
ba2a4aa
Compare
tas50
force-pushed
the
tim/dns-remediation-review
branch
from
1e1334b to
1467f5d
Compare
tas50
force-pushed
the
tim/dns-remediation-review
branch
from
1467f5d to
5d6697e
Compare
tas50
force-pushed
the
tim/dns-remediation-review
branch
from
5d6697e to
7d8cdde
Compare
tas50
force-pushed
the
tim/dns-remediation-review
branch
from
7d8cdde to
c5e7a98
Compare
…gateways - The wildcard check could never fail: the provider queries only the exact FQDN and wildcard answers are synthesized into the queried name, so a literal * never appears; the check now probes a random nonexistent subdomain, verified empirically to pass on mondoo.com and fail on wordpress.com's wildcard - The NS half of the IP-records check was vacuous for any domain with two or more nameservers because array != regex is only meaningful for single-element arrays; now rdata.none() per element, and no-mail domains no longer fail a check about IP-pointing records, with null MX per RFC 7505 documented - Remove any non-Microsoft and non-Google MX records would have broken intentional third-party mail gateways like Proofpoint in front of 365 or Workspace; removal is now scoped to the legacy hosts the checks flag, and the Google guidance adds the current single smtp.google.com record alongside the supported five-record layout - DNSSEC remediation adds the DS-at-registrar step without which a signed zone has zero spoofing protection, plus the unsign-safely sequencing that prevents taking the domain offline; apex CNAME and wildcard removals gain create-before-delete ordering; NS redundancy now updates both the zone RRset the check reads and the registrar delegation resolvers follow Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review feedback on the DNS security policy: - Add a comment explaining that domains with no MX records pass the MX assertion vacuously on purpose, since null-MX is a valid setup. - Correct the wildcard-probe comment to describe the label as a fixed sentinel rather than a randomly generated value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tas50
force-pushed
the
tim/dns-remediation-review
branch
from
c5e7a98 to
c7bbcd2
Compare
| // Domains with no MX records pass this MX assertion vacuously, which is | ||
| // intentional: not every domain receives mail, and a null-MX setup is a | ||
| // valid choice. When MX records exist, none may point to an IP address. |
There was a problem hiding this comment.
🟡 warning — Removing the dns.mx != empty assertion changes the policy semantics: domains with no MX records will now pass this check. The added comment says this is intentional, but this is a behavioral change that should be called out in the PR description. If the intent is to allow null-MX (RFC 7505) setups, consider explicitly checking for that (e.g., an MX record with target .) rather than silently passing domains with zero MX records, which could also be misconfigured domains that accidentally lost their MX records.
| dns.mx.all(domainName != regex.ipv4 && domainName != regex.ipv6) | ||
| dns.records.where(type == "NS") != empty | ||
| dns.records.where(type == "NS").all( rdata != regex.ipv4 && rdata != regex.ipv6 ) | ||
| dns.records.where(type == "NS").all( rdata.none(_ == regex.ipv4 || _ == regex.ipv6) ) |
There was a problem hiding this comment.
🔴 critical — The NS rdata field was previously compared as a scalar string (rdata != regex.ipv4), but is now treated as a list (rdata.none(...)). This is a semantic change in the MQL query. If rdata is actually a string (not a list), this will either error at runtime or silently pass everything. Please confirm that rdata is indeed a list type in this context. If it is a string, the original scalar comparison was correct and this change will break the check.
| // synthesizes an answer for it, so any A, AAAA, or CNAME result means | ||
| // the zone serves a wildcard. The label is a fixed sentinel chosen to | ||
| // be implausible as a real record rather than randomly generated. | ||
| dns(fqdn: "mondoo-wildcard-probe-f3a9." + domainName.fqdn).records.where(type.in(["A", "AAAA", "CNAME"])) == empty |
There was a problem hiding this comment.
🟡 warning — The hardcoded probe label mondoo-wildcard-probe-f3a9. is a clever approach, but if this sentinel subdomain is ever actually registered in a zone (unlikely but possible), this check would produce a false positive. More importantly, this relies on dns() being able to accept a constructed fqdn argument — verify that MQL's dns(fqdn: ...) function supports string concatenation inline like this. If not, this will fail at parse/runtime.
1 participant
Part of the series reviewing every remediation in
content/for correctness (#2775, #2780–#2832). This PR coversmondoo-dns-security.mql.yaml: all 8 checks were reviewed and 7 needed fixes. Policy version bumped. Verified against the network provider's dnsshake implementation, MQL's array-comparison semantics in llx, and empirically with cnquery against live domains.A check that could never fail (mql)
no-wildcard asserted
dns.records.none(name == /\*/), but the provider queries only the exact asset FQDN, and authoritative servers synthesize wildcard answers into the queried name — a literal*label never appears in any answer, so every domain passed, wildcards included. The check now probes a deliberately nonexistent subdomain and asserts it doesn't resolve. Verified empirically:mondoo.compasses (empty answer),wordpress.com— which serves wildcard subdomains — fails with the synthesized CNAME visible in the result.An assertion vacuous for every compliant domain (mql)
The NS half of no-ip-for-ns-mx-records compared the NS record's
rdataarray against a regex. Per MQL's array semantics,[]string != regexis unconditionally true for arrays with more than one element — and the provider collapses all NS targets into one record, while the sibling redundancy check requires two or more nameservers. So for every compliant domain the assertion was vacuous; an NS set of two IP addresses passed. Nowrdata.none(_ == ipv4 || _ == ipv6)per element. Thedns.mx != emptystatement also failed no-mail domains on a check about IP-pointing records; dropped, with RFC 7505 null MX documented for mail-less domains.Guidance that would break mail flow
Both vendor MX checks said to "remove any legacy or non-Microsoft/non-Google MX records", but organizations intentionally front 365/Workspace with third-party gateways like Proofpoint or Mimecast — non-vendor records by design that the checks don't flag. Removal is now scoped to the specific legacy hosts the checks inspect, with gateway records explicitly preserved. The Google remediation also presented the classic five-record layout as the only correct state; Google's current guidance for new setups is a single
smtp.google.comrecord, and both configurations are now shown (both pass the check).DNSSEC without the security
dnssec-enabled passes on DNSKEY presence alone, and its remediation stopped at "enable DNSSEC" — omitting the DS record at the registrar, without which a signed zone anchors no chain of trust and provides zero spoofing protection. The DS step is now explicit, along with the two offline-risk warnings: never publish a mismatched DS, and always remove the DS and wait out its TTL before unsigning or migrating providers.
Sequencing
The apex-CNAME and wildcard removals now create replacement records before deleting (the prior order left gaps in resolution), and NS redundancy now updates both the zone's NS RRset — what the check reads — and the registrar delegation — what resolvers actually follow.
Validation
cnspec policy lintpasses; both modified mql expressions verified empirically with cnquery against live domains🤖 Generated with Claude Code