From 3c333d02a06fb2aa753e80a795135bdff703b3d0 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Mon, 3 Apr 2023 14:20:20 +0100 Subject: [PATCH] More dns_qpkey_t safety checks My original idea had been that the core qp-trie code would be mostly independent of the storage for keys, so I did not make it check at run time that key lengths are sensible. However, the qp-trie search routines need to get keys out of leaf objects, for which they provide storage on the stack, which is particularly dangerous for unchecked buffer overflows. So this change checks that key lengths are in bounds at the API boundary between the qp-trie code and the rest of BIND, and there is no more pretence that keys might be longer. --- lib/dns/include/dns/qp.h | 18 ++++++++++++------ lib/dns/qp.c | 3 +++ lib/dns/qp_p.h | 6 ++++-- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index b089cc7db5..5e9a1f25a1 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -193,8 +193,9 @@ typedef uint8_t dns_qpkey_t[512]; * * The `makekey` method fills in a `dns_qpkey_t` corresponding to a * value object stored in the qp-trie. It returns the length of the - * key. This method will typically call dns_qpkey_fromname() with a - * name stored in the value object. + * key, which must be less than `sizeof(dns_qpkey_t)`. This method + * will typically call dns_qpkey_fromname() with a name stored in the + * value object. * * For logging and tracing, the `triename` method copies a human- * readable identifier into `buf` which has max length `size`. @@ -395,15 +396,18 @@ dns_qpkey_fromname(dns_qpkey_t key, const dns_name_t *name); * Requires: * \li `name` is a pointer to a valid `dns_name_t` * + * Ensures: + * \li returned length is less than `sizeof(dns_qpkey_t)` + * * Returns: * \li the length of the key */ isc_result_t -dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t searchk, size_t searchl, - void **pval_r, uint32_t *ival_r); +dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key, + size_t search_keylen, void **pval_r, uint32_t *ival_r); /*%< - * Find a leaf in a qp-trie that matches the given key + * Find a leaf in a qp-trie that matches the given search key * * The leaf values are assigned to `*pval_r` and `*ival_r` * @@ -411,6 +415,7 @@ dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t searchk, size_t searchl, * \li `qpr` is a pointer to a readable qp-trie * \li `pval_r != NULL` * \li `ival_r != NULL` + * \li `search_keylen < sizeof(dns_qpkey_t)` * * Returns: * \li ISC_R_NOTFOUND if the trie has no leaf with a matching key @@ -452,12 +457,13 @@ dns_qp_insert(dns_qp_t *qp, void *pval, uint32_t ival); */ isc_result_t -dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t key, size_t len); +dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t key, size_t keylen); /*%< * Delete a leaf from a qp-trie that matches the given key * * Requires: * \li `qp` is a pointer to a valid qp-trie + * \li `keylen < sizeof(dns_qpkey_t)` * * Returns: * \li ISC_R_NOTFOUND if the trie has no leaf with a matching key diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 9c02c94619..e990a61e3f 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -242,6 +242,7 @@ dns_qpkey_fromname(dns_qpkey_t key, const dns_name_t *name) { } /* mark end with a double NOBYTE */ key[len] = SHIFT_NOBYTE; + ENSURE(len < sizeof(dns_qpkey_t)); return (len); } @@ -1606,6 +1607,7 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key, qp_node_t *n; REQUIRE(QP_VALID(qp)); + REQUIRE(search_keylen < sizeof(dns_qpkey_t)); if (get_root(qp) == NULL) { return (ISC_R_NOTFOUND); @@ -1696,6 +1698,7 @@ dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key, REQUIRE(QP_VALID(qp)); REQUIRE(pval_r != NULL); REQUIRE(ival_r != NULL); + REQUIRE(search_keylen < sizeof(dns_qpkey_t)); n = get_root(qp); if (n == NULL) { diff --git a/lib/dns/qp_p.h b/lib/dns/qp_p.h index 6f9084e831..183335e4e4 100644 --- a/lib/dns/qp_p.h +++ b/lib/dns/qp_p.h @@ -937,8 +937,10 @@ detach_leaf(dns_qpreadable_t qpr, qp_node_t *n) { static inline size_t leaf_qpkey(dns_qpreadable_t qpr, qp_node_t *n, dns_qpkey_t key) { dns_qpreader_t *qp = dns_qpreader(qpr); - return (qp->methods->makekey(key, qp->uctx, leaf_pval(n), - leaf_ival(n))); + size_t len = qp->methods->makekey(key, qp->uctx, leaf_pval(n), + leaf_ival(n)); + INSIST(len < sizeof(dns_qpkey_t)); + return (len); } static inline char *