diff --git a/CHANGELOG.md b/CHANGELOG.md index 256e551..1e7823f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- **Three shapes of hide pattern matched nothing, and nothing reads as hidden (#309).** `hide('/index.php')`, `hide('cache/')` and `hide('logs/**')` are the forms an operator carries over from gitignore; the first two covered no path at all, and the third stopped one level down, so `logs/deep/app.log` was served. A leading separator anchored the pattern and then compared it against a mount-relative path that never carries one; a trailing separator was compared literally against paths that are never bare directories; `**` met `FNM_PATHNAME`, which stops each `*` at the separator. All three fail in the direction that discloses: the pattern is accepted without complaint and the file goes on the wire. #270 gave the rule its other two shapes; this finishes it — a leading `/` pins the mount root, a trailing `/` names a directory wherever it sits along with everything under it, and `**` crosses separators for the whole pattern. A bare pattern still reads the file name, so `cache/` is how to cover a directory's contents. Underneath sat a platform split worth naming: the Windows `fnmatch` shim cast its `flags` away, so any fix that turns `FNM_PATHNAME` off works on POSIX and does nothing on Windows — the shim reads the flag now. `hide()` also refuses a pattern over 512 bytes rather than accepting one that can only match nothing. Evidence: `static/024` serves `root-secret`, `cached` and `logged` for the three paths against `dbe66f0` and hands all three to the handler here; the `StaticHide` unit table gains a row per shape. - **`sendFile()` and `StaticHandler` walked around `open_basedir` (#280).** The server opens response files itself rather than through PHP streams, and never consulted the interpreter's policy, so a handler could hand out any file the process could read: with `open_basedir` set, `fopen()` refused the path and `sendFile()` answered `200 OK` with its contents. `open_basedir` is the operator's boundary, and the handler is exactly the code it exists to contain — the trust `sendFile()` places in handler code is a contract with the application, not a licence to overrule the operator. All three entry points now call `php_check_open_basedir_ex()`: `sendFile()` throws, the send-file engine answers 500, and a mount rooted outside the boundary is refused at construction. Scope: the mount root is validated once, at construction, so narrowing `open_basedir` at runtime below an existing mount does not retroactively close it; `sendFile()` is checked per call. Evidence: `sendfile/007-sendfile-open-basedir` serves the running binary against `main` and answers 500 here. ### Fixed diff --git a/include/static/http_static_path.h b/include/static/http_static_path.h index d5ab251..88dbc50 100644 --- a/include/static/http_static_path.h +++ b/include/static/http_static_path.h @@ -51,11 +51,28 @@ http_static_path_resolve(const http_static_handler_t *mount, const char *request * Used by the index-file resolution loop. */ bool http_static_path_join(char *buf, size_t cap, size_t *len, const char *name, size_t name_len); -/* Whether one hide glob covers one mount-relative path. A pattern naming a - * directory (`cache/*`) is anchored at the mount root and `*` stops at each - * separator; a pattern naming none (`*.php`) covers a file of that name at any - * depth. Takes the two strings rather than the mount so the rule can be held to - * a table. Both must be NUL-terminated. */ +/* Longest hide pattern the matcher reads, once a leading or trailing separator + * is off it. StaticHandler::hide measures the pattern as written against the + * same number, so it refuses a little sooner than the matcher would stop. */ +#define HTTP_STATIC_HIDE_GLOB_MAX 512 + +/* Whether one hide glob covers one mount-relative path, by gitignore's rule: + * + * *.php a file of that name at any depth + * /index.php that file at the mount root only + * cache/x anchored at the mount root, * stopping at each separator + * cache/ a directory of that name at any depth, and all it holds + * cache/** anchored, * crossing separators + * + * A pattern opening with a double star names every directory, and the mount + * root is one of them. + * + * A bare pattern reads the file name, so a directory named like it does not + * drag its contents in — `cache/` says that instead. Case follows the + * platform's own filesystem: a pattern is read case-insensitively on Windows + * and case-sensitively elsewhere. Takes the two strings rather than the mount + * so the rule can be held to a table. Both must be NUL-terminated; a pattern + * longer than HTTP_STATIC_HIDE_GLOB_MAX covers nothing. */ bool http_static_hide_glob_matches(const char *glob, const char *relative); /* Returns true when the relative path matches one of the mount's hide globs, diff --git a/src/static/http_static_path.c b/src/static/http_static_path.c index b07d85a..325c24e 100644 --- a/src/static/http_static_path.c +++ b/src/static/http_static_path.c @@ -17,21 +17,24 @@ #include #ifdef PHP_WIN32 -/* Minimal glob matching used only for hide-path patterns. Handles '?' - * (any single non-separator char) and '*' (any sequence of - * non-separator chars). Case-insensitive on Windows. */ +/* Minimal glob matching used only for hide-path patterns. Handles '?' (any + * single character) and '*' (any sequence). Without FNM_PATHNAME both cross + * the separator, which is how a caller spells "at any depth". + * Case-insensitive on Windows. */ # define FNM_PATHNAME 0x01 -static int win32_fnmatch_impl(const char *p, const char *s) +static int win32_fnmatch_impl(const char *p, const char *s, bool cross_separator) { while (*p) { if (*p == '?') { - if (!*s || *s == '/') return 1; + if (!*s || (!cross_separator && *s == '/')) return 1; p++; s++; } else if (*p == '*') { - p++; + /* A run of stars says no more than one does, and each extra star + * would double the positions the branch below tries. */ + while (*p == '*') p++; do { - if (win32_fnmatch_impl(p, s) == 0) return 0; - if (*s == '/' || !*s) break; + if (win32_fnmatch_impl(p, s, cross_separator) == 0) return 0; + if (!*s || (!cross_separator && *s == '/')) break; } while (*s++); return 1; } else { @@ -43,8 +46,7 @@ static int win32_fnmatch_impl(const char *p, const char *s) } static int fnmatch(const char *pattern, const char *string, int flags) { - (void)flags; - return win32_fnmatch_impl(pattern, string); + return win32_fnmatch_impl(pattern, string, (flags & FNM_PATHNAME) == 0); } #else # include @@ -293,30 +295,91 @@ bool http_static_path_join(char *buf, const size_t cap, size_t *len, return true; } +/* Whether the pattern names the path itself. An anchored pattern is read against + * the whole mount-relative path; a bare one against the file name, because the + * directory a file sits in is not part of what a bare pattern reads. */ +static bool http_static_hide_glob_names_path(const char *pattern, const char *relative, + bool anchored, int flags) +{ + if (anchored) { + if (fnmatch(pattern, relative, flags) == 0) { + return true; + } + + /* A pattern opening with a double star and a separator reads "in every + * directory", and the mount root is one: there the separator has + * nothing to match, so the root's own file needs the prefix off. */ + return strncmp(pattern, "**/", 3) == 0 && fnmatch(pattern + 3, relative, flags) == 0; + } + + const char *const basename = strrchr(relative, '/'); + + return fnmatch(pattern, basename != NULL ? basename + 1 : relative, flags) == 0; +} + bool http_static_hide_glob_matches(const char *glob, const char *relative) { if (glob == NULL || relative == NULL) { return false; } - if (fnmatch(glob, relative, FNM_PATHNAME) == 0) { + /* gitignore's rule, because it is the one an operator already knows and the + * one that fails safe: a separator at the front or the middle anchors the + * pattern at the mount root, a separator at the end names a directory, and + * a pattern with neither names a file wherever it sits. Getting the last one + * wrong costs a disclosure rather than a 404 — `*.php` covering `index.php` + * and handing `admin/tools.php` to the client as source. */ + const bool rooted = (glob[0] == '/'); + const char *const body = glob + (rooted ? 1 : 0); + size_t body_len = strlen(body); + const bool names_directory = (body_len > 0 && body[body_len - 1] == '/'); + + if (names_directory) { + body_len--; + } + + /* StaticHandler::hide refuses a longer pattern, so this is unreachable from + * the PHP API; the matcher is public and answers for itself. */ + if (body_len == 0 || body_len > HTTP_STATIC_HIDE_GLOB_MAX) { + return false; + } + + char pattern[HTTP_STATIC_HIDE_GLOB_MAX + 1]; + memcpy(pattern, body, body_len); + pattern[body_len] = '\0'; + + const bool anchored = rooted || memchr(pattern, '/', body_len) != NULL; + /* `**` is how an operator spells "across directories", so the flag that + * stops `*` at a separator comes off for the whole pattern. */ + const int flags = strstr(pattern, "**") != NULL ? 0 : FNM_PATHNAME; + + if (http_static_hide_glob_names_path(pattern, relative, anchored, flags)) { return true; } - /* A pattern that names no directory names a file, and that file is hidden - * wherever it sits — gitignore's rule, and the one an operator writing - * `*.php` means. FNM_PATHNAME stops `*` at the separator, so without this - * the pattern covers `index.php` and serves `admin/tools.php` as source: - * the surprise costs a disclosure rather than a 404. A pattern that does - * name a directory stays anchored at the mount root, which is the only way - * to say `cache/*` and mean that one directory. */ - if (strchr(glob, '/') != NULL) { + if (!names_directory) { return false; } - const char *const basename = strrchr(relative, '/'); + /* Hiding a directory hides what is under it, at any depth. Spelling "under" + * as a pattern keeps the path itself free of the per-request copy that + * walking its separators would need; `*` crosses them here whatever the + * pattern said, because depth below a hidden directory does not matter. */ + char under[HTTP_STATIC_HIDE_GLOB_MAX + 5]; + + snprintf(under, sizeof(under), "%s/*", pattern); + + if (fnmatch(under, relative, 0) == 0) { + return true; + } + + if (anchored) { + return false; + } + + snprintf(under, sizeof(under), "*/%s/*", pattern); - return basename != NULL && fnmatch(glob, basename + 1, FNM_PATHNAME) == 0; + return fnmatch(under, relative, 0) == 0; } bool http_static_path_is_hidden(const http_static_handler_t *mount, const char *relative, diff --git a/src/static/static_handler_class.c b/src/static/static_handler_class.c index 94bf03a..630dc0f 100644 --- a/src/static/static_handler_class.c +++ b/src/static/static_handler_class.c @@ -18,6 +18,7 @@ #include "php_http_server.h" #include "main/fopen_wrappers.h" /* php_check_open_basedir_ex */ #include "static/static_handler.h" +#include "static/http_static_path.h" /* HTTP_STATIC_HIDE_GLOB_MAX — the bound hide() holds */ #include #include @@ -770,6 +771,15 @@ ZEND_METHOD(TrueAsync_StaticHandler, hide) "StaticHandler hide pattern must not be empty", 0); return; } + + /* A pattern the matcher cannot read covers nothing, and nothing is the + * answer an operator reads as "hidden". Refuse it where it is written. */ + if (Z_STRLEN(args[i]) > HTTP_STATIC_HIDE_GLOB_MAX) { + zend_throw_exception_ex(http_server_invalid_argument_exception_ce, 0, + "StaticHandler hide pattern must be at most %d bytes", + HTTP_STATIC_HIDE_GLOB_MAX); + return; + } } const size_t new_count = mount->hide_count + argc; diff --git a/stubs/StaticHandler.php b/stubs/StaticHandler.php index a4a3ce2..d69b02d 100644 --- a/stubs/StaticHandler.php +++ b/stubs/StaticHandler.php @@ -148,8 +148,22 @@ public function setSymlinkPolicy(StaticSymlinks $policy): static {} /** * Glob patterns whose matching paths return 404 regardless of * existence. Patterns are matched against the path *relative to - * the root directory*, with `/` as the separator. + * the root directory*, with `/` as the separator, by gitignore's rule: * + * `*.php` a file of that name at any depth + * `/index.php` that file at the root directory only + * `cache/x` anchored at the root directory, `*` stopping at each `/` + * `cache/` a directory of that name at any depth, and all it holds + * `cache/**` anchored, `*` crossing `/` + * + * A pattern opening with a double star names every directory, the root + * directory among them. A pattern without `/` reads the file name, so a + * directory named like it keeps serving what it holds — `cache/` is how to + * cover a directory. Case follows the platform's own filesystem: + * case-insensitive on Windows, case-sensitive elsewhere. + * + * @throws HttpServerInvalidArgumentException when a pattern is empty or + * longer than 512 bytes. * @return static */ public function hide(string ...$globs): static {} diff --git a/tests/phpt/server/static/024-static-hide-gitignore-forms.phpt b/tests/phpt/server/static/024-static-hide-gitignore-forms.phpt new file mode 100644 index 0000000..f1ea2c2 --- /dev/null +++ b/tests/phpt/server/static/024-static-hide-gitignore-forms.phpt @@ -0,0 +1,126 @@ +--TEST-- +StaticHandler::hide(): a leading separator pins the root, a trailing one names a directory, ** crosses +--EXTENSIONS-- +true_async_server +true_async +--FILE-- + 'root-secret', + '/sub/index.php' => 'nested-copy', + '/var/cache/x.txt' => 'cached', + '/logs/deep/app.log' => 'logged', + '/secret.txt' => 'root-note', + '/sub/secret.txt' => 'nested-note', + '/app.svg' => 'svg-bytes', +]; + +foreach ($files as $path => $body) { + file_put_contents($root . $path, $body); +} + +register_shutdown_function(function () use ($root, $files) { + foreach (array_keys($files) as $path) { + @unlink($root . $path); + } + foreach (['/sub', '/var/cache', '/var', '/logs/deep', '/logs'] as $dir) { + @rmdir($root . $dir); + } + @rmdir($root); +}); + +$port = tas_free_port(); +$server = new HttpServer( + (new HttpServerConfig())->addListener('127.0.0.1', $port)->setReadTimeout(5) +); + +$mount = new StaticHandler('/', $root); +$mount->setOnMissing(StaticOnMissing::NEXT); +$mount->hide('/index.php', 'cache/', 'logs/**', '**/secret.txt'); +$server->addStaticHandler($mount); + +$server->addHttpHandler(function ($req, $res) { + $res->end('handler:' . $req->getPath()); +}); + +spawn(function () use ($port, $server) { + delay(50); + + foreach (array_keys($files = [ + '/app.svg' => null, + '/index.php' => null, + '/sub/index.php' => null, + '/var/cache/x.txt' => null, + '/logs/deep/app.log' => null, + '/secret.txt' => null, + '/sub/secret.txt' => null, + ]) as $path) { + $c = stream_socket_client("tcp://127.0.0.1:$port", $e1, $e2, 3); + stream_set_timeout($c, 3); + fwrite($c, "GET $path HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n"); + $raw = stream_get_contents($c); + fclose($c); + + $body = substr($raw, strpos($raw, "\r\n\r\n") + 4); + echo str_pad($path, 20), '-> ', trim($body), "\n"; + } + + /* A pattern the matcher cannot read would cover nothing, and nothing is + * what an operator reads as hidden — hide() refuses it instead. */ + $spare = new StaticHandler('/spare/', sys_get_temp_dir()); + + try { + $spare->hide(str_repeat('a', 512)); + echo "512 bytes: accepted\n"; + } catch (\Throwable $e) { + echo "512 bytes: ", get_class($e), "\n"; + } + + try { + $spare->hide(str_repeat('a', 513)); + echo "513 bytes: accepted\n"; + } catch (\Throwable $e) { + echo "513 bytes: ", get_class($e), ' - ', $e->getMessage(), "\n"; + } + + $server->stop(); +}); + +$server->start(); +echo "Done\n"; +?> +--EXPECT-- +/app.svg -> svg-bytes +/index.php -> handler:/index.php +/sub/index.php -> nested-copy +/var/cache/x.txt -> handler:/var/cache/x.txt +/logs/deep/app.log -> handler:/logs/deep/app.log +/secret.txt -> handler:/secret.txt +/sub/secret.txt -> handler:/sub/secret.txt +512 bytes: accepted +513 bytes: TrueAsync\HttpServerInvalidArgumentException - StaticHandler hide pattern must be at most 512 bytes +Done diff --git a/tests/unit/static/test_static_hide.c b/tests/unit/static/test_static_hide.c index e409bdd..3f738c7 100644 --- a/tests/unit/static/test_static_hide.c +++ b/tests/unit/static/test_static_hide.c @@ -53,6 +53,48 @@ static void test_rooted_pattern_stays_at_the_root(void **state) assert_false(http_static_hide_glob_matches("cache/*", "cache/deep/x.txt")); } +static void test_leading_separator_pins_the_root(void **state) +{ + (void)state; + assert_true(http_static_hide_glob_matches("/index.php", "index.php")); + assert_false(http_static_hide_glob_matches("/index.php", "sub/index.php")); + assert_true(http_static_hide_glob_matches("/*.php", "index.php")); + assert_false(http_static_hide_glob_matches("/*.php", "admin/tools.php")); +} + +static void test_trailing_separator_names_a_directory(void **state) +{ + (void)state; + assert_true(http_static_hide_glob_matches("cache/", "cache/x.txt")); + assert_true(http_static_hide_glob_matches("cache/", "cache/deep/x.txt")); + assert_true(http_static_hide_glob_matches("cache/", "var/cache/x.txt")); + assert_true(http_static_hide_glob_matches("cache/", "a/b/cache/deep/x.txt")); + /* The directory itself, when a request names it. */ + assert_true(http_static_hide_glob_matches("cache/", "var/cache")); + assert_false(http_static_hide_glob_matches("cache/", "var/cached/x.txt")); + assert_false(http_static_hide_glob_matches("cache/", "cache.txt")); + + /* A separator inside anchors it, as it does everywhere else. */ + assert_true(http_static_hide_glob_matches("var/cache/", "var/cache/x.txt")); + assert_false(http_static_hide_glob_matches("var/cache/", "app/var/cache/x.txt")); +} + +static void test_double_star_crosses_separators(void **state) +{ + (void)state; + assert_true(http_static_hide_glob_matches("cache/**", "cache/x.txt")); + assert_true(http_static_hide_glob_matches("cache/**", "cache/deep/x.txt")); + assert_false(http_static_hide_glob_matches("cache/**", "var/cache/x.txt")); + assert_true(http_static_hide_glob_matches("**/secret.txt", "a/b/secret.txt")); + /* Every directory includes the one the pattern is written against, and the + * separator after the stars has nothing to match there. */ + assert_true(http_static_hide_glob_matches("**/secret.txt", "secret.txt")); + assert_false(http_static_hide_glob_matches("**/secret.txt", "secret.txt.bak")); + + /* A run of stars says what one says, and costs what one costs. */ + assert_true(http_static_hide_glob_matches("logs/****", "logs/deep/app.log")); +} + static void test_pattern_covers_nothing_it_does_not_name(void **state) { (void)state; @@ -60,6 +102,8 @@ static void test_pattern_covers_nothing_it_does_not_name(void **state) assert_false(http_static_hide_glob_matches("*.php", "assets/app.js")); assert_false(http_static_hide_glob_matches(NULL, "index.php")); assert_false(http_static_hide_glob_matches("*.php", NULL)); + /* A separator and nothing else names no file. */ + assert_false(http_static_hide_glob_matches("/", "index.php")); } int main(void) @@ -68,6 +112,9 @@ int main(void) cmocka_unit_test(test_bare_pattern_covers_every_depth), cmocka_unit_test(test_bare_pattern_reads_the_name_not_the_path), cmocka_unit_test(test_rooted_pattern_stays_at_the_root), + cmocka_unit_test(test_leading_separator_pins_the_root), + cmocka_unit_test(test_trailing_separator_names_a_directory), + cmocka_unit_test(test_double_star_crosses_separators), cmocka_unit_test(test_pattern_covers_nothing_it_does_not_name), };