diff --git a/AUTHORS b/AUTHORS index 861d9094e..0b31814f3 100644 --- a/AUTHORS +++ b/AUTHORS @@ -14,6 +14,7 @@ Ali Asad Lotia Anbang Wen Andreas Rammhold Christophe Nowicki +Christopher Ng cronfy Daniel Kahn Gillmor Daniel Salzman diff --git a/NEWS b/NEWS index 7453ee685..f1cfa6791 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,14 @@ -Knot Resolver 5.7.0 (2023-0m-dd) +Knot Resolver 5.7.0 (2023-08-22) ================================ +Security +-------- +- avoid excessive TCP reconnections in a few more cases (!NNNN) + Like before, the remote server had to behave nonsensically in order + to inflict this upon itself, but it might be abusable for DoS. + + We thank Ivan Jedek from OryxLabs for reporting this. + Improvements ------------ - forwarding mode: tweak dealing with failures from forwarders, diff --git a/daemon/io.c b/daemon/io.c index 27119adbf..6d548d777 100644 --- a/daemon/io.c +++ b/daemon/io.c @@ -337,33 +337,6 @@ void tcp_timeout_trigger(uv_timer_t *timer) } } -static void tcp_disconnect(struct session *s, int errcode) -{ - if (kr_log_is_debug(IO, NULL)) { - struct sockaddr *peer = session_get_peer(s); - char *peer_str = kr_straddr(peer); - kr_log_debug(IO, "=> connection to '%s' closed by peer (%s)\n", - peer_str ? peer_str : "", - uv_strerror(errcode)); - } - - if (!session_was_useful(s) && session_flags(s)->outgoing) { - /* We want to penalize the IP address, if a task is asking a query. - * It might not be the right task, but that doesn't matter so much - * for attributing the useless session to the IP address. */ - struct qr_task *t = session_tasklist_get_first(s); - struct kr_query *qry = NULL; - if (t) { - struct kr_request *req = worker_task_request(t); - qry = array_tail(req->rplan.pending); - } - if (qry) /* We reuse the error for connection, as it's quite similar. */ - qry->server_selection.error(qry, worker_task_get_transport(t), - KR_SELECTION_TCP_CONNECT_FAILED); - } - worker_end_tcp(s); -} - static void tcp_recv(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf) { struct session *s = handle->data; @@ -381,7 +354,16 @@ static void tcp_recv(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf) } if (nread < 0 || !buf->base) { - tcp_disconnect(s, nread); + if (kr_log_is_debug(IO, NULL)) { + struct sockaddr *peer = session_get_peer(s); + char *peer_str = kr_straddr(peer); + kr_log_debug(IO, "=> connection to '%s' closed by peer (%s)\n", + peer_str ? peer_str : "", + uv_strerror(nread)); + } + + session_tcp_penalize(s); + worker_end_tcp(s); return; } diff --git a/daemon/session.c b/daemon/session.c index a1f22072b..ed0ff6869 100644 --- a/daemon/session.c +++ b/daemon/session.c @@ -123,11 +123,6 @@ void session_close(struct session *session) } } -bool session_was_useful(const struct session *session) -{ - return session->was_useful; -} - int session_start_read(struct session *session) { return io_start_read(session->handle); @@ -582,6 +577,24 @@ ssize_t session_wirebuf_trim(struct session *session, ssize_t len) return len; } +void session_tcp_penalize(struct session *s) +{ + if (s->was_useful || !s->sflags.outgoing) + return; + /* We want to penalize the IP address, if a task is asking a query. + * It might not be the right task, but that doesn't matter so much + * for attributing the useless session to the IP address. */ + struct qr_task *t = session_tasklist_get_first(s); + struct kr_query *qry = NULL; + if (t) { + struct kr_request *req = worker_task_request(t); + qry = array_tail(req->rplan.pending); + } + if (qry) /* We reuse the error for connection, as it's quite similar. */ + qry->server_selection.error(qry, worker_task_get_transport(t), + KR_SELECTION_TCP_CONNECT_FAILED); +} + knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm) { session->sflags.wirebuf_error = false; @@ -617,6 +630,7 @@ knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm) msg_size = knot_wire_read_u16(msg_start); if (msg_size >= session->wire_buf_size) { session->sflags.wirebuf_error = true; + session_tcp_penalize(session); return NULL; } if (msg_size + 2 > wirebuf_msg_data_size) { @@ -624,6 +638,7 @@ knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm) } if (msg_size == 0) { session->sflags.wirebuf_error = true; + session_tcp_penalize(session); return NULL; } msg_start += 2; @@ -631,6 +646,7 @@ knot_pkt_t *session_produce_packet(struct session *session, knot_mm_t *mm) msg_size = wirebuf_msg_data_size; } else { session->sflags.wirebuf_error = true; + session_tcp_penalize(session); return NULL; } diff --git a/daemon/session.h b/daemon/session.h index 603d7cb45..1f95ac5b6 100644 --- a/daemon/session.h +++ b/daemon/session.h @@ -91,8 +91,9 @@ int session_tasklist_finalize_expired(struct session *session); /** Both of task lists (associated & waiting). */ /** Check if empty. */ bool session_is_empty(const struct session *session); -/** Return whether session seems to have done something useful. */ -bool session_was_useful(const struct session *session); +/** Penalize this server if the session hasn't been useful (and is outgoing). */ +void session_tcp_penalize(struct session *session); + /** Get pointer to session flags */ struct session_flags *session_flags(struct session *session); /** Get pointer to peer address. */ diff --git a/meson.build b/meson.build index 41e04d10b..1dfc160b0 100644 --- a/meson.build +++ b/meson.build @@ -4,7 +4,7 @@ project( 'knot-resolver', ['c', 'cpp'], license: 'GPLv3+', - version: '5.6.0', + version: '5.7.0', default_options: ['c_std=gnu11', 'b_ndebug=true'], meson_version: '>=0.49', )