From 6c399ca48869c387dfa97aa72b8dba4f6a8fddc5 Mon Sep 17 00:00:00 2001 From: Michael Gmelin Date: Sat, 17 Aug 2024 15:36:48 +0200 Subject: [PATCH 1/5] Prevent server from exiting on ECONNABORTED On FreeBSD, accept can fail with ECONNABORTED, which means that "A connection arrived, but it was on the listen queue" (see `man 2 accept`). Without this patch, a server without the tls feature exits returing 0 in this case, which makes it vulnerable not only to intentional denial of service, but also to unintentional crashes, e.g., by haproxy TCP health checks. The problem can be reproduced on any FreeBSD system by running the tonic "helloworld" example (without feature TLS) and then sending a packet using nmap: cd examples cargo run --bin helloworld-server --no-default-features & nmap -sT -p 50051 -6 ::1 # server exits When running the example with the feature tls enabled, it won't exit (as the tls event loop in tonic/src/transport/server/incoming.rs handles errors gracefully): cd examples cargo run --bin helloworld-server --no-default-features \ features=tls & nmap -sT -p 50051 -6 ::1 # server keeps running This patch is not optimal - it removes some generic error parameters to gain access to `std::io::Error::kind()`. The logic itself should be sound. See also: - https://man.freebsd.org/cgi/man.cgi?accept(2) Accept man page - https://github.com/giampaolo/pyftpdlib/issues/105 https://github.com/giampaolo/pyftpdlib/commit/0f822329e3431ec1bc6250c03e938c65a61b2eb4 Basically the same issue (and its fix) in another project --- tonic/src/transport/server/incoming.rs | 11 ++++++++--- tonic/src/transport/server/mod.rs | 22 ++++++++-------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/tonic/src/transport/server/incoming.rs b/tonic/src/transport/server/incoming.rs index fe578b2a5..907939314 100644 --- a/tonic/src/transport/server/incoming.rs +++ b/tonic/src/transport/server/incoming.rs @@ -16,17 +16,22 @@ use tokio_stream::{Stream, StreamExt}; use tracing::warn; #[cfg(not(feature = "tls"))] -pub(crate) fn tcp_incoming( - incoming: impl Stream>, +pub(crate) fn tcp_incoming( + incoming: impl Stream>, ) -> impl Stream, crate::Error>> where IO: AsyncRead + AsyncWrite + Unpin + Send + 'static, - IE: Into, { async_stream::try_stream! { let mut incoming = pin!(incoming); while let Some(item) = incoming.next().await { + if let Some(e) = item.as_ref().err() { + if e.kind() == std::io::ErrorKind::ConnectionAborted { + tracing::debug!(message = e.to_string(), error = %e); + continue; + } + } yield item.map(ServerIo::new_io)? } } diff --git a/tonic/src/transport/server/mod.rs b/tonic/src/transport/server/mod.rs index d2bfb0bbc..90c6238e9 100644 --- a/tonic/src/transport/server/mod.rs +++ b/tonic/src/transport/server/mod.rs @@ -487,7 +487,7 @@ impl Server { } } - pub(crate) async fn serve_with_shutdown( + pub(crate) async fn serve_with_shutdown( self, svc: S, incoming: I, @@ -500,10 +500,9 @@ impl Server { <>::Service as Service>>::Future: Send + 'static, <>::Service as Service>>::Error: Into + Send + 'static, - I: Stream>, + I: Stream>, IO: AsyncRead + AsyncWrite + Connected + Unpin + Send + 'static, IO::ConnectInfo: Clone + Send + Sync + 'static, - IE: Into, F: Future, ResBody: http_body::Body + Send + 'static, ResBody::Error: Into, @@ -733,7 +732,7 @@ impl Router { let incoming = TcpIncoming::new(addr, self.server.tcp_nodelay, self.server.tcp_keepalive) .map_err(super::Error::from_source)?; self.server - .serve_with_shutdown::<_, _, future::Ready<()>, _, _, ResBody>( + .serve_with_shutdown::<_, _, future::Ready<()>, _, ResBody>( self.routes.prepare(), incoming, None, @@ -775,15 +774,11 @@ impl Router { /// This method discards any provided [`Server`] TCP configuration. /// /// [`Server`]: struct.Server.html - pub async fn serve_with_incoming( - self, - incoming: I, - ) -> Result<(), super::Error> + pub async fn serve_with_incoming(self, incoming: I) -> Result<(), super::Error> where - I: Stream>, + I: Stream>, IO: AsyncRead + AsyncWrite + Connected + Unpin + Send + 'static, IO::ConnectInfo: Clone + Send + Sync + 'static, - IE: Into, L: Layer, L::Service: Service, Response = Response> + Clone + Send + 'static, @@ -794,7 +789,7 @@ impl Router { ResBody::Error: Into, { self.server - .serve_with_shutdown::<_, _, future::Ready<()>, _, _, ResBody>( + .serve_with_shutdown::<_, _, future::Ready<()>, _, ResBody>( self.routes.prepare(), incoming, None, @@ -810,16 +805,15 @@ impl Router { /// This method discards any provided [`Server`] TCP configuration. /// /// [`Server`]: struct.Server.html - pub async fn serve_with_incoming_shutdown( + pub async fn serve_with_incoming_shutdown( self, incoming: I, signal: F, ) -> Result<(), super::Error> where - I: Stream>, + I: Stream>, IO: AsyncRead + AsyncWrite + Connected + Unpin + Send + 'static, IO::ConnectInfo: Clone + Send + Sync + 'static, - IE: Into, F: Future, L: Layer, L::Service: From dd4ce1b27589ef28972534f2181c9bd0e2b708e4 Mon Sep 17 00:00:00 2001 From: Michael Gmelin Date: Mon, 19 Aug 2024 13:54:02 +0200 Subject: [PATCH 2/5] Handle ECONNABORTED without breaking APIs This simplifies the previous patch a lot. The string search is ugly (also not 100% if it's needed or if we could handle errors like in the TLS enabled accept loop). --- tonic/src/transport/server/incoming.rs | 19 +++++++++++-------- tonic/src/transport/server/mod.rs | 22 ++++++++++++++-------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/tonic/src/transport/server/incoming.rs b/tonic/src/transport/server/incoming.rs index 907939314..a932403e8 100644 --- a/tonic/src/transport/server/incoming.rs +++ b/tonic/src/transport/server/incoming.rs @@ -16,23 +16,26 @@ use tokio_stream::{Stream, StreamExt}; use tracing::warn; #[cfg(not(feature = "tls"))] -pub(crate) fn tcp_incoming( - incoming: impl Stream>, +pub(crate) fn tcp_incoming( + incoming: impl Stream>, ) -> impl Stream, crate::Error>> where IO: AsyncRead + AsyncWrite + Unpin + Send + 'static, + IE: Into, { async_stream::try_stream! { let mut incoming = pin!(incoming); while let Some(item) = incoming.next().await { - if let Some(e) = item.as_ref().err() { - if e.kind() == std::io::ErrorKind::ConnectionAborted { - tracing::debug!(message = e.to_string(), error = %e); - continue; - } + if item.is_err() { + let e: crate::Error = item.err().unwrap().into(); + tracing::debug!(message = "Accept loop error.", error = %e); + if !e.to_string().contains("os error 53") { + break; + } + } else { + yield item.map(ServerIo::new_io)? } - yield item.map(ServerIo::new_io)? } } } diff --git a/tonic/src/transport/server/mod.rs b/tonic/src/transport/server/mod.rs index 90c6238e9..d2bfb0bbc 100644 --- a/tonic/src/transport/server/mod.rs +++ b/tonic/src/transport/server/mod.rs @@ -487,7 +487,7 @@ impl Server { } } - pub(crate) async fn serve_with_shutdown( + pub(crate) async fn serve_with_shutdown( self, svc: S, incoming: I, @@ -500,9 +500,10 @@ impl Server { <>::Service as Service>>::Future: Send + 'static, <>::Service as Service>>::Error: Into + Send + 'static, - I: Stream>, + I: Stream>, IO: AsyncRead + AsyncWrite + Connected + Unpin + Send + 'static, IO::ConnectInfo: Clone + Send + Sync + 'static, + IE: Into, F: Future, ResBody: http_body::Body + Send + 'static, ResBody::Error: Into, @@ -732,7 +733,7 @@ impl Router { let incoming = TcpIncoming::new(addr, self.server.tcp_nodelay, self.server.tcp_keepalive) .map_err(super::Error::from_source)?; self.server - .serve_with_shutdown::<_, _, future::Ready<()>, _, ResBody>( + .serve_with_shutdown::<_, _, future::Ready<()>, _, _, ResBody>( self.routes.prepare(), incoming, None, @@ -774,11 +775,15 @@ impl Router { /// This method discards any provided [`Server`] TCP configuration. /// /// [`Server`]: struct.Server.html - pub async fn serve_with_incoming(self, incoming: I) -> Result<(), super::Error> + pub async fn serve_with_incoming( + self, + incoming: I, + ) -> Result<(), super::Error> where - I: Stream>, + I: Stream>, IO: AsyncRead + AsyncWrite + Connected + Unpin + Send + 'static, IO::ConnectInfo: Clone + Send + Sync + 'static, + IE: Into, L: Layer, L::Service: Service, Response = Response> + Clone + Send + 'static, @@ -789,7 +794,7 @@ impl Router { ResBody::Error: Into, { self.server - .serve_with_shutdown::<_, _, future::Ready<()>, _, ResBody>( + .serve_with_shutdown::<_, _, future::Ready<()>, _, _, ResBody>( self.routes.prepare(), incoming, None, @@ -805,15 +810,16 @@ impl Router { /// This method discards any provided [`Server`] TCP configuration. /// /// [`Server`]: struct.Server.html - pub async fn serve_with_incoming_shutdown( + pub async fn serve_with_incoming_shutdown( self, incoming: I, signal: F, ) -> Result<(), super::Error> where - I: Stream>, + I: Stream>, IO: AsyncRead + AsyncWrite + Connected + Unpin + Send + 'static, IO::ConnectInfo: Clone + Send + Sync + 'static, + IE: Into, F: Future, L: Layer, L::Service: From 355b8b41d2a1159c5e802876a11a710de8e16de2 Mon Sep 17 00:00:00 2001 From: Michael Gmelin Date: Mon, 19 Aug 2024 14:43:24 +0200 Subject: [PATCH 3/5] Next iteration based on feedback --- tonic/src/transport/server/incoming.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tonic/src/transport/server/incoming.rs b/tonic/src/transport/server/incoming.rs index a932403e8..4723137f1 100644 --- a/tonic/src/transport/server/incoming.rs +++ b/tonic/src/transport/server/incoming.rs @@ -27,12 +27,16 @@ where let mut incoming = pin!(incoming); while let Some(item) = incoming.next().await { - if item.is_err() { - let e: crate::Error = item.err().unwrap().into(); - tracing::debug!(message = "Accept loop error.", error = %e); - if !e.to_string().contains("os error 53") { - break; + if let Err(e) = item { + if let Some(e) = Into::::into(e).downcast_ref::() { + tracing::debug!(message = "Accept loop error.", error = %e); + if e.kind() == std::io::ErrorKind::ConnectionAborted { + continue; + } + } else { + tracing::debug!(message = "Accept loop error (unknown error)."); } + break; } else { yield item.map(ServerIo::new_io)? } From bc7196da915e5ee87750914b2a561478fe287e8c Mon Sep 17 00:00:00 2001 From: Michael Gmelin Date: Tue, 20 Aug 2024 02:33:06 +0200 Subject: [PATCH 4/5] More review feedback --- tonic/src/transport/server/incoming.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tonic/src/transport/server/incoming.rs b/tonic/src/transport/server/incoming.rs index 4723137f1..c3701d0c2 100644 --- a/tonic/src/transport/server/incoming.rs +++ b/tonic/src/transport/server/incoming.rs @@ -2,6 +2,7 @@ use super::service::ServerIo; #[cfg(feature = "tls")] use super::service::TlsAcceptor; use std::{ + io, net::{SocketAddr, TcpListener as StdTcpListener}, pin::{pin, Pin}, task::{ready, Context, Poll}, @@ -27,18 +28,18 @@ where let mut incoming = pin!(incoming); while let Some(item) = incoming.next().await { - if let Err(e) = item { - if let Some(e) = Into::::into(e).downcast_ref::() { - tracing::debug!(message = "Accept loop error.", error = %e); - if e.kind() == std::io::ErrorKind::ConnectionAborted { - continue; + yield match item { + Ok(_) => item.map(ServerIo::new_io)?, + Err(e) => { + let e = e.into(); + tracing::debug!(error = %e, "accept loop error"); + if let Some(e) = e.downcast_ref::() { + if e.kind() == io::ErrorKind::ConnectionAborted { + continue; + } } - } else { - tracing::debug!(message = "Accept loop error (unknown error)."); + Err(e)? } - break; - } else { - yield item.map(ServerIo::new_io)? } } } @@ -77,7 +78,7 @@ where } SelectOutput::Err(e) => { - tracing::debug!(message = "Accept loop error.", error = %e); + tracing::debug!(error = %e, "accept loop error"); } SelectOutput::Done => { From 25b3b7ae2a41ff3668eee3d79406d9e36b12ea6d Mon Sep 17 00:00:00 2001 From: Michael Gmelin Date: Tue, 20 Aug 2024 03:21:05 +0200 Subject: [PATCH 5/5] Only use std::io if needed --- tonic/src/transport/server/incoming.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tonic/src/transport/server/incoming.rs b/tonic/src/transport/server/incoming.rs index c3701d0c2..b7d2f8d6c 100644 --- a/tonic/src/transport/server/incoming.rs +++ b/tonic/src/transport/server/incoming.rs @@ -1,8 +1,9 @@ use super::service::ServerIo; #[cfg(feature = "tls")] use super::service::TlsAcceptor; +#[cfg(not(feature = "tls"))] +use std::io; use std::{ - io, net::{SocketAddr, TcpListener as StdTcpListener}, pin::{pin, Pin}, task::{ready, Context, Poll},