Skip to content

Commit b3db535

Browse files
lannlann
authored
Merge pull request from GHSA-f3h7-gpjj-wcvh
Stop using untrusted user input for determining outbound request permissions
2 parents 7b8cd7f + 292843d commit b3db535

File tree

4 files changed

+81
-30
lines changed

4 files changed

+81
-30
lines changed

‎crates/trigger-http/benches/baseline.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,12 @@ async fn run(trigger: &HttpTrigger, path: &str) {
114114
.body(Default::default())
115115
.unwrap();
116116
let resp = trigger
117-
.handle(req, Scheme::HTTP, "127.0.0.1:55555".parse().unwrap())
117+
.handle(
118+
req,
119+
Scheme::HTTP,
120+
"127.0.0.1:3000".parse().unwrap(),
121+
"127.0.0.1:55555".parse().unwrap(),
122+
)
118123
.await
119124
.unwrap();
120125
assert_http_response_success(&resp);

‎crates/trigger-http/src/lib.rs

Lines changed: 47 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -175,9 +175,9 @@ impl TriggerExecutor for HttpTrigger {
175175

176176
let self_ = Arc::new(self);
177177
if let Some(tls) = tls {
178-
self_.serve_tls(listener, tls).await?
178+
self_.serve_tls(listener, listen_addr, tls).await?
179179
} else {
180-
self_.serve(listener).await?
180+
self_.serve(listener, listen_addr).await?
181181
};
182182

183183
Ok(())
@@ -227,9 +227,10 @@ impl HttpTrigger {
227227
&self,
228228
mut req: Request<Body>,
229229
scheme: Scheme,
230-
addr: SocketAddr,
230+
server_addr: SocketAddr,
231+
client_addr: SocketAddr,
231232
) -> Result<Response<Body>> {
232-
set_req_uri(&mut req, scheme)?;
233+
set_req_uri(&mut req, scheme, server_addr)?;
233234
strip_forbidden_headers(&mut req);
234235

235236
spin_telemetry::extract_trace_context(&req);
@@ -273,15 +274,27 @@ impl HttpTrigger {
273274
let res = match executor {
274275
HttpExecutorType::Http => {
275276
HttpHandlerExecutor
276-
.execute(self.engine.clone(), &self.base, &route_match, req, addr)
277+
.execute(
278+
self.engine.clone(),
279+
&self.base,
280+
&route_match,
281+
req,
282+
client_addr,
283+
)
277284
.await
278285
}
279286
HttpExecutorType::Wagi(wagi_config) => {
280287
let executor = WagiHttpExecutor {
281288
wagi_config: wagi_config.clone(),
282289
};
283290
executor
284-
.execute(self.engine.clone(), &self.base, &route_match, req, addr)
291+
.execute(
292+
self.engine.clone(),
293+
&self.base,
294+
&route_match,
295+
req,
296+
client_addr,
297+
)
285298
.await
286299
}
287300
};
@@ -347,14 +360,18 @@ impl HttpTrigger {
347360
fn serve_connection<S: AsyncRead + AsyncWrite + Unpin + Send + 'static>(
348361
self: Arc<Self>,
349362
stream: S,
350-
addr: SocketAddr,
363+
server_addr: SocketAddr,
364+
client_addr: SocketAddr,
351365
) {
352366
task::spawn(async move {
353367
if let Err(e) = http1::Builder::new()
354368
.keep_alive(true)
355369
.serve_connection(
356370
TokioIo::new(stream),
357-
service_fn(move |request| self.clone().instrumented_service_fn(addr, request)),
371+
service_fn(move |request| {
372+
self.clone()
373+
.instrumented_service_fn(server_addr, client_addr, request)
374+
}),
358375
)
359376
.await
360377
{
@@ -365,10 +382,11 @@ impl HttpTrigger {
365382

366383
async fn instrumented_service_fn(
367384
self: Arc<Self>,
368-
addr: SocketAddr,
385+
server_addr: SocketAddr,
386+
client_addr: SocketAddr,
369387
request: Request<Incoming>,
370388
) -> Result<Response<HyperOutgoingBody>> {
371-
let span = http_span!(request, addr);
389+
let span = http_span!(request, client_addr);
372390
let method = request.method().to_string();
373391
async {
374392
let result = self
@@ -378,7 +396,8 @@ impl HttpTrigger {
378396
.boxed()
379397
}),
380398
Scheme::HTTP,
381-
addr,
399+
server_addr,
400+
client_addr,
382401
)
383402
.await;
384403
finalize_http_span(result, method)
@@ -387,22 +406,28 @@ impl HttpTrigger {
387406
.await
388407
}
389408

390-
async fn serve(self: Arc<Self>, listener: TcpListener) -> Result<()> {
409+
async fn serve(self: Arc<Self>, listener: TcpListener, listen_addr: SocketAddr) -> Result<()> {
391410
self.print_startup_msgs("http", &listener)?;
392411
loop {
393-
let (stream, addr) = listener.accept().await?;
394-
Self::serve_connection(self.clone(), stream, addr);
412+
let (stream, client_addr) = listener.accept().await?;
413+
self.clone()
414+
.serve_connection(stream, listen_addr, client_addr);
395415
}
396416
}
397417

398-
async fn serve_tls(self: Arc<Self>, listener: TcpListener, tls: TlsConfig) -> Result<()> {
418+
async fn serve_tls(
419+
self: Arc<Self>,
420+
listener: TcpListener,
421+
listen_addr: SocketAddr,
422+
tls: TlsConfig,
423+
) -> Result<()> {
399424
let acceptor = tls.server_config()?;
400425
self.print_startup_msgs("https", &listener)?;
401426

402427
loop {
403428
let (stream, addr) = listener.accept().await?;
404429
match acceptor.accept(stream).await {
405-
Ok(stream) => self.clone().serve_connection(stream, addr),
430+
Ok(stream) => self.clone().serve_connection(stream, listen_addr, addr),
406431
Err(err) => tracing::error!(?err, "Failed to start TLS session"),
407432
}
408433
}
@@ -440,21 +465,15 @@ fn parse_listen_addr(addr: &str) -> anyhow::Result<SocketAddr> {
440465
addrs.into_iter().next().context("couldn't resolve address")
441466
}
442467

443-
fn set_req_uri(req: &mut Request<Body>, scheme: Scheme) -> Result<()> {
444-
const DEFAULT_HOST: &str = "localhost";
445-
446-
let authority_hdr = req
447-
.headers()
448-
.get(http::header::HOST)
449-
.map(|h| h.to_str().context("Expected UTF8 header value (authority)"))
450-
.unwrap_or(Ok(DEFAULT_HOST))?;
468+
/// The incoming request's scheme and authority
469+
///
470+
/// The incoming request's URI is relative to the server, so we need to set the scheme and authority
471+
fn set_req_uri(req: &mut Request<Body>, scheme: Scheme, addr: SocketAddr) -> Result<()> {
451472
let uri = req.uri().clone();
452473
let mut parts = uri.into_parts();
453-
parts.authority = authority_hdr
454-
.parse()
455-
.map(Option::Some)
456-
.map_err(|e| anyhow::anyhow!("Invalid authority {:?}", e))?;
474+
let authority = format!("{}:{}", addr.ip(), addr.port()).parse().unwrap();
457475
parts.scheme = Some(scheme);
476+
parts.authority = Some(authority);
458477
*req.uri_mut() = Uri::from_parts(parts).unwrap();
459478
Ok(())
460479
}

‎tests/integration.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1395,4 +1395,30 @@ route = "/..."
13951395

13961396
Ok(())
13971397
}
1398+
1399+
#[test]
1400+
/// Test that the HOST header does not allow outbound http arbitrary calls to hosts
1401+
fn test_spin_inbound_http_host_header() -> anyhow::Result<()> {
1402+
run_test(
1403+
"outbound-http-to-same-app",
1404+
SpinAppType::Http,
1405+
[],
1406+
testing_framework::ServicesConfig::none(),
1407+
move |env| {
1408+
let spin = env.runtime_mut();
1409+
assert_spin_request(
1410+
spin,
1411+
Request::full(
1412+
Method::GET,
1413+
"/test/outbound-allowed/hello",
1414+
&[("Host", "google.com")],
1415+
Some(""),
1416+
),
1417+
Response::new_with_body(200, "Hello, Fermyon!\n"),
1418+
)?;
1419+
Ok(())
1420+
},
1421+
)?;
1422+
Ok(())
1423+
}
13981424
}

‎tests/testing-framework/src/runtimes/in_process_spin.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@ impl InProcessSpin {
3333
.handle(
3434
req,
3535
http::uri::Scheme::HTTP,
36-
(std::net::Ipv4Addr::LOCALHOST, 80).into(),
36+
(std::net::Ipv4Addr::LOCALHOST, 3000).into(),
37+
(std::net::Ipv4Addr::LOCALHOST, 7000).into(),
3738
)
3839
.await?;
3940
use http_body_util::BodyExt;

0 commit comments

Comments
 (0)