kind of use-after-free condition in SIG(0) signing

Jinmei writes: I suspect the following code could in lib/dns/dnssec.c:dns_dnssec_signmessage cause a kind of "use after free" situation:

/*
 * If this is a response, digest the query.
 */
if (is_response(msg)) {
        RETERR(dst_context_adddata(ctx, &msg->query));
}

when it's called while building a response message in ns_client_send.

If my understanding of the code is correct, this msg->query is a shallow copy of region passed to ns__client_request: it's first (shallow) copied in 'msg.saved' in dns_message_parse, and then (also shallow) copied in 'msg.query' in dns_message_reply. But the original "region" was maintained separately in an isc__networker_t instance, and the region is reused for subsequent network events once ns__client_request returns.

That would mean, for example, if the query handling triggers a recursion "msg->query" can be already bogus at the time of building the response.

That would not be catastrophic like usual sense of "use after free" since the memory region is just recycled internally and dns_dnssec_signmessage uses it only for reading the data. But it would at least result in a broken signature. Also, while that seems to be the only use case of msg->query right now, some new usage of it might cause a more dangerous condition in the future. So, I'd suggest you make it more robust in a more fundamental way, e.g., by passing DNS_MESSAGEPARSE_CLONEBUFFER to dns_message_parse from ns__client_request.

A couple of additional notes:

• I actually didn't test the SIG(0) case, but confirmed that msg->query can store bogus data on resuming recursion, so I'm pretty sure that there's a defect here.
• I suspect (but didn't confirm) this problem didn't exist for previous versions of BIND 9 until it introduced "isc__networker"; previously the memory region for the received query data was maintained in ns_client (either in its 'recvbuf' or via its 'tcpmsg'), so it's guaranteed to be valid until that particular ns_client instance completes the request processing. Still, the relationship is so indirect and therefore seems to be error prone, and this obscurity may in fact be a background cause of this regression. That would probably be another argument for considering more robust and fundamental fix rather than just plug this particular issue.

see RT #17005