NULL pointer deref in `az_find_nsec_cover()` (latent bug)

[dimakuv]

The unit test testcode/unitauth.c can trigger a NULL pointer dereference at the beginning of the function az_find_nsec_cover(). To our knowledge, this bug can not be triggered in production (as it is hidden behind other, seemingly unrelated validations); the bug can only be hit by the unit test.

The below change exposes this bug. The below diff was applied on unbound commit 71bb60e (from 16. March 2025).

diff --git a/testcode/unitauth.c b/testcode/unitauth.c
index 23c57e0..086ebab 100644
--- a/testcode/unitauth.c
+++ b/testcode/unitauth.c
@@ -580,6 +580,9 @@ static struct q_ans example_com_queries[] = {
 "yy.example.com.       3600    IN      TXT     \"e\"\n"
 "yy.example.com.       3600    IN      TXT     \"f\"\n"
        },
+       { "example.com", "hello/xample.com. A", "",
+"ANSWER DOES NOT MATTER -- THIS QUERY MUST RESULT IN A CRASH\n"
+       },

        {NULL, NULL, NULL, NULL}
 };

Apply this diff and run the unittest:

$ make test
./libtool --tag=CC --mode=compile gcc -I.  -DSRCDIR=. -g -O2 -flto -pthread  -o unitauth.lo -c testcode/unitauth.c
...
./unittest
Start of unbound 1.22.1 unit test.test authzone functions
make: *** [Makefile:349: test] Segmentation fault (core dumped)

This was detected on Amazon Linux 2023 system, on unbound commit 71bb60e . This issue was introduced in 2017 and should be triggerable (in the unittest) on all systems and all versions from year 2017+.

---

The root cause is as follows. When generating an NXDOMAIN (Non-Existent Domain) response message, the unbound server wants to add the NSEC (Next SECure) DNS record type used in DNSSec to provide authenticated denial of existence for DNS records. However, function az_find_nsec_cover() that finds the corresponding NSEC record has this code snippet (I removed the pointer-to-pointer indirection in node for simplicity):

static struct auth_rrset*
az_find_nsec_cover(struct auth_zone* z, struct auth_data* node)
{
    uint8_t nm = (node)->name;
    size_t nmlen = (node)->namelen;
    ...

The bug in the unittest is triggered because the node pointer is NULL. The node == NULL means that the user sent a DNS query that has an associated authoritative zone but no DNS records with the associated info about the queried domain in this zone.

Function az_find_nsec_cover() is here: https://github.com/NLnetLabs/unbound/blob/30b9cb5f813003d0a2b1c2e678652396615b1b7d/services/authzone.c#L2779?plain=1. The first two lines directly access the *node object, which doesn't exist and *node == NULL. This was introduced in May 2017 in the commit 632c1e8. Interestingly, before this commit, the NULL node object was correctly handled. There is also a comment still that mentions the possibility of node == NULL.

Currently, there seems to be no way to trigger this NULL pointer dereference in practice. This is because node == NULL means that the authoritative zone has no associated DNS records, which in turn implies that there is no SOA (Start of Authority) record (like example.com. 3600 IN SOA ns.example.com. noc.example.com. ...). Otherwise, if there is a SOA record, then at least this SOA record would match to the client DNS query and the node will be set to this SOA record.

Currently, if there is no SOA record, then the unbound logic returns early, without getting to the buggy az_find_nsec_cover(). For example, the caller az_generate_nxdomain_answer() first calls az_add_negative_soa(), and if that function doesn't find a SOA record, then the caller returns immediately. (The function az_add_negative_soa() calls az_find_name(<zone>) to find the SOA record, and if this fails, it returns with an error.) See:

• https://github.com/NLnetLabs/unbound/blob/376f2ade2aedc308a9e989f0f1a548abd419e4b3/services/authzone.c#L3367?plain=1
• https://github.com/NLnetLabs/unbound/blob/eccf52e39d73c49267ebbcb228eac9715568e7f8/services/authzone.c#L2586?plain=1

Similar situation happens with two other call chains: az_nsec_wildcard_denial() and az_generate_wildcard_answer(). The buggy logic is not reached because these functions return early on node == NULL.

In general, the logic is like this:

• To trigger the bug, node == NULL must hold. In other words, there must be no DNS records associated with the incoming DNS query. This condition is possible to trigger in the unittest.
• But all code paths that lead to the bug enforce that node != NULL. In other words, all paths verify that there is at least one DNS record associated with the incoming DNS query. However, this verification seems accidental, and if in the future this verification will be modified/removed, then this (latent) bug may be hit.

---

If I understand correctly, a fix can be as simple as:

if (*node == NULL)
    return NULL;

[gthess]

Thanks for the detailed report!
Since this is a static function and the usage is internal, I propose to update the documentation/comments there and add a log_assert(*node) to convey current intent.
If things change internally while developing, it will raise alarms and we can rethink the function or the code leading to the function.
Does this sound right to you?

[dimakuv]

@gthess Yes, the assert sounds good to me (plus modifying the comment there).