Use dns_name_copynf() with dns_message_gettempname() when needed

dns_message_gettempname() returns an initialized name with a dedicated
buffer, associated with a dns_fixedname object.  Using dns_name_copynf()
to write a name into this object will actually copy the name data
from a source name. dns_name_clone() merely points target->ndata to
source->ndata, so it is faster, but it can lead to a use-after-free if
the source is freed before the target object is released via
dns_message_puttempname().

In a few places, clone was being used where copynf should have been;
this is now fixed.

As a side note, no memory was lost, because the ndata buffer used in
the dns_fixedname_t is internal to the structure, and is freed when
the dns_fixedname_t is freed regardless of the .ndata contents.

diff --git a/lib/dns/include/dns/name.h b/lib/dns/include/dns/name.h
index 0edbfd90eef9648403dbe5923cd4343947bd0af1..16bb5ffa33ae9be1dab8b3d5b703eb95d7d1a415 100644 (file)
--- a/lib/dns/include/dns/name.h
+++ b/lib/dns/include/dns/name.h
@@ -638,7 +638,7 @@ dns_name_clone(const dns_name_t *source, dns_name_t *target);
  * Notes:
  *
  * \li 'target' refers to the same memory as 'source', so 'source'
- *     must not be changed while 'target' is still in use.
+ *     must not be changed or freed while 'target' is still in use.
  *
  * \li This call is functionally equivalent to:
  *
@@ -1242,22 +1242,17 @@ dns_name_settotextfilter(dns_name_totextfilter_t *proc);
 
 isc_result_t
 dns_name_copy(const dns_name_t *source, dns_name_t *dest, isc_buffer_t *target);
-void
-dns_name_copynf(const dns_name_t *source, dns_name_t *dest);
 /*%<
- * Makes 'dest' refer to a copy of the name in 'source'.  The data are either
- * copied to 'target' or in case of dns_name_copynf the dedicated buffer in
- * 'dest'.
+ * Copies the name in 'source' into 'dest'.  The name data is copied to
+ * the 'target' buffer, which is then set as the buffer for 'dest'.
  *
  * Requires:
  * \li 'source' is a valid name.
  *
- * \li 'dest' is an initialized name with a dedicated buffer.
+ * \li 'dest' is an initialized name.
  *
  * \li 'target' is an initialized buffer.
  *
- * \li Either dest has a dedicated buffer or target != NULL.
- *
  * Ensures:
  *
  *\li  On success, the used space in target is updated.
@@ -1267,6 +1262,18 @@ dns_name_copynf(const dns_name_t *source, dns_name_t *dest);
  *\li  #ISC_R_NOSPACE
  */
 
