Stop using untrusted user input for determining outbound request permissions.

rylev · kate-goldenring · commit 292843df21c5 · 2024-05-06T17:04:25.000-07:00
[Spin outgoing http requests were using untrusted user input](https://github.com/fermyon/spin/blob/4987d4912a8c6ed766416f2d9544448d3f3bdfe4/crates/trigger-http/src/handler.rs#L367-L369) (namely the value of `Host` header) to determine the origin at which the Spin application is presumably running. Instead of taking the "Host" header as the authority for outbound requests, the host that the server is listening on is used instead.
diff --git a/crates/trigger-http/benches/baseline.rs b/crates/trigger-http/benches/baseline.rs
@@ -114,7 +114,12 @@ async fn run(trigger: &HttpTrigger, path: &str) {
         .body(Default::default())
         .unwrap();
     let resp = trigger
-        .handle(req, Scheme::HTTP, "127.0.0.1:55555".parse().unwrap())
+        .handle(
+            req,
+            Scheme::HTTP,
+            "127.0.0.1:3000".parse().unwrap(),
+            "127.0.0.1:55555".parse().unwrap(),
+        )
         .await
         .unwrap();
     assert_http_response_success(&resp);
diff --git a/crates/trigger-http/src/lib.rs b/crates/trigger-http/src/lib.rs
@@ -175,9 +175,9 @@ impl TriggerExecutor for HttpTrigger {
 
         let self_ = Arc::new(self);
         if let Some(tls) = tls {
-            self_.serve_tls(listener, tls).await?
+            self_.serve_tls(listener, listen_addr, tls).await?
         } else {
-            self_.serve(listener).await?
+            self_.serve(listener, listen_addr).await?
         };
 
         Ok(())
@@ -227,9 +227,10 @@ impl HttpTrigger {
         &self,
         mut req: Request<Body>,
         scheme: Scheme,
-        addr: SocketAddr,
+        server_addr: SocketAddr,
+        client_addr: SocketAddr,
     ) -> Result<Response<Body>> {
-        set_req_uri(&mut req, scheme)?;
+        set_req_uri(&mut req, scheme, server_addr)?;
         strip_forbidden_headers(&mut req);
 
         spin_telemetry::extract_trace_context(&req);
@@ -273,15 +274,27 @@ impl HttpTrigger {
                 let res = match executor {
                     HttpExecutorType::Http => {
                         HttpHandlerExecutor
-                            .execute(self.engine.clone(), &self.base, &route_match, req, addr)
+                            .execute(
+                                self.engine.clone(),
+                                &self.base,
+                                &route_match,
+                                req,
+                                client_addr,
+                            )
                             .await
                     }
                     HttpExecutorType::Wagi(wagi_config) => {
                         let executor = WagiHttpExecutor {
                             wagi_config: wagi_config.clone(),
                         };
                         executor
-                            .execute(self.engine.clone(), &self.base, &route_match, req, addr)
+                            .execute(
+                                self.engine.clone(),
+                                &self.base,
+                                &route_match,
+                                req,
+                                client_addr,
+                            )
                             .await
                     }
                 };
@@ -347,14 +360,18 @@ impl HttpTrigger {
     fn serve_connection<S: AsyncRead + AsyncWrite + Unpin + Send + 'static>(
         self: Arc<Self>,
         stream: S,
-        addr: SocketAddr,
+        server_addr: SocketAddr,
+        client_addr: SocketAddr,
     ) {
         task::spawn(async move {
             if let Err(e) = http1::Builder::new()
                 .keep_alive(true)
                 .serve_connection(
                     TokioIo::new(stream),
-                    service_fn(move |request| self.clone().instrumented_service_fn(addr, request)),
+                    service_fn(move |request| {
+                        self.clone()
+                            .instrumented_service_fn(server_addr, client_addr, request)
+                    }),
                 )
                 .await
             {
@@ -365,10 +382,11 @@ impl HttpTrigger {
 
     async fn instrumented_service_fn(
         self: Arc<Self>,
-        addr: SocketAddr,
+        server_addr: SocketAddr,
+        client_addr: SocketAddr,
         request: Request<Incoming>,
     ) -> Result<Response<HyperOutgoingBody>> {
-        let span = http_span!(request, addr);
+        let span = http_span!(request, client_addr);
         let method = request.method().to_string();
         async {
             let result = self
@@ -378,7 +396,8 @@ impl HttpTrigger {
                             .boxed()
                     }),
                     Scheme::HTTP,
-                    addr,
+                    server_addr,
+                    client_addr,
                 )
                 .await;
             finalize_http_span(result, method)
@@ -387,22 +406,28 @@ impl HttpTrigger {
         .await
     }
 
-    async fn serve(self: Arc<Self>, listener: TcpListener) -> Result<()> {
+    async fn serve(self: Arc<Self>, listener: TcpListener, listen_addr: SocketAddr) -> Result<()> {
         self.print_startup_msgs("http", &listener)?;
         loop {
-            let (stream, addr) = listener.accept().await?;
-            Self::serve_connection(self.clone(), stream, addr);
+            let (stream, client_addr) = listener.accept().await?;
+            self.clone()
+                .serve_connection(stream, listen_addr, client_addr);
         }
     }
 
-    async fn serve_tls(self: Arc<Self>, listener: TcpListener, tls: TlsConfig) -> Result<()> {
+    async fn serve_tls(
+        self: Arc<Self>,
+        listener: TcpListener,
+        listen_addr: SocketAddr,
+        tls: TlsConfig,
+    ) -> Result<()> {
         let acceptor = tls.server_config()?;
         self.print_startup_msgs("https", &listener)?;
 
         loop {
             let (stream, addr) = listener.accept().await?;
             match acceptor.accept(stream).await {
-                Ok(stream) => self.clone().serve_connection(stream, addr),
+                Ok(stream) => self.clone().serve_connection(stream, listen_addr, addr),
                 Err(err) => tracing::error!(?err, "Failed to start TLS session"),
             }
         }
@@ -440,21 +465,15 @@ fn parse_listen_addr(addr: &str) -> anyhow::Result<SocketAddr> {
     addrs.into_iter().next().context("couldn't resolve address")
 }
 
-fn set_req_uri(req: &mut Request<Body>, scheme: Scheme) -> Result<()> {
-    const DEFAULT_HOST: &str = "localhost";
-
-    let authority_hdr = req
-        .headers()
-        .get(http::header::HOST)
-        .map(|h| h.to_str().context("Expected UTF8 header value (authority)"))
-        .unwrap_or(Ok(DEFAULT_HOST))?;
+/// The incoming request's scheme and authority
+///
+/// The incoming request's URI is relative to the server, so we need to set the scheme and authority
+fn set_req_uri(req: &mut Request<Body>, scheme: Scheme, addr: SocketAddr) -> Result<()> {
     let uri = req.uri().clone();
     let mut parts = uri.into_parts();
-    parts.authority = authority_hdr
-        .parse()
-        .map(Option::Some)
-        .map_err(|e| anyhow::anyhow!("Invalid authority {:?}", e))?;
+    let authority = format!("{}:{}", addr.ip(), addr.port()).parse().unwrap();
     parts.scheme = Some(scheme);
+    parts.authority = Some(authority);
     *req.uri_mut() = Uri::from_parts(parts).unwrap();
     Ok(())
 }
diff --git a/tests/integration.rs b/tests/integration.rs
@@ -1395,4 +1395,30 @@ route = "/..."
 
         Ok(())
     }
+
+    #[test]
+    /// Test that the HOST header does not allow outbound http arbitrary calls to hosts
+    fn test_spin_inbound_http_host_header() -> anyhow::Result<()> {
+        run_test(
+            "outbound-http-to-same-app",
+            SpinAppType::Http,
+            [],
+            testing_framework::ServicesConfig::none(),
+            move |env| {
+                let spin = env.runtime_mut();
+                assert_spin_request(
+                    spin,
+                    Request::full(
+                        Method::GET,
+                        "/test/outbound-allowed/hello",
+                        &[("Host", "google.com")],
+                        Some(""),
+                    ),
+                    Response::new_with_body(200, "Hello, Fermyon!\n"),
+                )?;
+                Ok(())
+            },
+        )?;
+        Ok(())
+    }
 }
diff --git a/tests/testing-framework/src/runtimes/in_process_spin.rs b/tests/testing-framework/src/runtimes/in_process_spin.rs
@@ -33,7 +33,8 @@ impl InProcessSpin {
                 .handle(
                     req,
                     http::uri::Scheme::HTTP,
-                    (std::net::Ipv4Addr::LOCALHOST, 80).into(),
+                    (std::net::Ipv4Addr::LOCALHOST, 3000).into(),
+                    (std::net::Ipv4Addr::LOCALHOST, 7000).into(),
                 )
                 .await?;
             use http_body_util::BodyExt;
