diff --git a/docs/security.md b/docs/security.md index cbb4c06d19..b6f34665b7 100644 --- a/docs/security.md +++ b/docs/security.md @@ -46,7 +46,7 @@ These are the surfaces FrankenPHP owns. A vulnerability here is a FrankenPHP vul - **Request to superglobal mapping** (`cgi.go`, `frankenphp_register_server_vars`): building `$_SERVER`, `REMOTE_ADDR`, `SCRIPT_NAME`, `PATH_INFO`, and the other CGI variables from the request. - **PHP script-path resolution**: the request path is split on `split_path` (`.php` by default) into `SCRIPT_NAME` / `PATH_INFO`, then joined to the document root with `sanitizedPathJoin` (`filepath.Join(root, filepath.Clean("/"+reqPath))`), which keeps `SCRIPT_FILENAME` from escaping the document root (path traversal). The `php_server` directive additionally sets a default `try_files` rewrite that routes requests to existing files or the front controller, mitigating the classic PHP-FPM pitfall of executing the wrong file. -- **Worker-mode state isolation**: FrankenPHP resets `$_GET`, `$_POST`, `$_COOKIE`, `$_FILES`, `$_SERVER`, and `$_REQUEST` between requests, explicitly clears `$_SESSION` (which would otherwise leak between requests) and the errors returned by `error_get_last()` and `libxml_get_errors()`, and the headers returned by `http_get_last_response_headers()`, the `strtok()` state, and the variables added by `output_add_rewrite_var()`, but **`$_ENV` is not reset**, and `putenv()` writes, `static` variables, class static properties, globals, and runtime engine settings (`ini_set()`, the default stream context, the default timezone, the working directory, ...) persist across requests on the same thread. Request- or user-specific data left in that state can leak into a later request (see [Worker Mode](worker.md#state-persistence)). +- **Worker-mode state isolation**: FrankenPHP resets `$_GET`, `$_POST`, `$_COOKIE`, `$_FILES`, `$_SERVER`, and `$_REQUEST` between requests, explicitly clears `$_SESSION` (which would otherwise leak between requests) and the variables added by `output_add_rewrite_var()`, but **`$_ENV` is not reset**, and `putenv()` writes, the last-error state of PHP and its extensions (`error_get_last()`, `libxml_get_errors()`, ...), the state of other functions unrelated to the request (`strtok()`, `http_get_last_response_headers()`, ...), `static` variables, class static properties, globals, and runtime engine settings (`ini_set()`, the default stream context, the default timezone, the working directory, ...) persist across requests on the same thread. Request- or user-specific data left in that state can leak into a later request (see [Worker Mode](worker.md#state-persistence)). - **Per-thread environment sandboxing**: `frankenphp_putenv()` / `frankenphp_getenv()` operate on a thread-local `sandboxed_env`. `putenv()` writes stay inside that sandbox and never reach the process-global OS environment, so a secret set by one request cannot leak to child processes (`proc_open`, `exec`, ...), to the Go/Caddy side, or to other requests. A child process that must see such a variable has to be given it explicitly, through the environment argument of `proc_open()` or `exec()` (see [Internals](internals.md#per-thread-environment-sandboxing)). - **CGO memory boundary**: Go string pinning and `C.CString()` / `free()` lifetimes across the Go ↔ C boundary. - **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. diff --git a/docs/worker.md b/docs/worker.md index 1b6c86a225..b2f019ff88 100644 --- a/docs/worker.md +++ b/docs/worker.md @@ -236,13 +236,15 @@ Avoid storing request-specific or sensitive data in `$_ENV`. ## State persistence -Because worker mode keeps the PHP process alive between requests, the following state persists across requests: +Because worker mode keeps the PHP process alive between requests, FrankenPHP only resets the state tied to the HTTP request being handled: superglobals (except `$_ENV`), the request body, response headers and output, and the session. +Everything else is shared between the requests handled by the same thread, as in a long-running CLI script, including: - **Static variables**: Variables declared with `static` inside functions or methods retain their values between requests. - **Class static properties**: Static properties on classes persist between requests. - **Global variables**: Variables in the global scope of the worker script persist between requests. - **In-memory caches**: Any data stored in memory (arrays, objects) outside the request handler persists. - **Runtime engine settings**: Changes made with `ini_set()`, `stream_context_set_default()`, `date_default_timezone_set()`, `chdir()`, `stream_wrapper_register()`, `set_error_handler()` or `set_exception_handler()` stay in effect for later requests. Never store request- or user-specific values there, such as credentials in the default stream context: pass an explicit context to each call instead. +- **Engine and extension state**: `error_get_last()`, `libxml_get_errors()` and `libxml_get_last_error()` return the errors an earlier request left behind. `http_get_last_response_headers()` returns the response headers of the last HTTP call made on the thread, even by an earlier request. `strtok()` continues the string an earlier request gave it. After `mt_srand()` or `srand()` with a seed, `mt_rand()`, `rand()`, `shuffle()` and `array_rand()` continue that seeded sequence. The errors, headers and strings can contain another client's data, and a seeded sequence makes random values predictable: reset them as shown below. - **Process-wide settings**: `umask()` and `setlocale()` change the whole FrankenPHP process, not only the current thread. Worker mode doesn't restore them between requests, and in every mode they also affect requests running at the same time. Set them once at startup, or call `chmod()` on created files instead of changing the umask. This is by design and is what makes worker mode fast. However, it requires attention to avoid unintended side effects: @@ -264,4 +266,23 @@ while (\frankenphp_handle_request($handler)) { ``` When writing worker scripts, make sure to reset any request-specific state between requests. +PHP provides functions to reset the engine and extension state listed above: + +```php += 80400 - /* http_get_last_response_headers() */ - zval_ptr_dtor(&BG(last_http_headers)); - ZVAL_UNDEF(&BG(last_http_headers)); -#endif - - /* Called by name to avoid depending on libxml2 headers */ - zend_function *libxml_clear_errors = zend_hash_str_find_ptr( - CG(function_table), ZEND_STRL("libxml_clear_errors")); - if (libxml_clear_errors) { - zend_call_known_function(libxml_clear_errors, NULL, NULL, NULL, 0, NULL, - NULL); - } -} - /* Adapted from the url_scanner RSHUTDOWN, which isn't exported */ static void frankenphp_reset_url_scanner(url_adapt_state_ex_t *ctx) { smart_str_free(&ctx->result); @@ -574,13 +544,8 @@ static void frankenphp_reset_url_scanner(url_adapt_state_ex_t *ctx) { ctx->attr_type = 0; } -/* Request state of the standard extension, freed by its RSHUTDOWN */ +/* Response state of the standard extension, freed by its RSHUTDOWN */ static void frankenphp_reset_basic_state(void) { - if (BG(strtok_string)) { - zend_string_release(BG(strtok_string)); - BG(strtok_string) = NULL; - } - /* output_add_rewrite_var() and session.use_trans_sid */ frankenphp_reset_url_scanner(&BG(url_adapt_session_ex)); frankenphp_reset_url_scanner(&BG(url_adapt_output_ex)); @@ -610,8 +575,6 @@ static void frankenphp_worker_request_shutdown() { frankenphp_reset_session_state(); #endif - frankenphp_reset_last_errors(); - /* Shutdown output layer (send the set HTTP headers, cleanup output handlers, * etc.) */ zend_try { php_output_deactivate(); } diff --git a/frankenphp_test.go b/frankenphp_test.go index 84199ba81d..458d5e3b4e 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1254,61 +1254,13 @@ func TestFileStreamInWorkerMode(t *testing.T) { }, &testOptions{workerScript: "file-stream.php", nbParallelRequests: 1, nbWorkers: 1}) } -func TestLastErrorsNoLeakBetweenRequests_worker(t *testing.T) { - type lastErrors struct { - LastError *string `json:"last_error"` - LibxmlErrors *[]string `json:"libxml_errors"` - LibxmlLastError *string `json:"libxml_last_error"` - InternalErrors *bool `json:"internal_errors"` - LastResponse *[]string `json:"last_response_headers"` - Strtok any `json:"strtok"` - Rewriter *bool `json:"rewriter"` - } - - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("X-Secret", r.URL.Query().Get("s")) - })) - t.Cleanup(upstream.Close) - - get := func(url string, handler func(http.ResponseWriter, *http.Request)) lastErrors { - body, _ := testGet(url, handler, t) - - var e lastErrors - require.NoError(t, json.Unmarshal([]byte(body), &e), body) - - return e - } - +func TestURLRewriterRestartsBetweenRequests_worker(t *testing.T) { runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { - fetch := url.QueryEscape(fmt.Sprintf("%s/?s=secret_%d", upstream.URL, i)) - leaked := get(fmt.Sprintf("http://example.com/worker-last-errors.php?leak=%d&fetch=%s", i, fetch), handler) - require.NotNil(t, leaked.LastError) - assert.Equal(t, fmt.Sprintf("secret %d", i), *leaked.LastError) - - e := get("http://example.com/worker-last-errors.php", handler) - assert.Nil(t, e.LastError) - assert.Equal(t, false, e.Strtok) - require.NotNil(t, e.Rewriter) - assert.True(t, *e.Rewriter, "the URL rewriter must restart in each request") - - if leaked.LastResponse != nil { - assert.Contains(t, *leaked.LastResponse, fmt.Sprintf("X-Secret: secret_%d", i)) - assert.Nil(t, e.LastResponse) - } - - if leaked.LibxmlErrors == nil { - t.Log("libxml not available, skipping libxml assertions") - - return - } - - require.NotEmpty(t, *leaked.LibxmlErrors) - assert.Contains(t, (*leaked.LibxmlErrors)[0], fmt.Sprintf("secret_%d", i)) + testGet(fmt.Sprintf("http://example.com/worker-url-rewriter.php?leak=%d", i), handler, t) - assert.Empty(t, *e.LibxmlErrors) - assert.Nil(t, e.LibxmlLastError) - assert.True(t, *e.InternalErrors) - }, &testOptions{workerScript: "worker-last-errors.php", nbParallelRequests: 1, nbWorkers: 1}) + body, _ := testGet("http://example.com/worker-url-rewriter.php", handler, t) + assert.Equal(t, "true", body, "the URL rewriter must restart in each request") + }, &testOptions{workerScript: "worker-url-rewriter.php", nbParallelRequests: 1, nbWorkers: 1}) } // To run this fuzzing test use: go test -fuzz FuzzRequest diff --git a/testdata/worker-last-errors.php b/testdata/worker-last-errors.php deleted file mode 100644 index da707c54c4..0000000000 --- a/testdata/worker-last-errors.php +++ /dev/null @@ -1,35 +0,0 @@ -loadXML("&secret_{$_GET['leak']};"); - } - strtok("secret {$_GET['leak']}", ' '); - output_add_rewrite_var('token', "secret_{$_GET['leak']}"); - if (isset($_GET['fetch'])) { - file_get_contents($_GET['fetch']); - } - } - - echo json_encode([ - 'last_error' => error_get_last()['message'] ?? null, - // evaluated after last_error: strtok() warns when there is nothing to continue - 'strtok' => isset($_GET['leak']) ? null : @strtok(' '), - 'rewriter' => isset($_GET['leak']) ? null : output_add_rewrite_var('x', 'y') && in_array('URL-Rewriter', ob_list_handlers(), true), - 'libxml_errors' => $hasLibxml ? array_map(fn ($e) => trim($e->message), libxml_get_errors()) : null, - 'libxml_last_error' => $hasLibxml ? (libxml_get_last_error()->message ?? null) : null, - 'internal_errors' => $hasLibxml ? libxml_use_internal_errors() : null, - 'last_response_headers' => function_exists('http_get_last_response_headers') ? http_get_last_response_headers() : null, - ]); -}; diff --git a/testdata/worker-url-rewriter.php b/testdata/worker-url-rewriter.php new file mode 100644 index 0000000000..760d57f1b3 --- /dev/null +++ b/testdata/worker-url-rewriter.php @@ -0,0 +1,13 @@ +