fix: usr: Fix named crash when processing SIG records in dynamic updates

Previously, :iscman:`named` could abort if a client sent a dynamic update containing a SIG record (the legacy signature type) to a zone configured with an update-policy. The function `dns_db_findrdataset` had an incorrect requirements prerequisite that prevented SIG records being looked up, which was triggered as part of processing an UPDATE request and could be triggered remotely by any client permitted to send updates. This has been fixed by ensuring that SIG records are handled consistently with RRSIG records during update processing.

Closes #5818

Merge branch '5818-fix-update-of-sig' into 'main'

See merge request isc-projects/bind9!11864

bin/tests/system/nsupdate/ans11/ans.py

@@ -0,0 +1,117 @@
+# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+#
+# SPDX-License-Identifier: MPL-2.0
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0.  If a copy of the MPL was not distributed with this
+# file, you can obtain one at https://mozilla.org/MPL/2.0/.
+#
+# See the COPYRIGHT file distributed with this work for additional
+# information regarding copyright ownership.
+
+"""
+GL#5818 Finding 1 regression support — AsyncDnsServer primary.
+
+Serves a minimal zone "sigaxfr.nil." whose AXFR carries two SIG records
+at the same owner with different covered types (A and MX) and different
+TTLs (600 and 1200).  A buggy secondary running dns_diff_load() with
+rdata_covers() that only recognises RRSIG will file both rdatas under
+typepair (SIG, 0) with the first tuple's TTL; a fixed secondary keeps
+them under (SIG, A) and (SIG, MX) with their distinct TTLs.
+"""
+
+from collections.abc import AsyncGenerator
+
+import dns.name
+import dns.rcode
+import dns.rdata
+import dns.rdataclass
+import dns.rdatatype
+import dns.rrset
+
+from isctest.asyncserver import (
+    AsyncDnsServer,
+    DnsResponseSend,
+    DomainHandler,
+    QueryContext,
+    ResponseAction,
+)
+
+ZONE = dns.name.from_text("sigaxfr.nil.")
+NS_NAME = dns.name.from_text("ns.sigaxfr.nil.")
+HOST = dns.name.from_text("host.sigaxfr.nil.")
+
+SOA_TEXT = "ns.sigaxfr.nil. hostmaster.sigaxfr.nil. 1 3600 1200 604800 3600"
+
+
+def _make_sig_rdata(covered_text):
+    """Produce a legacy SIG (24) rdata via RRSIG (46) round-trip."""
+    rrsig = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.RRSIG, covered_text)
+    wire = rrsig.to_digestable()
+    return dns.rdata.from_wire(dns.rdataclass.IN, dns.rdatatype.SIG, wire, 0, len(wire))
+
+
+class SigAxfrServer(DomainHandler):
+    """Serve SOA and AXFR for sigaxfr.nil.; other qtypes get NOERROR/NODATA."""
+
+    domains = ["sigaxfr.nil."]
+
+    async def get_responses(
+        self, qctx: QueryContext
+    ) -> AsyncGenerator[ResponseAction, None]:
+        soa_rrset = dns.rrset.from_text(
+            ZONE, 3600, dns.rdataclass.IN, dns.rdatatype.SOA, SOA_TEXT
+        )
+
+        if qctx.qtype == dns.rdatatype.SOA:
+            resp = qctx.response
+            resp.answer.append(soa_rrset)
+            yield DnsResponseSend(resp)
+            return
+
+        if qctx.qtype != dns.rdatatype.AXFR:
+            # Other types: empty NOERROR response.
+            yield DnsResponseSend(qctx.response)
+            return
+
+        # AXFR: opening SOA, NS, NS's A, two SIG RRs at the same owner
+        # with distinct covered types and TTLs, closing SOA.
+        resp = qctx.response
+        resp.answer.append(soa_rrset)
+
+        ns_rrset = dns.rrset.from_text(
+            ZONE, 3600, dns.rdataclass.IN, dns.rdatatype.NS, str(NS_NAME)
+        )
+        resp.answer.append(ns_rrset)
+
+        a_rrset = dns.rrset.from_text(
+            NS_NAME, 3600, dns.rdataclass.IN, dns.rdatatype.A, "10.53.0.11"
+        )
+        resp.answer.append(a_rrset)
+
+        sig_a = _make_sig_rdata("A 6 2 600 20260331170000 20260318160000 21831 . 0000")
+        sig_a_rrset = dns.rrset.RRset(HOST, dns.rdataclass.IN, dns.rdatatype.SIG)
+        sig_a_rrset.add(sig_a, ttl=600)
+        resp.answer.append(sig_a_rrset)
+
+        sig_mx = _make_sig_rdata(
+            "MX 6 2 1200 20260331170000 20260318160000 21831 . 0000"
+        )
+        sig_mx_rrset = dns.rrset.RRset(HOST, dns.rdataclass.IN, dns.rdatatype.SIG)
+        sig_mx_rrset.add(sig_mx, ttl=1200)
+        resp.answer.append(sig_mx_rrset)
+
+        # Closing SOA terminates the AXFR.
+        resp.answer.append(soa_rrset)
+
+        yield DnsResponseSend(resp)
+
+
+def main() -> None:
+    server = AsyncDnsServer(default_aa=True, default_rcode=dns.rcode.NOERROR)
+    server.install_response_handler(SigAxfrServer())
+    server.run()
+
+
+if __name__ == "__main__":
+    main()

