validator: similarly also limit excessive NSEC3 salt length

Limit combination of iterations and salt length, based on estimated
expense of the computation.  Note that the result only differs for
salt length > 44 which is rather nonsensical and very rare:
https://chat.dns-oarc.net/community/pl/h58qx9sjkbgt9dajb7x988p78a

diff --git a/lib/cache/api.c b/lib/cache/api.c
index 116d775e92d390310e944f682072a652ba373a25..bb627ea7cc4d43bf44fca3ba26276e20a8f49f18 100644 (file)
--- a/lib/cache/api.c
+++ b/lib/cache/api.c
@@ -500,7 +500,7 @@ static ssize_t stash_rrset(struct kr_cache *cache, const struct kr_query *qry,
                return kr_ok();
        }
        if (rr->type == KNOT_RRTYPE_NSEC3 && rr->rrs.count
-           && knot_nsec3_iters(rr->rrs.rdata) > KR_NSEC3_MAX_ITERATIONS) {
+           && kr_nsec3_limited_rdata(rr->rrs.rdata)) {
                /* This shouldn't happen often, thanks to downgrades during validation. */
                VERBOSE_MSG(qry, "=> skipping NSEC3 with too many iterations\n");
                return kr_ok();

diff --git a/lib/cache/nsec3.c b/lib/cache/nsec3.c
index 0b707759e2f28bc5a87086be1ead7d32b38ee4fc..98326309a71ec1decb157dac5992edfc3238eb00 100644 (file)
--- a/lib/cache/nsec3.c
+++ b/lib/cache/nsec3.c
@@ -84,7 +84,7 @@ static knot_db_val_t key_NSEC3_name(struct key *k, const knot_dname_t *name,
                .data = (uint8_t *)/*const-cast*/name,
        };
 
-       if (kr_fails_assert(nsec_p->libknot.iterations <= KR_NSEC3_MAX_ITERATIONS)) {
+       if (kr_fails_assert(!kr_nsec3_limited_params(&nsec_p->libknot))) {
                /* This is mainly defensive; it shouldn't happen thanks to downgrades. */
                return VAL_EMPTY;
        }

diff --git a/lib/dnssec/nsec3.c b/lib/dnssec/nsec3.c
index 037d5bdc5d8863d50e128ef7019265b02bc8c683..e4d314bc5285c8a65e0217544975adeedb6dc9e6 100644 (file)
--- a/lib/dnssec/nsec3.c
+++ b/lib/dnssec/nsec3.c
@@ -71,7 +71,7 @@ static int hash_name(dnssec_binary_t *hash, const dnssec_nsec3_params_t *params,
                return kr_error(EINVAL);
        if (!name)
                return kr_error(EINVAL);
-       if (kr_fails_assert(params->iterations <= KR_NSEC3_MAX_ITERATIONS)) {
+       if (kr_fails_assert(!kr_nsec3_limited_params(params))) {
                /* This if is mainly defensive; it shouldn't happen. */
                return kr_error(EINVAL);
        }
@@ -565,7 +565,7 @@ int kr_nsec3_wildcard_answer_response_check(const knot_pkt_t *pkt, knot_section_
                const knot_rrset_t *rrset = knot_pkt_rr(sec, i);
                if (rrset->type != KNOT_RRTYPE_NSEC3)
                        continue;
-               if (knot_nsec3_iters(rrset->rrs.rdata) > KR_NSEC3_MAX_ITERATIONS) {
+               if (kr_nsec3_limited_rdata(rrset->rrs.rdata)) {
                        /* Avoid hashing with too many iterations.
                         * If we get here, the `sname` wildcard probably ends up bogus,
                         * but it gets downgraded to KR_RANK_INSECURE when validator

diff --git a/lib/dnssec/nsec3.h b/lib/dnssec/nsec3.h
index 723dc4a1379ce7634988bd1d0cb3ea658426cbc3..76ef2e9e728a6e186659a43a254be79e836f5f72 100644 (file)
--- a/lib/dnssec/nsec3.h
+++ b/lib/dnssec/nsec3.h
@@ -5,15 +5,39 @@
 #pragma once
 
 #include <libknot/packet/pkt.h>
+#include <libknot/rrtype/nsec3.h>
+#include <libdnssec/nsec.h>
+
+
+static inline unsigned int kr_nsec3_price(unsigned int iterations, unsigned int salt_len)
+{
+       // SHA1 works on 64-byte chunks.
+       // On iterating we hash the salt + 20 bytes of the previous hash.
+       int chunks_per_iter = (20 + salt_len - 1) / 64 + 1;
+       return (iterations + 1) * chunks_per_iter;
+}
 
 /** High numbers in NSEC3 iterations don't really help security
  *
- * ...so we avoid doing all the work.  The value is a current compromise;
- * zones shooting over get downgraded to insecure status.
+ * ...so we avoid doing all the work.  The limit is a current compromise;
+ * answers using NSEC3 over kr_nsec3_limited* get downgraded to insecure status.
  *
    https://datatracker.ietf.org/doc/html/rfc9276#name-recommendation-for-validati
  */
-#define KR_NSEC3_MAX_ITERATIONS 50
+static inline bool kr_nsec3_limited(unsigned int iterations, unsigned int salt_len)
+{
+       const int MAX_ITERATIONS = 50; // limit with short salt length
+       return kr_nsec3_price(iterations, salt_len) > MAX_ITERATIONS + 1;
+}
+static inline bool kr_nsec3_limited_rdata(const knot_rdata_t *rd)
+{
+       return kr_nsec3_limited(knot_nsec3_iters(rd), knot_nsec3_salt_len(rd));
+}
+static inline bool kr_nsec3_limited_params(const dnssec_nsec3_params_t *params)
+{
+       return kr_nsec3_limited(params->iterations, params->salt.size);
+}
+
 
 /**
  * Name error response check (RFC5155 7.2.2).
@@ -36,7 +60,7 @@ int kr_nsec3_name_error_response_check(const knot_pkt_t *pkt, knot_section_t sec
  *                     KNOT_ERANGE - NSEC3 RR that covers a wildcard
  *                     has been found, but has opt-out flag set;
  *                     otherwise - error.
- * Records over KR_NSEC3_MAX_ITERATIONS are skipped, so you probably get kr_error(ENOENT).
+ * Too expensive NSEC3 records are skipped, so you probably get kr_error(ENOENT).
  */
 int kr_nsec3_wildcard_answer_response_check(const knot_pkt_t *pkt, knot_section_t section_id,
                                             const knot_dname_t *sname, int trim_to_next);

diff --git a/lib/layer/validate.c b/lib/layer/validate.c
index 17f9074001aa7bf340f2a6b9540149c7f8196465..1b1237da4a54231619d80925dfb836193d44c886 100644 (file)
--- a/lib/layer/validate.c
+++ b/lib/layer/validate.c
@@ -128,14 +128,15 @@ static bool maybe_downgrade_nsec3(const ranked_rr_array_entry_t *e, struct kr_qu
        const knot_rdataset_t *rrs = &e->rr->rrs;
        knot_rdata_t *rd = rrs->rdata;
        for (int j = 0; j < rrs->count; ++j, rd = knot_rdataset_next(rd)) {
-               if (knot_nsec3_iters(rd) > KR_NSEC3_MAX_ITERATIONS)
+               if (kr_nsec3_limited_rdata(rd))
                        goto do_downgrade;
        }
        return false;
 
 do_downgrade: // we do this deep inside calls because of having signer name available
-       VERBOSE_MSG(qry, "<= DNSSEC downgraded due to NSEC3 iterations %d > %d\n",
-                       (int)knot_nsec3_iters(rd), (int)KR_NSEC3_MAX_ITERATIONS);
+       VERBOSE_MSG(qry,
+               "<= DNSSEC downgraded due to expensive NSEC3: %d iterations, %d salt length\n",
+               (int)knot_nsec3_iters(rd), (int)knot_nsec3_salt_len(rd));
        qry->flags.DNSSEC_WANT = false;
        qry->flags.DNSSEC_INSECURE = true;
        rank_records(qry, true, KR_RANK_INSECURE, vctx->zone_name);