Re: [pbs-devel] [PATCH v2 proxmox 3/3] fix #5105: rest-server: connection: overhaul TLS handshake check logic

["Max Carrara" <m.carrara@proxmox.com>]

On Tue Jul 9, 2024 at 12:29 PM CEST, Wolfgang Bumiller wrote:
> On Mon, Jul 08, 2024 at 06:48:17PM GMT, Max Carrara wrote:
> > On rare occasions, the TLS "client hello" message [1] is delayed after
> > a connection with the server was established, which causes HTTPS
> > requests to fail before TLS was even negotiated. In these cases, the
> > server would incorrectly respond with "HTTP/1.1 400 Bad Request"
> > instead of closing the connection (or similar).
> > 
> > The reasons for the "client hello" being delayed seem to vary; one
> > user noticed that the issue went away completely after they turned off
> > UFW [2]. Another user noticed (during private correspondence) that the
> > issue only appeared when connecting to their PBS instance via WAN, but
> > not from within their VPN. In the WAN case a firewall was also
> > present. The same user kindly provided tcpdumps and strace logs on
> > request.
> > 
> > The issue was finally reproduced with the following Python script:
> > 
> >   import socket
> >   import time
> > 
> >   HOST: str = ...
> >   PORT: int = ...
> > 
> >   with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
> >       sock.connect((HOST, PORT))
> >       time.sleep(1.5) # simulate firewall / proxy / etc. delay
> >       sock.sendall(b"\x16\x03\x01\x02\x00")
> >       data = sock.recv(256)
> >       print(data)
> > 
> > The additional delay before sending the first 5 bytes of the "client
> > hello" message causes the handshake checking logic to incorrectly fall
> > back to plain HTTP.
> > 
> > All of this is fixed by the following:
> > 
> >   1. Increase the timeout duration to 10 seconds (from 1)
> >   2. Instead of falling back to plain HTTP, refuse to accept the
> >      connection if the TLS handshake wasn't initiated before the
> >      timeout limit is reached
> >   3. Only accept plain HTTP if the first 5 bytes do not correspond to
> >      a TLS handshake fragment [3]
> >   4. Do not take the last number of bytes that were in the buffer into
> >      account; instead, only perform the actual handshake check if
> >      5 bytes are in the peek buffer using some of tokio's low-level
> >      functionality
> > 
> > Regarding 1.: This should be generous enough for any client to be able
> > to initiate a TLS handshake, despite its surrounding circumstances.
> > 
> > Regarding 4.: While this is not 100% related to the issue, peeking into
> > the buffer in this manner should ensure that our implementation here
> > remains correct, even if the kernel's underlying behaviour regarding
> > edge-triggering is changed [4]. At the same time, there's no need for
> > busy-waiting and continuously yielding to the event loop anymore.
> > 
> > [1]: https://www.rfc-editor.org/rfc/rfc8446.html#section-4.1.2
> > [2]: https://forum.proxmox.com/threads/disable-default-http-redirects-on-8007.142312/post-675352
> > [3]: https://www.rfc-editor.org/rfc/rfc8446.html#section-5.1
> > [4]: https://lwn.net/Articles/864947/
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> > Changes v1 --> v2:
> >   * use `socket.async_io` instead of `socket.peek` and rework the future
> >     that performs the handshake check
> >   * adapt commit message
> >   * fix reference in commit message
> > 
> >  proxmox-rest-server/src/connection.rs | 110 ++++++++++++++------------
> >  1 file changed, 61 insertions(+), 49 deletions(-)
> > 
> > diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
> > index 470021d7..63fa8640 100644
> > --- a/proxmox-rest-server/src/connection.rs
> > +++ b/proxmox-rest-server/src/connection.rs
> > @@ -2,7 +2,9 @@
> >  //!
> >  //! Hyper building block.
> >  
> > +use std::io;
> >  use std::net::SocketAddr;
> > +use std::os::fd::FromRawFd;
> >  use std::os::unix::io::AsRawFd;
> >  use std::path::PathBuf;
> >  use std::pin::Pin;
> > @@ -418,70 +420,80 @@ impl AcceptBuilder {
> >          secure_sender: ClientSender,
> >          insecure_sender: InsecureClientSender,
> >      ) {
> > +        const CLIENT_HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(10);
> > +
> >          let peer = state.peer;
> >  
> > -        let client_initiates_handshake = {
> > -            #[cfg(feature = "rate-limited-stream")]
> > -            let socket_ref = state.socket.inner();
> > +        #[cfg(feature = "rate-limited-stream")]
> > +        let socket_ref = state.socket.inner();
> >  
> > -            #[cfg(not(feature = "rate-limited-stream"))]
> > -            let socket_ref = &state.socket;
> > +        #[cfg(not(feature = "rate-limited-stream"))]
> > +        let socket_ref = &state.socket;
> >  
> > -            match Self::wait_for_client_tls_handshake(socket_ref).await {
> > -                Ok(initiates_handshake) => initiates_handshake,
> > -                Err(err) => {
> > -                    log::error!("[{peer}] error checking for TLS handshake: {err}");
> > -                    return;
> > -                }
> > -            }
> > -        };
> > -
> > -        if !client_initiates_handshake {
> > -            let insecure_stream = Box::pin(state.socket);
> > +        let handshake_res =
> > +            Self::wait_for_client_tls_handshake(socket_ref, CLIENT_HANDSHAKE_TIMEOUT).await;
> >  
> > -            if insecure_sender.send(Ok(insecure_stream)).await.is_err() && flags.is_debug {
> > -                log::error!("[{peer}] detected closed connection channel")
> > +        match handshake_res {
> > +            Ok(true) => {
> > +                Self::do_accept_tls(state, flags, secure_sender).await;
> >              }
> > +            Ok(false) => {
> > +                let insecure_stream = Box::pin(state.socket);
> >  
> > -            return;
> > +                if let Err(send_err) = insecure_sender.send(Ok(insecure_stream)).await {
> > +                    log::error!("[{peer}] failed to accept connection - connection channel closed: {send_err}");
> > +                }
> > +            }
> > +            Err(err) => {
> > +                log::error!("[{peer}] failed to check for TLS handshake: {err}");
> > +            }
> >          }
> > -
> > -        Self::do_accept_tls(state, flags, secure_sender).await
> >      }
> >  
> > -    async fn wait_for_client_tls_handshake(incoming_stream: &TcpStream) -> Result<bool, Error> {
> > -        const MS_TIMEOUT: u64 = 1000;
> > -        const BYTES_BUF_SIZE: usize = 128;
> > -
> > -        let mut buf = [0; BYTES_BUF_SIZE];
> > -        let mut last_peek_size = 0;
> > +    async fn wait_for_client_tls_handshake(
> > +        incoming_stream: &TcpStream,
> > +        timeout: Duration,
> > +    ) -> Result<bool, Error> {
> > +        const HANDSHAKE_BYTES_LEN: usize = 5;
> >  
> >          let future = async {
> > -            loop {
> > -                let peek_size = incoming_stream
> > -                    .peek(&mut buf)
> > -                    .await
> > -                    .context("couldn't peek into incoming tcp stream")?;
> > -
> > -                if contains_tls_handshake_fragment(&buf) {
> > -                    return Ok(true);
> > -                }
> > -
> > -                // No more new data came in
> > -                if peek_size == last_peek_size {
> > -                    return Ok(false);
> > -                }
> > -
> > -                last_peek_size = peek_size;
> > -
> > -                // explicitly yield to event loop; this future otherwise blocks ad infinitum
> > -                tokio::task::yield_now().await;
> > -            }
> > +            incoming_stream
> > +                .async_io(tokio::io::Interest::READABLE, || {
> > +                    let mut buf = [0; HANDSHAKE_BYTES_LEN];
> > +
> > +                    // Convert to standard lib TcpStream so we can peek without interfering
> > +                    // with tokio's internals
> > +                    let raw_fd = incoming_stream.as_raw_fd();
> > +                    let std_stream = unsafe { std::net::TcpStream::from_raw_fd(raw_fd) };
> > +
> > +                    let peek_res = std_stream.peek(&mut buf);
> > +
> > +                    // Prevent destructor from being called, closing the connection
> > +                    // and messing up invariants
> > +                    std::mem::forget(std_stream);
>
> ^ Even better would be to use `std::mem::ManuallyDrop` around
> `std_stream`, then we can just forget about it.

Good point, thanks! Will send in a v3 soon.

>
> > +
> > +                    match peek_res {
> > +                        // If we didn't get enough bytes, raise an EAGAIN / EWOULDBLOCK which tells
> > +                        // tokio to await the readiness of the socket again. This should normally
> > +                        // only be used if the socket isn't actually ready, but is fine to do here
> > +                        // in our case.
> > +                        //
> > +                        // This means we will peek into the stream's queue until we got
> > +                        // HANDSHAKE_BYTE_LEN bytes or an error.
> > +                        Ok(peek_len) if peek_len < HANDSHAKE_BYTES_LEN => {
> > +                            Err(io::ErrorKind::WouldBlock.into())
> > +                        }
> > +                        // Either we got Ok(HANDSHAKE_BYTES_LEN) or some error.
> > +                        res => res.map(|_| contains_tls_handshake_fragment(&buf)),
> > +                    }
> > +                })
> > +                .await
> > +                .context("couldn't peek into incoming TCP stream")
> >          };
> >  
> > -        tokio::time::timeout(Duration::from_millis(MS_TIMEOUT), future)
> > +        tokio::time::timeout(timeout, future)
> >              .await
> > -            .unwrap_or(Ok(false))
> > +            .context("timed out while waiting for client to initiate TLS handshake")?
> >      }
> >  }
> >  
> > -- 
> > 2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel