diff --git a/caddy/caddy.go b/caddy/caddy.go index 9cbc219f3d..e1d285a558 100644 --- a/caddy/caddy.go +++ b/caddy/caddy.go @@ -19,6 +19,11 @@ const ( // reads when the directive is omitted; mirrors nginx's client_body_timeout. // Set request_body_timeout to 0 to disable. defaultRequestBodyTimeout = caddy.Duration(60 * time.Second) + + // defaultResponseWriteTimeout is the idle timeout applied to response + // writes when the directive is omitted; mirrors nginx's send_timeout. + // Set response_write_timeout to 0 to disable. + defaultResponseWriteTimeout = caddy.Duration(60 * time.Second) ) func init() { diff --git a/caddy/config_test.go b/caddy/config_test.go index dbe6a82d58..ad6efb2b5c 100644 --- a/caddy/config_test.go +++ b/caddy/config_test.go @@ -129,6 +129,34 @@ func TestModuleRequestBodyTimeoutDisabled(t *testing.T) { require.Equal(t, caddy.Duration(0), *module.RequestBodyTimeout) } +func TestModuleResponseWriteTimeout(t *testing.T) { + d := caddyfile.NewTestDispenser(` + { + php { + response_write_timeout 5s + } + }`) + module := &FrankenPHPModule{} + + require.NoError(t, module.UnmarshalCaddyfile(d)) + require.NotNil(t, module.ResponseWriteTimeout) + require.Equal(t, caddy.Duration(5*time.Second), *module.ResponseWriteTimeout) +} + +func TestModuleResponseWriteTimeoutDisabled(t *testing.T) { + d := caddyfile.NewTestDispenser(` + { + php { + response_write_timeout 0 + } + }`) + module := &FrankenPHPModule{} + + require.NoError(t, module.UnmarshalCaddyfile(d)) + require.NotNil(t, module.ResponseWriteTimeout) + require.Equal(t, caddy.Duration(0), *module.ResponseWriteTimeout) +} + func TestModuleWorkerDuplicateFilenamesFail(t *testing.T) { // Create a test configuration with duplicate worker filenames configWithDuplicateFilenames := ` diff --git a/caddy/module.go b/caddy/module.go index 02d410155b..6051a57540 100644 --- a/caddy/module.go +++ b/caddy/module.go @@ -50,6 +50,8 @@ type FrankenPHPModule struct { RouteGroup string `json:"route_group,omitempty"` // RequestBodyTimeout is an idle timeout on request body reads: a stalled (slow POST) client is cut off while a steady upload of any size succeeds. Defaults to 60s when omitted; set to 0 to disable. RequestBodyTimeout *caddy.Duration `json:"request_body_timeout,omitempty"` + // ResponseWriteTimeout is an idle timeout on response writes: a stalled or dead client is cut off instead of blocking the PHP thread forever. Defaults to 60s when omitted; set to 0 to disable. + ResponseWriteTimeout *caddy.Duration `json:"response_write_timeout,omitempty"` resolvedDocumentRoot string preparedEnv frankenphp.PreparedEnv @@ -137,6 +139,14 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { f.requestOptions = append(f.requestOptions, frankenphp.WithRequestBodyTimeout(time.Duration(*f.RequestBodyTimeout))) } + if f.ResponseWriteTimeout == nil { + f.ResponseWriteTimeout = new(defaultResponseWriteTimeout) + } + + if *f.ResponseWriteTimeout > 0 { + f.requestOptions = append(f.requestOptions, frankenphp.WithResponseWriteTimeout(time.Duration(*f.ResponseWriteTimeout))) + } + if f.ResolveRootSymlink == nil { f.ResolveRootSymlink = new(true) } @@ -337,8 +347,21 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { } f.RequestBodyTimeout = new(caddy.Duration(v)) + case "response_write_timeout": + if !d.NextArg() { + return d.ArgErr() + } + v, err := caddy.ParseDuration(d.Val()) + if err != nil { + return err + } + if d.NextArg() { + return d.ArgErr() + } + f.ResponseWriteTimeout = new(caddy.Duration(v)) + default: - return wrongSubDirectiveError("php or php_server", "hot_reload, name, root, split, env, resolve_root_symlink, request_body_timeout, worker", d.Val()) + return wrongSubDirectiveError("php or php_server", "hot_reload, name, root, split, env, resolve_root_symlink, request_body_timeout, response_write_timeout, worker", d.Val()) } } } diff --git a/context.go b/context.go index c582c6e453..f2f1fe6356 100644 --- a/context.go +++ b/context.go @@ -26,6 +26,8 @@ type frankenPHPContext struct { // idle timeout per body read; zero disables it requestBodyTimeout time.Duration + // idle timeout per response write; zero disables it + responseWriteTimeout time.Duration docURI string pathInfo string diff --git a/docs/config.md b/docs/config.md index 9f5e1b19e2..b1b7530b0e 100644 --- a/docs/config.md +++ b/docs/config.md @@ -190,6 +190,7 @@ php_server [] { env # Sets an extra environment variable to the given value. Can be specified more than once for multiple environment variables. file_server off # Disables the built-in file_server directive. request_body_timeout # Sets an idle timeout on request body reads: a stalled (slow POST) client is cut off while a steady upload of any size succeeds. Default: 60s. Set to 0 to disable. + response_write_timeout # Sets an idle timeout on response writes: a stalled or dead client is cut off instead of blocking the PHP thread forever. Default: 60s. Set to 0 to disable. worker { # Creates a worker specific to this server. Can be specified more than once for multiple workers. file # Sets the path to the worker script, can be relative to the php_server root num # Sets the number of PHP threads to start, defaults to 2x the number of available diff --git a/docs/security.md b/docs/security.md index 819767ef43..de6efb8551 100644 --- a/docs/security.md +++ b/docs/security.md @@ -52,6 +52,7 @@ These are the surfaces FrankenPHP owns. A vulnerability here is a FrankenPHP vul - **Caddy admin API**: the `/frankenphp/workers/restart` and `/frankenphp/threads` endpoints, exposed through Caddy's admin API (which listens on `localhost:2019` by default). Exposing that endpoint beyond localhost is an operator decision. - **Trusted proxy handling**: incoming `X-Forwarded-*` headers always reach PHP as tainted `$_SERVER['HTTP_X_FORWARDED_*']` values; they are only trusted to derive the real client IP and scheme when [`trusted_proxies`](production.md#running-behind-a-reverse-proxy) is configured. - **Slow request bodies**: a client that announces a body then dribbles or stalls it holds the handling thread for the duration. With a bounded thread pool, enough such connections exhaust it (slow-POST DoS). FrankenPHP applies a 60s idle timeout on body reads by default ([`request_body_timeout`](config.md#caddyfile-config)), resetting the deadline before each read so a steady upload of any size succeeds while a stalled one is cut off and the thread released. +- **Stalled response writes**: a client that stops reading the response (slow-read DoS) holds the handling thread in a blocking write indefinitely: `frankenphp_force_kill_thread` cannot interrupt it, since the stall isn't a blocking syscall but a write parked in Go's non-blocking netpoller. FrankenPHP applies a 60s idle timeout on response writes by default ([`response_write_timeout`](config.md#caddyfile-config)), resetting the deadline before each write so the thread is released once a peer goes unresponsive. ## Out of scope diff --git a/frankenphp.go b/frankenphp.go index ad2dedc42a..12a44cf374 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -437,23 +437,46 @@ func go_ub_write(threadIndex C.uintptr_t, cBuf *C.char, length C.size_t) (C.size } var writer io.Writer + var rc *http.ResponseController if fc.responseWriter == nil { var b bytes.Buffer // log the output of the worker writer = &b } else { writer = fc.responseWriter + + if fc.responseWriteTimeout > 0 { + if fc.responseController == nil { + fc.responseController = http.NewResponseController(fc.responseWriter) + } + rc = fc.responseController + _ = rc.SetWriteDeadline(time.Now().Add(fc.responseWriteTimeout)) + } } var ctx context.Context i, e := writer.Write(unsafe.Slice((*byte)(unsafe.Pointer(cBuf)), length)) + + if rc != nil { + _ = rc.SetWriteDeadline(time.Time{}) + } + if e != nil { ctx = thread.context() if fc.logger.Enabled(ctx, slog.LevelWarn) { fc.logger.LogAttrs(ctx, slog.LevelWarn, "write error", slog.Any("error", e)) } + + if errors.Is(e, os.ErrDeadlineExceeded) { + // The peer stopped reading and the write timeout fired: treat this + // exactly like a genuine disconnect (rather than fc.clientHasClosed(), + // which only reflects the request context and won't have fired yet) so + // php_handle_aborted_connection() runs instead of the script quietly + // continuing to write into a stalled connection on every future write. + return C.size_t(i), C.bool(true) + } } if fc.responseWriter == nil { diff --git a/requestoptions.go b/requestoptions.go index e6df428945..58914fdb89 100644 --- a/requestoptions.go +++ b/requestoptions.go @@ -192,6 +192,20 @@ func WithRequestBodyTimeout(timeout time.Duration) RequestOption { } } +// WithResponseWriteTimeout sets an idle timeout on response writes: a client +// that stops reading (a stalled or dead peer) is cut off instead of blocking +// the PHP thread forever, since frankenphp_force_kill_thread cannot interrupt +// a write parked in Go's non-blocking netpoller. Zero (the default) disables +// it. Requires a ResponseWriter that exposes a write deadline (net/http and +// Caddy do); otherwise the write has no timeout. +func WithResponseWriteTimeout(timeout time.Duration) RequestOption { + return func(o *frankenPHPContext) error { + o.responseWriteTimeout = timeout + + return nil + } +} + // WithWorkerName sets the worker that should handle the request func WithWorkerName(name string) RequestOption { return func(o *frankenPHPContext) error { diff --git a/responsewritetimeout_test.go b/responsewritetimeout_test.go new file mode 100644 index 0000000000..043de11a01 --- /dev/null +++ b/responsewritetimeout_test.go @@ -0,0 +1,58 @@ +package frankenphp_test + +import ( + "fmt" + "net" + "net/http" + "os" + "testing" + "time" + + "github.com/dunglas/frankenphp" + "github.com/stretchr/testify/require" +) + +// TestResponseWriteTimeout proves that WithResponseWriteTimeout bounds a +// stalled reader: the client never reads the response, so the server's socket +// send buffer fills up and the underlying Write() blocks. Without the option +// the PHP thread would block in ub_write forever, since +// frankenphp_force_kill_thread cannot interrupt a write parked in Go's +// non-blocking netpoller (see php/frankenphp#2553, php/frankenphp#2573); with +// it, the write is cut off and the handler returns. +func TestResponseWriteTimeout(t *testing.T) { + require.NoError(t, frankenphp.Init()) + defer frankenphp.Shutdown() + + cwd, _ := os.Getwd() + done := make(chan struct{}) + handler := func(w http.ResponseWriter, r *http.Request) { + defer close(done) + req, err := frankenphp.NewRequestWithContext(r, + frankenphp.WithRequestDocumentRoot(cwd+"/testdata/", false), + frankenphp.WithResponseWriteTimeout(300*time.Millisecond), + ) + require.NoError(t, err) + require.NoError(t, frankenphp.ServeHTTP(w, req)) + } + + ts := newRawServer(t, handler) + defer ts.Close() + + conn, err := net.Dial("tcp", ts.Addr()) + require.NoError(t, err) + defer func() { _ = conn.Close() }() + + // Shrink the client's receive window so the server's send buffer fills + // after only a handful of writes instead of relying on OS defaults. + require.NoError(t, conn.(*net.TCPConn).SetReadBuffer(1024)) + + _, err = fmt.Fprintf(conn, "GET /slow-write.php HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", ts.Addr()) + require.NoError(t, err) + + // Never read the response: that's the stalled reader. + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("the write timeout did not release the PHP thread") + } +} diff --git a/testdata/slow-write.php b/testdata/slow-write.php new file mode 100644 index 0000000000..5c6987e2cc --- /dev/null +++ b/testdata/slow-write.php @@ -0,0 +1,11 @@ +