From: Ondřej Surý Date: Fri, 21 May 2021 13:30:00 +0000 (+0200) Subject: Use dns_name_copynf() with dns_message_gettempname() when needed X-Git-Tag: v9.17.14~34^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ce3e1abc1d7d2357d884cb1f223c22cebf37f2b7;p=thirdparty%2Fbind9.git 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 0edbfd90eef..16bb5ffa33a 100644 --- 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 f751b9a0676..fd9a8ced3b7 100644 --- 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 759583514e8..4267cbf1841 100644 --- 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 3c986f65072..a35a959179d 100644 --- 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 57a74ea403c..7d01872f53f 100644 --- 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");