daemon: more avoidance of excessive TCP reconnections

Previously this penalization was only triggered if the remote server
closed TCP.  Now it's extended to us closing it when the server
(only) sends back some nonsense.  At least for the cases which I could
see immediately.

That's just three trivial one-line additions; the rest is refactoring.

diff --git a/NEWS b/NEWS
index 7453ee6851c7b0f7787c2b642e04c864e0a6212a..42fabe2da5256978f4e47e6a340c7442efb8bbc5 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -1,6 +1,14 @@
 Knot Resolver 5.7.0 (2023-0m-dd)
 ================================
 
+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 27119adbf0384c62207d9437f521180ee16809cc..6d548d7778c6c636d58be0c4c23e1a4e53dfc25c 100644 (file)
--- 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 a1f22072b0ccf89a01262ab397d6abbf507358c1..ed0ff6869472cb37eb58a4c12852e01c44ea0c57 100644 (file)
--- 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 603d7cb450f2cc72f90839d9ad26e4570fb2890e..1f95ac5b6888ea61c214f373b9894fe76aaf82cd 100644 (file)
--- 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. */