Commit 9924fc10 authored by Michał Kępień's avatar Michał Kępień
Browse files

Merge branch '1850-cleanup-client_allocsendbuf' into security-master

parents 3a5a508a ed4b69ab
......@@ -7,7 +7,9 @@
 
5437. [bug] Fix a data race in resolver log_formerr. [GL #1808]
 
5436. [placeholder]
5436. [security] It was possible to trigger an INSIST when determining
whether a record would fit into a TCP message buffer.
(CVE-2020-8618) [GL #1850]
 
5435. [placeholder]
 
......
......@@ -25,6 +25,9 @@ Security Fixes
- Replaying a TSIG BADTIME response as a request could trigger an
assertion failure. This was disclosed in CVE-2020-8617. [GL #1703]
- It was possible to trigger an assertion when attempting to fill an
oversized TCP buffer. This was disclosed in CVE-2020-8618. [GL #1850]
Known Issues
~~~~~~~~~~~~
......
......@@ -287,45 +287,20 @@ client_senddone(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
isc_nmhandle_unref(handle);
}
/*%
* We only want to fail with ISC_R_NOSPACE when called from
* ns_client_sendraw() and not when called from ns_client_send(),
* tcpbuffer is NULL when called from ns_client_sendraw() and
* length != 0. tcpbuffer != NULL when called from ns_client_send()
* and length == 0.
*/
static isc_result_t
static void
client_allocsendbuf(ns_client_t *client, isc_buffer_t *buffer,
isc_buffer_t *tcpbuffer, uint32_t length,
unsigned char **datap) {
unsigned char *data;
uint32_t bufsize;
isc_result_t result;
REQUIRE(datap != NULL);
REQUIRE((tcpbuffer == NULL && length != 0) ||
(tcpbuffer != NULL && length == 0));
if (TCP_CLIENT(client)) {
INSIST(client->tcpbuf == NULL);
if (length + 2 > NS_CLIENT_TCP_BUFFER_SIZE) {
result = ISC_R_NOSPACE;
goto done;
}
client->tcpbuf = isc_mem_get(client->mctx,
NS_CLIENT_TCP_BUFFER_SIZE);
data = client->tcpbuf;
if (tcpbuffer != NULL) {
isc_buffer_init(tcpbuffer, data,
NS_CLIENT_TCP_BUFFER_SIZE);
isc_buffer_init(buffer, data,
NS_CLIENT_TCP_BUFFER_SIZE);
} else {
isc_buffer_init(buffer, data,
NS_CLIENT_TCP_BUFFER_SIZE);
INSIST(length <= 0xffff);
}
isc_buffer_init(buffer, data, NS_CLIENT_TCP_BUFFER_SIZE);
} else {
data = client->sendbuf;
if ((client->attributes & NS_CLIENTATTR_HAVECOOKIE) == 0) {
......@@ -343,17 +318,9 @@ client_allocsendbuf(ns_client_t *client, isc_buffer_t *buffer,
if (bufsize > NS_CLIENT_SEND_BUFFER_SIZE) {
bufsize = NS_CLIENT_SEND_BUFFER_SIZE;
}
if (length > bufsize) {
result = ISC_R_NOSPACE;
goto done;
}
isc_buffer_init(buffer, data, bufsize);
}
*datap = data;
result = ISC_R_SUCCESS;
done:
return (result);
}
static isc_result_t
......@@ -385,8 +352,10 @@ ns_client_sendraw(ns_client_t *client, dns_message_t *message) {
goto done;
}
result = client_allocsendbuf(client, &buffer, NULL, mr->length, &data);
if (result != ISC_R_SUCCESS) {
client_allocsendbuf(client, &buffer, &data);
if (mr->length > isc_buffer_length(&buffer)) {
result = ISC_R_NOSPACE;
goto done;
}
......@@ -422,7 +391,6 @@ ns_client_send(ns_client_t *client) {
isc_result_t result;
unsigned char *data;
isc_buffer_t buffer = { .magic = 0 };
isc_buffer_t tcpbuffer = { .magic = 0 };
isc_region_t r;
dns_compress_t cctx;
bool cleanup_cctx = false;
......@@ -491,13 +459,7 @@ ns_client_send(ns_client_t *client) {
}
}
/*
* XXXRTH The following doesn't deal with TCP buffer resizing.
*/
result = client_allocsendbuf(client, &buffer, &tcpbuffer, 0, &data);
if (result != ISC_R_SUCCESS) {
goto done;
}
client_allocsendbuf(client, &buffer, &data);
result = dns_compress_init(&cctx, -1, client->mctx);
if (result != ISC_R_SUCCESS) {
......@@ -619,7 +581,6 @@ renderend:
client->sendcb(&buffer);
} else if (TCP_CLIENT(client)) {
isc_buffer_usedregion(&buffer, &r);
isc_buffer_add(&tcpbuffer, r.length);
#ifdef HAVE_DNSTAP
if (client->view != NULL) {
dns_dt_send(client->view, dtmsgtype, &client->peeraddr,
......@@ -628,11 +589,10 @@ renderend:
}
#endif /* HAVE_DNSTAP */
/* don't count the 2-octet length header */
respsize = isc_buffer_usedlength(&tcpbuffer) - 2;
respsize = isc_buffer_usedlength(&buffer);
isc_nmhandle_ref(client->handle);
result = client_sendpkg(client, &tcpbuffer);
result = client_sendpkg(client, &buffer);
if (result != ISC_R_SUCCESS) {
/* We won't get a callback to clean it up */
isc_nmhandle_unref(client->handle);
......
......@@ -81,7 +81,7 @@
*** Types
***/
#define NS_CLIENT_TCP_BUFFER_SIZE (65535 + 2)
#define NS_CLIENT_TCP_BUFFER_SIZE 65535
#define NS_CLIENT_SEND_BUFFER_SIZE 4096
/*!
......
......@@ -649,14 +649,13 @@ typedef struct {
dns_db_t *db;
dns_dbversion_t *ver;
isc_quota_t *quota;
rrstream_t *stream; /* The XFR RR stream */
bool question_added; /* QUESTION section sent? */
bool end_of_stream; /* EOS has been reached */
isc_buffer_t buf; /* Buffer for message owner
* names and rdatas */
isc_buffer_t txlenbuf; /* Transmit length buffer */
isc_buffer_t txbuf; /* Transmit message buffer */
size_t cbytes; /* Length of current message */
rrstream_t *stream; /* The XFR RR stream */
bool question_added; /* QUESTION section sent? */
bool end_of_stream; /* EOS has been reached */
isc_buffer_t buf; /* Buffer for message owner
* names and rdatas */
isc_buffer_t txbuf; /* Transmit message buffer */
size_t cbytes; /* Length of current message */
void *txmem;
unsigned int txmemlen;
dns_tsigkey_t *tsigkey; /* Key used to create TSIG */
......@@ -1269,12 +1268,11 @@ xfrout_ctx_create(isc_mem_t *mctx, ns_client_t *client, unsigned int id,
/*
* Allocate another temporary buffer for the compressed
* response message and its TCP length prefix.
* response message.
*/
len = 2 + 65535;
len = NS_CLIENT_TCP_BUFFER_SIZE;
mem = isc_mem_get(mctx, len);
isc_buffer_init(&xfr->txlenbuf, mem, 2);
isc_buffer_init(&xfr->txbuf, (char *)mem + 2, len - 2);
isc_buffer_init(&xfr->txbuf, (char *)mem, len);
xfr->txmem = mem;
xfr->txmemlen = len;
......@@ -1324,7 +1322,6 @@ sendstream(xfrout_ctx_t *xfr) {
int n_rrs;
isc_buffer_clear(&xfr->buf);
isc_buffer_clear(&xfr->txlenbuf);
isc_buffer_clear(&xfr->txbuf);
is_tcp = ((xfr->client->attributes & NS_CLIENTATTR_TCP) != 0);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment