From 97caab0e7a40c4034888e3269cdcbb858e47b45b Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Mon, 7 Nov 2022 00:06:43 +0000 Subject: PHP: Fix a potential problem parsing the path. @dward on GitHub reported an issue with a URL like http://foo.bar/test.php?blah=test.php/foo where we would end up trying to run the script test.php?blah=test.php In the PHP module the format 'file.php/' is treated as a special case in nxt_php_dynamic_request() where we check the _path_ part of the url for the string '.php/'. The problem is that the path actually also contains the query string, thus we were finding 'test.php/' in the above URL and treating that whole path as the script to run. The fix is simple, replace the strstr(3) with a memmem(3), where we can limit the amount of path we use for the check. The trick here and what is not obvious from the code is that while path.start points to the whole path including the query string, path.length only contains the length of the _path_ part. NOTE: memmem(3) is a GNU extension and is neither specified by POSIX or ISO C, however it is available on a number of other systems, including: FreeBSD, OpenBSD, NetBSD, illumos, and macOS. If it comes to it we can implement a simple alternative for systems which lack memmem(3). This also adds a test case (provided by @dward) to cover this. Closes: Cc: Andrei Zeliankou Reviewed-by: Alejandro Colomar Reviewed-by: Andrei Zeliankou [test] Signed-off-by: Andrew Clayton --- src/nxt_php_sapi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/nxt_php_sapi.c') diff --git a/src/nxt_php_sapi.c b/src/nxt_php_sapi.c index 126a4684..d2494938 100644 --- a/src/nxt_php_sapi.c +++ b/src/nxt_php_sapi.c @@ -1025,7 +1025,8 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) nxt_str_null(&script_name); - ctx->path_info.start = (u_char *) strstr((char *) path.start, ".php/"); + ctx->path_info.start = memmem(path.start, path.length, ".php/", + strlen(".php/")); if (ctx->path_info.start != NULL) { ctx->path_info.start += 4; path.length = ctx->path_info.start - path.start; -- cgit From 0686740f2053abf2b398c6620bb2e74090209fc6 Mon Sep 17 00:00:00 2001 From: Alejandro Colomar Date: Fri, 27 Jan 2023 14:14:43 +0100 Subject: PHP: Factored out code into a helper function. We're going to use zend_stream_init_filename in a following commit. To reduce the diff of that change, move the current code that will be replaced, to a function that has the same interface. We use strlen(3) here to be able to use an interface without passing the length, but we will remove that call in a following code, so it has no performance issues. Co-developed-by: Andrew Clayton Signed-off-by: Alejandro Colomar Reviewed-by: Andrew Clayton Cc: Andrei Zeliankou Signed-off-by: Andrew Clayton --- src/nxt_php_sapi.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) (limited to 'src/nxt_php_sapi.c') diff --git a/src/nxt_php_sapi.c b/src/nxt_php_sapi.c index d2494938..0358ebdf 100644 --- a/src/nxt_php_sapi.c +++ b/src/nxt_php_sapi.c @@ -106,6 +106,8 @@ static nxt_int_t nxt_php_do_301(nxt_unit_request_info_t *req); static void nxt_php_request_handler(nxt_unit_request_info_t *req); static void nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r); +static void nxt_zend_stream_init_filename(zend_file_handle *handle, + const char *filename); static void nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r); nxt_inline void nxt_php_vcwd_chdir(nxt_unit_request_info_t *req, u_char *dir); @@ -1109,6 +1111,21 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) } +static void +nxt_zend_stream_init_filename(zend_file_handle *handle, const char *filename) +{ + nxt_memzero(handle, sizeof(zend_file_handle)); + + handle->type = ZEND_HANDLE_FILENAME; +#if (PHP_VERSION_ID >= 80100) + handle->filename = zend_string_init(filename, strlen(filename), 0); + handle->primary_script = 1; +#else + handle->filename = filename; +#endif +} + + static void nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) { @@ -1179,16 +1196,8 @@ nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) nxt_php_vcwd_chdir(ctx->req, ctx->script_dirname.start); } - nxt_memzero(&file_handle, sizeof(file_handle)); - - file_handle.type = ZEND_HANDLE_FILENAME; -#if (PHP_VERSION_ID >= 80100) - file_handle.filename = zend_string_init((char *) ctx->script_filename.start, - ctx->script_filename.length, 0); - file_handle.primary_script = 1; -#else - file_handle.filename = (char *) ctx->script_filename.start; -#endif + nxt_zend_stream_init_filename(&file_handle, + (const char *) ctx->script_filename.start); php_execute_script(&file_handle TSRMLS_CC); -- cgit From 3923de987efa79645b3e30a55adca4375e0843e2 Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Fri, 27 Jan 2023 14:16:56 +0000 Subject: PHP: Make use of zend_stream_init_filename(). Where possible make use of the zend_stream_init_filename() function introduced in PHP 7.4. This is essentially a preparatory patch for switching to using an already opened file-pointer in nxt_php_execute(). While wrapping this new code in a PHP version check with a fallback to our own function is perhaps slightly overkill, it does reduce the diff of the commit that switches to a FILE *. Reviewed-by: Alejandro Colomar Cc: Andrei Zeliankou Signed-off-by: Andrew Clayton --- src/nxt_php_sapi.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'src/nxt_php_sapi.c') diff --git a/src/nxt_php_sapi.c b/src/nxt_php_sapi.c index 0358ebdf..0445533e 100644 --- a/src/nxt_php_sapi.c +++ b/src/nxt_php_sapi.c @@ -106,8 +106,10 @@ static nxt_int_t nxt_php_do_301(nxt_unit_request_info_t *req); static void nxt_php_request_handler(nxt_unit_request_info_t *req); static void nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r); +#if (PHP_VERSION_ID < 70400) static void nxt_zend_stream_init_filename(zend_file_handle *handle, const char *filename); +#endif static void nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r); nxt_inline void nxt_php_vcwd_chdir(nxt_unit_request_info_t *req, u_char *dir); @@ -1111,19 +1113,17 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) } +#if (PHP_VERSION_ID < 70400) static void nxt_zend_stream_init_filename(zend_file_handle *handle, const char *filename) { nxt_memzero(handle, sizeof(zend_file_handle)); - handle->type = ZEND_HANDLE_FILENAME; -#if (PHP_VERSION_ID >= 80100) - handle->filename = zend_string_init(filename, strlen(filename), 0); - handle->primary_script = 1; -#else handle->filename = filename; -#endif } +#else +#define nxt_zend_stream_init_filename zend_stream_init_filename +#endif static void -- cgit From fafdb7a57ad976e048147fe23079dca9c602e88a Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Wed, 25 Jan 2023 21:46:01 +0000 Subject: PHP: Simplify ctx->script_filename.start in nxt_php_execute(). Create a const char *filename variable to hold ctx->script_filename.start, which is a much more manageable name and will negate the need for any more casting in the following commit when we switch to using a FILE * instead of a filename in php_execute_script(). Reviewed-by: Alejandro Colomar Cc: Andrei Zeliankou Signed-off-by: Andrew Clayton --- src/nxt_php_sapi.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'src/nxt_php_sapi.c') diff --git a/src/nxt_php_sapi.c b/src/nxt_php_sapi.c index 0445533e..b517f7c3 100644 --- a/src/nxt_php_sapi.c +++ b/src/nxt_php_sapi.c @@ -1132,11 +1132,13 @@ nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) #if (PHP_VERSION_ID < 50600) void *read_post; #endif + const char *filename; nxt_unit_field_t *f; zend_file_handle file_handle; - nxt_unit_req_debug(ctx->req, "PHP execute script %s", - ctx->script_filename.start); + filename = (const char *) ctx->script_filename.start; + + nxt_unit_req_debug(ctx->req, "PHP execute script %s", filename); SG(server_context) = ctx; SG(options) |= SAPI_OPTION_NO_CHDIR; @@ -1196,8 +1198,7 @@ nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) nxt_php_vcwd_chdir(ctx->req, ctx->script_dirname.start); } - nxt_zend_stream_init_filename(&file_handle, - (const char *) ctx->script_filename.start); + nxt_zend_stream_init_filename(&file_handle, filename); php_execute_script(&file_handle TSRMLS_CC); -- cgit From bebc03c729df1d7efc81a5af8b8dac40b333d408 Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Thu, 26 Jan 2023 12:09:43 +0000 Subject: PHP: Implement better error handling. Previously the PHP module would produce one of four status codes 200 OK 301 Moved Permanently 500 Internal Server Error 503 Service Unavailable 200 for successful requests, 301 for cases where the url was a directory without a trailing '/', 500 for bad PHP or non-existing PHP file and 503 for all other errors. With this commit we now handle missing files and directories, returning 404 Not Found and files and directories that don't allow access, returning 403 Forbidden. We do these checks in two places, when we check if we should do a directory redirect (bar -> bar/) and in the nxt_php_execute() function. One snag with the latter is that the php_execute_script() function only returns success/failure (no reason). However while it took a zend_file_handle structure with the filename of the script to run, we can instead pass through an already opened file-pointer (FILE *) via that structure. So we can try opening the script ourselves and do the required checks before calling php_execute_script(). We also make use of the zend_stream_init_fp() function that initialises the zend_file_handle structure if it's available otherwise we use our own version. This is good because the zend_file_handle structure has changed over time and the zend_stream_init_fp() function should change with it. Closes: Reviewed-by: Alejandro Colomar Cc: Andrei Zeliankou Signed-off-by: Andrew Clayton --- src/nxt_php_sapi.c | 45 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 5 deletions(-) (limited to 'src/nxt_php_sapi.c') diff --git a/src/nxt_php_sapi.c b/src/nxt_php_sapi.c index b517f7c3..32a13a70 100644 --- a/src/nxt_php_sapi.c +++ b/src/nxt_php_sapi.c @@ -102,12 +102,13 @@ static void nxt_php_str_trim_lead(nxt_str_t *str, u_char t); nxt_inline u_char *nxt_realpath(const void *c); static nxt_int_t nxt_php_do_301(nxt_unit_request_info_t *req); +static nxt_int_t nxt_php_handle_fs_err(nxt_unit_request_info_t *req); static void nxt_php_request_handler(nxt_unit_request_info_t *req); static void nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r); #if (PHP_VERSION_ID < 70400) -static void nxt_zend_stream_init_filename(zend_file_handle *handle, +static void nxt_zend_stream_init_fp(zend_file_handle *handle, FILE *fp, const char *filename); #endif static void nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r); @@ -984,6 +985,24 @@ nxt_php_do_301(nxt_unit_request_info_t *req) } +static nxt_int_t +nxt_php_handle_fs_err(nxt_unit_request_info_t *req) +{ + switch (nxt_errno) { + case ELOOP: + case EACCES: + case ENFILE: + return nxt_unit_response_init(req, NXT_HTTP_FORBIDDEN, 0, 0); + case ENOENT: + case ENOTDIR: + case ENAMETOOLONG: + return nxt_unit_response_init(req, NXT_HTTP_NOT_FOUND, 0, 0); + } + + return NXT_UNIT_ERROR; +} + + static void nxt_php_request_handler(nxt_unit_request_info_t *req) { @@ -1062,6 +1081,8 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) ret = stat(tpath, &sb); if (ret == 0 && S_ISDIR(sb.st_mode)) { ec = nxt_php_do_301(ctx->req); + } else if (ret == -1) { + ec = nxt_php_handle_fs_err(ctx->req); } nxt_unit_request_done(ctx->req, ec); @@ -1115,20 +1136,23 @@ nxt_php_dynamic_request(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) #if (PHP_VERSION_ID < 70400) static void -nxt_zend_stream_init_filename(zend_file_handle *handle, const char *filename) +nxt_zend_stream_init_fp(zend_file_handle *handle, FILE *fp, + const char *filename) { nxt_memzero(handle, sizeof(zend_file_handle)); - handle->type = ZEND_HANDLE_FILENAME; + handle->type = ZEND_HANDLE_FP; + handle->handle.fp = fp; handle->filename = filename; } #else -#define nxt_zend_stream_init_filename zend_stream_init_filename +#define nxt_zend_stream_init_fp zend_stream_init_fp #endif static void nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) { + FILE *fp; #if (PHP_VERSION_ID < 50600) void *read_post; #endif @@ -1140,6 +1164,17 @@ nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) nxt_unit_req_debug(ctx->req, "PHP execute script %s", filename); + fp = fopen(filename, "re"); + if (fp == NULL) { + nxt_int_t ec; + + nxt_unit_req_debug(ctx->req, "PHP fopen(\"%s\") failed", filename); + + ec = nxt_php_handle_fs_err(ctx->req); + nxt_unit_request_done(ctx->req, ec); + return; + } + SG(server_context) = ctx; SG(options) |= SAPI_OPTION_NO_CHDIR; SG(request_info).request_uri = nxt_unit_sptr_get(&r->target); @@ -1198,7 +1233,7 @@ nxt_php_execute(nxt_php_run_ctx_t *ctx, nxt_unit_request_t *r) nxt_php_vcwd_chdir(ctx->req, ctx->script_dirname.start); } - nxt_zend_stream_init_filename(&file_handle, filename); + nxt_zend_stream_init_fp(&file_handle, fp, filename); php_execute_script(&file_handle TSRMLS_CC); -- cgit From edbc43558d40768d91b378205c2d52bd7ba9d00a Mon Sep 17 00:00:00 2001 From: Andrew Clayton Date: Fri, 31 Mar 2023 14:01:43 +0100 Subject: PHP: Make the filter_input() function work. On GitHub, @jamesRUS52 reported that the PHP filter_input()[0] function would just return NULL. To enable this function we need to run the variables through the sapi_module.input_filter() function when we call php_register_variable_safe(). In PHP versions prior to 7.0.0, input_filter() takes 'len' as an unsigned int, while later versions take it as a size_t. Now, with this commit and the following PHP you get $ curl 'http://localhost:8080/854.php?get=foo<>' string(3) "::1" string(18) "/854.php?get=foo<>" string(13) "foo<>" [0]: Tested-by: Closes: Reviewed-by: Alejandro Colomar Signed-off-by: Andrew Clayton --- src/nxt_php_sapi.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) (limited to 'src/nxt_php_sapi.c') diff --git a/src/nxt_php_sapi.c b/src/nxt_php_sapi.c index 32a13a70..ba000fc0 100644 --- a/src/nxt_php_sapi.c +++ b/src/nxt_php_sapi.c @@ -1532,14 +1532,23 @@ static void nxt_php_set_sptr(nxt_unit_request_info_t *req, const char *name, nxt_unit_sptr_t *v, uint32_t len, zval *track_vars_array TSRMLS_DC) { - char *str; + char *str; +#if NXT_PHP7 + size_t new_len; +#else + unsigned int new_len; +#endif str = nxt_unit_sptr_get(v); nxt_unit_req_debug(req, "php: register %s='%.*s'", name, (int) len, str); - php_register_variable_safe((char *) name, str, len, - track_vars_array TSRMLS_CC); + if (sapi_module.input_filter(PARSE_SERVER, (char *) name, &str, len, + &new_len TSRMLS_CC)) + { + php_register_variable_safe((char *) name, str, new_len, + track_vars_array TSRMLS_CC); + } } -- cgit