bin/tests/system/nsupdate/ns6/named.conf.j2

@@ -36,3 +36,10 @@ zone "2.0.0.2.ip6.arpa" {
 	file "2.0.0.2.ip6.addr.db";
 	update-policy {	grant * 6to4-self . NS(10) DS(4); };
 };
+
+zone "sigaxfr.nil" {
+	type secondary;
+	primaries { 10.53.0.11; };
+	file "sigaxfr.bk";
+	request-ixfr no; # ans11 serves AXFR only
+};

bin/tests/system/nsupdate/tests_sh_nsupdate.py

@@ -60,6 +60,7 @@ pytestmark = pytest.mark.extra_artifacts(
         "ns5/local.db",
         "ns6/2.0.0.2.ip6.addr.db",
         "ns6/in-addr.db",
+        "ns6/sigaxfr.bk",
         "ns7/_default.tsigkeys",
         "ns7/example.com.db",
         "ns7/in-addr.db",

bin/tests/system/nsupdate/tests_update_sig.py

@@ -0,0 +1,342 @@
+# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+#
+# SPDX-License-Identifier: MPL-2.0
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0.  If a copy of the MPL was not distributed with this
+# file, you can obtain one at https://mozilla.org/MPL/2.0/.
+#
+# See the COPYRIGHT file distributed with this work for additional
+# information regarding copyright ownership.
+
+"""
+Regression tests for GL#5818: legacy DNSSEC types on the dynamic-update path.
+
+SIG (24) and NXT (30) are obsolete DNSSEC record types, superseded by RRSIG
+and NSEC in RFC 3755.  Allowing a client to inject them via dynamic update
+exposed two bugs in sequence:
+
+  * dns__db_findrdataset() asserted `covers == 0 || type == RRSIG`, which
+    crashed named when a SIG update reached the prescan foreach_rr() call.
+  * diff.c rdata_covers() dropped the covered type for SIG rdatas, so the
+    zone DB stored every SIG rdataset under typepair (SIG, 0) instead of
+    (SIG, covered_type); a second SIG add with a different covers and
+    different TTL then tripped DNS_DBADD_EXACTTTL in qpzone and came back
+    as SERVFAIL.
+
+The adopted defence is to outright refuse SIG and NXT updates at the front
+door (ns/update.c), keeping KEY updates permitted for SIG(0) transaction
+signatures.  These tests verify the refusal.  The reachability of the
+diff.c:rdata_covers() bug via inbound zone transfer is covered separately
+by the AXFR-based regression test in this file.
+"""
+
+from re import compile as Re
+
+import re
+import time
+
+import dns.rcode
+import dns.rdata
+import dns.rdataclass
+import dns.rdataset
+import dns.rdatatype
+import dns.tsigkeyring
+import dns.update
+import pytest
+
+import isctest
+
+pytestmark = pytest.mark.extra_artifacts(
+    [
+        "ans*/ans.run",
+        "ns*/*.bk",
+        "ns*/*.conf",
+        "ns*/*.db",
+        "ns*/*.db.jnl",
+        "ns*/*.db.signed",
+        "ns*/*.jnl",
+        "ns*/*.key",
+        "ns*/K*.key",
+        "ns*/K*.private",
+        "ns*/K*.state",
+        "ns*/dsset-*.",
+        "ns6/sigaxfr.bk",
+        "verylarge",
+    ]
+)
+
+
+def _make_sig_rdata(text):
+    """Create a SIG rdata from text.
+
+    dnspython has no native text parser for the legacy SIG type (24),
+    but the wire format is identical to RRSIG (46).  Parse as RRSIG,
+    then re-wrap as SIG via the wire representation.
+    """
+    rrsig = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.RRSIG, text)
+    wire = rrsig.to_digestable()
+    return dns.rdata.from_wire(dns.rdataclass.IN, dns.rdatatype.SIG, wire, 0, len(wire))
+
+
+def _make_nxt_rdata():
+    """Create a minimal NXT rdata.
+
+    NXT wire format (RFC 2535) is: next-name + type-bitmap.  The exact
+    content does not matter for the refusal test; we just need a
+    syntactically valid NXT rdata.
+    """
+    # next-name = root (\x00), type bitmap covering type A only.
+    wire = b"\x00\x00\x00\x00\x40"
+    return dns.rdata.from_wire(dns.rdataclass.IN, dns.rdatatype.NXT, wire, 0, len(wire))
+
+
+def test_tcp_self_sig_record(ns6):
+    """SIG (type 24) updates must be refused at the front door.
+
+    Prior to the fix in dns__db_findrdataset(), a SIG update here
+    crashed named.  Prior to the fix in diff.c rdata_covers(), the
+    record was silently misfiled under typepair (SIG, 0).  The
+    adopted policy outright refuses SIG (obsolete; use RRSIG) so the
+    buggy dynamic-update paths are no longer reachable.  A PTR add
+    first ensures the node exists, which is the original
+    crash-reproducing precondition.
+    """
+    ptr_update = dns.update.UpdateMessage("in-addr.arpa.")
+    ptr_update.add("1.0.53.10.in-addr.arpa.", 600, "PTR", "localhost.")
+    response = isctest.query.tcp(
+        ptr_update, ns6.ip, port=ns6.ports.dns, source="10.53.0.1"
+    )
+    assert response.rcode() == dns.rcode.NOERROR
+
+    sig = _make_sig_rdata("A 6 0 86400 20260331170000 20260318160000 21831 . 0000")
+    rds = dns.rdataset.Rdataset(dns.rdataclass.IN, dns.rdatatype.SIG)
+    rds.update_ttl(600)
+    rds.add(sig)
+    sig_update = dns.update.UpdateMessage("in-addr.arpa.")
+    sig_update.add("1.0.0.127.in-addr.arpa.", rds)
+
+    with ns6.watch_log_from_here() as watcher:
+        response = isctest.query.tcp(
+            sig_update, ns6.ip, port=ns6.ports.dns, source="10.53.0.1"
+        )
+        assert response.rcode() == dns.rcode.REFUSED
+
+        watcher.wait_for_line(
+            "updating zone 'in-addr.arpa/IN': update failed: SIG updates are not allowed (REFUSED)"
+        )
+
+    # Confirm nothing of type SIG was stored.
+    msg = isctest.query.create("1.0.53.10.in-addr.arpa.", "SIG")
+    res = isctest.query.tcp(msg, ns6.ip, port=ns6.ports.dns)
+    stored = any(rrset.rdtype == dns.rdatatype.SIG for rrset in res.answer)
+    assert not stored, "SIG record was stored despite REFUSED response"
+
+
+def test_tcp_self_nxt_record(ns6):
+    """NXT (type 30) updates must be refused at the front door.
+
+    NXT is the legacy DNSSEC denial-of-existence type, obsolete since
+    RFC 3755 replaced it with NSEC.  Accepting it via dynamic update
+    would let an authorised updater inject records that the signing
+    and cut-point logic has no provision for.
+    """
+    # A second owner under a source that also matches tcp-self.
+    source = "10.53.0.2"
+    owner = "2.0.53.10.in-addr.arpa."
+
+    ptr_update = dns.update.UpdateMessage("in-addr.arpa.")
+    ptr_update.add(owner, 600, "PTR", "localhost.")
+    response = isctest.query.tcp(ptr_update, ns6.ip, port=ns6.ports.dns, source=source)
+    assert response.rcode() == dns.rcode.NOERROR
+
+    nxt = _make_nxt_rdata()
+    rds = dns.rdataset.Rdataset(dns.rdataclass.IN, dns.rdatatype.NXT)
+    rds.update_ttl(600)
+    rds.add(nxt)
+    nxt_update = dns.update.UpdateMessage("in-addr.arpa.")
+    nxt_update.add(owner, rds)
+
+    with ns6.watch_log_from_here() as watcher:
+        response = isctest.query.tcp(
+            nxt_update, ns6.ip, port=ns6.ports.dns, source="10.53.0.1"
+        )
+        assert response.rcode() == dns.rcode.REFUSED
+
+        watcher.wait_for_line(
+            "updating zone 'in-addr.arpa/IN': update failed: NXT updates are not allowed (REFUSED)"
+        )
+
+
+def test_sig_covers_preserved_via_axfr(ns6):
+    """Regression test for GL#5818 Finding 1, reached via AXFR.
+
+    ans11 serves an AXFR for sigaxfr.nil. containing two SIG rdatas at
+    the same owner with different covered types (A, MX) and different
+    TTLs (600, 1200).  ns6 pulls the zone via dns_diff_load(), which
+    calls diff.c rdata_covers(); before the fix that helper returned 0
+    for SIG, so both tuples were grouped and filed under typepair
+    (SIG, 0) with the first TTL (600) — the MX-covering record's TTL
+    (1200) was silently dropped.  With the fix the records land in
+    distinct typepairs and both TTLs survive.
+
+    rndc dumpdb is used to inspect the secondary's stored state
+    directly; the wire-level response can merge same-(owner,type,class)
+    RRs and mask the difference.
+    """
+    zone = "sigaxfr.nil"
+    owner = f"host.{zone}."
+    dump_path = ns6.directory / "named_dump.db"
+
+    # ns6 may have tried to SOA-poll ans11 before it was listening; force
+    # a fresh refresh attempt and wait for the transfer to complete.
+    with ns6.watch_log_from_here() as watcher:
+        ns6.rndc(f"refresh {zone}")
+        watcher.wait_for_line(f"zone {zone}/IN: transferred serial 1")
+
+    # Remove any stale dump and ask named for a fresh one.
+    if dump_path.exists():
+        dump_path.unlink()
+    ns6.rndc("dumpdb -zones")
+
+    # rndc dumpdb is asynchronous; wait for the file and for its
+    # trailing "Dump complete" marker.
+    deadline_marker = "; Dump complete"
+    for _ in range(50):
+        if dump_path.exists():
+            text = dump_path.read_text()
+            if deadline_marker in text:
+                break
+        time.sleep(0.1)
+    else:
+        raise AssertionError(f"{dump_path} never contained {deadline_marker!r}")
+
+    # Collect every SIG line for the owner from the dump.  Format is:
+    #   <owner>. <ttl> IN SIG <covered> <alg> <labels> ...
+    sig_lines = []
+    for line in text.splitlines():
+        fields = line.split()
+        if len(fields) < 6:
+            continue
+        if not fields[0].lower().startswith("host.sigaxfr.nil"):
+            continue
+        if fields[2] != "IN" or fields[3] != "SIG":
+            continue
+        sig_lines.append(fields)
+
+    assert (
+        len(sig_lines) == 2
+    ), f"expected 2 SIG records at {owner}, got {len(sig_lines)}: {sig_lines}"
+
+    ttl_by_covers = {fields[4]: int(fields[1]) for fields in sig_lines}
+    assert ttl_by_covers == {"A": 600, "MX": 1200}, (
+        f"SIG records lost their covers/TTL binding: {ttl_by_covers}.  With "
+        "the Finding 1 bug both records are filed under typepair (SIG, 0) "
+        "and share the first-seen TTL (600)."
+    )
+
+
+def parse_named_conf_keys(conf_text):
+    """
+    Extract TSIG keys from a BIND named.conf-style string.
+    Returns a dict suitable for dns.tsigkeyring.from_text().
+    """
+    key_re = Re(
+        r'key\s+"(?P<name>[^"]+)"\s*\{(?P<body>.*?)\};', re.DOTALL | re.IGNORECASE
+    )
+    field_re = Re(r"(?P<field>algorithm|secret)\s+(?P<value>[^;]+);", re.IGNORECASE)
+
+    keys = {}
+
+    for match in key_re.finditer(conf_text):
+        name = match.group("name")
+        body = match.group("body")
+
+        fields = {}
+        for fmatch in field_re.finditer(body):
+            field = fmatch.group("field").lower()
+            value = fmatch.group("value").strip().strip('"')
+            fields[field] = value
+
+        if "secret" not in fields:
+            continue  # skip incomplete entries
+
+        algorithm = fields.get("algorithm", "hmac-sha256")
+
+        # Ensure FQDN key name
+        key_name = name if name.endswith(".") else name + "."
+
+        keys[key_name] = (algorithm.lower(), fields["secret"])
+
+    return keys
+
+
+def keyring_from_file(keyfile):
+    with open(keyfile, "r", encoding="utf-8") as file:
+        data = file.read()
+
+    keys = parse_named_conf_keys(data)
+    return dns.tsigkeyring.from_text(keys)
+
+
+def test_prereq_sig_record(ns1):
+    keyring = keyring_from_file("ns1/ddns.key")
+
+    # First, create the node.
+    node_update = dns.update.UpdateMessage("example.nil.", keyring=keyring)
+    node_update.add("sig.example.nil.", 600, "A", "10.53.0.11")
+    response = isctest.query.tcp(
+        node_update, ns1.ip, port=ns1.ports.dns, source="10.53.0.1"
+    )
+    assert response.rcode() == dns.rcode.NOERROR
+
+    # Now require a SIG record at the same node — this triggers the
+    # dns_db_findrdataset() call with type=SIG and covers=A.
+    sig = _make_sig_rdata("A 6 0 86400 20260331170000 20260318160000 21831 . 0000")
+    rds = dns.rdataset.Rdataset(dns.rdataclass.IN, dns.rdatatype.SIG)
+    rds.update_ttl(0)
+    rds.add(sig)
+
+    # First attempt with no matching credentials.
+    sig_update = dns.update.UpdateMessage("example.nil.")  # no key
+    sig_update.present("sig.example.nil.", rds)
+    sig_update.add("sig.example.nil.", 600, "TXT", "I require SIG")
+
+    with ns1.watch_log_from_here() as watcher:
+        response = isctest.query.tcp(
+            sig_update, ns1.ip, port=ns1.ports.dns, source="10.53.0.1"
+        )
+        assert response.rcode() == dns.rcode.REFUSED
+
+        watcher.wait_for_sequence(
+            [
+                "update-policy: using: signer= name=sig.example.nil addr=10.53.0.1 tcp=1 type=TXT target=",
+                "update-policy: trying: grant zonesub-key.example.nil zonesub TXT",
+                "update-policy: trying: grant ddns-key.example.nil subdomain example.nil ANY",
+                "update-policy: no match found",
+                "updating zone 'example.nil/IN': update failed: rejected by secure update (REFUSED)",
+            ]
+        )
+
+    # Second attempt with the right key.
+    sig_update = dns.update.UpdateMessage("example.nil.", keyring=keyring)
+    sig_update.present("sig.example.nil.", rds)
+    sig_update.add("sig.example.nil.", 600, "TXT", "I require SIG")
+
+    with ns1.watch_log_from_here() as watcher:
+        response = isctest.query.tcp(
+            sig_update, ns1.ip, port=ns1.ports.dns, source="10.53.0.1"
+        )
+        assert response.rcode() == dns.rcode.NXRRSET
+
+        watcher.wait_for_sequence(
+            [
+                "update-policy: using: signer=ddns-key.example.nil name=sig.example.nil addr=10.53.0.1 tcp=1 type=TXT target=",
+                "update-policy: trying: grant zonesub-key.example.nil zonesub TXT",
+                "update-policy: trying: grant ddns-key.example.nil subdomain example.nil ANY",
+                "update-policy: matched: grant ddns-key.example.nil subdomain example.nil ANY",
+                "updating zone 'example.nil/IN': update section prescan OK",
+                "updating zone 'example.nil/IN': update unsuccessful: sig.example.nil/SIG: 'RRset exists (value dependent)' prerequisite not satisfied (NXRRSET)",
+                "updating zone 'example.nil/IN': rolling back",
+            ]
+        )