+void
+dns_name_copynf(const dns_name_t *source, dns_name_t *dest);
+/*%<
+ * Copies the name in 'source' into 'dest'.  The name data is copied to
+ * the dedicated buffer for 'dest'.
+ *
+ * Requires:
+ * \li 'source' is a valid name.
+ *
+ * \li 'dest' is an initialized name with a dedicated buffer.
+ */
+
 bool
 dns_name_ishostname(const dns_name_t *name, bool wildcard);
 /*%<

diff --git a/lib/dns/name.c b/lib/dns/name.c
index f751b9a0676d38472538ab51124197b17f484015..fd9a8ced3b7cd9db2b3415cb7b3243a74c9c5dcf 100644 (file)
--- a/lib/dns/name.c
+++ b/lib/dns/name.c
@@ -2464,7 +2464,7 @@ dns_name_fromstring2(dns_name_t *target, const char *src,
 
 static isc_result_t
 name_copy(const dns_name_t *source, dns_name_t *dest, isc_buffer_t *target) {
-       unsigned char *ndata;
+       unsigned char *ndata = NULL;
 
        /*
         * Make dest a copy of source.
@@ -2496,7 +2496,7 @@ name_copy(const dns_name_t *source, dns_name_t *dest, isc_buffer_t *target) {
        }
 
        if (dest->labels > 0 && dest->offsets != NULL) {
-               if (source->offsets != NULL) {
+               if (source->offsets != NULL && source->labels != 0) {
                        memmove(dest->offsets, source->offsets, source->labels);
                } else {
                        set_offsets(dest, dest->offsets, NULL);

diff --git a/lib/dns/tkey.c b/lib/dns/tkey.c
index 759583514e83c095546ee9b4b1cd90f053815c6c..4267cbf1841fba5a1f04555143933367fdff30d3 100644 (file)
--- a/lib/dns/tkey.c
+++ b/lib/dns/tkey.c
@@ -180,7 +180,7 @@ add_rdata_to_list(dns_message_t *msg, dns_name_t *name, dns_rdata_t *rdata,
        dns_message_takebuffer(msg, &tmprdatabuf);
 
        RETERR(dns_message_gettempname(msg, &newname));
-       dns_name_clone(name, newname);
+       dns_name_copynf(name, newname);
 
        RETERR(dns_message_gettemprdatalist(msg, &newlist));
        newlist->rdclass = newrdata->rdclass;

diff --git a/lib/dns/tsig.c b/lib/dns/tsig.c
index 3c986f650727b0156984f13365cba62d79af1417..a35a959179d0fb29702f96960e8dec57011a2993 100644 (file)
--- a/lib/dns/tsig.c
+++ b/lib/dns/tsig.c
@@ -1020,7 +1020,7 @@ dns_tsig_sign(dns_message_t *msg) {
        if (ret != ISC_R_SUCCESS) {
                goto cleanup_rdata;
        }
-       dns_name_clone(&key->name, owner);
+       dns_name_copynf(&key->name, owner);
 
        ret = dns_message_gettemprdatalist(msg, &datalist);
        if (ret != ISC_R_SUCCESS) {

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 57a74ea403ca66dfc05ef54b1454b801acf9803e..7d01872f53f4dbc7d786e44b3c810198bd81ffe5 100644 (file)
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -9804,7 +9804,7 @@ query_synthcnamewildcard(query_ctx_t *qctx, dns_rdataset_t *rdataset,
        RUNTIME_CHECK(result == ISC_R_SUCCESS);
        dns_rdata_reset(&rdata);
 
-       dns_name_clone(&cname.cname, tname);
+       dns_name_copynf(&cname.cname, tname);
 
        dns_rdata_freestruct(&cname);
        ns_client_qnamereplace(qctx->client, tname);
@@ -10416,7 +10416,7 @@ query_cname(query_ctx_t *qctx) {
        RUNTIME_CHECK(result == ISC_R_SUCCESS);
        dns_rdata_reset(&rdata);
 
-       dns_name_clone(&cname.cname, tname);
+       dns_name_copynf(&cname.cname, tname);
 
        dns_rdata_freestruct(&cname);
        ns_client_qnamereplace(qctx->client, tname);
@@ -10518,7 +10518,7 @@ query_dname(query_ctx_t *qctx) {
        RUNTIME_CHECK(result == ISC_R_SUCCESS);
        dns_rdata_reset(&rdata);
 
-       dns_name_clone(&dname.dname, tname);
+       dns_name_copynf(&dname.dname, tname);
        dns_rdata_freestruct(&dname);
 
        /*
@@ -10616,7 +10616,8 @@ query_addcname(query_ctx_t *qctx, dns_trust_t trust, dns_ttl_t ttl) {
        if (result != ISC_R_SUCCESS) {
                return (result);
        }
-       dns_name_clone(client->query.qname, aname);
+
+       dns_name_copynf(client->query.qname, aname);
 
        result = dns_message_gettemprdatalist(client->message, &rdatalist);
        if (result != ISC_R_SUCCESS) {
@@ -10751,7 +10752,13 @@ query_addsoa(query_ctx_t *qctx, unsigned int override_ttl,
        if (result != ISC_R_SUCCESS) {
                return (result);
        }
+
+       /*
+        * We'll be releasing 'name' before returning, so it's safe to
+        * use clone instead of copying here.
+        */
        dns_name_clone(dns_db_origin(qctx->db), name);
+
        rdataset = ns_client_newrdataset(client);
        if (rdataset == NULL) {
                CTRACE(ISC_LOG_ERROR, "unable to allocate rdataset");