dnssec: do not read the window length byte past the end of an NSEC bitmap

[router0mail]

What this fixes

ldns_nsec_bitmap_covers_type(), ldns_nsec_bitmap_set_type() and
ldns_nsec_bitmap_clear_type() in dnssec.c walk an NSEC/NSEC3 type bitmap as
a sequence of windows, each introduced by a two-byte header (window number,
bitmap length), per RFC 3845 section 2.1.2.

The loop condition only guaranteed that one byte remained:

while (dptr < dend && dptr[0] <= window) {
        if (dptr[0] == window && subtype / 8 < dptr[1] &&
                        dptr + dptr[1] + 2 <= dend) {
                return dptr[2 + subtype / 8] & (0x80 >> (subtype % 8));
        }
        dptr += dptr[1] + 2; /* next window */
}

Both the if condition and the advance step read dptr[1], the length byte.
The existing dptr + dptr[1] + 2 <= dend check guards the bitmap bytes from
the third byte onward, but nothing guards the read of dptr[1] itself. A
bitmap whose data ends immediately after a window number is one byte short of
a complete header, and all three functions then read one byte past the end of
the buffer.

The fix requires two bytes before entering the loop, so a trailing partial
window header is treated as the end of the bitmap.

Reproducer

Built against this tree with -fsanitize=address,undefined:

uint8_t *data = malloc(1);
data[0] = 0x00;                         /* window 0, no length byte follows */
ldns_rdf *bitmap = ldns_rdf_new(LDNS_RDF_TYPE_BITMAP, 1, data);
ldns_nsec_bitmap_covers_type(bitmap, 0);

Before:

==3462717==ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 1 at 0x602000000011 thread T0
    #0 ldns_nsec_bitmap_covers_type dnssec.c:1425:42
0x602000000011 is located 0 bytes after 1-byte region

After: no ASan report, the call returns false.

Checks run

• The reproducer above: faults on the parent commit, clean with this change.
• A bitmap built the ordinary way, ldns_rr_new_frm_str() on
  example.com. 3600 IN NSEC a.example.com. A MX RRSIG NSEC TYPE1234, gives
  byte-identical answers before and after for covers_type on present types
  (including TYPE1234, which lives in the last window and is the case a
  too-strict guard would break), on absent types, and for a
  set_type/clear_type round trip.
• test/12-unit-tests-dnssec, test/13-unit-tests-base,
  test/15-unit-tests-rrtypes and test/16-unit-tests-edns, built against
  the patched library under ASan/UBSan: all pass.

Reachability, for the record

This is a one-byte overread with no write primitive. On the
ldns_wire2pkt() path it is currently not reachable, because
ldns_wire2rdf() allocates each generic rdata field using the RR's total
RDLENGTH rather than that field's own length, which leaves at least one byte
of slack past the bitmap rdf's logical size and absorbs the overread. That
masking is incidental rather than intended: allocating cur_rdf_length
instead, which is the obvious correctness fix for that allocation, would
unmask this for every wire-parsed NSEC/NSEC3 RR. Code that builds an
LDNS_RDF_TYPE_BITMAP rdf directly through ldns_rdf_new() or
ldns_rdf_new_frm_data() with a tightly sized buffer, as the reproducer does
and as the Python bindings allow, reaches it today.

Reported by mail first; posting as a public pull request at NLnet Labs'
suggestion.

[wtoorop]

Excellent! Thanks Alex J.