lib/dns/db.c

@@ -600,7 +600,7 @@ dns__db_findrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version,
 	REQUIRE(node != NULL);
 	REQUIRE(DNS_RDATASET_VALID(rdataset));
 	REQUIRE(!dns_rdataset_isassociated(rdataset));
-	REQUIRE(covers == dns_rdatatype_none || type == dns_rdatatype_rrsig);
+	REQUIRE(covers == dns_rdatatype_none || dns_rdatatype_issig(type));
 	REQUIRE(type != dns_rdatatype_any);
 	REQUIRE(sigrdataset == NULL ||
 		(DNS_RDATASET_VALID(sigrdataset) &&

lib/dns/diff.c

@@ -38,7 +38,7 @@
 
 static dns_rdatatype_t
 rdata_covers(dns_rdata_t *rdata) {
-	return rdata->type == dns_rdatatype_rrsig ? dns_rdata_covers(rdata) : 0;
+	return dns_rdatatype_issig(rdata->type) ? dns_rdata_covers(rdata) : 0;
 }
 
 void

lib/ns/update.c

@@ -1682,6 +1682,12 @@ send_update(ns_client_t *client, dns_zone_t *zone) {
 		} else if (rdata.type == dns_rdatatype_nsec) {
 			FAILC(DNS_R_REFUSED, "explicit NSEC updates are not "
 					     "allowed in secure zones");
+		} else if (rdata.type == dns_rdatatype_sig) {
+			FAILC(DNS_R_REFUSED, "SIG updates are not "
+					     "allowed");
+		} else if (rdata.type == dns_rdatatype_nxt) {
+			FAILC(DNS_R_REFUSED, "NXT updates are not "
+					     "allowed");
 		} else if (rdata.type == dns_rdatatype_rrsig &&
 			   !dns_name_equal(name, zonename))
 		{