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.

diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h
index b089cc7db584e38e563c05ef7f8f5a0f98bf5479..5e9a1f25a1fe297959c3335d785bd6634a25a039 100644 (file)
--- 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 9c02c94619edbcb36fe2c1d335da24973ebf46aa..e990a61e3f704d005f2adc06a6775979aa8db3e5 100644 (file)
--- 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 6f9084e8316283650a05b9d6e3a2913f5107b487..183335e4e4fbf545ac408aa6cd8c7957c0178a05 100644 (file)
--- 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 *