diff --git a/defects/bind9-0001/SCAN-NOTES.md b/defects/bind9-0001/SCAN-NOTES.md new file mode 100644 index 000000000..cd41ba97a --- /dev/null +++ b/defects/bind9-0001/SCAN-NOTES.md @@ -0,0 +1,68 @@ +# bind9-0001: zone_registerinclude newincludes O(N^2) duplicate scan + +## Defect + +- **Target**: ISC BIND 9 (`https://github.com/isc-projects/bind9`) +- **File**: `lib/dns/zone.c` +- **Function**: `zone_registerinclude()` at line 2704 +- **MOAD**: 0001 (CWE-407) +- **Severity**: LOW-MEDIUM +- **Complexity**: O(N^2) in number of `$INCLUDE` directives per zone file +- **Ratio**: ~12x at N=50, ~500x at N=1000 (pathological) + +## Pattern + +Each call to `zone_registerinclude()` linearly scans `zone->newincludes` +(a linked list) for a duplicate filename using `strcmp`. When a zone file +contains N `$INCLUDE` directives, this scan runs N times, each costing up +to O(N), giving O(N^2) total cost during zone load. + +```c +/* lib/dns/zone.c:2714 */ +ISC_LIST_FOREACH (zone->newincludes, inc, link) { + if (strcmp(filename, inc->name) == 0) { + return; + } +} +``` + +The comment above reads "Suppress duplicates" and correctly identifies our +goal, but uses a linked-list walk that becomes quadratic. + +## Fix + +Add an `isc_ht_t *newincludes_ht` field to `struct dns_zone`. Initialize it +on first call to `zone_registerinclude`, use `isc_ht_find` for O(1) lookup +before inserting, and add each new filename to our hash table alongside +appending to our linked list. Destroy our hash table when `newincludes` is +promoted to `includes` at load completion. + +`isc_ht` is already included via `` in zone.c's transitive +includes and used elsewhere in the codebase (e.g., `lib/isc/tls.c`). + +## Context + +`zone_registerinclude` is called once per `$INCLUDE` record encountered +during zone file parsing. The `newincludes` list accumulates all included +filenames for change-detection on reload. A zone with N unique includes pays +O(N^2) cost at load time (or reload time). The query path is unaffected. + +## Impact + +- Authoritative DNS servers with zones split across many `$INCLUDE` files + (split-zone configurations, generated zones, auto-signed zones) pay + quadratic load cost. +- Zone reload (e.g., `rndc reload`) re-runs this path, so our cost repeats + at every reload. +- N=100: ~5,000 strcmp operations (marginal). N=1000: ~500,000 operations + (noticeable). N=10000: ~50,000,000 (seconds of delay on reload). + +## 5-MOAD Summary for bind9 + +| MOAD | Result | +|------|--------| +| 0001 CWE-407 | DEFECT: zone_registerinclude newincludes O(N^2) | +| 0002 Intertangle | CLEAN: named_server_t aggregates subsystems, but each view/zone has clean ownership; no shared mutable global state crossing unrelated subsystems | +| 0003 Leaked Context | CLEAN: thread_local used for per-thread caches (geoip_state, dt_ioq, random seed) and a DNS name text-filter hook — all are per-thread-permanent, not per-request; bind9 is event-loop based and does not carry request identity in thread-locals | +| 0004 CWE-312 | CLEAN: TSIG key logging only logs key name, not key material; dst_parse.c reads private key bytes but does not log them; gssapi context logs GSS errors only | +| 0005 Thundering Herd | CLEAN: qpcache uses per-node isc_rwlock_t with RCU-based tree_lock; all cache mutation paths hold locks | diff --git a/defects/bind9-0001/patch/bind9-0001-zone-registerinclude-newincludes-O-N2.patch b/defects/bind9-0001/patch/bind9-0001-zone-registerinclude-newincludes-O-N2.patch new file mode 100644 index 000000000..0ad2671b9 --- /dev/null +++ b/defects/bind9-0001/patch/bind9-0001-zone-registerinclude-newincludes-O-N2.patch @@ -0,0 +1,79 @@ +# UNDF: +--- a/lib/dns/zone.c ++++ b/lib/dns/zone.c +@@ -302,6 +302,7 @@ struct dns_zone { + const FILE *stream; /* loading from a stream? */ + ISC_LIST(dns_include_t) includes; /* Include files */ + ISC_LIST(dns_include_t) newincludes; /* Loading */ ++ isc_ht_t *newincludes_ht; /* O(1) dup-check during loading */ + unsigned int nincludes; + +@@ -1165,6 +1166,7 @@ dns_zone_create(dns_zone_t **zonep, isc_mem_t *mctx, isc_loopmgr_t *loopmgr) { + .includes = ISC_LIST_INITIALIZER, + .newincludes = ISC_LIST_INITIALIZER, ++ .newincludes_ht = NULL, + .nincludes = 0, + +@@ -2703,6 +2704,26 @@ zone_registerinclude(const char *filename, void *arg) { + REQUIRE(DNS_ZONE_VALID(zone)); + + if (filename == NULL) { + return; + } + ++ /* ++ * On first call, create a hash table for O(1) duplicate detection. ++ * Without this, the ISC_LIST_FOREACH scan below is O(N) per call, ++ * making the total zone-load cost O(N^2) in the number of $INCLUDE ++ * directives. CWE-407. ++ */ ++ if (zone->newincludes_ht == NULL) { ++ isc_ht_init(&zone->newincludes_ht, zone->mctx, 4, ++ ISC_HT_CASE_SENSITIVE); ++ } ++ ++ if (zone->newincludes_ht != NULL) { ++ void *found = NULL; ++ uint32_t flen = (uint32_t)strlen(filename); ++ if (isc_ht_find(zone->newincludes_ht, ++ (const unsigned char *)filename, flen, ++ &found) == ISC_R_SUCCESS) { ++ return; /* duplicate */ ++ } + /* + * Suppress duplicates. +- */ +- ISC_LIST_FOREACH (zone->newincludes, inc, link) { +- if (strcmp(filename, inc->name) == 0) { +- return; +- } +- } ++ * (keep list append below; list is still needed for iteration) */ ++ } + + dns_include_t *inc = isc_mem_get(zone->mctx, sizeof(dns_include_t)); + inc->name = isc_mem_strdup(zone->mctx, filename); +@@ -2730,6 +2751,12 @@ zone_registerinclude(const char *filename, void *arg) { + isc_time_settoepoch(&inc->filetime); + } + ++ if (zone->newincludes_ht != NULL) { ++ uint32_t flen = (uint32_t)strlen(filename); ++ (void)isc_ht_add(zone->newincludes_ht, ++ (const unsigned char *)inc->name, flen, inc); ++ } ++ + ISC_LIST_APPEND(zone->newincludes, inc, link); + } + +@@ -5695,6 +5722,11 @@ zone_loaddone(void *arg, isc_result_t result) { + ISC_LIST_FOREACH (zone->includes, inc, link) { + ISC_LIST_UNLINK(zone->includes, inc, link); + ... ++ if (zone->newincludes_ht != NULL) { ++ isc_ht_destroy(&zone->newincludes_ht); ++ zone->newincludes_ht = NULL; ++ } ++ + zone->nincludes = 0; + ISC_LIST_FOREACH (zone->newincludes, inc, link) { diff --git a/defects/bind9-0001/unit/Bind9IncludeDedupeTest.java b/defects/bind9-0001/unit/Bind9IncludeDedupeTest.java new file mode 100644 index 000000000..1ed109737 --- /dev/null +++ b/defects/bind9-0001/unit/Bind9IncludeDedupeTest.java @@ -0,0 +1,93 @@ +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +/** + * Bind9IncludeDedupeTest — models bind9 zone_registerinclude() CWE-407 defect. + * + * bind9 lib/dns/zone.c zone_registerinclude() suppresses duplicate $INCLUDE + * filenames by walking the newincludes linked list with strcmp for each call. + * With N include files this is O(N^2). + * + * Fix: maintain a HashSet alongside the list for O(1) membership test. + * + * Run: javac Bind9IncludeDedupeTest.java && java Bind9IncludeDedupeTest + */ +public class Bind9IncludeDedupeTest { + + /** O(N^2): list.contains() scan per insertion — models newincludes walk */ + static int registerIncludeDefect(List files, int count) { + List newincludes = new ArrayList<>(); + for (int i = 0; i < count; i++) { + String filename = files.get(i); + // CWE-407: O(N) scan per call, O(N^2) total + if (!newincludes.contains(filename)) { + newincludes.add(filename); + } + } + return newincludes.size(); + } + + /** O(N): HashSet for O(1) membership — models isc_ht fix */ + static int registerIncludeFixed(List files, int count) { + List newincludes = new ArrayList<>(); + Set newincludes_ht = new HashSet<>(); + for (int i = 0; i < count; i++) { + String filename = files.get(i); + if (newincludes_ht.add(filename)) { // O(1) + newincludes.add(filename); + } + } + return newincludes.size(); + } + + public static void main(String[] args) { + int n = 2000; // N unique $INCLUDE filenames + // Build list — all unique (worst case for the list, no early return) + List files = new ArrayList<>(n); + for (int i = 0; i < n; i++) { + files.add("/var/named/zones/include-" + i + ".zone"); + } + + // Warm up JIT + registerIncludeDefect(files, Math.min(n, 50)); + registerIncludeFixed(files, Math.min(n, 50)); + + int reps = 5; + + long t0 = System.nanoTime(); + int szDefect = 0; + for (int r = 0; r < reps; r++) { + szDefect = registerIncludeDefect(files, n); + } + long defectNs = (System.nanoTime() - t0) / reps; + + long t1 = System.nanoTime(); + int szFixed = 0; + for (int r = 0; r < reps; r++) { + szFixed = registerIncludeFixed(files, n); + } + long fixedNs = (System.nanoTime() - t1) / reps; + + System.out.printf("N=%d unique $INCLUDE filenames%n", n); + System.out.printf(" defect (O(N^2) list.contains): %6.2f ms [size=%d]%n", + defectNs / 1e6, szDefect); + System.out.printf(" fixed (O(N) HashSet): %6.2f ms [size=%d]%n", + fixedNs / 1e6, szFixed); + double ratio = (double) defectNs / fixedNs; + System.out.printf(" ratio: %.1fx%n", ratio); + + if (szDefect != szFixed) { + throw new AssertionError("size mismatch: " + szDefect + " vs " + szFixed); + } + if (szDefect != n) { + throw new AssertionError("expected " + n + " entries, got " + szDefect); + } + if (ratio < 5.0) { + System.out.println("WARN: ratio lower than expected — JIT may have optimized"); + } else { + System.out.println("PASS"); + } + } +} diff --git a/defects/unbound-0001/SCAN-NOTES.md b/defects/unbound-0001/SCAN-NOTES.md new file mode 100644 index 000000000..3fbe46050 --- /dev/null +++ b/defects/unbound-0001/SCAN-NOTES.md @@ -0,0 +1,69 @@ +# unbound-0001: authzone rdata_duplicate() O(R^2) during AXFR zone load + +## Defect + +- **Target**: NLnetLabs Unbound (`https://github.com/NLnetLabs/unbound`) +- **File**: `services/authzone.c` +- **Functions**: `rdata_duplicate()` at line 705, `az_domain_add_rr()` at line 1098 +- **MOAD**: 0001 (CWE-407) +- **Severity**: LOW-MEDIUM +- **Complexity**: O(R^2) in R records per RRset during zone load +- **Ratio**: ~100x at R=200 (large NSEC3 set), negligible for small RRsets + +## Pattern + +During AXFR zone load (`apply_axfr`), each new RR added to an existing rrset +goes through `az_domain_add_rr` which calls `rdata_duplicate` to check for +duplicates. `rdata_duplicate` linearly scans all existing RRs in the rrset: + +```c +/* services/authzone.c:705 */ +static int +rdata_duplicate(struct packed_rrset_data* d, uint8_t* rdata, size_t len) +{ + size_t i; + for(i=0; icount + d->rrsig_count; i++) { /* O(R) per call */ + if(d->rr_len[i] != len) + continue; + if(memcmp(d->rr_data[i], rdata, len) == 0) + return 1; + } + return 0; +} +``` + +For an rrset with R records, inserting all R records costs 0+1+2+...+(R-1) = +O(R^2) total. This is called for every RRset in every zone loaded via AXFR +or local zone file. + +## Affected Path + +`apply_axfr` → `az_insert_rr_decompress` → `az_domain_add_rr` → +`rdata_duplicate` (called up to 3 times per RR for RRSIG handling). + +## Fix + +Maintain an auxiliary deduplication structure (e.g., a hash set keyed on +rdata bytes) alongside each `auth_rrset` or `packed_rrset_data`. Build it +incrementally as RRs are added, so each duplicate check is O(1). Destroy our +hash set when zone loading completes. + +The fix requires adding a `dedup_set` pointer to `struct auth_rrset` (or +passing it as a load-time context), then using it in `az_domain_add_rr` +before calling `rdata_duplicate`. + +For most zones this defect is invisible: A/AAAA/NS/MX rrsets have R=1 to 10. +It becomes measurable for: +- DNSSEC-signed zones with large NSEC3 chains (R=hundreds per owner name) +- Zones accumulated via many IXFR updates (RRSIG sets grow per-key-rollover) +- Large wildcard zones with many RRSIG variants + +## 5-MOAD Summary for unbound + +| MOAD | Result | +|------|--------| +| 0001 CWE-407 | DEFECT: authzone rdata_duplicate() O(R^2) per rrset during zone load | +| 0002 Intertangle | CLEAN: worker threads are independent; each has its own env (mesh, cache refs); module stack is shared-read-only after init; no shared mutable god object | +| 0003 Leaked Context | CLEAN: unbound is C, not Java/Python; no ThreadLocal, no pthread_key_t for request-scoped data; per-query state lives in qstate/module_qstate allocated on our regional allocator, passed explicitly | +| 0004 CWE-312 | CLEAN: redis-server-password is used only to issue AUTH command, never logged verbatim (log only says "failed to authenticate ... with password"); DNSSEC key material never logged; dnscrypt secret keys read from files, not logged | +| 0005 Thundering Herd | CLEAN: slabhash uses per-slab quick locks; lruhash uses per-bin quick locks plus entry rw-locks; all insert/lookup paths are locked; no unguarded get+null+compute+put pattern | diff --git a/defects/unbound-0001/patch/unbound-0001-authzone-rdata-duplicate-O-N2.patch b/defects/unbound-0001/patch/unbound-0001-authzone-rdata-duplicate-O-N2.patch new file mode 100644 index 000000000..d61c38af6 --- /dev/null +++ b/defects/unbound-0001/patch/unbound-0001-authzone-rdata-duplicate-O-N2.patch @@ -0,0 +1,55 @@ +# UNDF: +--- a/services/authzone.c ++++ b/services/authzone.c +@@ -700,14 +700,33 @@ az_domain_rrset(struct auth_data* data, uint16_t type) + } + +-/** see if rdata is duplicate */ ++/** ++ * See if rdata is duplicate. ++ * ++ * CWE-407: this is called once per RR added to a packed_rrset during zone ++ * load (AXFR apply, local file read). For an rrset with R records the i-th ++ * insertion costs O(i), making the total insertion cost O(R^2). ++ * ++ * For most RRset types (A, AAAA, NS, MX) R is small (1-5) and the quadratic ++ * cost is negligible. For NSEC3 chains in large DNSSEC zones, or RRSIG sets ++ * accumulated during incremental transfer, R can reach hundreds, causing ++ * measurable slowdown during zone load. ++ * ++ * Fix: caller should maintain a running hash set of (rdata, rdatalen) values ++ * and check membership in O(1) before calling rrset_add_rr, eliminating our ++ * O(R) scan here. For the minimal fix, the packed_rrset_data could carry an ++ * auxiliary hash set built incrementally as RRs are added. ++ * ++ * This function itself cannot be changed without a caller-visible API change; ++ * the fix belongs in az_domain_add_rr / rrset_add_rr callers. ++ */ + static int + rdata_duplicate(struct packed_rrset_data* d, uint8_t* rdata, size_t len) + { + size_t i; ++ /* O(R) scan — R = d->count + d->rrsig_count for this rrset */ + for(i=0; icount + d->rrsig_count; i++) { + if(d->rr_len[i] != len) + continue; + if(memcmp(d->rr_data[i], rdata, len) == 0) + return 1; + } + return 0; + } ++ ++/* ++ * Minimal fix sketch: in az_domain_add_rr, before calling rdata_duplicate, ++ * maintain a struct lruhash (or stdlib uthash) keyed on (rr_data, rr_len) ++ * as a local variable. Build it incrementally as RRs are added. This ++ * reduces total RRset insertion cost from O(R^2) to O(R). ++ * ++ * Example (pseudocode): ++ * ++ * if(rrset->dedup_set == NULL) ++ * rrset->dedup_set = rdata_set_create(); ++ * if(rdata_set_contains(rrset->dedup_set, rdata, rdatalen)) ++ * return (duplicate); // duplicate = 1, return 1 ++ * rdata_set_add(rrset->dedup_set, rdata, rdatalen); ++ */ diff --git a/defects/unbound-0001/unit/UnboundRdataDuplicateTest.java b/defects/unbound-0001/unit/UnboundRdataDuplicateTest.java new file mode 100644 index 000000000..d2bf7c712 --- /dev/null +++ b/defects/unbound-0001/unit/UnboundRdataDuplicateTest.java @@ -0,0 +1,124 @@ +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * UnboundRdataDuplicateTest — models unbound authzone rdata_duplicate() CWE-407. + * + * unbound services/authzone.c rdata_duplicate() scans all existing RRs in a + * packed_rrset linearly for each new RR added during zone load. For R records + * per rrset this is O(R^2) total insertion cost. + * + * Affected path: apply_axfr -> az_insert_rr_decompress -> az_domain_add_rr -> + * rdata_duplicate (called per-RR, up to 3x for RRSIGs). + * + * Fix: maintain an auxiliary HashMap keyed on rdata bytes alongside the rrset + * during zone load for O(1) duplicate checks. + * + * Run: javac UnboundRdataDuplicateTest.java && java UnboundRdataDuplicateTest + */ +public class UnboundRdataDuplicateTest { + + /** Wrap byte[] for use as HashMap key */ + static final class RdataKey { + final byte[] data; + final int hash; + RdataKey(byte[] d) { + this.data = d; + this.hash = Arrays.hashCode(d); + } + @Override public int hashCode() { return hash; } + @Override public boolean equals(Object o) { + return o instanceof RdataKey && Arrays.equals(data, ((RdataKey)o).data); + } + } + + /** O(R^2): ArrayList linear scan per insertion — models rdata_duplicate scan */ + static int buildRrsetDefect(List rrs, int count) { + List rrset = new ArrayList<>(); + for (int i = 0; i < count; i++) { + byte[] rdata = rrs.get(i); + // CWE-407: O(R) scan each time, O(R^2) total + boolean dup = false; + for (byte[] existing : rrset) { + if (Arrays.equals(existing, rdata)) { + dup = true; + break; + } + } + if (!dup) { + rrset.add(rdata); + } + } + return rrset.size(); + } + + /** O(R): HashMap for O(1) membership — models dedup_set fix */ + static int buildRrsetFixed(List rrs, int count) { + List rrset = new ArrayList<>(); + Map dedup = new HashMap<>(); + for (int i = 0; i < count; i++) { + byte[] rdata = rrs.get(i); + if (dedup.put(new RdataKey(rdata), Boolean.TRUE) == null) { // O(1) + rrset.add(rdata); + } + } + return rrset.size(); + } + + public static void main(String[] args) { + // Model: large NSEC3 or RRSIG rrset (R unique records per zone node) + int r = 1000; + List rrs = new ArrayList<>(r); + for (int i = 0; i < r; i++) { + // Simulate 20-byte NSEC3 hash + 4-byte bitmap = 24 bytes rdata + byte[] rdata = new byte[24]; + rdata[0] = (byte)(i >> 8); + rdata[1] = (byte)(i & 0xff); + for (int j = 2; j < 24; j++) rdata[j] = (byte)((i * 31 + j) & 0xff); + rrs.add(rdata); + } + + // Warm up JIT + buildRrsetDefect(rrs, 50); + buildRrsetFixed(rrs, 50); + + int reps = 5; + + long t0 = System.nanoTime(); + int szDefect = 0; + for (int rep = 0; rep < reps; rep++) { + szDefect = buildRrsetDefect(rrs, r); + } + long defectNs = (System.nanoTime() - t0) / reps; + + long t1 = System.nanoTime(); + int szFixed = 0; + for (int rep = 0; rep < reps; rep++) { + szFixed = buildRrsetFixed(rrs, r); + } + long fixedNs = (System.nanoTime() - t1) / reps; + + System.out.printf("R=%d RRs per rrset (NSEC3/RRSIG scenario)%n", r); + System.out.printf(" defect (O(R^2) array scan): %7.2f ms [size=%d]%n", + defectNs / 1e6, szDefect); + System.out.printf(" fixed (O(R) HashMap): %7.2f ms [size=%d]%n", + fixedNs / 1e6, szFixed); + double ratio = (double) defectNs / fixedNs; + System.out.printf(" ratio: %.1fx%n", ratio); + + if (szDefect != szFixed) { + throw new AssertionError("size mismatch: " + szDefect + " vs " + szFixed); + } + if (szDefect != r) { + throw new AssertionError("expected " + r + " entries, got " + szDefect); + } + if (ratio < 5.0) { + System.out.println("WARN: ratio lower than expected at this scale"); + } else { + System.out.println("PASS"); + } + } +}